Replace euclid with glam (proof-of-concept) #713

Demindiro · 2021-03-18T15:37:05Z

I had some time to take a look at #594. I decided to go with glam since it compiles the fastest.

I went with the "front-end" approach as I believe that is the most flexible approach.

I did rename a few things (like zero() to ZERO, quaternion() to new(), ...) but that is easy to revert if required.

It doesn't seem like many changes are required. Wrapping all the methods in glam is easy to do with extensive use of glam() and into(). The crate does seem to miss implementations for a few types and methods like Rect2 and rotated, so we'll have to implement those from scratch.

the impl Mul/Div/Add/...s for Vector2 and Vector3 are pretty much identical, maybe there is some way to reduce duplication?

Closes #594
Closes #670 (Vector3 is fully implemented)

Bromeon

Thanks a lot for the PR!

I haven't had a chance to look at everything yet, but I already left some remarks. Also some files are indented with Tab instead of Spaces (which is The Right Way ™️ but unfortunately not what godot-rust uses 😬). You might want to use cargo fmt.

Regarding glam vs. other vector libraries: since they become an implementation detail, I'm fine with starting with glam without very deep API comparison. If necessary, we can always change the back-end later without breaking the API.

examples/scene_create/Cargo.toml

gdnative-core/src/core_types/quat.rs

gdnative-core/src/core_types/mod.rs

Bromeon · 2021-03-18T17:17:41Z

gdnative-core/src/core_types/mod.rs

@@ -22,6 +25,9 @@ pub mod vector2;
 pub mod vector2_array;


This is more a comment towards @toasteater and the dev team, not specific to this PR:

Do we export both the inner module and core_types? In my opinion, it could make sense to keep these mod declarations private, and just the pub mod re-export public.

try

Build failed:

ci

Demindiro · 2021-03-23T12:14:08Z

I'll try to fix the tests this evening and then it should be ready.

Bromeon · 2021-03-23T14:50:26Z

by @Bromeon:
One thing we should still discuss: before we had a dedicated Angle type, now the functions just expose f32. Would it make sense to keep that for type safety (at a cost of verbosity) or not?

If we decide to have our own Angle, let's move it to a separate PR, so we can also discuss the API and so on.

@toasteater Let's discuss that in a separate issue then, to keep the scope of this PR limited?

ghost · 2021-03-23T16:11:33Z

@Bromeon I agree. Issue created: #716

Demindiro · 2021-03-23T20:52:38Z

