Make query for GL_MAX_VIEWPORT_DIMS compatible with web exports

[patwork]

Info

This is an attempt to fix #92601 which turned out to be more puzzling than I originally thought.

Investigation

As described in the original bug, tooltips in web exports were not moving correctly to avoid being cut off by the edge of the screen. At the same time, Windows and Linux exports were working correctly.

The problem was that in godot/scene/main/viewport.cpp the function panel->get_max_size() was returning values of 0/0 making the function min() to set the tooltip size to zero. In fact, it happened only in web exports.

godot/scene/main/viewport.cpp

Line 1505 in e96ad5a

r.size = r.size.min(panel->get_max_size());

Going a step further, get_max_size() returned the value of the max_viewport_size variable from the Config class of drivers/gles3/storage/config.cpp. This value should be set by querying the GL driver for the GL_MAX_VIEWPORT_DIMS variable.

godot/drivers/gles3/storage/config.h

Line 63 in e96ad5a

int64_t max_viewport_size[2] = { 0, 0 };

godot/drivers/gles3/storage/config.cpp

Lines 83 to 87 in e96ad5a

	glGetInteger64v(GL_MAX_VERTEX_TEXTURE_IMAGE_UNITS, &max_vertex_texture_image_units);
	glGetInteger64v(GL_MAX_TEXTURE_IMAGE_UNITS, &max_texture_image_units);
	glGetInteger64v(GL_MAX_TEXTURE_SIZE, &max_texture_size);
	glGetInteger64v(GL_MAX_UNIFORM_BLOCK_SIZE, &max_uniform_buffer_size);
	glGetInteger64v(GL_MAX_VIEWPORT_DIMS, max_viewport_size);

Here is the source of the problem, because it turns out that in the case of web export, query for GL_MAX_VIEWPORT_DIMS does not save anything. Of these above 5 queries, the first 4 work correctly and the last 4 do not. The variable is not changed at all, if we write some values to it beforehand it remains the same.

However, it turns out that in the drivers/gles3/rasterizer_gles3.cpp file, a different glGetIntegerv function is used for the same query. It works in both Windows / Linux and web exports.

godot/drivers/gles3/rasterizer_gles3.cpp

Lines 401 to 402 in e96ad5a

	GLsizei max_vp[2] = {};
	glGetIntegerv(GL_MAX_VIEWPORT_DIMS, max_vp);

Fix

My suggested fix is to replace glGetInteger64v with glGetIntegerv. Unfortunately max_viewport_size in Config class is of type int64_t [2] and glGetIntegerv operates on GLint * data (32 bit).

Please review if it is possible to somehow rewrite the values of these variables more elegantly?

max_viewport_size[0] = max_vp[0];
max_viewport_size[1] = max_vp[1];

https://www.khronos.org/opengl/wiki/OpenGL_Type

Notes

The glGetInteger64v function is used in Godot only in the drivers/gles3/storage/config.cpp and drivers/gles3/shader_gles3.cpp files. It seems that except for one case it doesn't cause any other problems, but the mystery for me remains why it doesn't work for GL_MAX_VIEWPORT_DIMS (this is the only case where it sets an array and not a single variable).

Perhaps the problem is that the web export is in WASM32? I have not tested 32-bit Windows / Linux exports.

In addition, according to the Khronos table, glGetInteger64v officialy appeared in OpenGL 3.2 / OpenGL ES 3.0 and glGetIntegerv had been around since OpenGL 2.0 / OpenGL ES 2.0.

https://registry.khronos.org/OpenGL-Refpages/gl4/html/glGet.xhtml

https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glGet.xhtml

[clayjohn]

I can't take a look now. But we should double check the specification for OpenGL 3.3 and OpenGL ES 3.0 and see if the return value for GL_MAX_VIEWPORT_DIMS is a 32 but value or 64 but value. That will inform whether we use the normal variant or the 32 but variant of getIntegerV. You can find that information in the PDF copy of the specification.

[patwork]

From https://registry.khronos.org/OpenGL/specs/es/3.0/es_spec_3.0.pdf

Get value MAX VIEWPORT DIMS
Type 2 × Z+ (Non-negative integer or enumerated token value)
Get Command GetIntegerv
Description Maximum viewport dimensions

I think we're safe with 32 bit GetIntegerv.

[patwork]

So I more or less reverted most of the changes from #80909

From the documentation, it seems that actually MAX_UNIFORM_BLOCK_SIZE should be queried as 64 bit. The remaining variables remain GLint (32-bit).

The total amount of buffer object storage available for any given uniform block is subject to an implementation-dependent limit. The maximum amount of avail- able space, in basic machine units, can be queried by calling GetInteger64v with the constant MAX_UNIFORM_BLOCK_SIZE. If the amount of storage required for a uniform block exceeds this limit, a program may fail to link.

buffer size

By the way, I checked where the references to config->max_uniform_buffer_size are located and I have a question, what to do with the following?

godot/drivers/gles3/storage/material_storage.cpp

Lines 1111 to 1115 in 5833f59

	global_shader_uniforms.buffer_size = MAX(4096, (int)GLOBAL_GET("rendering/limits/global_shader_variables/buffer_size"));
	if (global_shader_uniforms.buffer_size > uint32_t(Config::get_singleton()->max_uniform_buffer_size)) {
	global_shader_uniforms.buffer_size = uint32_t(Config::get_singleton()->max_uniform_buffer_size);
	WARN_PRINT("Project setting \"rendering/limits/global_shader_variables/buffer_size\" exceeds maximum uniform buffer size of: " + itos(Config::get_singleton()->max_uniform_buffer_size));
	}

