Fix integer value for GL_MAX_UNIFORM_BLOCK_SIZE overflowing

[adamscott]

This PR fixes the issue of shader compilation on the web platform.

It happens that the following line (on my PC, at least) makes the int Config::max_uniform_buffer_size to overflow to -2147483648, which is exactly INT_MIN (or INT_MAX + 1).

godot/drivers/gles3/storage/config.cpp

Line 94 in 6758a7f

glGetIntegerv(GL_MAX_UNIFORM_BLOCK_SIZE, &max_uniform_buffer_size);

Then, int Config::max_renderable_lights (defaults to 32) is set to the minimum value of itself or our Config::max_uniform_buffer_size, which we remember is -2147483648. Then the same thing happens for int Config::max_lights_per_object (defaults to 8).

godot/drivers/gles3/rasterizer_scene_gles3.cpp

Lines 2626 to 2627 in 6758a7f

	config->max_renderable_lights = MIN(config->max_renderable_lights, config->max_uniform_buffer_size / (int)sizeof(RasterizerSceneGLES3::LightData));
	config->max_lights_per_object = MIN(config->max_lights_per_object, config->max_renderable_lights);

This makes two scene.glsl "#defines" with INT_MIN value. It's pretty easy to understand then why the shader can't compile.

godot/drivers/gles3/rasterizer_scene_gles3.cpp

Lines 2664 to 2673 in 6758a7f

	{
	String global_defines;
	global_defines += "#define MAX_GLOBAL_SHADER_UNIFORMS 256\n"; // TODO: this is arbitrary for now
	global_defines += "\n#define MAX_LIGHT_DATA_STRUCTS " + itos(config->max_renderable_lights) + "\n";
	global_defines += "\n#define MAX_DIRECTIONAL_LIGHT_DATA_STRUCTS " + itos(MAX_DIRECTIONAL_LIGHTS) + "\n";
	global_defines += "\n#define MAX_FORWARD_LIGHTS uint(" + itos(config->max_lights_per_object) + ")\n";
	material_storage->shaders.scene_shader.initialize(global_defines);
	scene_globals.shader_default_version = material_storage->shaders.scene_shader.version_create();
	material_storage->shaders.scene_shader.version_bind_shader(scene_globals.shader_default_version, SceneShaderGLES3::MODE_COLOR);
	}

My PR is quite simple. If the value of Config::max_uniform_buffer_size is negative, it means that it overflowed. So INT_MAX is set as the value.

Edit: used glGetInteger64v() and int64_t for Config instead.

Fixes #68180

[bitsawer]

Interesting bug. I found possibly related MESA PR's and issues as at least the original bug report happens when on web and Linux MESA. https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/18512 and https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/922

[lawnjelly]

This is indeed strange bug:
https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glGet.xhtml

There is also a glGetInteger64v(), should we be using that version? I'm not entirely clear when to use glGetIntegerv() or the 64 bit version (i.e. which parameters could overflow). 🤷‍♂️

Are there any GL errors being flagged? (i.e. glGetError or similar)
It does seem suspiciously like it is trying to return an error rather than out of range, given the problem with max_renderable_lights and max_lights_per_object, in which case setting the value to some maximum might not be a good idea (as the true value may be lower than this).

It might be an idea to set all these to the minimum value in the spec if even the 64 bit version returns a negative value, unless we can figure out what is going on.

[adamscott]

Interesting bug. I found possibly related MESA PR's and issues as at least the original bug report happens when on web and Linux MESA. https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/18512 and https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/922

I confirm that my workstation use Linux MESA.

[bruvzg]

Since this is an issue on Web platform, it's probably using ANGLE. And ANGLE specify use 64-bit for this value (and some other "size" values):

https://github.com/google/angle/blob/4723a32df4e48c946dc7aabe024fb80badc63bdf/src/libANGLE/queryutils.cpp#L3866-L3875

There's also the following comment in the code (actual clamping is done only when using Vulkan as backend, when using desktop GL is seems to be used as it):

 // Clamp the maxUniformBlockSize to 64KB (majority of devices support up to this size
 // currently), on AMD the maxUniformBufferRange is near uint32_t max.

[adamscott]

@bruvzg I'm actually using an AMD card. (AMD Radeon™ RX Vega 64)

[adamscott]

For the record, I changed my code to make the config use int64_t ints instead of int. And effectively use glGetInteger64v() instead of the regular one.

[lawnjelly]

For the record, I changed my code to make the config use int64_t ints instead of int. And effectively use glGetInteger64v() instead of the regular one.

What value does it return as int64_t? 🤔
Ah just read @bruvzg comment, so maybe near uint32_t max is expected.

[adamscott]

What value does it return as int64_t? 🤔
Ah just read @bruvzg comment, so maybe near uint32_t max is expected.

It returns for me 2147483648, which is exactly INT_MAX + 1.

[clayjohn]

Looks great! Good work

[akien-mga]

Thanks!

[YuriSizov]

Cherry-picked for 4.1.2.