Integer samplers in shaders don't seem to provide integers representing the integer values set in the texture

[andrew-wilkes]

Godot version

v3.4.2.stable.arch_linux

System information

Linux, GLES3

Issue description

This link provides context and a workaround.

Values are expected to be passed as 32 bit integers that directly map to the RGBA values of a texture, but it seems that floating point values are passed and encoded into an integer.

Steps to reproduce

Create an image from data and apply its texture to a shader like so:
img.create_from_data(num_samples, 1, false, Image.FORMAT_RGBA8, rgba) var texture = ImageTexture.new() texture.create_from_image(img, 0) material.set_shader_param("traces", texture)

In the shader, have:
shader_type canvas_item; uniform usampler2D traces;

Minimal reproduction project

No response

[Zylann]

Still happens in Godot 3.5 rc8, and also happens in Godot 4.0.
Windows 10 64 bits GLES3 GeForce GTX 1060 6GB/PCIe/SSE2

I use a texture to store data and did not want to use regular float sampler2D because normalization is prone to precision mistakes and was making the shader more complicated.

Reproduced with a shader like this on icon.png:

shader_type canvas_item;

uniform usampler2D u_texture;

void fragment() {
	uvec4 col = texelFetch(u_texture, ivec2(UV*64.0), 0);
	float r = uintBitsToFloat(col.r);
	COLOR = vec4(r, 0.0, 0.0, 1.0);
}

This is not the expected result.

[viktor-ferenczi]

This bug is a pain while working with voxel data (lots of integers) in fragment shaders. Always have to do the uint(... * 255) dance which is inefficient and looks fishy (any rounding errors possible?).

[hw762]

After some discussion on the dev-channel, we found that Godot always binds texture as UNORM format. However, isampler2D and usampler2D should probably bind SINT and UINT format instead.

BTW, issue #69880 contains a minimal project for reproduction.

[hw762]

In the current API, the texture format is determined when texture is initialized (which is most likely UNORM). For integer samplers to work, there must be a way to:

1. Specify that the texture is integer valued
2. Or create the texture in the desired format only when binding happens (so Godot knows exactly what format to expect)
3. Or allow multiple formats to be specified on the same pixel data

[hw762]

With further discussion, I propose that Texture classes should provide an option to choose between UNORM, SINT, UINT, or any other on-GPU format. The reasoning are of following:

1. Godot currently determines on-GPU format solely by the pixel format of a texture's underlying Image data
2. There is not an easy way for Godot to determine whether a texture is bound to a sampler or isampler/usampler
3. Texture objects are quite lightweight, because underlying data is shared
4. The developer most definitely knows whether a texture should be normalized float or integer, because this person writes the shader too

This means, the developer can create an Image to store data, then have a Texture with UNORM representation for a sampler, and one with UINT for usampler. Data is shared, nothing is duplicated except for a texture handle on GPU.

Now technically, I think, that Vulkan might duplicate the texture on GPU. However, I'd say correctness first.

[otonashixav]

I think this still happens in 4.0.beta16. Not 100% certain since shaders are new to me, though.

My use case is passing a 3d array of ids corresponding to the items in a gridmap as a usampler3D, which seems to give me the wrong values.

What I'm doing that seems to work as a workaround is using Image.FORMAT_RF to store the integers, using sampler3D as the uniform, reading .r to retrieve 32 bits, and reinterpreting the bits as ints with floatBitsToUint to retrieve my ids. This doesn't waste any bytes (assuming everything packs nicely into 4n bytes) and doesn't lose any precision. I'm not sure whether this works as expected on all devices though.

[LiBooks]

I think this still happens in 4.0.2. With further discussion, I propose that Texture classes should provide an option to choose between UNORM, SINT, UINT, or any other on-GPU format,too.

[winston-yallow]

I'm wondering why the proposed solution is to set this on the texture? Usually the format is defined in the image, like when you want one red 32 bit float channel you use FORMAT_RF. Why not add other image formats, like FORMAT_RI to specify it should store integers? From a a user perspective this seems more coherent than having two different places to define the format.

[mcardellg]

I think this still happens in 4.0.beta16. Not 100% certain since shaders are new to me, though.

My use case is passing a 3d array of ids corresponding to the items in a gridmap as a usampler3D, which seems to give me the wrong values.

What I'm doing that seems to work as a workaround is using Image.FORMAT_RF to store the integers, using sampler3D as the uniform, reading .r to retrieve 32 bits, and reinterpreting the bits as ints with floatBitsToUint to retrieve my ids. This doesn't waste any bytes (assuming everything packs nicely into 4n bytes) and doesn't lose any precision. I'm not sure whether this works as expected on all devices though.

I landed here looking for a way to pass uInts in to shaders with accuracy, I can confirm otonashixav 's work around appears to work (many thanks!), for any others landing here this is what the final code looks like (pseudo code):