buffer_size is type of uint32_t and max_uniform_buffer_size is 64 bit (since Aug 28, 2023) and sometimes can be INT_MAX + 1 #80909 (comment) 🤔

viewport dims

I changed the reading of GL_MAX_VIEWPORT_DIMS in rasterizer_gles3.cpp to use the previously retrieved value from Config. This gave rise to two topics:

https://github.com/godotengine/godot/blob/20d5b6c1117579a3187bb2a80f620b40d2dc5ce5/drivers/gles3/rasterizer_gles3.cpp#L396-L406

CC: @RandomShaper #90913

1. As I understand it, referencing a singleton from Utilities will not cause threading issues here?

2. When the game screen is smaller than the window, this code creates a viewport of size GL_MAX_VIEWPORT_DIMS in each image frame and fills it with black. Isn't this overkill? Using my Firefox / Windows / GTX960 example, this means creating a black 32767x 32767 rectangle every frame. 💣

shader_gles3

For consistency and DRY, is it safe to add a reference to the singleton from Config in shader_gles3.cpp?

godot/drivers/gles3/shader_gles3.cpp

Line 804 in 5833f59

glGetInteger64v(GL_MAX_TEXTURE_IMAGE_UNITS, &max_image_units);

[clayjohn]

By the way, I checked where the references to config->max_uniform_buffer_size are located and I have a question, what to do with the following?

godot/drivers/gles3/storage/material_storage.cpp

Lines 1111 to 1115 in 5833f59

	global_shader_uniforms.buffer_size = MAX(4096, (int)GLOBAL_GET("rendering/limits/global_shader_variables/buffer_size"));
	if (global_shader_uniforms.buffer_size > uint32_t(Config::get_singleton()->max_uniform_buffer_size)) {
	global_shader_uniforms.buffer_size = uint32_t(Config::get_singleton()->max_uniform_buffer_size);
	WARN_PRINT("Project setting \"rendering/limits/global_shader_variables/buffer_size\" exceeds maximum uniform buffer size of: " + itos(Config::get_singleton()->max_uniform_buffer_size));
	}

buffer_size is type of uint32_t and max_uniform_buffer_size is 64 bit (since Aug 28, 2023) and sometimes can be INT_MAX + 1 #80909 (comment) 🤔

We should probably clamp max_uniform_buffer_size to a reasonable size in config.cpp . Maybe 1048576 (1 mb). Realistically almost every GPU only supports a maximum of 65536. I suspect many drivers only support 65536 even when they advertise support for more

viewport dims

I changed the reading of GL_MAX_VIEWPORT_DIMS in rasterizer_gles3.cpp to use the previously retrieved value from Config. This gave rise to two topics:

1. As I understand it, referencing a singleton from Utilities will not cause threading issues here?

That's right. Reading from the Window is what caused the issue, reading from Config/Utilities is fine.

2. When the game screen is smaller than the window, this code creates a viewport of size GL_MAX_VIEWPORT_DIMS in each image frame and fills it with black. Isn't this overkill? Using my Firefox / Windows / GTX960 example, this means creating a black 32767x 32767 rectangle every frame. 💣

Its not a problem. This just sets the Viewport size. The actual drawing will be limited to the size of the framebuffer/texture.

shader_gles3

For consistency and DRY, is it safe to add a reference to the singleton from Config in shader_gles3.cpp?

godot/drivers/gles3/shader_gles3.cpp

Line 804 in 5833f59

glGetInteger64v(GL_MAX_TEXTURE_IMAGE_UNITS, &max_image_units);

Yes.

[patwork]

OK, how does it look now?

I've added CLAMP in config.cpp to max_uniform_buffer_size (GL_MAX_UNIFORM_BLOCK_SIZE) from minimal 16K (according to khronos.org) to maximum 1MB.

max_uniform_buffer_size = CLAMP(max_uniform_buffer_size, 16384, 1048576);

💣 One more thing, as I am assuming, initialize in shader_gles3.cpp is performed in threads (in my case it runs 12 times). can I be 100% sure that this will always happen already after Config initialisation?

[clayjohn]

💣 One more thing, as I am assuming, initialize in shader_gles3.cpp is performed in threads (in my case it runs 12 times). can I be 100% sure that this will always happen already after Config initialisation?

Yes, Config is initialized before any shaders. I don't think any of the shaders are initialized from threads by the way. I'm fairly certain all shader initialization happens on the rendering thread and at initialization time.

[patwork]

Ah, yes, my mistake. ShaderGLES3::initialize is run multiple times because it is used by different modules called from RasterizerGLES3::RasterizerGLES3.

godot/drivers/gles3/rasterizer_gles3.cpp

Lines 348 to 363 in ac95e0f

	// OpenGL needs to be initialized before initializing the Rasterizers
	config = memnew(GLES3::Config);
	utilities = memnew(GLES3::Utilities);
	texture_storage = memnew(GLES3::TextureStorage);
	material_storage = memnew(GLES3::MaterialStorage);
	mesh_storage = memnew(GLES3::MeshStorage);
	particles_storage = memnew(GLES3::ParticlesStorage);
	light_storage = memnew(GLES3::LightStorage);
	copy_effects = memnew(GLES3::CopyEffects);
	cubemap_filter = memnew(GLES3::CubemapFilter);
	glow = memnew(GLES3::Glow);
	post_effects = memnew(GLES3::PostEffects);
	gi = memnew(GLES3::GI);
	fog = memnew(GLES3::Fog);
	canvas = memnew(RasterizerCanvasGLES3());
	scene = memnew(RasterizerSceneGLES3());

[clayjohn]

Looks good!

[akien-mga]

Thanks!