noise: perlin2d fix + 3d implementation #193
Conversation
WalkthroughThe Changes
Poem
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? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- noise/perlin2d.v (1 hunks)
- noise/perlin2d_test.v (1 hunks)
- noise/perlin3d.v (1 hunks)
- noise/perlin3d_test.v (1 hunks)
Additional comments: 14
noise/perlin2d_test.v (4)
6-12: The constants
single_perlinandsquare_perlinare used as expected values in tests. Ensure these values are correctly representing the expected output of theperlin2dfunction for the given inputs.14-28: The
test_perlin2dfunction seeds the random number generator and performs assertions to check the correctness of theperlin2dfunction. It's important to ensure that the seed used here is appropriate for the test and that the assertions cover a sufficient range of cases.18-18: The assertion for a single point test is correct, but it's important to verify that the expected value
single_perlinis accurate and that the tolerance1.0e-6is appropriate for the precision of theperlin2dfunction.21-27: The nested loops test a 3x3 grid of points. The test coverage is good, but it's crucial to verify that the expected values in
square_perlinare correct and that the chosen tolerance is suitable for the function's precision.noise/perlin3d_test.v (4)
5-23: The constants
single_perlin_3dandcube_perlin_3dare used as expected values in tests. It's important to ensure these values accurately represent the expected output of theperlin3dfunction for the given inputs.27-45: The
test_perlin3dfunction seeds the random number generator and performs assertions to check the correctness of theperlin3dfunction. It's important to ensure that the seed used here is appropriate for the test and that the assertions cover a sufficient range of cases.32-32: The assertion for a single point test is correct, but it's important to verify that the expected value
single_perlin_3dis accurate and that the tolerance1.0e-6is appropriate for the precision of theperlin3dfunction.35-44: The nested loops test a 3x3x3 grid of points. The test coverage is good, but it's crucial to verify that the expected values in
cube_perlin_3dare correct and that the chosen tolerance is suitable for the function's precision.noise/perlin3d.v (3)
3-32: The
perlin3dfunction implementation uses bitwise operations, linear interpolation, and gradient calculations to generate 3D Perlin noise. Ensure that the logic correctly implements the Perlin noise algorithm and that there are no off-by-one errors or incorrect assumptions about the input range.34-54: The
grad3dfunction calculates the gradient for a given hash and coordinates. Verify that the hash function and the gradient calculation are correctly implemented according to the Perlin noise algorithm.31-31: The
lerpfunction is used for linear interpolation. Ensure that this function is correctly implemented and used within the context of the Perlin noise algorithm.noise/perlin2d.v (3)
3-29: The
perlin2dfunction implementation uses bitwise operations, linear interpolation, and gradient calculations to generate 2D Perlin noise. Ensure that the logic correctly implements the Perlin noise algorithm and that there are no off-by-one errors or incorrect assumptions about the input range.35-55: The
grad2dfunction calculates the gradient for a given hash and coordinates. Verify that the hash function and the gradient calculation are correctly implemented according to the Perlin noise algorithm.31-31: The
lerpfunction is used for linear interpolation. Ensure that this function is correctly implemented and used within the context of the Perlin noise algorithm.
|
Just realized that I didn't make it random, actually the permutation set never change... |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- noise/perlin2d.v (1 hunks)
- noise/perlin2d_test.v (1 hunks)
- noise/perlin3d.v (1 hunks)
- noise/perlin3d_test.v (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- noise/perlin2d.v
- noise/perlin2d_test.v
- noise/perlin3d.v
- noise/perlin3d_test.v
|
Also run |
|
completely forgot sorry |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- noise/perlin2d.v (1 hunks)
- noise/perlin2d_test.v (1 hunks)
- noise/perlin3d.v (1 hunks)
- noise/perlin3d_test.v (1 hunks)
Additional comments: 5
noise/perlin2d_test.v (1)
- 8-13: The test case uses a fixed seed for reproducibility, which is good practice. However, ensure that the constant
single_perlinused in the assertion reflects a known correct value for the given seed and coordinates. If this value is arbitrary or not well-documented, it could lead to confusion or false positives/negatives in the future.noise/perlin3d_test.v (1)
- 8-13: Similar to the 2D test, the 3D test uses a fixed seed for reproducibility. It's important to ensure that the constant
single_perlin_3drepresents an expected correct value for the given seed and coordinates. Documentation or comments explaining how this value was determined would be beneficial for maintainability.noise/perlin3d.v (1)
- 33-53: The
grad3dfunction uses amatchstatement to determine the gradient based on the hash value. Ensure that the hash function provides a good distribution to avoid visual artifacts in the noise pattern. Additionally, theelsecase returns0, which could be a silent failure if an unexpected hash value is encountered. Consider logging or handling this case explicitly.noise/perlin2d.v (2)
520-529: The
Perlinstruct initializes the permutation table with a shuffled version of the default set. Therandomizemethod also reshuffles the table. Ensure that the shuffling provides the necessary randomness for the noise function. Additionally, consider documenting the fallback to the default permutation set in case of a shuffling error.555-583: The
fadeandlerpfunctions are utility functions used in noise calculation. Ensure that these functions are correctly implemented according to the Perlin noise algorithm. Thefadefunction, in particular, should have a comment explaining the magic numbers used in the polynomial for maintainability.
|
@ulises-jeremias I'm unsure what the linter errors are about 🤔 |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- noise/lookup.v (1 hunks)
- noise/perlin2d.v (1 hunks)
- noise/perlin3d.v (1 hunks)
Additional comments: 6
noise/perlin2d.v (3)
40-41: The
fadefunction is correct and follows the Perlin noise algorithm.44-64: The
grad2dfunction uses a match statement to return gradient values. The cases are exhaustive and correct.66-67: The
lerpfunction correctly implements linear interpolation, which is a key part of the Perlin noise algorithm.noise/perlin3d.v (2)
3-38: The
perlin3dfunction correctly implements the 3D Perlin noise algorithm. The permutation table access pattern is optimized following the previous suggestions.41-61: The
grad3dfunction is correct and follows the Perlin noise algorithm for 3D space.noise/lookup.v (1)
- 3-516: The permutation table is a predefined set of values used in the Perlin noise algorithm. It appears to be correctly defined and matches the typical Perlin noise implementation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- noise/perlin2d.v (1 hunks)
- noise/perlin3d.v (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- noise/perlin2d.v
- noise/perlin3d.v
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- noise/perlin2d.v (1 hunks)
Additional comments: 3
noise/perlin2d.v (3)
41-42: The
fadefunction is correctly implemented as per the Perlin noise algorithm.45-65: The
grad2dfunction is correctly implemented and matches the expected behavior for gradient computation in Perlin noise.67-68: The
lerpfunction is a standard linear interpolation function and is correctly implemented.
| pub struct Perlin { | ||
| mut: | ||
| perm []int = rand.shuffle_clone(permutations) or { panic(err) } |
There was a problem hiding this comment.
Initialization of perm array within the Perlin struct using rand.shuffle_clone without a seed may lead to non-random behavior as mentioned in the PR comments.
- perm []int = rand.shuffle_clone(permutations) or { panic(err) }
+ perm []intConsider initializing the perm array in a separate method where a seed can be provided to ensure proper randomization.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub struct Perlin { | |
| mut: | |
| perm []int = rand.shuffle_clone(permutations) or { panic(err) } | |
| pub struct Perlin { | |
| mut: | |
| perm []int |
| // randomize is a function that shuffle the permutation set inside the Perlin struct | ||
| // will not shuffle if rand.seed is not changed | ||
| pub fn (mut perlin Perlin) randomize() { | ||
| perlin.perm = rand.shuffle_clone(permutations) or { panic(err) } | ||
| } |
There was a problem hiding this comment.
The randomize method relies on an external seed change for rand. It would be more reliable to allow an optional seed parameter for explicit control.
- pub fn (mut perlin Perlin) randomize() {
+ pub fn (mut perlin Perlin) randomize(seed int) {
+ rand.seed(seed)
perlin.perm = rand.shuffle_clone(permutations) or { panic(err) }
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // randomize is a function that shuffle the permutation set inside the Perlin struct | |
| // will not shuffle if rand.seed is not changed | |
| pub fn (mut perlin Perlin) randomize() { | |
| perlin.perm = rand.shuffle_clone(permutations) or { panic(err) } | |
| } | |
| // randomize is a function that shuffle the permutation set inside the Perlin struct | |
| // will not shuffle if rand.seed is not changed | |
| pub fn (mut perlin Perlin) randomize(seed int) { | |
| rand.seed(seed) | |
| perlin.perm = rand.shuffle_clone(permutations) or { panic(err) } | |
| } |
| // perlin2d is a function that return a single value of perlin noise for a given 2d position | ||
| pub fn (perlin Perlin) perlin2d(x f64, y f64) f64 { | ||
| xi := int(x) & 0xFF | ||
| yi := int(y) & 0xFF | ||
|
|
||
| xf := x - int(x) | ||
| yf := y - int(y) | ||
|
|
||
| u := fade(xf) | ||
| v := fade(yf) | ||
|
|
||
| pxi := perlin.perm[xi] | ||
| pxi1 := perlin.perm[xi + 1] | ||
|
|
||
| aa := perlin.perm[pxi + yi] | ||
| ab := perlin.perm[pxi + yi + 1] | ||
| ba := perlin.perm[pxi1 + yi] | ||
| bb := perlin.perm[pxi1 + yi + 1] | ||
|
|
||
| x1 := lerp(grad2d(aa, xf, yf), grad2d(ba, xf - 1, yf), u) | ||
| x2 := lerp(grad2d(ab, xf, yf - 1), grad2d(bb, xf - 1, yf - 1), u) | ||
|
|
||
| return (lerp(x1, x2, v) + 1) / 2 |
There was a problem hiding this comment.
The perlin2d function accesses the perm array multiple times which could be optimized by caching the relevant values.
+ let pxi_perm = perlin.perm[pxi]
+ let pxi1_perm = perlin.perm[pxi1]
- aa := perlin.perm[pxi + yi]
- ab := perlin.perm[pxi + yi + 1]
- ba := perlin.perm[pxi1 + yi]
- bb := perlin.perm[pxi1 + yi + 1]
+ aa := pxi_perm + yi
+ ab := pxi_perm + yi + 1
+ ba := pxi1_perm + yi
+ bb := pxi1_perm + yi + 1Consider caching the perm[xi] and perm[xi + 1] values to reduce array access and potentially improve performance.
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // perlin2d is a function that return a single value of perlin noise for a given 2d position | |
| pub fn (perlin Perlin) perlin2d(x f64, y f64) f64 { | |
| xi := int(x) & 0xFF | |
| yi := int(y) & 0xFF | |
| xf := x - int(x) | |
| yf := y - int(y) | |
| u := fade(xf) | |
| v := fade(yf) | |
| pxi := perlin.perm[xi] | |
| pxi1 := perlin.perm[xi + 1] | |
| aa := perlin.perm[pxi + yi] | |
| ab := perlin.perm[pxi + yi + 1] | |
| ba := perlin.perm[pxi1 + yi] | |
| bb := perlin.perm[pxi1 + yi + 1] | |
| x1 := lerp(grad2d(aa, xf, yf), grad2d(ba, xf - 1, yf), u) | |
| x2 := lerp(grad2d(ab, xf, yf - 1), grad2d(bb, xf - 1, yf - 1), u) | |
| return (lerp(x1, x2, v) + 1) / 2 | |
| // perlin2d is a function that return a single value of perlin noise for a given 2d position | |
| pub fn (perlin Perlin) perlin2d(x f64, y f64) f64 { | |
| xi := int(x) & 0xFF | |
| yi := int(y) & 0xFF | |
| xf := x - int(x) | |
| yf := y - int(y) | |
| u := fade(xf) | |
| v := fade(yf) | |
| pxi := perlin.perm[xi] | |
| pxi1 := perlin.perm[xi + 1] | |
| let pxi_perm = perlin.perm[pxi] | |
| let pxi1_perm = perlin.perm[pxi1] | |
| aa := pxi_perm + yi | |
| ab := pxi_perm + yi + 1 | |
| ba := pxi1_perm + yi | |
| bb := pxi1_perm + yi + 1 | |
| x1 := lerp(grad2d(aa, xf, yf), grad2d(ba, xf - 1, yf), u) | |
| x2 := lerp(grad2d(ab, xf, yf - 1), grad2d(bb, xf - 1, yf - 1), u) | |
| return (lerp(x1, x2, v) + 1) / 2 |
|
I think it's good like this, I can add some examples later if needed. |
Fixed 2d perlin noise and added 3d function too !
using this code, the 2d perlin noise look like this:
and using this code and by increasing depth we have this 3d perlin noise:
Summary by CodeRabbit