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

Use highp precision in the gles2 fragment shader if available #29014

Merged
merged 1 commit into from May 27, 2019

Conversation

@mbrlabs
Copy link
Contributor

mbrlabs commented May 19, 2019

This fixes #28199.

The problem was, that the custom shader code (uniforms starting with m_; added via ShaderCompilerGLES2) did not have explicit precision types set, so the default ones for the fragmant shader (mediump) were used while the vertex used highp.
The specs say, that uniforms should have the same precision.

See the specs (4.5.3 Default Precision Qualifiers):
https://www.khronos.org/registry/OpenGL/specs/es/2.0/GLSL_ES_Specification_1.00.pdf

This fix uses highp for the fragment shader by default (if available).
Another possiblity which also works whould be to set the default precision in the vertex shader to mediump for ints and floats.

@mbrlabs mbrlabs requested a review from reduz as a code owner May 19, 2019
@YeldhamDev YeldhamDev added this to the 3.2 milestone May 20, 2019
@akien-mga akien-mga requested a review from clayjohn May 24, 2019
@clayjohn

This comment has been minimized.

Copy link
Contributor

clayjohn commented May 24, 2019

We discussed this on IRC, and reduz is concerned about the performance impact on mobile if we default to high precision always. The issue with this fix is that all calculations in fragment shaders are going to use highp by default. Most users don't specify precision for each variable, so everyone's projects will take a big performance hit (on mobile the speed difference between highp and mediump can be huge).

While this does correctly fix the bug, I think a better fix can be found. We need to make it so that all uniforms are specified highp by default, but all others are not (they would use default value). This will likely take some tinkering in https://github.com/godotengine/godot/blob/master/drivers/gles2/shader_gles2.cpp

Reduz on IRC:

When you write a shader, on desktop, everything should be high precision by default, both on GLES2 and GLES3. On mobile, vertex program should be high precision by default and fragment program should be mediump, because performance on mobile is limited and because many devices don't even support highp properly

Let me know if you have any questions/need any help!

@mbrlabs

This comment has been minimized.

Copy link
Contributor Author

mbrlabs commented May 24, 2019

Yes, that was what i was worried about as well. We could explicitly set appropriate uniforms to highp (if not already set by the user) and leave the shaders unchanged.
I'm gonna have a look at this over the weekend. Thanks for the feedback!

The use of different default precision values (highp in vertex; mediump
in fragment) for uniform variables caused the shader program to not link properly on some android
devices/emulators.
@mbrlabs mbrlabs force-pushed the mbrlabs:gles2_precision_fix branch from 38024b4 to 502dbc7 May 25, 2019
@mbrlabs

This comment has been minimized.

Copy link
Contributor Author

mbrlabs commented May 25, 2019

@clayjohn
Now, only uniforms have a default precision of highp. Users can still change it though if they expliclity set precision values for uniforms.

Copy link
Contributor

clayjohn left a comment

Looks good to me!

Have you confirmed that this still fixes #28199? (I was never able to replicate)

@mbrlabs

This comment has been minimized.

Copy link
Contributor Author

mbrlabs commented May 25, 2019

Yes, i tested it on android emulators with API levels 18, 25 and 28. All of them were affected by the issue and they work now.

@akien-mga akien-mga merged commit b9ee3f3 into godotengine:master May 27, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented May 27, 2019

Thanks!

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 5, 2019

Note to release maintainers: if/when cherrpicking this PR, #30331 should be included too.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Jul 17, 2019

Cherry-picked for 3.1.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.