The tests will still fail due to mentioned ICE and is_equal_approx seemingly being stricter (or perhaps it has something to do with glam implementing things differently? Switching to Vec3 doesn't seem to change anything though).

ghost

Please remember to run cargo fmt, cargo clippy, and the test suite before pushing.

Regarding approx strictness, you can pass custom epsilons to the macros: relative_eq!(a, b, epsilon = 0.0001). We may or may not want that to be a parameter on is_equal_approx, but I suppose probably yes?

ghost · 2021-03-24T07:12:55Z

gdnative-core/src/core_types/vector3.rs

+    /// Returns the axis of the vector's largest value. See `Axis` enum.
+    /// If all components are equal, this method returns `Axis::X`.
+    #[inline]
+	#[allow(clippy::collapsible-if)]


collapsible_if. Also this is a tab: run cargo fmt before pushing. May I ask why did you ignore instead of use the lint suggestion?

I ignore it because I believe the current version is more readable than the collapsed version.

if self.z > self.y { if self.z > self.x { Axis::Z } else { Axis::X } } else { if self.y > self.x { Axis::Y } else { Axis::X } }

vs

if self.z > self.y { if self.z > self.x { Axis::Z } else { Axis::X } } else if self.y > self.x { Axis::Y } else { Axis::X }

ghost · 2021-03-24T07:13:08Z

gdnative-core/src/core_types/vector3.rs

+    /// Returns the axis of the vector's smallest value. See `Axis` enum.
+    /// If all components are equal, this method returns `Axis::Z`.
+    #[inline]
+	#[allow(clippy::collapsible-if)]


Same as before.

gdnative-core/src/core_types/vector3.rs

ghost · 2021-03-24T07:24:33Z

As far as I can tell everything is functional (the examples work). The derive tests do fail though due to hardcoded output for one test and an ICE for a couple others.

I did not see any ICEs. Which version of rustc are you using?

Demindiro · 2021-03-24T13:45:04Z

I did not see any ICEs. Which version of rustc are you using?

I was using rustc 1.51.0-nightly (04caa632d 2021-01-30), but rustc 1.53.0-nightly (673d0db5e 2021-03-23) seems to have fixed the issue.

I also reran the tests and they fail again due to very slightly differing output. Should I update the tests or keep it as it is? (the tests do pass when running cargo test +stable)

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: unknown argument, expected one of:
    to_variant_with, from_variant_with, with, skip_to_variant, skip_from_variant, skip
 --> $DIR/from_variant_fail_07.rs:5:15
  |
5 |     #[variant(aoeu = "aoeu")]
  |               ^^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: unknown argument, expected one of:
to_variant_with, from_variant_with, with, skip_to_variant, skip_from_variant, skip
 --> $DIR/from_variant_fail_07.rs:5:15
  |
5 |     #[variant(aoeu = "aoeu")]
  |               ^^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ghost · 2021-03-24T14:14:57Z

@Demindiro Keep them as-is. UI tests should always use stable as a reference, never nightly.

ghost · 2021-03-28T08:35:56Z

While there are certainly things that are still debatable, like the placement of the internal trait, I feel that this PR has gone through enough, and further detailed things can be taken care of in additional follow-up PRs instead.

@Demindiro When you think that this PR is ready, please remove any dbg! macros and commented code, and ping one of us for merging! Please remember to run cargo fmt, cargo clippy, and the test suite before pushing to avoid problems with CI. We use bors to ensure that CI always passes on the master branch. We're unable to merge a PR that doesn't pass CI.

bors try

bors · 2021-03-28T08:37:18Z

try

Build failed:

ci

Bromeon · 2021-03-28T19:11:59Z

bors try

bors · 2021-03-28T19:22:29Z

try

Build failed:

ci

Demindiro · 2021-03-28T19:22:34Z

clippy on stable passes but nightly fails (even though I use nightly and it doesn't fail for me). Do I need to change anything?

Also, apparently clamp is unstable even though the linked issue has been closed since November 2020. Do I need to change that too?

It also seems the stable output of the tests matches that of my nightly output. However, if I run the tests as stable it passes. I've also never touched those tests.

ghost · 2021-03-29T09:36:18Z

According to the build log you can apparently update the UI tests with TRYBUILD=overwrite. I've looked at the current test outputs and I think they're acceptable.

clippy on stable passes but nightly fails (even though I use nightly and it doesn't fail for me). Do I need to change anything?

No, it's allowed for nightly clippy to fail.

Also, apparently clamp is unstable even though the linked issue has been closed since November 2020. Do I need to change that too?

That's from rustc 1.46 which was a lot older than that I think? We haven't decided on a MSRV policy yet, but if you can work around it, please do for the moment being.

Bromeon · 2021-03-30T08:29:49Z

bors try

bors · 2021-03-30T08:41:07Z

try

Build failed:

ci

ghost · 2021-03-30T08:48:04Z

error: Invalid syntax
expecting #[variant(...)]. See documentation:
https://docs.rs/gdnative/0.9.0/gdnative/core_types/trait.ToVariant.html#field-attributes
 --> $DIR/from_variant_fail_02.rs:6:15
  |
6 |     #[variant(baz::quux)]
  |               ^^^^^^^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Invalid syntax
expecting #[variant(...)]. See documentation:
https://docs.rs/gdnative/0.9.0/gdnative/core_types/trait.ToVariant.html#field-attributes
 --> $DIR/from_variant_fail_02.rs:6:15
  |
6 |     #[variant(baz::quux)]
  |               ^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Found baz::quux, expected one of:
to_variant_with, from_variant_with, with, skip_to_variant, skip_from_variant, skip
 --> $DIR/from_variant_fail_03.rs:6:15
  |
6 |     #[variant(baz::quux = "path::to::function")]
  |               ^^^^^^^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error: Found baz::quux, expected one of:
to_variant_with, from_variant_with, with, skip_to_variant, skip_from_variant, skip
 --> $DIR/from_variant_fail_03.rs:6:15
  |
6 |     #[variant(baz::quux = "path::to::function")]
  |               ^^^
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

I think the expected outputs are preferrable here, but I'm not sure why the output for those errors changed. Shouldn't the error spans be provided by the derive macros? I don't think this PR touched those. Perhaps a rustc regression?

EDIT: It turns out that the current expected outputs are from a nightly compiler. Presumably the difference is caused by the availability of Span::join there. Maybe we'll want to disable those tests for stable compilers instead of changing the expected output back.

ghost · 2021-03-30T08:52:33Z

@Demindiro It appears that you're having difficulty with the test suite. Since you have enabled the option to allow maintainers to push to your PR branch, would you mind if I fixed things and squashed the commits directly on your branch?

Demindiro · 2021-03-30T11:18:12Z

Please do, this is getting ridiculous.

ghost

This is definitely a big improvement over what we had. Thanks for the PR! I'm sorry that you've hit a few speedbumps with your first PR. I hope you'll take our enforcement of CI and bikeshedding of seemingly trivial points as evidence that we really care about the code here :)

Dealt with tests and squashed.

bors r+

713: Replace euclid with glam (proof-of-concept) r=toasteater a=Demindiro I had some time to take a look at #594. I decided to go with `glam` since it compiles the fastest. I went with the "front-end" approach as I believe that is the most flexible approach. I did rename a few things (like `zero()` to `ZERO`, `quaternion()` to `new()`, ...) but that is easy to revert if required. It doesn't seem like many changes are required. Wrapping all the methods in `glam` is easy to do with extensive use of `glam()` and `into()`. The crate does seem to miss implementations for a few types and methods like `Rect2` and `rotated`, so we'll have to implement those from scratch. the `impl Mul/Div/Add/...`s for `Vector2` and `Vector3` are pretty much identical, maybe there is some way to reduce duplication? Closes #594 Closes #670 (`Vector3` is fully implemented) Co-authored-by: David Hoppenbrouwers <david@salt-inc.org> Co-authored-by: toasteater <48371905+toasteater@users.noreply.github.com>

bors · 2021-03-30T18:25:45Z

Build failed:

ci

Update tests with the added `glam` dependency. Run tests that depend on Span::join only on nightly.

ghost · 2021-03-30T18:31:55Z

Well this is embarrasing

bors retry

bors · 2021-03-30T18:48:44Z

Build succeeded:

ci

Bromeon · 2021-03-30T19:16:59Z

Cool to see this merged! 🍾

Thanks a lot to @Demindiro for your contribution and the patience, and @toasteater for the finishing touches! 🙂

Bromeon reviewed Mar 18, 2021

View reviewed changes

ghost reviewed Mar 19, 2021

View reviewed changes

Bromeon reviewed Mar 20, 2021

View reviewed changes

gdnative-core/src/core_types/vector2.rs Show resolved Hide resolved

ghost reviewed Mar 23, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Mar 23, 2021

Try #713:

9329fbb

ghost mentioned this pull request Mar 23, 2021

[discussion] Adding an Angle type #716

Closed

Demindiro marked this pull request as ready for review March 23, 2021 20:50

ghost reviewed Mar 24, 2021

View reviewed changes

Demindiro force-pushed the glam branch from 2a304a6 to d755789 Compare March 24, 2021 13:48

bors bot added a commit that referenced this pull request Mar 28, 2021

Try #713:

b2584da

Demindiro force-pushed the glam branch from 61ef718 to 7c45702 Compare March 28, 2021 17:56

Demindiro requested a review from a user March 28, 2021 17:58

bors bot added a commit that referenced this pull request Mar 28, 2021

Try #713:

12417be

bors bot added a commit that referenced this pull request Mar 30, 2021

Try #713:

a303392

Replace euclid with glam

fce0a9b

ghost force-pushed the glam branch from 411e542 to b1213f9 Compare March 30, 2021 18:09

ghost approved these changes Mar 30, 2021

View reviewed changes

Update UI tests

1d2be53

Update tests with the added `glam` dependency. Run tests that depend on Span::join only on nightly.

ghost force-pushed the glam branch from b1213f9 to 1d2be53 Compare March 30, 2021 18:31

bors bot merged commit 3ad9b65 into godot-rust:master Mar 30, 2021

Bromeon mentioned this pull request May 1, 2021

Make a 0.9.4 release #733

Closed

7 tasks

+              pub mod quat;
+              pub mod rect2;
               pub mod rid;
               pub mod string;
               pub mod string_array;
+              pub mod transform2d;

-              pub mod variant;
-              pub mod variant_array;
-              pub mod vector2;
-              pub mod vector2_array;
               pub mod vector3;

+                  }
+              }
+              macro_rules! derive_op_impl {

Replace euclid with glam (proof-of-concept) #713

Replace euclid with glam (proof-of-concept) #713

Conversation

Demindiro commented Mar 18, 2021 • edited

Bromeon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

ghost left a comment

Choose a reason for hiding this comment

bors bot commented Mar 23, 2021

try

Demindiro commented Mar 23, 2021

Bromeon commented Mar 23, 2021

ghost commented Mar 23, 2021

Demindiro commented Mar 23, 2021

ghost 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

ghost commented Mar 24, 2021 • edited by ghost

Demindiro commented Mar 24, 2021 • edited

ghost commented Mar 24, 2021

ghost commented Mar 28, 2021 • edited by ghost

bors bot commented Mar 28, 2021

try

Bromeon commented Mar 28, 2021

bors bot commented Mar 28, 2021

try

Demindiro commented Mar 28, 2021

ghost commented Mar 29, 2021

Bromeon commented Mar 30, 2021

bors bot commented Mar 30, 2021

try

ghost commented Mar 30, 2021 • edited by ghost

ghost commented Mar 30, 2021

Demindiro commented Mar 30, 2021

ghost left a comment

Choose a reason for hiding this comment

bors bot commented Mar 30, 2021

ghost commented Mar 30, 2021

bors bot commented Mar 30, 2021

Bromeon commented Mar 30, 2021

Demindiro commented Mar 18, 2021 •

edited

Bromeon left a comment •

edited

ghost commented Mar 24, 2021 •

edited by ghost

Demindiro commented Mar 24, 2021 •

edited

ghost commented Mar 28, 2021 •

edited by ghost

ghost commented Mar 30, 2021 •

edited by ghost