Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more validation to UBO size and alignment in Compatibility renderer #92568

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

clayjohn
Copy link
Member

This validation is to ensure we don't break the UBO alignment rules which can have disastrous performance implications on some buggy drivers.

This is mostly static checks to ensure we don't accidentally break things with subsequent changes. But it does add a check that the material buffer size does not exceed the max uniform buffer size.

The one substantial change here is that we change the validation for the global uniform buffer size. Previously it was MAXed to be at least 4096 items long and the raw size (number of items) was compared against the maximum UBO size (measured in bytes). So we still ended up requesting a buffer that was 16 times the maximum size supported by the device.

In practice, this hasn't resulted in any issues as Uniform buffers are allowed to be bigger than the device limit. The device limit specifies how much you can use in a draw call and the Compatibility renderer limits the number that can be used in a draw call to 256.

global_defines += "#define MAX_GLOBAL_SHADER_UNIFORMS 256\n"; // TODO: this is arbitrary for now

At any rate, we need to fix this validation before users run into the problem (at best they will get cryptic driver errors, but they could easily get a crash)

Partially addresses: #85374

Putting on the docket for 4.3 as this is a very safe change and will potentially have benefits for web exports which struggle with large UBOs

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and docs look good to me.

@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga merged commit a75654f into godotengine:master Jun 17, 2024
16 checks passed
@patwork
Copy link
Contributor

patwork commented Jun 19, 2024

@clayjohn hi, now every time I run Godot in compability mode I get a warning:

WARNING: Project setting "rendering/limits/global_shader_variables/buffer_size" exceeds maximum uniform buffer size of: 4096. Falling back on maximum buffer size.
     at: MaterialStorage (drivers/gles3/storage/material_storage.cpp:1121)

static_assert(sizeof(GlobalShaderUniforms::Value) == 16);
global_shader_uniforms.buffer_size = MAX(16, (int)GLOBAL_GET("rendering/limits/global_shader_variables/buffer_size"));
if (global_shader_uniforms.buffer_size * sizeof(GlobalShaderUniforms::Value) > uint32_t(Config::get_singleton()->max_uniform_buffer_size)) {
global_shader_uniforms.buffer_size = uint32_t(Config::get_singleton()->max_uniform_buffer_size) / sizeof(GlobalShaderUniforms::Value);
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 / sizeof(GlobalShaderUniforms::Value)) + ". Falling back on maximum buffer size.");
}

  • GLOBAL_GET("rendering/limits/global_shader_variables/buffer_size") is by default 65536 (clean config)
  • sizeof(GlobalShaderUniforms::Value) is 16
  • Config::get_singleton()->max_uniform_buffer_size is 65536 on my system (windows 10, nvidia gtx 960)

So in line 1113 there's check if (65536 * 16 > 65536) and it's always true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants