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 doubles for time in many other places #51294

Merged
merged 2 commits into from
Aug 9, 2021

Conversation

aaronfranke
Copy link
Member

Follow-up to #38880 and a subset of #21922 split into its own PR for easier reviewing.

Single-precision floats should never be used for time, because it fails after a few days as errors accumulate over time. With doubles, this code should work for about 4.7 million years before failing, assuming 60 FPS.

Unlike #38880 which aims to solve a specific problem (#38878), this PR is just broadly replacing float with double in time APIs. This should not impact performance on 64-bit systems (or extremely little), the main downside is slightly increased memory usage.

An alternative implementation would be to use real_t, in which case this code would only work for more than a few days if Godot was also using doubles for vectors, though I think it's better to use doubles for time always.

@akien-mga
Copy link
Member

Needs a rebase.

@akien-mga akien-mga merged commit 7bcfc66 into godotengine:master Aug 9, 2021
@akien-mga
Copy link
Member

Thanks!

@MohammadKhashashneh
Copy link
Contributor

@aaronfranke I'm fixing a PR regarding cumulative time to use doubles as well, and I noticed the binding of both _process and _physics_process in core/os/main_loop.main are still set as float.
shouldn't it be double as well?

@aaronfranke
Copy link
Member Author

@MohammadKhashashneh Are you referring to this?

	BIND_VMETHOD(MethodInfo(Variant::BOOL, "_physics_process", PropertyInfo(Variant::FLOAT, "delta")));
	BIND_VMETHOD(MethodInfo(Variant::BOOL, "_process", PropertyInfo(Variant::FLOAT, "delta")));

It's a bit confusing, but Variant::FLOAT is actually C++ double, see core/variant/variant.h#L204.

@MohammadKhashashneh
Copy link
Contributor

indeed!
Well, what about the documentation in doc/classes/Node.xml ? same story as well?

@aaronfranke
Copy link
Member Author

aaronfranke commented Aug 17, 2021

@MohammadKhashashneh Yes. Variant only has 64-bit floats and ints, and this is what is bound and used in GDScript and other languages. If other sizes are used internally (such as C++ float) then it's automatically casted to and from Variant's internal type (aka C++ double in the case of floats). This type is named Variant::FLOAT in the binding and "float" in the docs.

@MohammadKhashashneh
Copy link
Contributor

@aaronfranke well noted, thank you

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.

3 participants