-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: [core] Add a unidirectional Perlin Ground HeightmapFunction #799
base: dev
Are you sure you want to change the base?
Conversation
What do you mean ? |
To be able to get this type of terrain: |
I see. It looks like a quantisation based on the vertical value only. Something like I think |
Are you planning to work on it ? If not, then I suggest closing this issue because I'm not planning to work on it either. |
Yes, I did a bit more work on it but didn’t push anything yet. I’m planning to work on/finish it in a couple of weeks. |
Ok perfect ! Let's keep it open then :) |
5167e9f
to
75575c6
Compare
Apparently you made good progress ! Nice :) What is your future plan ? You want to add quantisation ? Can you add a unit test checking if the gradient is right ? |
The problem is not that it asserts to false because asserts are disabled for release build type. The issue is rather the contrary, we should systematically check this condition and raise an exception if not satisfying this condition causes segfault. I will issue a fix for this. |
Yes, I still need to do :
|
Nope it is fine :)
I'm afraid doing this is not straightforward. It has been a long-lasting issue as you know. Once everything else is working, I could help you on this part to improve the current heuristic that has not sufficient in many cases. |
return (yRight - yLeft) * dRatio + lerp(ratio, yLeft / dtLeft, yRight / dtRight); | ||
} | ||
|
||
double AbstractPerlinNoiseOctave::operator()(double x, double y) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why do you need all. 2D Perlin process is not the same as the sum of 1D processes along x-axis and y-axis respectively ? If so, just do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now I see the issue. It is indeed more tricky than expected at first glance. I'm going to think about it to see if there is a way to get the same result without duplicating everything. The big issue is that the contribution of each process independent on each axis.
I'm wondering if passing the 2D position encoding (hash) to two independent random processes should do the trick. I will try that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After check, it is indeed more tricky than that and you really need a 2D implementation. Position encoding is not enough per se. I forgot about random Perlin processes were evaluated in the first place. Since it is an interpolation, it is always necessary to have a dedicated 2D implementation.
|
||
template<typename PerlinProcessType> | ||
void computePerlinHeightmap(PerlinProcessType perlinProcess, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make this signature more general to support specifying pairs (axis, PerlinProcess)
, this way:
template<typename PerlinProcessType, int N>
void computePerlinHeightmap(std::array<std::pair<Eigen::Vector3, PerlinProcessType>, N> perlinProcesses,
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then all you have to do is to project the current pose only each axes and aggregate the result. You can do this automatically using sumHeightmaps
, which has been designed for this exact purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like shown in the photo above, I don't think that's what we want.
To many code duplication. There should be no need to implement anything specific for the 2D scenario. |
I'm going to help you on this, so that we can close this long-lasting PR. |
If you have time for it, I'm pretty happy to get your help. I didn't have much time to work on it the last few weeks, but hoping to finish it this week. |
|
||
// Constant for avoiding discontinuity at (0.0, 0.0) | ||
const double offset = 100.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a non-ideal fix. Since the 1d perlin process is defined only on t >0, I needed a way to have it well defined for t<0 as well. Using the absolute value of t doesn't work since you end up with a discontinuity at t=0. So instead I shifted the perlin process by a offset. But it doesn't change the fact that the perlin process remains undefined for t < offset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I will check if it can be refactored to work with negative values.
{ | ||
// Compute random gradient | ||
// Get knot | ||
uint64_t knot = (static_cast<uint64_t>(ix) << 32) | static_cast<uint32_t>(iy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced by this. The periodicity is supposed to have the spatial interpretation in physical world. A repeating pattern is supposed to be clearly visible. I'm wondering what it looks like in this case. In the case of periodic Perlin noise, I have the feeling that sumHeightmaps
is the way to go. Anyway, I don't think it really makes sense for ground profile generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option would be to do:
uint32_t deltaPeriod = static_cast<uint32_t>(period_ / wavelength_)
ix %= deltaPeriod;
iy %= deltaPeriod;
uint64_t knot = (static_cast<uint64_t>(ix) << 32) | static_cast<uint32_t>(iy);
const double s = perm_[knot] / 256.0;
By the way, I think there was an issue in the 1D case if deltaPeriod > 256
Ok I skimmed through the code. Now I remember. Since those processes at not used for anything inside core itself, I'm going to refactor them a bit. In particular to replace |
Since you're taking over for the PerlinNoise part, I started creating some unit tests (using the current methods' name) and fixed a typo in the 1D gradient computation. There is also a bug in the 2D gradient computation. |
I am proposing to add a unidirection Perlin ground
HeightmapFunction
, 2D Perlin groundHeightmapFunction
and a quantization methods to quantize any ground to certain z-values.