GD Script:

	var pba : PackedByteArray = []
	pba.resize(4) # 4 bytes for 32 bits uInt
	# Change 32 bit into 4 x 8 bit, this goes into the shader as a sampler2D with FORMAT_RF (32bit float), floatBitsToUint then converts back to uint in the shader
	pba.encode_u32(0, 123456)
	var img := Image.create_from_data(1, 1, false, Image.FORMAT_RF, pba)
	var tex := ImageTexture.create_from_image(img) # Pass this in to a sampler2D in the shader

Shader Code:

	uint value = floatBitsToUint(texelFetch(mysampler, ivec2(0, 0), 0).r);

Note that there also exists encode_s32() (signed 32bit) and an equivalent floatBitsToInt().

[TokisanGames]

Building on previous comments, I found I needed additional elements. This is a complete listing of code for 4.1.2, C++ and GDScript.

To recap the issue: We want to store integers in an integer format texture, and in the shader read integers, without any conversion. The solution is for Image and Texture to support standard opengl integer texture formats. The RenderingDevice already does.

As a workaround, we can now store integers in a float format texture, then in the shader directly read this as an integer texture. The conversion is done in C++ and is inconsequential, it just reinterprets memory differently or shifts some bits around at worse. There's no recalculation of the significand and exponent, so no precision loss.

Creation

encode_u32 requires the byte offset into the PackedByteArray you are inserting. So when filling the array with uint32s multiply the index value by 4 (i.e. sizeof(uint32_t)).

//GDExtension C++
uint32_t value = 12345;
int width = 1024, height = 1024;
PackedByteArray pba;
pba.resize(width * height * sizeof(uint32_t));
for (int x = 0; x < width; x++) {
	for (int y = 0; y < height; y++) {
		pba.encode_u32((y * width + x) * sizeof(uint32_t), value);
	}
}
Ref<Image> new_img = Image::create_from_data(width, height, false, Image::FORMAT_RF, pba);

#GDScript
var value: int = 12345
var width: int = 1024
var height: int = 1024
var pba : PackedByteArray = []
pba.resize(width * height * 4) # 4 bytes in 32 bit uint
for x in width:
	for y in height:
		pba.encode_u32((y*width + x) * 4, value)
var img: Image = Image.create_from_data(width, height, false, Image.FORMAT_RF, pba)
var tex: ImageTexture = ImageTexture.create_from_image(img)

Per pixel reading/writing

After creation, if you wish to write this data on the CPU one pixel at a time, do not use the PackedByteArray. It works, but it is copy on write and tanked my performance as soon as I wrote to it for terrain painting, reducing down to 2-3s per click.
That looks something like this:

// don't do this
PackedByteArray pba = img->get_data()
uint32_t value = pba.decode_u32(index);
value = 54321;
pba.encode_u32(index, value);  //copy on write
img->set_data(width, height, false, Image::FORMAT_RF, pba);

Instead, since we can't write to the data container directly without triggering a copy, we'll have to use Image.set_pixel() and Image.get_pixel() and work with Color. We can undo the exact same conversions Color uses for FORMAT_RF.

//GDExtension C++
uint32_t i = 12345;
float r = *(float *)&i;
img->set_pixel(0, 0, Color(r, 0.f, 0.f, 1.0f));

i = 0; 
r = 0.f;

r = img->get_pixel(0, 0).r;
*(float *)&i = r;
UtilityFunctions::print("i: ", i, ", r: ", r, ", pix: ", img->get_pixel(0, 0));

// Output:
i: 12345, r: 0, pix: (0, 0, 0, 1)

#GDScript
var i: int = 12345
var pba: PackedByteArray
pba.resize(4)
pba.encode_u32(0, i)
var r: float = pba.decode_float(0)
var img: Image = Image.create(1, 1, false, Image.FORMAT_RF)
img.set_pixel(0, 0, Color(r, 0., 0., 1.0));

pba.clear()
pba.resize(4)
i = 0
r = 0.0

r = img.get_pixel(0, 0).r;
pba.encode_float(0, r)
i = pba.decode_u32(0)
print("i: ", i, ", r: ", r, ", pix: ", img.get_pixel(0, 0));

# Output:
i: 12345, r: 0, pix: (0, 0, 0, 1)

Printing the value of the pixel or color.r is useless as the number is not a valid float. It prints 0, but as shown by i, there actually is a non-zero, non-float value. Looking at it in a debugger MSVC reports it as #DEN (a denormalized number, too close to zero).

Shader

At least in Godot 4.1.2, the shader doesn't need any special conversion. Comments above show using sampler2D on the uniform to read pixels as floats, then converting with floatBitsToUint. However, we can interpret the FORMAT_RF texture as int directly.

uniform usampler2D mysampler;
uint value = texelFetch(mysampler, ivec2(0, 0), 0).r;

