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

global_shader_variables/buffer_size set too high in new Compatibility project #85374

Closed
lostminds opened this issue Nov 26, 2023 · 22 comments · Fixed by #93645
Closed

global_shader_variables/buffer_size set too high in new Compatibility project #85374

lostminds opened this issue Nov 26, 2023 · 22 comments · Fixed by #93645

Comments

@lostminds
Copy link

Godot version

4.2.rc2

System information

macOS 14.1

Issue description

Creating a new Compatibility project the default setting for rendering/limits/global_shader_variables/buffer_size is set to 65536, but apparently this is too high for Compatibility renderer limits, because you get a warning when running:

W 0:00:00:0093   MaterialStorage: Project setting "rendering/limits/global_shader_variables/buffer_size" exceeds maximum uniform buffer size of: 16384
  <C++ Source>   drivers/gles3/storage/material_storage.cpp:1114 @ MaterialStorage()

Steps to reproduce

Create a new blank Compatibility project, with an empty scene and run it and see the warning

Minimal reproduction project

Just create a new project from the project manager as I think the issue is the default value for new Compatibility projects.

@lostminds lostminds changed the title global_shader_variables/buffer_size set to high in new Compatibility project global_shader_variables/buffer_size set too high in new Compatibility project Nov 26, 2023
@jsjtxietian
Copy link
Contributor

Yes, the max_uniform_buffer_size is read from GL using glGetInteger64v(GL_MAX_UNIFORM_BLOCK_SIZE, &max_uniform_buffer_size);, if it's smaller than the project setting's value (default is 65536) it will give a warning. Is there anything we can do other than just ignore the warning ?

@Jakub-Doucek
Copy link

In version: 4.2.1 the default value is 16384
But on iOS device I'm getting the warning anyway
USER WARNING: Project setting "rendering/limits/global_shader_variables/buffer_size" exceeds maximum uniform buffer size of: 16384

The warning is probably ok, because the limit is for some reason really small. And that's what I see as the main problem. 3D models need to have a very small polycount. The same device has no problem with the Vulkan renderer.

@jsjtxietian
Copy link
Contributor

Can we define "rendering/limits/global_shader_variables/buffer_size" based on OS and rendering method? If so we can give it seperate definition.

@Jakub-Doucek
Copy link

From my user-usage perspective, I don't think a split limit per platform would be an advantage. The project needs to be tailored to the lowest value to work properly. So the lowest value can be common to all platforms.

However, it seems that the core of the problem is that this limit exists at all. For mobile devices, the limit is very restrictive. It is not possible to render even a relatively simple low-poly 3D object.

I found a related problem here: #55674

Where @clayjohn suggests:

we could automatically split the uniforms up into multiple UBOs internally, or use a SSBO internally

That's the solution I'd like. I haven't found any open issue for it.

@jsjtxietian
Copy link
Contributor

split the uniforms up into multiple UBOs internally, or use a SSBO internally

Yeah that includes more work, and potentially less performant if SSBO is used.

@clayjohn
Copy link
Member

clayjohn commented Jan 5, 2024

SSBOs aren't supported in OpenGL 3.3, nor can we use multiple UBOs for the global buffer. So we are kinda stuck with the low limit for compatibility devices.

@Jakub-Doucek

This comment was marked as off-topic.

@Jakub-Doucek

This comment was marked as off-topic.

@clayjohn
Copy link
Member

clayjohn commented Jan 5, 2024

@Jakub-Doucek Indeed, 3.5 doesn't support global shader uniforms, and you don't appear to be using them in your project anyway, so your issue isn't related at all.

@lostminds
Copy link
Author

I just tested this in 4.2.1 to see if it was still an issue and when creating a new Compatibility project and running it I'm no longer getting the warning. However, when checking the project settings rendering/limits/global_shader_variables/buffer_size is still 65536, so the default hasn't changed.

Can anyone confirm if this mean the issue is now solved so I can close this issue (that the Compatibility renderer now supports this buffer size?) or is there some other change that just causes the warning not to show even if the limit is still there?

@Calinou
Copy link
Member

Calinou commented Feb 5, 2024

I think you got this warning 4.2.rc2 because it was using ANGLE by default on macOS, but this is no longer the case. If you edit project settings to use the opengl3_angle rendering driver (not rendering method), you will probably see that warning again.

@lostminds
Copy link
Author

If you edit project settings to use the opengl3_angle rendering driver (not rendering method), you will probably see that warning again.

I see, that may be the case. However, it seems I can't use the Angle OpenGL drivers anymore (They used to work). Changing the project setting to used Angle and restarting I get the following error and get switched back to native OpenGL again:

_get_gldisplay_id: Condition "eglGetError() != 0x3000" is true. Returning: -1
  <C++ Source>   drivers/egl/egl_manager.cpp:63 @ _get_gldisplay_id()
 DisplayServerMacOS: Your video card drivers seem not to support the required Metal version, switching to native OpenGL.
  <C++ Source>   platform/macos/display_server_macos.mm:4529 @ DisplayServerMacOS()

