Add support for precision=double builds of godot by lilizoey · Pull Request #149 · godot-rust/gdext (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation41 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Add a feature precision-double which when enabled will make the various structs that use real_t in godot have f64 values instead of f32 values.
Add a type real which is an alias for f32 or f64 depending on the above
Add the ability to pass --features features to check.sh, so we can easily run check.sh with precision-double enabled
Add double-builds in the full-ci
Add double-build in the minimal-ci for clippy
One question:
Should we maintain the glam::Vec2 convention somehow? maybe make a module glam_real and do glam_real::RVec2?
Resolves #135
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this feature! 👍
precision-double sounds weird somehow. I see where it's coming from (precision=double argument), but I would probably name the feature double-precision. What do you think?
Sorry for merge conflicts, I renamed binary-filename into godot-binary on master.
| # Don't use latest Ubuntu (22.04) as it breaks lots of ecosystem compatibility. |
| # If ever moving to ubuntu-latest, need to manually install libtinfo5 for LLVM. |
| - name: linux |
| os: ubuntu-20.04 |
| rust-toolchain: stable |
| godot-binary: godot.linuxbsd.editor.dev.x86_64 |
| with-llvm: false |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, the convention was to only provide arguments which don't have a default value (empty in this case).
| @@ -9,12 +9,14 @@ |
|---|
| // |
| // Nice: |
| // self.glam2(&with, |a, b |
| // self.glam2(&with, glam::f32::Quat::dot) |
| // self.glam2(&with, glam::Real::Quat::dot) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
search&replace mistake?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah, i spotted a couple earlier, missed this one
Comment on lines +137 to +140
| // Clippy complains that this is an `as_*` function but it takes a `self` |
|---|
| // however, since this uses `as` internally it makes much more sense for |
| // it to be named `as_f32` rather than `to_f32`. |
| #[allow(clippy::wrong_self_convention)] |
| fn as_f32(self) -> f32; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Rust, as_ functions almost always return a reference or raw pointer by convention.
They express: "return a different representation of this very object", i.e. similar to a transmute, and not a copy.
On the other hand, to_ suggests that a different type is returned (a conversion takes place), but here this is only true in half the cases. So maybe not ideal either.
I'm not very sure about this, more opinions on this matter?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does look more similar to the code the user wants to write, i.e
vs
Comment on lines 199 to 212
| /// A glam [`Vec2`](glam::Vec2) with floating-point format compatible |
|---|
| /// with [`real`]. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use a single line for these comments?
In general you use very short line widths, while screens are super wide 😉 probably a matter of taste, but you could easily use 1.5x your current width in my opinion.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair, it'd be nice if there was a standard line width in the project tho
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is, as long as there is no tooling to enforce it, such a convention will just be ignored in every PR, and I can spend the rest of my free time bikeshedding line breaks 😉
rustfmt nightly has max_width, which may be a start -- but it seems like it only breaks longer lines, and doesn't extend shorter ones -- which would make it rather useless.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can at least say that i would follow it if there was one
| /// Floating point type used for many structs and functions in Godot. |
| /// |
| /// This is not the `float` type in gdscript, that type is always 64-bits. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalization: "GDScript"
and maybe semicolon after it
| pub fn fov(&self) -> f64 { |
|---|
| self.as_inner().get_fov() |
| pub fn fov(&self) -> real { |
| self.as_inner().get_fov() as real |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use real::from_f64, no?
Comment on lines 181 to 195
| // pub fn spherical_cubic_interpolate(self, b: Self, pre_a: Self, post_b: Self, weight: f32) -> Self {} |
|---|
| // pub fn spherical_cubic_interpolate(self, b: Self, pre_a: Self, post_b: Self, weight: Real) -> Self {} |
| // TODO: Implement godot's function in rust |
| /* |
| pub fn spherical_cubic_interpolate_in_time( |
| self, |
| b: Self, |
| pre_a: Self, |
| post_b: Self, |
| weight: f32, |
| b_t: f32, |
| pre_a_t: f32, |
| post_b_t: f32, |
| weight: Real, |
| b_t: Real, |
| pre_a_t: Real, |
| post_b_t: Real, |
| ) -> Self { |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover capitals 🙂
| @@ -42,8 +42,8 @@ impl Vector2 { |
|---|
| /// Vector with all components set to `1.0`. |
| pub const ONE: Self = Self::splat(1.0); |
| /// Vector with all components set to `f32::INFINITY`. |
| pub const INF: Self = Self::splat(f32::INFINITY); |
| /// Vector with all components set to `Real::INFINITY`. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more capital 💰
| @@ -167,7 +167,7 @@ macro_rules! impl_vector_operators { |
|---|
| ( |
| // Name of the vector type to be implemented, for example `Vector2`. |
| $Vector:ty, |
| // Type of each individual component, for example `f32`. |
| // Type of each individual component, for example `Real`. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
capitals again
| @@ -143,7 +143,7 @@ fn basis_equiv() { |
|---|
| assert_eq_approx!( |
| inner, |
| outer, |
| |a, b |
| |a, b |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
real::from_*?
Comment on lines +233 to +238
| // As this is a scalar value, we will use a non-standard type name. |
|---|
| #[allow(non_camel_case_types)] |
| pub type real = f64; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about that, shouldn't it be Real? Even if it's an alias for a primitive, it's still an alias. It feels weird to me to name it using the convention that's normally reserved for built-in primitives (and modules! causing you a naming conflict here).
I don't know of any prior art in std, but there's some in the Rust By Example book.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's upon my request, see Discord discussion 🙂
@SayakS Could you add real to the prelude in godot/src/lib.rs?
@SayakS Could you add
realto the prelude ingodot/src/lib.rs?
Like, explicitly? it already is in the prelude because of the pub use super::builtin::*;
Ah true, it's already re-exported. No, in that case not 🙂
Maybe some general points regarding check.sh: I see it as an abstraction over the most common workflows, not a highly configurable build tool. For the latter, the user/developer can (and should) always call cargo directly. This means, it needs to provide only the most common operations out of the box.
- As such, we don't need nightly support, because CI doesn't run on nightly, either (or only for specific jobs). If the user really wants another toolchain, there is
rustup defaultandrustup overridethat would be picked up automatically bycheck.sh.- If we want to enforce running on stable, we can parse
cargo --versionfor occurrences of "nightly". I'm not saying we should do this, but there are other ways to make surecheck.shis run on stable, besides passing+stable.
- If we want to enforce running on stable, we can parse
- The extra
--is conventionally used when disambiguating positional/named arguments, or when having two "levels" (like the frontendcargoand backendrustc). - Similar point about features: we generally want the checks to run with a fixed set of features, in order to pass CI or generate docs. It makes most sense to run local checks for "switch features" that are mutually exclusive, not additive -- such as
double-precision.- I thus think it may be easier to have a single flag
--double, like before this commit, but even shorter.
- I thus think it may be easier to have a single flag
Overall, usage of check.sh should remain as simple as possible, and the number of operations/flags very limited. It's a best-effort tool that we provide to assist the user, not a replacement for CI or a sandbox for experimentation.
I feel a bit sorry for the effort that went into building this complex bash functionality; I hope it can be useful somewhere else. To avoid this, we could maybe quickly discuss these things beforehand 🙂
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks very good by now! Maybe revert the check.sh to the previous revision (with --double instead of --features double-precision), other than that only minor things left!
| @@ -98,6 +124,13 @@ jobs: |
|---|
| rust-toolchain: stable |
| rust-special: -minimal-deps |
| godot-binary: godot.linuxbsd.editor.dev.x86_64 |
| with-llvm: false |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove this
Comment on lines 69 to 72
| To run the testing suite with `double-precision` enabled you may add `--features double-precision` to a `check.sh` invocation: |
|---|
| ``` |
| $ check.sh -- --features double-precision |
| ``` |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would become a simple check.sh --double, see above
Comment on lines +240 to +256
| #[cfg(not(feature = "double-precision"))] |
|---|
| assert_eq_approx!( |
| lerp_angle(0.0, PI + 3.0 * TAU, 0.5), |
| FRAC_PI_2, |
| is_angle_equal_approx |
| ); |
| #[cfg(feature = "double-precision")] |
| assert_eq_approx!( |
| lerp_angle(0.0, PI + 3.0 * TAU, 0.5), |
| -FRAC_PI_2, |
| is_angle_equal_approx |
| ); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this pi / 2 for floats, but -pi / 2 for doubles?
Rounding errors?
Could you please add a TODO so we write a more robust angle comparison function later (outside of scope for this PR, let's not delay further 🙂)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's documented higher up that it can be unpredictable which way lerp goes when lerping towards multiples of PI, so this is just showing off that. and it kinda unsurprisingly is just different for f32 and f64
besides its a bit hard to know what would be the right thing to do when lerping towards a multiple of PI anyway, and I'm skeptical that it's like technically possible to write a completely consistent one just because of float inaccuracies
It looks very good by now! Maybe revert the
check.shto the previous revision (with--doubleinstead of--features double-precision), other than that only minor things left!
there was never a version that had just --double but i can do that. though there were some bugs with the previous version that i fixed in this revision so it wont be a full revert but i can just make it take --double as an argument. i mostly did it this way just to have an easy way to pass in any extra arguments as needed, thus the -- thing since it's passing along the arguments after to the inner command.
nightly is mainly cause it's useful to run it with nightly locally sometimes even if the CI doesn't run witb nightly. cause it can spot issues stable cant that'd be useful to fix anyway even if stable wont complain about them.
the intent isn't to make it super flexible but just an easy way to do the things that I've noticed myself wanting:
- pass in arbitrary extra things to the command
- easily pick a toolchain with stable as default
there was never a version that had just
--doublebut i can do that
My wording was a bit imprecise, the "with" was meant as "additionally". I fully agree that you shouldn't do a full revert 🙂
nightly is mainly cause it's useful to run it with nightly locally sometimes even if the CI doesn't run witb nightly. cause it can spot issues stable cant that'd be useful to fix anyway even if stable wont complain about them.
Out of curiosity, do you remember some of the issues that nightly could find?
Out of curiosity, do you remember some of the issues that nightly could find?
i think mainly that the new itest macro impl has some unnecessary returns, i can double check later
also dont feel sorry about the effort i put in for the check script lol i just did it cause it came naturally to me and im not super attached to my code in that sense. either way i know bash better :p
and like i know that when adding some feature that i havent discussed with anyone it's a risk that it wont be accepted
I think the "allow any configuration to be passed in" is a kind of symptom fighting. Having limited options has the advantage that people either work with the available API, or are incentivized to add concrete flags/switches that help their workflows.
In practice, this would mean:
- if a certain cargo configuration is very often-used by multiple people, consider adding another flag like
--double - if nightly adds some extra checks, see if those can be enabled in stable as well
Doing so would also benefit everyone, not just one developer who has figured out "the one true configuration" 😎
I think we should give it some time though, definitely not in this PR 🙂
I kept some things that are strictly speaking unneccessary in the check.sh script but which are not visible from the outside. such as keeping toolchain stored in a variable and extraArgs as an array. so if someone wants to run check.sh in a non-standard way they can modify those variables to do that, but it's not a thing you can do with command-line arguments.
And also just cause it was easier to just keep the code mostly the same as before but just hardcode in the values, since it already worked pretty well.
Also since check.sh is meant to like be a local version of running CI-checks basically, i did just make it always run on stable by passing in +stable. As otherwise you will, (like me) sometimes have check.sh fail even when CI doesn't just cause you have nightly as your default.
Also since
check.shis meant to like be a local version of running CI-checks basically, i did just make it always run on stable by passing in+stable.
Does this also work if someone explicitly installs a Rust version like 1.65?
Also, this still doesn't guarantee that they run the same toolchain as CI. They may have updated their stable channel a year ago.
- If we want to enforce running on stable, we can parse
cargo --versionfor occurrences of "nightly". I'm not saying we should do this, but there are other ways to make surecheck.shis run on stable, besides passing+stable.
Not specifying +stable would also keep the cargo statements clutter-free, and with it the printing of each command.
Your change is not hidden to the user, they see what is being executed.
Does this also work if someone explicitly installs a Rust version like
1.65?
I can check
Also, this still doesn't guarantee that they run the same toolchain as CI. They may have updated their stable channel a year ago.
Well yeah, but someone running on a recently updated version of rust will never use the same toolchain as CI if they're using nightly. Stable at least is the right one for most common configurations.
Not specifying
+stablewould also keep the cargo statements clutter-free, and with it the printing of each command. Your change is not hidden to the user, they see what is being executed.
Which was part of my issue, i ran identical commands and got different outcomes. That is cargo clippy ..., ./check.sh both failed to run whereas CI succeeded. But the command run by ./check.sh was identical to the one run by the CI. At least this way i could've noticed that ./check.sh would succeed where cargo clippy ... failed, and the one difference was the +stable.
Hiding this from the user would've lead to more confusion. Because then i would run ./check.sh and it would succeed, but manually running the clippy command from check.sh would fail. I would've probably eventually realized that check.sh was using stable rust, but making it explicit that it needs to run with stable makes it more clear what is happening. It can also suggest to the user that to get identical output to CI you should have the most recent stable.
Not to mention that until we have a better solution (and making a bigger more complicated solution to figure this out should probably not be in this PR), then we'll just end up with the check.sh potentially not working properly for people who use nightly as their default toolchain.
Does this also work if someone explicitly installs a Rust version like
1.65?
It seems that if a toolchain is missing, cargo will error. So if someone has manually installed a rust version that is not a stable version of rust and has no other stable version of rust, this could cause an error.
But at least until we do a better solution than this, we're more likely to see people having their check.sh fail because they're using nightly, than we are to see someone having their check.sh fail because they dont have a stable toolchain installed.
In that case, what do you think about this?
- If we want to enforce running on stable, we can parse
cargo --versionfor occurrences of "nightly". I'm not saying we should do this, but there are other ways to make surecheck.shis run on stable, besides passing+stable.
The big advantage I see, is that it ensures that the toolchain is configured properly for the whole directory, not just inside check.sh. For example using rustup override.
Otherwise, someone runs check.sh (with stable) and then cargo build (with nightly), they still get discrepancies. In other words, here I prefer the approach "tell the user they're wrong" over "silently correct what they probably meant".
Have we considered adding a rust-toolchain file to just pin everyone (including CI) to a particular rustc version? It overrides the system default, but can still be overridden by the contributor if they have a need for that.
Toolchain files are quite cool! 🙂 Although I'm not sure they would help a lot here -- I don't think we should pin our project to a specific Rust and/or toolchain version.
What matters is:
- there's a strict MSRV
- if it passes CI, it's good, otherwise not
Why not leave developers some freedom? It's up to everyone to use nightly version if that helps them in some ways, or to use an older compiler if it's within MSRV bounds. In the end, CI makes the judgement call, and I'd encourage everyone to commit/push regularly in their forks, not only after 3 days of work.
I also think there are limits regarding how much tooling we should provide in the repo itself. I spent quite a bit of effort on an elaborate CI infrastructure, and have no plans replicating more of it locally, let alone maintaining both in sync. Yes, it may help some users to find discrepancies between local and upstream setups, but it may as well limit others that have a perfectly valid configuration, so I'm not convinced being overly strict would cause a net decrease in support cases. Furthermore, every piece of infrastructure in existence is something that needs to be maintained, updated, can break and cause problems. I find time is more meaningfully spent on the library than on check.sh and client-side tooling 😉
add transform2d/3d-itests back Add --double to check.sh Make check.sh run in the stable toolchain, since that's what the CI does
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for bringing this to a polished state! 👍
As discussed on Discord: there are now quite a lot of CI runs. Maybe some (like unit-tests) don't need to be run for both single and double configuration. Even if vectors etc. contain real, this would likely only affect us if we specifically test for floating-point inaccuracies; as such, the engine/integration tests may be more interesting. But it's good for now, let's see how it goes!
bors r+
Labels
Godot classes (nodes, resources, ...)
Adds functionality to the library