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

Added option to enable high precision float in GLES2 #33646

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

NeoSpark314
Copy link
Contributor

An additional project setting under rendering/shading with the name
gles2_high_float_precision.mobile is introduced that enables #define USE_HIGHP_PRECISION
in GLES2 shader on Android when it is supported by the shader compiler.
This fixes #33633 and #32813 and also GodotVR/godot_oculus_mobile#60
and GodotVR/godot_oculus_mobile#68 on devices that support the highp (high precision) modifier.

@clayjohn suggested on discord to introduce a project setting to solve this problem.

Two open questions from my side are

  1. Is it possible to show project settings only if the GLES2 backend is selected? If it is possible would this be desired for this option?
  2. At the moment it is only active for Android. I don't know if this is needed for iOS or if it would work there.

@Calinou
Copy link
Member

Calinou commented Nov 15, 2019

Is it possible to show project settings only if the GLES2 backend is selected? If it is possible would this be desired for this option?

We recommend always defining the option and prefixing its name with gles2, so that it's always present in the generated documentation:

godot/main/main.cpp

Lines 962 to 963 in 98caeb6

// Assigning here even though it's GLES2-specific, to be sure that it appears in docs
GLOBAL_DEF("rendering/quality/2d/gles2_use_nvidia_rect_flicker_workaround", false);

@lawnjelly
Copy link
Member

I agree with making this optional, however assuming it is turned on, If we consider 3 possibilities:

  1. Hardware that supports highp in fragment shaders at full speed
  2. Hardware that supports highp in fragment shaders at low speed
  3. Hardware that doesn't support highp (reverts to mediump)

This PR will help in situation (1) however it can potentially make situation (2) worse, and leaves situation (3) unfixed. As is this would be a case to authors of 'beware, use at your own risk', but it would be nice to have some kind of scalable solution, to what is admittedly a difficult problem.

It would be nice to be able to detect situations (2) and (3), and apply a different shader (or a different path within the shader).