Not sure what's going on there, but it seems unlikely the graphics card doesn't support the required Metal version (unless it's some old outdated Metal version) as it's quite a new M2 mac. And I did use the angle driver briefly back when that was the default on mac.

@clayjohn
Copy link
Member

If you edit project settings to use the opengl3_angle rendering driver (not rendering method), you will probably see that warning again.

I see, that may be the case. However, it seems I can't use the Angle OpenGL drivers anymore (They used to work). Changing the project setting to used Angle and restarting I get the following error and get switched back to native OpenGL again:

_get_gldisplay_id: Condition "eglGetError() != 0x3000" is true. Returning: -1
  <C++ Source>   drivers/egl/egl_manager.cpp:63 @ _get_gldisplay_id()
 DisplayServerMacOS: Your video card drivers seem not to support the required Metal version, switching to native OpenGL.
  <C++ Source>   platform/macos/display_server_macos.mm:4529 @ DisplayServerMacOS()

Not sure what's going on there, but it seems unlikely the graphics card doesn't support the required Metal version (unless it's some old outdated Metal version) as it's quite a new M2 mac. And I did use the angle driver briefly back when that was the default on mac.

It turns out this is an issue with our build scripts. We are using the wrong version of ANGLE in official builds. At any rate, the angle build has a few issues that we are still working on while we fixed several issues in the native opengl backend that make it perform better

@clayjohn
Copy link
Member

Bad news, while looking into a related issue I realized that our validation here was wrong. The setting sets the total number of uniforms that can be used by the buffer, but we treated the total number of uniforms as if were the total number of bytes. So even after validation, we only reduced the size of the buffer to 16384, which then allocated a buffer with a size of 16384 * 16. The actual limit for mobile devices is 1024 items. :( For desktop devices the limit is 4096.

This is unfortunately a huge limitation. If users need more than 1024 items, we will have to use a texture instead of a uniform buffer. If we do so, the limit can be removed, but using the global buffer (and per-instance buffer) will become much more expensive at draw time.

@lostminds
Copy link
Author

The setting sets the total number of uniforms that can be used by the buffer, but we treated the total number of uniforms as if were the total number of bytes. ... This is unfortunately a huge limitation.

Considering the size of the values I also guessed these were sizes in bytes rather than number of uniforms. It's probably my lack of imagination, but to me 1024 global shader uniforms (where you can use 256 in a single draw call?) seems like quite a lot to me. So I wouldn't see it as such a big limitation even if the number is much smaller.

Considering all projects have a setting for this that is likely using the old buffer_size default values that will now be too high, I think this will mostly be an issue about warnings about that. To most users it'll be a number they don't understand (and doesn't have a description) that is now suddenly allowed a much smaller number and has to be changed. To avoid confusion about this after 4.3 update perhaps you could simply replace buffer_size with a new better named max_items setting, with a description to explain what this number means and with new correct defaults based on renderer for the project?

@lostminds
Copy link
Author

Just noticed in 4.3beta2 we now get a warning that the setting is too high and that it will fall back to 4096 (running on desktop) after #92568. However, the default value if you go into Project settings and reset it to default is still 65536. So I'm guessing this should be changed to 4096 for desktop projects and 1024 for mobile to avoid this warning for all new projects?

@clayjohn
Copy link
Member

However, the default value if you go into Project settings and reset it to default is still 65536. So I'm guessing this should be changed to 4096 for desktop projects and 1024 for mobile to avoid this warning for all new projects?

Not exactly, this same setting is used by all renderers. On the Mobile and Forward+ renderer the default of 65536 is fine. Changing the limit for everyone to 4096 will break compatibility. For the Compatibility renderer its fine because using more than the number supported by the device wouldn't have worked anyway, but for the other backends its a problem.

I suspect that we should do the following:

  1. Suppress the warning for now and only warn if a user tries actually uses more variables than the limit
  2. Investigate another method to provide global shader variables that isn't limited to so few items (either using a texture, or paging uniform buffers)

@lostminds
Copy link
Author

  1. Suppress the warning for now and only warn if a user tries actually uses more variables than the limit

Is there some benefit to setting this setting to anything lower than the max allowed? If not, perhaps the setting could just be removed and the max allowed always be used if the engine would instead correctly validate the number of variables actually used and block/warn if there were too many to be supported.

@patwork
Copy link
Contributor

patwork commented Jun 22, 2024

You would lose the possibility to save GPU memory by manually changing the setting to some small value.

Realistically, most (desktop) GPUs will return GL_MAX_UNIFORM_BLOCK_SIZE = 65536, but there are some exceptions:

https://stackoverflow.com/questions/47932609/uniform-buffer-size-on-nvidia-gpus

As I researched this problem, I found GL_MAX_UNIFORM_BLOCK_SIZE variable which returns ~500MB on the AMD and 65.54 kB on both Nvidia chips.

Note: currently GL_MAX_UNIFORM_BLOCK_SIZE is clamped after quering with glGetInteger64v() to 16K..1MB.

@clayjohn clayjohn added this to the 4.3 milestone Jun 22, 2024
@clayjohn clayjohn moved this from Unassessed to Bad in 4.x Release Blockers Jun 22, 2024
@AThousandShips AThousandShips pinned this issue Jun 22, 2024
@terrybader
Copy link

Getting similar with no value shown

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

@clayjohn
Copy link
Member

  1. Suppress the warning for now and only warn if a user tries actually uses more variables than the limit

Is there some benefit to setting this setting to anything lower than the max allowed? If not, perhaps the setting could just be removed and the max allowed always be used if the engine would instead correctly validate the number of variables actually used and block/warn if there were too many to be supported.

For the compatibility renderer (especially on web and low end devices) shader compile time is faster with a lower limit.

For the RD renderers, the maximum size can be several mbs, so the limit helps to save memory.

@clayjohn
Copy link
Member

It looks like we already provide an error when we attempt to use too many.

ERR_FAIL_COND_MSG(gv.buffer_index < 0, vformat("Failed allocating global variable '%s' out of buffer memory. Consider increasing it in the Project Settings.", String(p_name)));

ERR_FAIL_COND_V_MSG(pos < 0, -1, "Too many instances using shader instance variables. Increase buffer size in Project Settings.");

We can just improve the error message so that it specifies the hardware maximum

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Bad
8 participants