Add support for `precision=double` builds of godot #149

lilizoey · 2023-03-06T17:08:46Z

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

Bromeon

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.

Bromeon · 2023-03-06T20:46:49Z

.github/workflows/full-ci.yml


          # 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


So far, the convention was to only provide arguments which don't have a default value (empty in this case).

Bromeon · 2023-03-06T21:20:09Z

godot-core/src/builtin/glam_helpers.rs

@@ -9,12 +9,14 @@
 //
 // Nice:
 //   self.glam2(&with, |a, b| a.dot(b))
-//   self.glam2(&with, glam::f32::Quat::dot)
+//   self.glam2(&with, glam::Real::Quat::dot)


search&replace mistake?

oh yeah, i spotted a couple earlier, missed this one

Bromeon · 2023-03-06T21:27:08Z

godot-core/src/builtin/mod.rs

+    // 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;


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?

it does look more similar to the code the user wants to write, i.e

r as f64

vs

r.as_f64()

Bromeon · 2023-03-06T21:29:33Z

godot-core/src/builtin/mod.rs

+    /// A glam [`Vec2`](glam::Vec2) with floating-point format compatible
+    /// with [`real`].


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.

That's fair, it'd be nice if there was a standard line width in the project tho

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.

I can at least say that i would follow it if there was one

Bromeon · 2023-03-06T21:30:24Z

godot-core/src/builtin/mod.rs

+
+    /// 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.


Capitalization: "GDScript"

and maybe semicolon after it

Bromeon · 2023-03-06T21:31:39Z

godot-core/src/builtin/projection.rs

