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

[Feature Request] Consider adding a feature flag for enhanced determinism option of glam #789

Closed
Ughuuu opened this issue Jul 5, 2024 · 11 comments
Labels
feature Adds functionality to the library

Comments

@Ughuuu
Copy link
Contributor

Ughuuu commented Jul 5, 2024

Consider adding option:

enchanced-determinism = ["glam/libm"]

For glam inside godot-core/Cargo.toml. This option would allow people that want to use the libm flag which would make the math lib cross platform determinism at the expense of speed be able to do so without needing to fork gdext.

@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2024

Can you specify more precisely which operations currently suffer from non-determinism?

The thing is that glam is an implementation detail, and we may re-implement some of their functions in Rust (or move our own functions to use glam). So people relying on such a property may encounter breaking changes.

It would also add a significant maintenance level + test coverage to ensure determinism, which I'm not sure is justified unless multiple people benefit from it, and someone volunteers to put in the necessary work.

@Bromeon Bromeon added the feature Adds functionality to the library label Jul 8, 2024
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jul 8, 2024

My understanding is that the lack of cross platform determinism would come from functions such as: sin, cos, sqrt, etc.

These are in glam and can be cross platform deterministic in case of the flag libm:
bevyengine/bevy#2480

bevy also uses this flag for their cross platform deterministic exports.

already has a libm flag that forces num-traits to use libm where possible,
so we should enable that flag when the enhanced-determinism flag is enabled.
It should be publicly documented that this may have a significant impact on runtime performance

I agree it is an implementation detail, and the workaround for me would be to fork gdext and add this flag (for my use cases). Not sure if it is justified to add this flag unless many people would use it, agree. So you decide.

@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2024

It's hard to make a decision right away, it depends on several factors:

  1. how much of the gdext API is affected
  2. how much of the underlying Godot API is affected, and do we have any control over it
  3. is someone willing to spend effort on determinism tests that run in CI -- having such a feature is pointless if it's not verified

And glam only controls operations on vectors and a handful of other geometric primitives, but what about f32/f64 scalars? Does the Rust standard say anything about those?

@Bromeon Bromeon closed this as completed Jul 8, 2024
@Bromeon Bromeon reopened this Jul 8, 2024
@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jul 8, 2024

Cross platform determinism is hard to test. If the implementation you use for sqrt for eg. is cross platform deterministic, then most likely the whole thing will be. There might be other places that lack of determinism will come from, but that can be found one by one, possibly by a system that integrates this one as well.

  1. From what I think I can find it's only the functions that use those math functions. So eg. if you call Vector::forward(), that internally finds normalized value so it has to use sqrt.
  2. I would assume none of it is affected. Whatever you call into Godot is not going to be cross platform deterministic. So if I wanted to make a lib that is, I need to re-export some stuff, eg. DVector2, DVector3, etc.(D from deterministic).
  3. Maybe I could at some point, or someone else interested in this specific feature. Right now I know this is needed for determinism, but I don't know what else. I just enable it and hope for the best. I am not really an expert in this, so I would need to research to see what constitutes a cross platform deterministic test(my guess is just running a bunch of such math operations until you find one that doesnt equal same on multiple platforms). And then from there try to use functions that godot has to offer(or gdext in this case) to see if they are also not cross platform deterministic without this flag.

I don't know if determinism is something that you should take very seriously, as in test it extensively, or just enable that flag and get it done with. For my use case that flag enabled should be enough. At least as a starting point. The thing is I use the gdext math functions a lot inside my lib logic, wether it is Vector::length or other such functions.

@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2024

The thing is, we already have some "best-effort features" like experimental-threads and experimental-wasm, which are half-implemented and without any guarantees for users. While convenient, it's not really useful to the user if they cannot rely on it -- so I would be careful with advertising that a Cargo feature enables cross-platform determinism, but is extremely vague regarding what that means.

In other words, if we add

enchanced-determinism = ["glam/libm"]

then I want to know and document how it improves determinism, which APIs are covered and which aren't. It's probably just a matter of research, but what I want to avoid is just delegate the responsibility in a "may or may not improve determinism, but find out yourself" way. Also because some of our math may be rewritten in Rust without glam, and it would thus bypass any determinism guarantees.


Cross platform determinism is hard to test.

Why? Can we not serialize the result of operations with known edge cases, and run CI on multiple platforms to make sure the results match the expectations?

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jul 8, 2024

Why would we rewrite our own math instead of using glam? It offers a lot of things out of the box, this included. If we would write our own, what features might we target and what would be the reason to do so?

We can do the last bit, but finding those edge cases is usually hard.

@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2024

Why would we rewrite our own math instead of using glam? It offers a lot of things out of the box, this included. If we would write our own, what features might we target and what would be the reason to do so?

This is not a hypothetical statement, a lot of gdext's vector math is hand-implemented. Sometimes because Godot has different semantics, sometimes because glam does extra logic we don't need, etc. See #721 for recent changes in that direction.

And again: how does determinism generalize to f32/f64 scalar values?


We can do the last bit, but finding those edge cases is usually hard.

That's what I meant with "It would also add a significant maintenance level + test coverage to ensure determinism, which I'm not sure is justified unless multiple people benefit from it, and someone volunteers to put in the necessary work."

Whether hard or not, it needs some time to research. But we're not the first to attempt this, there might even be existing test suites.

@Bromeon
Copy link
Member

Bromeon commented Jul 21, 2024

Ok, to support this, we'd need the following:

  1. forward the libm feature to glam
  2. a way to switch our own vector math to libm as well
  3. some test cases that fail without the enchanced-determinism feature and pass with it -- to demonstrate that it works

1 without 2 makes no sense, as many operations would still be non-deterministic.

Would you be interested in working on points 2 and/or 3?

@Ughuuu
Copy link
Contributor Author

Ughuuu commented Jul 21, 2024

I would be interested, however it would come down to priorities. Possibly I will take this later on, as right now I have quite a few other things to implement on the rapier plugin, and there are also performance issues on gdext I am more interested in hacking if possible. I think we can even close this for now and I can reopen it at a more opportune time.

@Bromeon
Copy link
Member

Bromeon commented Jul 21, 2024

Definitely, no worries! We can keep it open, I would remind you in a few months, and then we can still decide if we want to pursue it or not.

@Bromeon
Copy link
Member

Bromeon commented Sep 10, 2024

@Ughuuu Do you still think this is something you're interested in? If there's no concrete plan on how we can achieve determinism with confidence, I would probably close this. As mentioned above, forwarding the libm feature to glam is just a small part of the story, and on its own, it would only give people a false sense of security.

@Ughuuu Ughuuu closed this as not planned Won't fix, can't repro, duplicate, stale Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adds functionality to the library
Projects
None yet
Development

No branches or pull requests

2 participants