uniform usampler2DArray mysamplerarr;
uint value = texelFetch(mysamplerarr, ivec3(0, 0, 0), 0).r;

[viktor-ferenczi]

Do we have examples of passing various integer types via texture in the shader documentation?

[sammonius]

Using this instead of manipulating PackedByteArray's makes the hack above slightly easier:

my_image = Image.create_from_data(w, h, false, Image.FORMAT_RF, my_image.get_data())

[TV4Fun]

I have been taking a stab at trying to fix this, but the way image formatting is currently implemented on the rendering device is making this tricky. Data formats are implemented in one giant enum in RenderingDeviceCommons::DataFormat, which presumably map directly to their equivalent internal formats in OpenGL, Vulkan, and DX12. At least in a few places, like in RD_TO_VK_FORMAT or in RenderingDeviceDriverD3D12::RD_TO_D3D12_FORMAT, the values are hard-coded in to assume they are all in a single linear array in that order. Most of the actual determination of on-device texture format is made in RendererRD::TextureStorage::_validate_texture_format for RenderingDevice-based drivers, or in GLES3::TextureStorage::_get_gl_image_and_format for GLES3. Both of which are giant switch statements that are already difficult to read and maintain, and which make the decision of what type of on-device storage to use based solely on the format of the input image, which makes no sense.

What would make the most sense would be to allow separate specification of data layout (R8, RG8, RGB8, etc.) and data type (UNORM, UINT, SINT, etc.), similar to what OpenGL and other APIs already allow for specifying input format. We could add additional switch statements to each case for each format to _validate_texture_format and _get_gl_image_and_format, but this would make them even more complex. A simple array lookup table from an image format and device data-type table would work for the simpler cases, but in more complex cases where what format to chooses depends on device and driver capabilities, like FORMAT_RGB8, we would still need logic to check these. It is also unclear to me why some of the internal formats are chosen the way they are. For example, I have no idea why _validate_texture_format maps Image::FORMAT_RGBA4444 to RD::DATA_FORMAT_B4G4R4A4_UNORM_PACK16 instead of RD::DATA_FORMAT_R4G4B4A4_UNORM_PACK16, but I'm sure there's probably a good reason, and I am not comfortable enough with all the ins and outs of device texture formatting to try completely rewriting it myself.

As a stopgap, we could add a RenderingDevice::DataFormat member to Texture and pass it to all of the various texture_*_create/initialize methods to override whatever internal format the logic above selects. This would also be a major refactor to touch every piece of code that uses this. Even if we specify default values for compatibility with outside code, we would want all of our Texture subclasses to make use of this new member. It would also raise a lot of issues around validating if the selected type is valid for the input format, the driver, and the device being run on, which would mean a lot more validation code, potentially requiring an even larger refactor than what I proposed above.

I would encourage the Godot devs to not let this issue drop. It is still a major usability problem as of 4.3, and while the workarounds above do work in many cases, they add overhead and in some cases may result in a loss of precision. The current method of selecting internal format carries a large amount of tech debt that is only going to get harder to fix and maintain as time passes and more capabilities are added to the engine.

The Godot dev team will save themselves a lot of headaches in the long run if they take the time to properly refactor this now. I am happy to contribute to this, but it is more than I can do single-handed.

[TokisanGames]

while the workarounds above do work in many cases, they add overhead and in some cases may result in a loss of precision.

My solution has no overhead or precision loss in Terrain3D (C++). We have exposed the conversion functions that eliminate the overhead in GDScript as well. We're reading uints directly in the shader without issue.

The only issue I'd like to see resolved is that Image and Texture don't support uint formats, but since the shader already reinterprets the memory, it's more of a convenience factor at this point.

[TV4Fun]

@TokisanGames Your solution works if your inputs are 32 bit integers. It wouldn't work for 8 or 16-bit integers. There is still a concern of precision loss in converting an 8-bit int to an 8-bit UNORM, as well as the overhead in float conversion, division, and multiplication.

[TV4Fun]

(BTW @TokisanGames, I believe you can save the extra overhead of copying the PackedByteArray if instead of calling img->get_data() and writing to that, you keep a local copy of the PackedByteArray you passed, write changes to that, and call img->set_data() with the same array. This might be a little faster, as this avoids passing multiple unused float arguments, and the PackedByteArray is passed by reference, so there won't be any extra copies made.)

[TokisanGames]

Thanks for the suggestion, but we don't use PackedByteArray. That was an example for completeness. We use the example where we directly write ints into the Image's float memory with set_pixel rather than construct a buffer.

We then let the shader read that texture as int memory. We also expose these functions to GDScript.

You're right about 32-bit. FORMAT_RF-FORMAT_RGBAF are the only formats that bypass any conversion. Not only do Image and Texture need int formats, they need more unfiltered input functions to support writing the appropriate type.