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

Decrease shader TIME rollover to 30 seconds on mobile platforms #59990

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Apr 7, 2022

This avoids precision issues that became obvious when exporting a project to a mobile platform.

Shader animations will have to loop over 30 seconds to avoid visual discrepancies when TIME is reset back to 0.

@Calinou Calinou requested review from a team as code owners April 7, 2022 16:46
@Calinou Calinou added this to the 4.0 milestone Apr 7, 2022
@Calinou Calinou force-pushed the shader-time-rollover-secs-mobile-lower-default branch 2 times, most recently from de00b97 to bf0c941 Compare April 7, 2022 18:35
@@ -1756,6 +1756,10 @@
<member name="rendering/limits/spatial_indexer/update_iterations_per_frame" type="int" setter="" getter="" default="10">
</member>
<member name="rendering/limits/time/time_rollover_secs" type="float" setter="" getter="" default="3600">
When the [code]TIME[/code] shader built-in variable is incremented, it will roll over to [code]0.0[/code] when the value specified in [member rendering/limits/time/time_rollover_secs] is exceeded (in seconds). Since large floating-point values are less precise than small floating-point values, this should be set as low as possible to maximize the precision of the [code]TIME[/code] built-in variable in shaders. However, if this is set too low, shader animations may appear to restart from the beginning while the project is running.
</member>
<member name="rendering/limits/time/time_rollover_secs.mobile" type="int" setter="" getter="" default="30">
Copy link
Member

Choose a reason for hiding this comment

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

The property hint doesn't seem to apply to the override, it's typed as int (and likely doesn't have a range slider).

Also, why 30 seconds specifically? That sounds really low.

Copy link
Member Author

@Calinou Calinou Apr 7, 2022

Choose a reason for hiding this comment

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

Also, why 30 seconds specifically? That sounds really low.

@clayjohn recommended to use 30 instead of 60, as 16-bit floating point precision gets low very quickly. If the rollover time was 60, the worst precision you'd get would be 0.03, and this is already too imprecise for many shader effects: https://en.wikipedia.org/wiki/Half-precision_floating-point_format#Precision_limitations

In comparison, this is not nearly as much of an issue with 32-bit floats.

@clayjohn
Copy link
Member

In general, this looks good. Before merging we should double check what the behaviour on the vulkan-mobile backend is (e.g. does it default to 16p or 32p floats)

@Calinou
Copy link
Member Author

Calinou commented Apr 10, 2022

In general, this looks good. Before merging we should double check what the behaviour on the vulkan-mobile backend is (e.g. does it default to 16p or 32p floats)

#52500 wasn't merged, so it might still be defaulting to 32-bit floats right now.

This avoids precision issues that became obvious when exporting
a project to a mobile platform.

Shader animations will have to loop over 30 seconds to avoid visual
discrepancies when `TIME` is reset back to 0.
@Calinou Calinou force-pushed the shader-time-rollover-secs-mobile-lower-default branch from bf0c941 to 20f1c51 Compare April 13, 2022 23:11
@reduz
Copy link
Member

reduz commented Aug 30, 2022

Some notes:

  • All mobile hardware supports 32 bits (highp) on the vertex shader.
  • All mobile hardware supports > 16 bits as highp on the fragment shader. Only some (Qualcom old devices) I think are restricted to 20.
  • I would check we are not mislabeling as mediump by mistake
  • In Godot 4 we have global shaders, so if you really need something specific, you can use that.
  • If there is no workaround, I would add a new constant like TIME_SHORT, or something.

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