godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.15k stars 97 forks source link

Implement float Image formats for Noise/NoiseTexture, or document the difference between get_noise and get_image data #6161

Open lewiji opened 1 year ago

lewiji commented 1 year ago

Describe the project you are working on

A Godot 4 (beta 14) project using FastNoiseLite for mesh vertex shader displacement and collision shape generation (ie terrain heightmap).

Describe the problem or limitation you are having in your project

Using the get_noise* functions to create collision shape data, and the get_image function or NoiseTexture2D resource to upload the noise to a shader sampler uniform results in different enough values to give the collision shape a significant and inconsistent offset from the mesh vertex displacement.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I believe this is due to the FastNoiseLite library returning noise values as 32 bit floats, but the image generation in Noise normalises and casts the noise values to uint8 values for the L8 or RGBA8 image formats that Noise can output.

This can be worked around by making the decision to consistently use only the image sampling methods (shader or get_pixel methods) over the get_noise* methods, at the cost of some precision, or perhaps by uploading a PackedFloatArray of raw noise data to the shader instead of a texture.

The Noise get_image function supports using L8 instead of RGBA if the noise is grayscale as a memory optimisation, but it has no option to have the outputted image format match the actual high precision values of the noise itself, which I think would be the general expectation when using these features.

It's not obvious to the general user that using the Image/Texture would give different values to the get_noise* functions. Though Godot, in general, uses 8 bit textures across the board, and the get_noise* functions return floats, it makes sense when you stop to think about it (and/or read the C++ source like I had to, as my thinking didn't work very well), it's not totally obvious that this conversion takes place and that the values would be so different.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

At the very least this should probably be more clearly documented across all the docs about Noise related classes and functions, as mixed precision issues like this are very easy to get caught out by and can cause bugs that may be difficult to discover and pinpoint.

I noticed that the PR that implemented the changes to the Noise api, including introducing FastNoiseLite, the author mentioned "Allow float texture formats (FORMAT_RF, FORMAT_RGBAF)" as an optional todo, so I think this issue was probably on the radar already but hasn't seen significant interest or bugs yet to make it worthwhile.

Another option might be to have a new class property or function parameter to limit the precision of the get_noise functions to match the normalised, uint8 image data if high precision isn't needed, or the memory/bandwidth optimisations are more important.

If this enhancement will not be used often, can it be worked around with a few lines of script?

The raw data of the Noise Image isn't exposed to the user prior to the conversion, so you would have to iterate in a scripting language over every desired point in the noise to create an Image from scratch, which would largely defeat the point of this being a highly optimised noise generator.

Potentially, the reverse conversion from 32 bit float values returned by get_noise* to normalised 8 bit int values could be reimplimented on the user scripting side, but it wouldn't be very ergonomic and may be prone to further errors.

Is there a reason why this should be core and not an add-on in the asset library?

The refactored Noise code allows for the get_image function to be overridden by specific noise generators for this kind of purpose, and the FastNoiseLite support is built into the core engine code.

Calinou commented 1 year ago

GradientTexture has a use_hdr property to use RGBAH format instead of RGBA8. I believe the same property could be added to NoiseTexture.

That said, if you also want RGBAF, this would require exposing a format enum instead with 3 values ("8-bit integer", "16-bit floating-point", "32-bit floating-point").

lewiji commented 1 year ago

@Calinou I guess it might depend on the noise generator - for example, using FastNoiseLite, I can't see a reason why you'd want 16 bit images if you're getting 32 bit values, but maybe there's a use case for it. Other noise generators may use 16 bit noise values internally for whatever reason, I suppose. So maybe it should be up to the noise generator implementations to specify what format(s) they support internally, enabling more options, there'd be no point letting the user choose 32 bit images if the generator uses 8 bit values internally, though I suppose it's easier to implement that way and down to the user to make the right choice (or else leave the sensible default of 8 bit).

Edit: though I do feel like the default should be that the precision of the image data matches the precision of the noise data, one way or the other. Perhaps it should default to limiting both to 8 bit values, unless some kind of high precision tickbox is checked, then it makes both the noise and image use the maximum available precision from the library?