Use doubles for time everywhere in Timer/SceneTree

[aaronfranke]

Single-precision floats should never be used for time, because it fails after a few days.

With doubles, Timer should work for about 4.7 million years before failing, assuming 60 FPS.

EDIT: Here's a cool video that shows how Team Fortress 2 breaks due to how it uses single-precision floats for time: https://www.youtube.com/watch?v=RdTJHVG_IdU

[lawnjelly]

An alternative is the more standard solution for timers: use 64 (or 128) bit unsigned integers for time, often starting at 0 at game start.

Doubles and floats have different behaviour depending on where you are in the range. Differences in integer times can be converted to floating point as close as possible to use, in order to minimize floating point error.

[aaronfranke]

@lawnjelly Yes, integers are better in many cases. The downside is that it can be annoying to use them to store measurements of time accurate to less than a second. You can instead store milliseconds or microseconds or nanoseconds, the latter being common for time APIs, but storing the time in nanoseconds in int64_t leads to the year 2262 problem.

I'm not sure what the use cases are for Timer, but feel free to rewrite them to use integers.

[Calinou]

@aaronfranke I don't think we're guaranteeing better-than-millisecond precision with Timer as of now, so using milliseconds in an int64_t is probably fine.

See also #32382, although the jittering there isn't due to timer precision (as far as I know).

[lawnjelly]

Don't get me wrong BTW, I'm not saying that I think using doubles here in the Timer node is the wrong thing. Using float / double directly does make certain things simpler, such as time scaling. 👍

It's more of a 'while we are examining it' / consider / kind of comment. Integers are often used for time because they have the property of constant resolution and repeatable behaviour across the range. Not so much in the case of double, but particularly with float, as you get to the limits of the range the resolution decreases which can lead to strange behaviour - it increase the 'bug surface area' as there is one more thing to worry about.

You could also go for a millisecond integer as @Calinou suggests (you could probably use a 32 bit uint for this), although you have to consider sampling error with small deltas when you are running at 1000 frames per second and time scaling (if the frame delta is small enough, the timer may never progress as it adds 0 each time, and yes, this has happened to me lol 😁 ).

[aaronfranke]

@lawnjelly There are no 32-bit types suitable to represent time. With milliseconds, an int32_t fails after 248 days, and unsigned numbers only double that to 496 days, while removing the ability to represent times in the past (going unsigned is never worth it for time).

The ideal types are 128-bit, since they come with no compromises. 128-bit ints can store nanoseconds for over a sextillion years, 128-bit floats are similarly... (durable? flexible?)

[lawnjelly]

It seems time_left is already a double before this PR. Afaik decrements to this being a float shouldn't really matter all that much - they will get promoted to double in the expression. (They could still be argued to be better as a double, but it doesn't necessarily fix anything to do with the bug.)

It is only the numbers which are likely to be large that need to be a double (or large integer). This may be why it is structured how it is already.

We really need a min reproduction project for the test case, setting the start clock forward for instance to create the conditions for the bug, reproduce it, then fix it.

There could be for example duff values for delta being passed in from the timing code earlier, at the moment it seems like frantically changing everything to double in the hope that by chance we will catch the right spot, rather than focusing on where the bug is.

[aaronfranke]

I don't see the problem with using doubles here. If the bug still exists after this PR, then there are other issues, but this issue is still valid, since Timer could not be expected to work reliably for more than a few days if it uses single-precision floats due to the loss of precision.

64-bit systems can typically handle doubles at the same speed as floats, aside from very slightly increased memory usage. Godot 4.0 will be targeting 64-bit systems, so I guess there could be a concern that this should not be cherry-picked.

It is only the numbers which are likely to be large that need to be a double

10 days is almost a million seconds. I'd say that OP's use case is perfectly valid and reasonable, and therefore the number needs to be a double.

[lawnjelly]

I am, in case you didn't guess, to a large extent playing devil's advocate. 😁 It's all good stuff for peer review, you have to be able to defend your changes, rather than change for change sake.

I agree that the times stored within timer for the time_left and wait_time might be reasonable to have as double (and their accessors). Changing deltas to double, the case is less compelling (but still might be worth doing, if it does turn out to be effectively free, that will I'm sure come down to e.g. a PR meeting).

In terms of the issue, to me it is more of a concern that we actually pin down what is going on there, rather than using it for a reason for changes that may not solve it.

[Calinou]

Thanks!