-    pub fn fov(&self) -> f64 {
-        self.as_inner().get_fov()
+    pub fn fov(&self) -> real {
+        self.as_inner().get_fov() as real


This should probably use real::from_f64, no?

Bromeon · 2023-03-06T21:32:46Z

godot-core/src/builtin/quaternion.rs

-    // 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 {
        }


Leftover capitals 🙂

Bromeon · 2023-03-06T21:34:58Z

godot-core/src/builtin/vector2.rs

@@ -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`.


more capital 💰

Bromeon · 2023-03-06T21:35:46Z

godot-core/src/builtin/vector_macros.rs

@@ -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`.


capitals again

Bromeon · 2023-03-06T21:36:09Z

itest/rust/src/basis_test.rs

@@ -143,7 +143,7 @@ fn basis_equiv() {
        assert_eq_approx!(
            inner,
            outer,
-            |a, b| is_equal_approx(a as f32, b),
+            |a, b| is_equal_approx(a as real, b),


real::from_*?

ttencate · 2023-03-07T08:13:46Z

godot-core/src/builtin/mod.rs

+    // As this is a scalar value, we will use a non-standard type name.
+    #[allow(non_camel_case_types)]
+    pub type real = f64;


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.

That's upon my request, see Discord discussion 🙂

Bromeon · 2023-03-07T08:58:41Z

@SayakS Could you add real to the prelude in godot/src/lib.rs?

lilizoey · 2023-03-07T16:01:39Z

@SayakS Could you add real to the prelude in godot/src/lib.rs?

Like, explicitly? it already is in the prelude because of the pub use super::builtin::*;

Bromeon · 2023-03-07T16:18:31Z

Ah true, it's already re-exported. No, in that case not 🙂

Bromeon · 2023-03-07T20:59:28Z

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 default and rustup override that would be picked up automatically by check.sh.
- If we want to enforce running on stable, we can parse cargo --version for occurrences of "nightly". I'm not saying we should do this, but there are other ways to make sure check.sh is run on stable, besides passing +stable.
The extra -- is conventionally used when disambiguating positional/named arguments, or when having two "levels" (like the frontend cargo and backend rustc).
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.

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 🙂

Bromeon

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!

Bromeon · 2023-03-07T21:00:20Z

.github/workflows/full-ci.yml

@@ -98,6 +124,13 @@ jobs:
            rust-toolchain: stable
            rust-special: -minimal-deps
            godot-binary: godot.linuxbsd.editor.dev.x86_64
+            with-llvm: false


We could remove this

Bromeon · 2023-03-07T21:00:58Z

Contributing.md

+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
+```


This would become a simple check.sh --double, see above

Bromeon · 2023-03-07T21:03:49Z

godot-core/src/builtin/math.rs

+        #[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
+        );


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 🙂)?

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

lilizoey · 2023-03-07T21:22:15Z

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!

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

Bromeon · 2023-03-07T21:27:10Z

there was never a version that had just --double but 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?

lilizoey · 2023-03-07T21:28:08Z

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

lilizoey · 2023-03-07T21:32:39Z

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

Bromeon · 2023-03-07T21:33:49Z

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 🙂

lilizoey · 2023-03-07T22:24:49Z

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.

Bromeon · 2023-03-07T23:26:49Z

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.

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.

As I mentioned earlier:

If we want to enforce running on stable, we can parse cargo --version for occurrences of "nightly". I'm not saying we should do this, but there are other ways to make sure check.sh is 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.

lilizoey · 2023-03-08T00:18:35Z

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 +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.

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.

lilizoey · 2023-03-08T00:25:22Z

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.

Bromeon · 2023-03-08T08:21:30Z

In that case, what do you think about this?

If we want to enforce running on stable, we can parse cargo --version for occurrences of "nightly". I'm not saying we should do this, but there are other ways to make sure check.sh is 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".

ttencate · 2023-03-08T08:50:49Z

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.

Bromeon · 2023-03-08T10:23:16Z

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

Bromeon

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+

bors · 2023-03-08T16:09:39Z

Build succeeded:

full-ci

Bromeon added feature Adds functionality to the library c: engine Godot classes (nodes, resources, ...) labels Mar 6, 2023

Bromeon mentioned this pull request Mar 6, 2023

Project status overview #24

Open

lilizoey mentioned this pull request Mar 6, 2023

Add a real type #135

Closed

Bromeon reviewed Mar 6, 2023

View reviewed changes

lilizoey force-pushed the feature/real branch from 8cffe7f to 7f2a673 Compare March 6, 2023 23:34

ttencate reviewed Mar 7, 2023

View reviewed changes

Bromeon reviewed Mar 7, 2023

View reviewed changes

lilizoey force-pushed the feature/real branch from 55703c9 to f165c03 Compare March 7, 2023 22:21

Add support for precision=double

341bc96

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

lilizoey force-pushed the feature/real branch from f165c03 to 341bc96 Compare March 8, 2023 15:44

Bromeon approved these changes Mar 8, 2023

View reviewed changes

bors bot merged commit 48b509a into godot-rust:master Mar 8, 2023

lilizoey deleted the feature/real branch April 16, 2023 16:23

		/// A glam [`Vec2`](glam::Vec2) with floating-point format compatible
		/// with [`real`].

Add support for precision=double builds of godot #149

Add support for precision=double builds of godot #149

Conversation

lilizoey commented Mar 6, 2023 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Mar 7, 2023

lilizoey commented Mar 7, 2023

Bromeon commented Mar 7, 2023

Bromeon commented Mar 7, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

lilizoey commented Mar 7, 2023 • edited Loading

Bromeon commented Mar 7, 2023 • edited Loading

lilizoey commented Mar 7, 2023

lilizoey commented Mar 7, 2023

Bromeon commented Mar 7, 2023 • edited Loading

lilizoey commented Mar 7, 2023 • edited Loading

Bromeon commented Mar 7, 2023 • edited Loading

lilizoey commented Mar 8, 2023 • edited Loading

lilizoey commented Mar 8, 2023

Bromeon commented Mar 8, 2023 • edited Loading

ttencate commented Mar 8, 2023

Bromeon commented Mar 8, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

bors bot commented Mar 8, 2023

Add support for `precision=double` builds of godot #149

Add support for `precision=double` builds of godot #149

lilizoey commented Mar 6, 2023 •

edited

Loading

lilizoey Mar 7, 2023 •

edited

Loading

lilizoey commented Mar 7, 2023 •

edited

Loading

Bromeon commented Mar 7, 2023 •

edited

Loading

Bromeon commented Mar 7, 2023 •

edited

Loading

lilizoey commented Mar 7, 2023 •

edited

Loading

Bromeon commented Mar 7, 2023 •

edited

Loading

lilizoey commented Mar 8, 2023 •

edited

Loading

Bromeon commented Mar 8, 2023 •

edited

Loading

Bromeon left a comment •

edited

Loading