-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Make precision of time_step consistently float. #41853
Conversation
c0425c8
to
03bad74
Compare
03bad74
to
5cc634e
Compare
Rebased following #43052. |
5cc634e
to
de816e2
Compare
Rebased following #44094. |
de816e2
to
76465e4
Compare
I agree with @aaronfranke that we should probably use double everywhere there. There is no extra cost for doing that and it is common to accumulate time over some frames sometimes. I know that in practice it wont really make a difference, but given the choice it sounds like the better one. |
I also agree on doubles, if in doubt. The purist in me would use an encapsulated 64 (or 128) bit integer timer class and only convert to floating point for differences where required, because this offers completely stable behaviour over time. This is why operating systems use integers for timers instead of double. But I can see that might be hard to understand for most contributors, and simply using a double might be a good compromise. In this particular case of course, the situation is even worse (irrespective of what precision time is fed in), because the time value is being passed to shaders which may be using less precision than 32 bits, hence the need for a wraparound. But I guess that can't be helped easily. |
Whether to use |
@madmiraal No, it's not beyond the scope of this PR. Merging #38880 would make this PR obsolete (as would merging an |
76465e4
to
6c11986
Compare
Rebased following merge of #45672. |
6c11986
to
328179f
Compare
Rebased following merge of #45852. |
Superseded by #38880. |
The precision of
time_step
isfloat
. However, there are a number of locations wheretime_step
is converted to adouble
. This PR makes the uses oftime_step
consistently afloat
.Note: The driver for this change is to avoid lgtm warning about potential
float
overflows: If the reason for a conversion to adouble
were the need to increase the maximum size of thefloat
, and the conversion to adouble
was done after a calculation, there is a risk of afloat
overflow that would go unnoticed. In other words, if there is actually a need for adouble
when usingtime_step
in a calculation to prevent afloat
overflow (or a loss of precision, for example, when the numbers are larger) then the conversion to adouble
should be done earlier and made explicit.The only place I have found where the conversion to a
double
may be required is when storingRasterizerCanvasRD::State.time
:godot/servers/rendering/rasterizer_rd/rasterizer_rd.cpp
Lines 84 to 87 in 5bf28c7
However, I think it's unlikely this needs to be a
double
, because, at most the size is limited to the size ofrendering/limits/time/time_rollover_secs
, which, by default, is 3,600, and even withinRasterizerCanvasRD::State
theRasterizerCanvasRD::State.Buffer.time
uses afloat
too:godot/servers/rendering/rasterizer_rd/rasterizer_canvas_rd.h
Lines 397 to 422 in 5bf28c7