godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.37k stars 20.24k forks source link

OpenSimplexNoise octave count should not be limited to 6 #28714

Closed Zylann closed 4 years ago

Zylann commented 5 years ago

Godot master

OpenSimplexNoise currently can't have more than 6 octaves, and it's hardcoded: https://github.com/godotengine/godot/blob/master/modules/opensimplex/open_simplex_noise.cpp#L72 And in fact, it's always initializing 6 octaves no matter what number you set.

When experimenting a large voxel terrain, I quickly noticed over 6 octaves had no effect. I see no reason for this limitation, so can we remove it? (or at least increase it way above that)

clayjohn commented 5 years ago

@JFonS Any reason why this number is clamped at six?

More than 6 octaves is going to be rather slow to compute, but maybe instead of a hard cap we just add an editor warning?

slapin commented 5 years ago

yeah, can't see any reason to limit octaves to 6, probably there is some bug which is hidden by this setting. "slow" in this case is relative, and I suggest no warning here, No need to babysit user so much. When I implement the same algorithm myself using simple 2d noise library, I can go as far as 16 octaves without any problems.

Xrayez commented 5 years ago

More than 6 octaves is going to be rather slow to compute

True, this is not suitable for run-time needs but for things like the editor plugins there should be no limitation, though very high octaves can easily freeze any application for too long.

JFonS commented 5 years ago

I had to limit the amount of octaves since we are storing the context of then in an array: https://github.com/godotengine/godot/blob/87ee2a9239928e2811ce1711bc381f601edd278a/modules/opensimplex/open_simplex_noise.h#L44

and 6 octaves seemed like a reasonable limit. If you think it's not enough feel free to increase it, but then we should consider dynamically allocating the contexts.

slapin commented 5 years ago

You can also want to generate high octave count noise on startup displaying nice progress bar.

On Mon, May 6, 2019 at 11:36 AM JFonS notifications@github.com wrote:

I had to limit the amount of octaves since we are storing the context of then in an array:

https://github.com/godotengine/godot/blob/87ee2a9239928e2811ce1711bc381f601edd278a/modules/opensimplex/open_simplex_noise.h#L44

and 6 octaves seemed like a reasonable limit. If you think it's not enough feel free to increase it, but then we should consider dynamically allocating the contexts.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/godotengine/godot/issues/28714#issuecomment-489547370, or mute the thread https://github.com/notifications/unsubscribe-auth/AAABPUYM4T6T3W4EYPFCD2DPT7UY3ANCNFSM4HK4MJKQ .

Zylann commented 5 years ago

Dynamically allocating a context shouldnt be that heavy in the end, since each one weights about 512 bytes, which is half the size of an empty Button node (1208 bytes). Once created, it's not going to be destroyed and recreated often (if anytime). What could be slow would be generating a NoiseTexture only if you set too many octaves, which can actually be clamped on the fly if (period >> (octave_count - 1)) == 0, since units are pixels and generating under a unit wouldn't contribute to the result (while it could matter for other usages). In my case, I had 3 more octaves to go :p