refactor(noise): fix naming issues / file organisation #221
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I also renamed the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (11)
noise/noise.v (1)
Line range hint
1-18: Well-structured foundation for noise generationThe Generator struct and its methods provide a clean, well-encapsulated foundation for both Perlin and Simplex noise implementations. The consistent naming and clear separation of concerns support the PR's goal of better file organization.
noise/perlin_test.v (1)
6-13: Consider enhancing test coverage and documentation.While the test implementation is correct, consider these improvements:
- Add test cases for edge cases (e.g., 0.0, 1.0, negative values)
- Document the derivation of the expected value
- Define the tolerance as a named constant
+const tolerance = 1.0e-6 + fn test_perlin_2d() { + // Fixed seed for reproducible test results rand.seed([u32(3155200429), u32(3208395956)]) mut gen := Generator.new() gen.randomize() + // Test case: Known value at (0.125, 0.125) + // Expected value derived from reference implementation result := gen.perlin_2d(0.125, 0.125) expected := 0.4948387311305851 - assert float64.tolerance(result, expected, 1.0e-6) + assert float64.tolerance(result, expected, tolerance) + + // Additional test cases for edge conditions + edge_cases := [ + (0.0, 0.0), + (1.0, 1.0), + (-0.5, 0.5) + ] + for point in edge_cases { + _ := gen.perlin_2d(point.0, point.1) // Ensure no panics + } }noise/simplex.v (1)
Line range hint
166-387: Consider improving readability of long gradient function calls.While the parameter renaming and implementation are correct, some gradient function calls span multiple lines and could be formatted more readably. Consider introducing intermediate variables for the permission calculations.
Example for lines 237-238:
- n0 = t0 * t0 * simplex_grad_3d(gen.perm[ii + gen.perm[jj + gen.perm[kk]]], x0, - y0, z0) + perm_index := gen.perm[ii + gen.perm[jj + gen.perm[kk]]] + n0 = t0 * t0 * simplex_grad_3d(perm_index, x0, y0, z0)This pattern could be applied to similar instances throughout the 3D and 4D implementations to improve code clarity.
noise/perlin.v (8)
3-3: Correct grammatical error in function commentIn the comment for
perlin_2d, "return" should be "returns" for correct grammar.Apply this diff to correct the comment:
-// perlin_2d is a function that return a single value of perlin gen for a given 2d position +// perlin_2d is a function that returns a single value of Perlin noise for a given 2D position
28-28: Correct grammatical error in function commentIn the comment for
perlin_3d, "return" should be "returns" for correct grammar.Apply this diff to correct the comment:
-// perlin_3d is a function that return a single value of perlin gen for a given 3d position +// perlin_3d is a function that returns a single value of Perlin noise for a given 3D position
93-93: Correct grammatical error in function commentIn the comment for
lerp, "return" should be "returns" for correct grammar.Apply this diff to correct the comment:
-// lerp is a function that return a linear interpolation value for 2 given values and a factor +// lerp is a function that returns a linear interpolation value between two given values based on a factor
99-99: Correct grammatical error in function commentIn the comment for
fade, "return" should be "returns" for correct grammar.Apply this diff to correct the comment:
-// fade is a function that return a fade value for a given value +// fade is a function that returns a fade value for a given input
105-105: Correct grammatical error in function commentIn the comment for
perlin_grad_2d, "return" should be "returns" for correct grammar.Apply this diff to correct the comment:
-// perlin_grad_2d is a function that return a gradient value for a given hash and 2d position +// perlin_grad_2d is a function that returns a gradient value for a given hash and 2D position
72-90: Simplify duplicate cases inperlin_grad_3dmatch statementThere are duplicate return values in the
matchstatement ofperlin_grad_3d, which can be combined to reduce redundancy and improve readability.For example, you can combine cases that have the same return value:
fn perlin_grad_3d(hash int, x f64, y f64, z f64) f64 { match hash & 0xF { - 0x0 { return x + y } - 0x1 { return -x + y } - 0x2 { return x - y } - 0x3 { return -x - y } - 0x4 { return x + z } - 0x5 { return -x + z } - 0x6 { return x - z } - 0x7 { return -x - z } - 0x8 { return y + z } - 0x9 { return -y + z } - 0xA { return y - z } - 0xB { return -y - z } - 0xC { return y + x } - 0xD { return -y + z } - 0xE { return y - x } - 0xF { return -y - z } + 0x0, 0xC { return x + y } + 0x1 { return -x + y } + 0x2 { return x - y } + 0x3 { return -x - y } + 0x4 { return x + z } + 0x5 { return -x + z } + 0x6 { return x - z } + 0x7 { return -x - z } + 0x8 { return y + z } + 0x9, 0xD { return -y + z } + 0xA { return y - z } + 0xB, 0xF { return -y - z } + 0xE { return y - x } else { return 0 } } }
107-125: Simplify duplicate cases inperlin_grad_2dmatch statementSeveral cases in the
matchstatement ofperlin_grad_2dreturn the same value. Combining these cases can make the code cleaner.For example:
fn perlin_grad_2d(hash int, x f64, y f64) f64 { match hash & 0xF { - 0x0 { return x + y } - 0x1 { return -x + y } - 0x2 { return x - y } - 0x3 { return -x - y } - 0x4 { return x } - 0x5 { return -x } - 0x6 { return x } - 0x7 { return -x } - 0x8 { return y } - 0x9 { return -y } - 0xA { return y } - 0xB { return -y } - 0xC { return y + x } - 0xD { return -y } - 0xE { return y - x } - 0xF { return -y } + 0x0 { return x + y } + 0x1 { return -x + y } + 0x2 { return x - y } + 0x3 { return -x - y } + 0x4, 0x6 { return x } + 0x5, 0x7 { return -x } + 0x8, 0xA { return y } + 0x9, 0xB, 0xD, 0xF { return -y } + 0xC { return y + x } + 0xE { return y - x } else { return 0 } } }
40-55: Improve variable naming for clarity inperlin_3dfunctionThe variable names like
pxi_yi,pxi_yi1,aaa,aba, etc., can be hard to understand. Consider using more descriptive names to enhance readability.For example:
- pxi := gen.perm[xi] - pxi_yi := gen.perm[pxi + yi] - pxi_yi1 := gen.perm[pxi + yi + 1] - pxi1 := gen.perm[xi + 1] - pxi1_yi := gen.perm[pxi1 + yi] - pxi1_yi1 := gen.perm[pxi1 + yi + 1] + perm_xi := gen.perm[xi] + perm_xi_yi := gen.perm[perm_xi + yi] + perm_xi_yi1 := gen.perm[perm_xi + yi + 1] + perm_xi1 := gen.perm[xi + 1] + perm_xi1_yi := gen.perm[perm_xi1 + yi] + perm_xi1_yi1 := gen.perm[perm_xi1 + yi + 1] - aaa := gen.perm[pxi_yi + zi] - aba := gen.perm[pxi_yi1 + zi] - aab := gen.perm[pxi_yi + zi + 1] - abb := gen.perm[pxi_yi1 + zi + 1] - baa := gen.perm[pxi1_yi + zi] - bba := gen.perm[pxi1_yi1 + zi] - bab := gen.perm[pxi1_yi + zi + 1] - bbb := gen.perm[pxi1_yi1 + zi + 1] + hash_aaa := gen.perm[perm_xi_yi + zi] + hash_aba := gen.perm[perm_xi_yi1 + zi] + hash_aab := gen.perm[perm_xi_yi + zi + 1] + hash_abb := gen.perm[perm_xi_yi1 + zi + 1] + hash_baa := gen.perm[perm_xi1_yi + zi] + hash_bba := gen.perm[perm_xi1_yi1 + zi] + hash_bab := gen.perm[perm_xi1_yi + zi + 1] + hash_bbb := gen.perm[perm_xi1_yi1 + zi + 1]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
noise/noise.v(1 hunks)noise/perlin.v(1 hunks)noise/perlin2d.v(0 hunks)noise/perlin2d_test.v(0 hunks)noise/perlin3d.v(0 hunks)noise/perlin3d_test.v(0 hunks)noise/perlin_test.v(1 hunks)noise/simplex.v(16 hunks)noise/simplex_test.v(1 hunks)
💤 Files with no reviewable changes (4)
- noise/perlin2d.v
- noise/perlin2d_test.v
- noise/perlin3d.v
- noise/perlin3d_test.v
✅ Files skipped from review due to trivial changes (1)
- noise/simplex_test.v
🔇 Additional comments (5)
noise/noise.v (1)
17-18: LGTM! Parameter rename improves clarity
The change from generator to gen for the receiver parameter follows V's idiomatic conventions for receiver parameter naming. This makes the code more consistent with the rest of the codebase while maintaining clear readability.
noise/perlin_test.v (2)
1-5: LGTM! Module declaration and imports are well-structured.
The imports are appropriate for the testing requirements, and the module name follows the new organization structure.
1-22: Verify file organization and naming consistency.
Let's ensure this file organization aligns with the PR objectives and check for any remaining naming inconsistencies.
✅ Verification successful
File organization and naming patterns are consistent
The verification shows that:
- Test files follow a consistent organization pattern within the
noisemodule - Function naming follows a clear and consistent pattern (
perlin_2d,perlin_3d, etc.) - Implementation and test files are properly separated (
perlin.v,perlin_test.v) - No legacy files or inconsistent naming patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check file organization and naming patterns
# Check if old files still exist
echo "Checking for old file structure..."
fd "perlin_(2|3)d.*\.v$"
# Check for inconsistent naming patterns
echo "Checking for inconsistent function naming patterns..."
rg "perlin.*d|simplex.*d" --type v
# Check test file organization
echo "Checking test file organization..."
fd "test\.v$" -d 2
Length of output: 1109
Script:
#!/bin/bash
# Let's check the content of both noise test files and the implementation files
echo "Checking noise module test and implementation files..."
rg "fn.*perlin.*d|fn.*simplex.*d" -A 2
# Check for any potential old implementation files
echo "Checking for implementation files..."
fd "^(perlin|simplex).*\.v$" --exclude "*test.v"
Length of output: 3053
noise/simplex.v (2)
Line range hint 28-91: Well-structured gradient function renaming!
The addition of the 'simplex_' prefix to gradient functions improves clarity and maintains consistency with the module's naming conventions. The documentation and implementation remain clear and correct.
Line range hint 94-163: Consistent parameter renaming in 1D and 2D implementations.
The parameter renaming from 'generator' to 'gen' improves code conciseness while maintaining readability. The gradient function calls have been correctly updated to use the new names.
| fn test_perlin_3d() { | ||
| rand.seed([u32(3155200429), u32(3208395956)]) | ||
| mut gen := Generator.new() | ||
| gen.randomize() | ||
| result := gen.perlin_3d(0.125, 0.125, 0.125) | ||
| expected := 0.3713334855776509 | ||
| assert float64.tolerance(result, expected, 1.0e-6) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication and apply consistent improvements.
The test follows good practices but has duplicated setup code. Consider extracting common setup into a helper function.
+fn setup_generator() !Generator {
+ rand.seed([u32(3155200429), u32(3208395956)])
+ mut gen := Generator.new()
+ gen.randomize()
+ return gen
+}
+
fn test_perlin_3d() {
- rand.seed([u32(3155200429), u32(3208395956)])
- mut gen := Generator.new()
- gen.randomize()
+ mut gen := setup_generator()!
+ // Test case: Known value at (0.125, 0.125, 0.125)
+ // Expected value derived from reference implementation
result := gen.perlin_3d(0.125, 0.125, 0.125)
expected := 0.3713334855776509
- assert float64.tolerance(result, expected, 1.0e-6)
+ assert float64.tolerance(result, expected, tolerance)
+
+ // Additional test cases for edge conditions
+ edge_cases := [
+ (0.0, 0.0, 0.0),
+ (1.0, 1.0, 1.0),
+ (-0.5, 0.5, 0.0)
+ ]
+ for point in edge_cases {
+ _ := gen.perlin_3d(point.0, point.1, point.2) // Ensure no panics
+ }
}Committable suggestion skipped: line range outside the PR's diff.
This PR cleans up some naming issues that slipped through when I added Perlin and Simplex noise.
Some functions were named like noisetype_dimension, while others used noisetypedimensions without underscore. It got a bit confusing, so I’ve fixed everything to be consistent now.
I also combined the perlin_2d and perlin_3d files into a single perlin file to match how the Simplex code is organized. Same thing for the test files.
Summary by CodeRabbit
New Features
Bug Fixes
Chores