Could it be possible to detect situation (3) outside the shader itself? Perhaps by attempting to compile a test fragment shader containing highp, or using the define itself to cause compilation failure and testing for this compilation failure (assuming OpenGL doesn't just crash!)? This could potentially be used to swap to a different shader.

For situation (2) it might be possible to have an in game settings to swap between 2 alternative shaders?

A real life example shader is a fancy water shader - you might use a highp fragment shader on hardware that supports it well, but fallback to a simpler shader on hardware that doesn't, so those people can still play the game.

In addition there is the performance question of whether highp should be used throughout a fragment shader (I don't know whether this PR applies it throughout or just to varyings / variables that have been marked as highp in the godot shader source).

The problem of scalable shaders is also generally applicable, to enable users to scale their game experience according to their hardware, I don't know if we have any in built solution for this as yet but it may be worth thinking about within this framework. Having the option of 2 (or more) tiers of shaders to choose from would make the games far more scalable, if this isn't already available, and could be used to address this problem without making the games unplayable for a large number of players.

@NeoSpark314
Copy link
Contributor Author

@lawnjelly Detecting the support can be done outside via glGetShaderPrecisionFormat(GL_FRAGMENT_SHADER, GL_HIGH_FLOAT, range, &precision);
If highp is unsupported this will return [0,0] for range and 0 for precision.

Regarding the question of where the change introduces highp computation in the fragment shader: as far as I understand it this will apply to all variables/uniforms/varyings that have no precision qualifier applied (and as you said: this should only be used on high-end devices).

Regarding a scalable shader pipeline: this sounds like a monumental task with a lot of trade-offs depending on how you define the capabilities of the low-end hardware vs. mid-range and high-end. Especially since a user would then have to be very aware of how the system behaves on different hardware in a very detailed way to author the content properly and not rely on sth. that might disappear for some players of the game (but arguably this is already the case at the moment).

There are already some options in the existing shader that go into this direction (phong vs. ggx, lit vs. unlit) so maybe providing some query functions for the hardware capabilities to GDScript would allow (advanced) users to build system to manually select the right shader; also supporting the opengl precision keywords in the godot shader language could help there if someone wants to go deep into performance optimization.

@Chaosus Chaosus added this to the 4.0 milestone Nov 16, 2019
@NeoSpark314
Copy link
Contributor Author

I updated the PR with GLOBAL_DEF("rendering/quality/shading/gles2_high_float_precision.mobile", false); in the main.cpp as @Calinou suggested.

@clayjohn clayjohn added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 5, 2020
@@ -821,6 +821,9 @@
</member>
<member name="rendering/quality/shading/force_lambert_over_burley.mobile" type="bool" setter="" getter="" default="true">
</member>
<member name="rendering/quality/shading/gles2_high_float_precision.mobile" type="bool" setter="" getter="" default="false">
If [code]true[/code] and available on the target device high floating point precision for all shader computations in GLES2 is enabled.
Copy link
Member

Choose a reason for hiding this comment

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

Change to ->
"If [code]true[/code] and available on the target device, enables high floating point precision for all shader computations in GLES2.
[b]Warning:[/b] High floating point precision can be extremely slow on older devices and is often not available at all. Use with caution."

main/main.cpp Outdated
@@ -961,6 +961,7 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph

// Assigning here even though it's GLES2-specific, to be sure that it appears in docs
GLOBAL_DEF("rendering/quality/2d/gles2_use_nvidia_rect_flicker_workaround", false);
GLOBAL_DEF("rendering/quality/shading/gles2_high_float_precision.mobile", false);
Copy link
Member

Choose a reason for hiding this comment

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

This should only be in VisualServer.cpp

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, you can add it after:

servers/visual_server.cpp
2446:   GLOBAL_DEF("rendering/gles2/compatibility/disable_half_float", false);

It could be renamed to "rendering/gles2/compatibility/enable_high_float" I guess to match the other gles2/compatibility setting?

Copy link
Member

Choose a reason for hiding this comment

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

Actually this one should be removed now that it has been put in VisualServer. It should only be defined once.

@@ -2401,6 +2401,7 @@ VisualServer::VisualServer() {
GLOBAL_DEF("rendering/quality/shading/force_lambert_over_burley.mobile", true);
GLOBAL_DEF("rendering/quality/shading/force_blinn_over_ggx", false);
GLOBAL_DEF("rendering/quality/shading/force_blinn_over_ggx.mobile", true);
GLOBAL_DEF("rendering/quality/shading/gles2_high_float_precision.mobile", false);
Copy link
Member

Choose a reason for hiding this comment

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

Should have both the normal and the .mobile variant here.

Copy link
Member

Choose a reason for hiding this comment

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

It's a bit of a weird situation though as the non-.mobile variant (or .Android if we go this route) wouldn't do anything currently.

@m4gr3d
Copy link
Contributor

m4gr3d commented May 25, 2020

@clayjohn @NeoSpark314 Any updates on this PR?

@clayjohn
Copy link
Member

@m4gr3d I'm good with it. It just needs reduz's approval to get merged. Would be a good candidate for a PR review meeting.

@aaronfranke
Copy link
Member

@NeoSpark314 When you get a chance, this PR needs to be rebased on the latest master branch.

@m4gr3d
Copy link
Contributor

m4gr3d commented Jul 1, 2020

@aaronfranke @NeoSpark314 Actually I think this PR should be rebased against the 3.2 branch since gles2 is removed from the master branch.

@lawnjelly
Copy link
Member

lawnjelly commented Jul 4, 2020

Also note that since the PR was originally made there is now a project setting group rendering/gles2/ which might be a better place for the setting (although it would be fine to leave it till it gets discussed in a PR meeting).

@akien-mga
Copy link
Member

@NeoSpark314 Could you rebase this on 3.2? You can change the base branch for the PR by editing the title (yes, very obvious :)) and then force push a version rebased on 3.2 to update the PR.
Screenshot_20200720_102256

@NeoSpark314 NeoSpark314 changed the base branch from master to 3.2 July 20, 2020 17:55
@NeoSpark314
Copy link
Contributor Author

@akien-mga done; rebased this PR on top of 3.2 and changed the target branch to 3.2

@akien-mga
Copy link
Member

Here's an example of a user that needed this feature for an Oculus Quest project too: https://twitter.com/acegiak/status/1285535845806510080 (she compiled Godot with this PR to solve it).

@reduz is OK with the changes 👍

There are some outstanding nitpicks from @clayjohn though which would be good to address.

Should there also be some additional documentation about the caveats mentioned by @lawnjelly?

@@ -180,6 +181,14 @@ ShaderGLES2::Version *ShaderGLES2::get_current_version() {
strings.push_back("#define USE_HIGHP_PRECISION\n");
#endif

#ifdef ANDROID_ENABLED
Copy link
Member

@akien-mga akien-mga Jul 21, 2020

Choose a reason for hiding this comment

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

Should it really be Android-specific? If so, instead of exposing it as a .mobile override, it could be an .Android override (see platform/android/export/export.cpp::get_platform_features).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have experience with iOS and don't know if the shader compiler would support it. I made it android specific now by moving it to .Android

Copy link
Member

Choose a reason for hiding this comment

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

The #ifdef can be removed.

@NeoSpark314
Copy link
Contributor Author

@akien-mga I updated the documentation as suggested by @clayjohn, moved the setting to rendering/gles2/compatibility/enable_high_float.Android in VisualServer.cpp and removed it from main.cpp. Let me know if there is something missing or need to be changed.

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.

Changes look good to me.

Just double check with @akien that the #ifdef ANDROID_ENABLED is still needed now that you are using .Android

An additional project setting under rendering/gles2/compatibility with the name
enable_high_float.Android is introduced that enables #define USE_HIGHP_PRECISION
in GLES2 shader on Android when it is supported by the shader compiler.
This fixes godotengine#33633 and godotengine#32813 and also GodotVR/godot_oculus_mobile#60
and GodotVR/godot_oculus_mobile#68 on devices that
support the highp (high precision) modifier.
@NeoSpark314
Copy link
Contributor Author

@akien-mga the #ifdef is removed now

@akien-mga akien-mga merged commit 19802f7 into godotengine:3.2 Jul 30, 2020
@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.

GLES2 shading issue
9 participants