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 shader cache to GLES3 #76092

Merged
merged 2 commits into from
May 9, 2023
Merged

Conversation

ChibiDenDen
Copy link
Contributor

Implement shader cache read/write in the gles3 rendering pipeline

This is only supported on mobile platforms as desktop platforms are using gles over gl version 3.3, and 4.1 is needed.
Upgrading glad to 4.1 could be done in a separate PR.

@ChibiDenDen ChibiDenDen requested a review from a team as a code owner April 15, 2023 14:17
@ChibiDenDen ChibiDenDen force-pushed the shader_cache_gles3 branch 3 times, most recently from a0cfb04 to a491a8d Compare April 15, 2023 15:05
@Calinou
Copy link
Member

Calinou commented Apr 15, 2023

For reference, here's the 3.x implementation of shader caching: #53411
(Shader cache can only be used with asynchronous shader compilation enabled in 3.x.)

Note that the OpenGL program binary extension is poorly supported on HTML5/WebGL, so this will likely have to be disabled there.

@ChibiDenDen
Copy link
Contributor Author

Thanks for the reference.

As far as I can see, async shader compilation is not supported for gles3 on godot 4 right now.

@clayjohn
Copy link
Member

clayjohn commented Apr 19, 2023

We don't need to implement async shader compilation right away.

We don't actually need OpenGL 4.1 on Desktop, we just need support for the ARB_get_program_binary extension (which was elevated to core in OpenGL 4.1). What you should do is use GLAD to load the extension and then check if it is supported

config.program_binary_supported = GLAD_GL_ARB_get_program_binary;

We need to do this anyway for Web support as you can't cache shader binaries in WebGL2.

The extension is very well supported on Desktop. GPUInfo only has 24 reports of OpenGL 3.3 capable devices not supporting it and in all those cases it looks like out of date drivers are responsible (they are all on Windows 7, Windows XP, or using Parellels to run Windows 10 on macOS).

So all that is to say, please go ahead and add support for Desktop as well I think it is best to do in this PR. But I don't think it will be very much additional work for you.

@clayjohn
Copy link
Member

clayjohn commented May 2, 2023

I think this is still going to fail on Web exports as the GLES_OVER_GL section won't be included but glGetProgramBinary will still be null.

edit: running locally it seems that WebGL is choking on glGetProgramiv, specifically because it references GL_PROGRAM_BINARY_LENGTH which is undefined on WebGL. glGetProgramBinary ends up never getting called because glGetProgramiv fails. We just need to add an early return on webGL as well, and then this should be ready to merge. Great work so far!

@ChibiDenDen
Copy link
Contributor Author

ChibiDenDen commented May 5, 2023

I removed the ifdef GLES_OVER_GL hoping this would solve the WebGL issue.
Did not test the WebGL version myself yet as i'm having some trouble compiling for web locally.

Edit:
This didnt pass build.
Instead put WEB_ENABLED checks on both methods.
Should work properly now (no shader cache code at all in web builds)

@ChibiDenDen ChibiDenDen force-pushed the shader_cache_gles3 branch 3 times, most recently from 2a3fc12 to 4c4b3e0 Compare May 5, 2023 20:20
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Seems good to me now!

I tested locally on my linux device with an intel integrated GPU and on my Android phone. On the phone the improvement is substantial with my test scene (which has a couple of really heavy shaders loaded at the same time). On Desktop it compiles so fast in either case that you can't really notice any improvement.

@akien-mga akien-mga merged commit 1c7a62d into godotengine:master May 9, 2023
@akien-mga
Copy link
Member

Thanks!

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.

4 participants