Implement some of the Transform2D API in rust #791

setzer22 · 2021-10-05T18:23:12Z

As we discussed, here's an initial implementation of the Transform2D. The implementation is currently untested, so I'm marking it as a work-in-progress. I'm just posting this early so we can discuss if you thing this is the right approach 👍

The code closely follows the exposed GDScript API and even copies its documentation strings. The Implementation is adapted from Godot's C++ source code, delegating to glam (via Vector2) when necessary.

I decided against adding a conversion between glam::Affine2 and Transform2D because it was not immediately clear to me if the mapping would be 1:1 (godot's Transform2D does not necessarily need to be an affine transformation, although it's assumed in certain places).

Instead, I decided to just follow the C++ implementation, which was simple enough, to ensure results would be predictable when porting code from GDScript. We are still using glam underneath when delegating all of Vector3's operations, which happens in many places.

Bromeon

Thanks a lot!

Style-wise, you could use Self to refer to the current type, and use #[inline] for methods. I'm not a huge fan of the latter, but we have currently configured Clippy to check it. Might need to do a benchmark whether it has an impact on speed/code size.

One thing I'm not very sure about, is the getters/setters for translation (=origin), rotation and scale. Their presence kind of implies that the transform is affine, but with field public, this invariant cannot be upheld. What do we return if someone creates a non-affine transform? Even something like shear (which is affine) cannot be represented.

Also, I would like to go with the existing convention for getters that have no get_() prefix.

I haven't checked the algorithms themselves yet, this is where tests might come in handy.

Bromeon · 2021-10-05T18:48:43Z

gdnative-core/src/core_types/transform2d.rs

+    pub fn from_rotation_position(rotation: f32, position: Vector2) -> Transform2D {
+        todo!()
+    }


This is a bit a strange method, even the way it exists in Godot. Several things are unclear:

Is position actually the translation?

Is there a scale component (length of position)

How does the resulting transform look, if I had to describe it in terms of the identity + rotated(), translated() and scaled()?

I'm also OK to remove this method for these reasons; we already use sometimes more idiomatic and expressive APIs rather than providing a 1:1 GDScript port.

For example, in interpolate_with() below you use this method, but still have to provide a scale a posteriori; it would have been clearer to use translated() & Co.

I believe there is some value in keeping a method like this, as it seems to be the most widely used constructor used in GDScript. It is a bit unnatural to think of rotations in terms of their axis vectors when, in 2d, rotations are typically represented with a single angle. So having a more user-friendly constructor feels like a good idea.

What we can do, is extending the API by also accepting a scale vector, and have the implementation be expressed in terms of .translated(), .rotated() and .scaled() to avoid code duplication. I'm including a proposal for this on my next commit 👍

gdnative-core/src/core_types/transform2d.rs

Bromeon · 2021-10-05T18:59:07Z

gdnative-core/src/core_types/transform2d.rs

+    /// Returns the transform's origin (translation).
+    pub fn get_origin(&self) -> Vector2 {
+        self.origin
+    }
+
+    /// Sets the transform's origin (translation).
+    pub fn set_origin(&mut self, origin: Vector2) {
+        self.origin = origin;
+    }


I think these 2 methods are not useful, since origin is already public.

What could be an option is to name them set/get_translation() instead, that would be symmetric with the methods concerning rotation and scale. Would need to be made obvious in the doc that for affine transforms, it's the same as accessing origin directly.

I agree this method is not very useful, I just added it to be symmetrical with the rotation and scale properties. But I think it makes sense to remove it. This way users won't get confused with two different things that are essentially the same.

Would need to be made obvious in the doc that for affine transforms, it's the same as accessing origin directly.

Actually, if we keept this implementation, it's not just for affine transforms, the method is always accessing the origin field directly 😅

gdnative-core/src/core_types/transform2d.rs

jacobsky

Thank you for the PR! It looks good to me and I agree with all points on @Bromeon 's feedback.

One thing that I did notice is that the clippy lints requesting #[inline] fail, so please make sure to check with clippy that all of the necessary inlining has been done :)

setzer22 · 2021-10-09T10:38:36Z

Thanks @Bromeon and @jacobsky for the reviews! I think I've addressed all of the comments (please let me know if you think there's anything else missing).

I will have a look now at some tests against the godot API to make sure the current implementation is working as expected.

jacobsky

Thanks for the good work! I think it's looking good. There were a couple minor things that I noticed related to the comment blocks.

jacobsky · 2021-10-09T11:05:28Z

gdnative-core/src/core_types/transform2d.rs

@@ -1,10 +1,216 @@
 use super::Vector2;

+/// A 2×3 matrix (2 rows, 3 columns) used for 2D linear transformations. It can represent
+/// transformations such as translation, rotation, or scaling. It consists of a three


Slight nitpick. "It consists of three Vector2 values" without the 'a'

Oops! In my defense I'll say the typo was copied straight from the Godot docs 😁

jacobsky · 2021-10-09T11:07:10Z

gdnative-core/src/core_types/transform2d.rs

@@ -1,10 +1,216 @@
 use super::Vector2;

+/// A 2×3 matrix (2 rows, 3 columns) used for 2D linear transformations. It can represent
+/// transformations such as translation, rotation, or scaling. It consists of a three
+/// Vector2 values: x, y, and the origin.
 #[derive(Copy, Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[repr(C)]
 pub struct Transform2D {


Maybe my understanding is a little off here, but I thought that the x and y components were there to represent the scale and rotation along each whereas the origin represents the translation.

If I'm mistaken please let me know.

Yes, the x and y components are the basis vectors of the transformation. So they can represent both a rotation and a translation of the coordinate space defined by the transform.

I don't think this docstring is stating otherwise, but perhaps the wording could be a bit better. I'll change it so it calls the vectors "x axis" and "y axis" instead.

We had to keep the same x and y names to be consistent with Godot's naming conveniton, but I prefer writing x_axis and y_axis to make the distinction clear where possible (such as in function arguments).

Sorry, it's not the doc string that I was commenting on, it was the x, y and origin that were following the doc string. Apologies for the confusion.

In particular the following

/// The x basis vector of the transform. Objects will move along this vector when /// moving on the X axis in the coordinate space of this transform pub x: Vector2, /// The y basis vector of the transform. Objects will move along this vector when /// moving on the Y axis in the coordinate space of this transform pub y: Vector2, /// The origin of the transform. The coordinate space defined by this transform /// starts at this point. pub origin: Vector2,

It feels a smidge like x and y translate the object when they should be morphing it, unless my understanding is wrong.

I agree that x and y are terrible names, and to be honest, I don't think Godot having them is reason enough to keep them. It's not like there's automated tooling to convert GDScript -> Rust. It's only important that the manual mapping is more or less clear.

This might be a candidate for #773. Maybe basis_x and basis_y would be better, but I'm open to suggestions. The 3D class Transform has the fields basis and origin.

What about this? Maybe field renaming should be part of a separate PR, e.g. one for #773?

setzer22 · 2021-10-09T15:37:20Z

Just uploaded a first draft for the tests. Since I'm new at this and there's quite a bit more work to test the remaining functions, I'd like to wait for some early feedback before continuing. If you thing the current strategy's fine, I can go ahead and add the remaining methods 👍

Bromeon

Thank you! Added some comments.

Bromeon · 2021-10-10T16:42:22Z

gdnative-core/src/core_types/transform2d.rs

+
+    let new_transform_godot = unsafe {
+        let mut tr = Transform2D::IDENTITY;
+        let tr_p = &mut tr as *mut _ as *mut sys::godot_transform2d;


This should probably be encapsulated in a sys() or to_sys() method on the transform. Same for from_sys() below. You could have a look at how Color does it.

Also, could you only wrap the unsafe statements in unsafe {}, even if it means having 2? If things explode, it's easier to locate the potential places :)

I noticed a slight inconsistency with the meaning of sys(), from_sys() and to_sys(). For instance, in Vector2, sys() performs a cast from &Vector2 to *const godot_vector2. This is the version I implemented. But in Color, it transmutes to a reference instead: from &Color to &godot_color. Which convention should we keep for Transform2d?

Moreover, In this case calling the constructor requires passing in a *mut godot_transform2d. Using sys() is not useful here since we need a mutable pointer. Should I add a sys_mut() to encapsulate this logic instead?

From the docs and the dark corners of the Nomicon:

transmute is incredibly unsafe. There are a vast number of ways to cause undefined behavior with this function. transmute should be the absolute last resort.

Get out of our way type system! We're going to reinterpret these bits or die trying! Even though this book is all about doing things that are unsafe, I really can't emphasize that you should deeply think about finding Another Way than the operations covered in this section. This is really, truly, the most horribly unsafe thing you can do in Rust. The guardrails here are dental floss. [...] The ways to cause Undefined Behavior with this are mind boggling.

We might want to go with pointer casts, while dangerous they don't entirely suspend the type system.

Yes, sys_mut() is OK.

gdnative-core/src/core_types/transform2d.rs

Bromeon · 2021-10-10T16:49:19Z

Let's run some intermediate tests...
bors try

bors · 2021-10-10T16:59:43Z

try

Build failed:

full-ci

Bromeon · 2021-10-14T14:33:54Z

The current MSRV (minimum supported Rust version) 1.48 does not have clamp stabilized yet, so we get this error:

error[E0658]: use of unstable library feature 'clamp'
   --> gdnative-core/src/core_types/transform2d.rs:190:19
    |
190 |         let dot = f32::clamp(v1.dot(v2), -1.0, 1.0);
    |                   ^^^^^^^^^^
    |
    = note: see issue #44095 <https://github.com/rust-lang/rust/issues/44095> for more information

If we want to keep that compatibility, we'd probably have to write our own clamp for now...

Let's see if there are other issues:
bors try

bors · 2021-10-14T14:42:01Z

try

Build failed:

full-ci

setzer22 · 2021-10-14T16:01:24Z

The current MSRV (minimum supported Rust version) 1.48 does not have clamp stabilized yet, so we get this error:
If we want to keep that compatibility, we'd probably have to write our own clamp for now...

Oops, sometimes I forget I'm on nightly. Thanks for catching that! f32::clamp is a very handy method! 🙂

I will add an implementation for the time being, with a comment that says it can be removed once f32::clamp is stabilized.

setzer22 · 2021-10-14T16:58:33Z

I pushed a new version with one more test case, which uncovered several bugs in the implementation. I will keep adding more test for the remaining functions and let you know once it's ready.

@Bromeon I left a few replies on your review's comments. Please have a look whenever you have some time!

gdnative-core/src/core_types/transform2d.rs

Bromeon · 2021-10-14T17:16:52Z

gdnative-core/src/core_types/transform2d.rs

+    #[inline]
+    pub fn from_rotation_translation_scale(
+        rotation: f32,
+        translation: Vector2,
+        scale: Vector2,
+    ) -> Transform2D {
+        Self::IDENTITY
+            .translated(translation)
+            .rotated(rotation)
+            .scaled(scale)
+    }


It would be clearer if the method name and parameter order matched the order of transform application, no?

Also, doc is no longer matching the parameters.

Bromeon · 2021-10-14T17:21:16Z

gdnative-core/src/core_types/transform2d.rs

+    #[doc(hidden)]
+    #[inline]
+    pub fn to_sys(self) -> sys::godot_transform2d {
+        unsafe { std::mem::transmute(self) }
+    }


It doesn't look like this method it used; I would remove it. We can always add it if necessary.

Bromeon · 2021-10-14T17:22:57Z

bors try

bors · 2021-10-14T20:32:04Z

try

Timed out.

Bromeon · 2021-10-14T20:45:27Z

bors try

bors · 2021-10-14T20:54:00Z

try

Build succeeded:

full-ci

setzer22 · 2021-10-20T16:21:00Z

I've just added a few more tests and I believe this is ready for a final review 😄

Bromeon

Looks mostly good, just small remarks/questions.
Maybe @jacobsky could have another look?

After that, once you combine the commits to one, we can merge it in my opinion. Let's get it out there, addendums can still be done later (like the mentioned field names).

Bromeon · 2021-10-25T14:30:05Z

gdnative-core/src/core_types/transform2d.rs

@@ -1,10 +1,216 @@
 use super::Vector2;

+/// A 2×3 matrix (2 rows, 3 columns) used for 2D linear transformations. It can represent
+/// transformations such as translation, rotation, or scaling. It consists of a three
+/// Vector2 values: x, y, and the origin.
 #[derive(Copy, Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[repr(C)]
 pub struct Transform2D {


What about this? Maybe field renaming should be part of a separate PR, e.g. one for #773?

Bromeon · 2021-10-25T14:31:18Z

gdnative-core/src/core_types/transform2d.rs

+        let mut inverted = *self;
+
+        let det = self.basis_determinant();
+        // assert!(det != 0.0, "The determinant cannot be zero");


Should we still have this assertion, or maybe a debug_assert!?

What about this? Maybe field renaming should be part of a separate PR, e.g. one for #773?

If you want, I could rename this to axis_x, axis_y and origin. I didn't change it to avoid introducing a breaking change, but I agree it's better naming. Just let me know 👍

Should we still have this assertion, or maybe a debug_assert!?

I was meaning to ask, then forgot 😅. The assert was "ported" from Godot's cpp codebase, but then I commented it because I wasn't sure if a panic was acceptable in this kind of function.

A debug_assert! looks like a good tradeoff to me, I'm pushing it with my next commit 👍

Bromeon · 2021-10-25T14:35:27Z

gdnative-core/src/core_types/transform2d.rs

+    /// Returns the transform's scale.
+    #[inline]
+    pub fn scale(&self) -> Vector2 {
+        let det_sign = f32::signum(self.basis_determinant());


Is there a particular reason why you use the notation f32::signum(val) instead of val.signum()?
signum() is not an associated method.

No particular reason, I'm used to treating these as plain functions (like sin and cos) as it looks more natural and closer to how it's written in math.

But it's best to keep whatever the current convention is 👍 I will change ti to val.signum()

bors · 2021-10-25T15:04:48Z

try

Build succeeded:

full-ci

setzer22 · 2021-10-25T15:45:52Z

After that, once you combine the commits to one

@Bromeon I'm unsure how to do this. There's usually a way to squash the commits when merging on your end and that's typically how I do this.

Do you need me to do anything on my fork? 😅

Added some of the Transform2D methods from the Godot API to its Rust counterpart. The implementation uses rust, but closely follows the C++ codebase. Tests have been added to validate that calling the native transform2d API is equivalent to using the Rust methods.

setzer22 · 2021-10-26T08:03:47Z

Alright, I took care of the squashing on my end. Let me know if that works for you 😄

jacobsky · 2021-10-29T22:48:49Z

@setzer22 Thank you for the hard work! I think everything looks good to go!

bors r+

bors · 2021-10-29T22:48:50Z

🔒 Permission denied

Existing reviewers: click here to make jacobsky a reviewer

jacobsky · 2021-10-29T22:50:26Z

bors r+

bors · 2021-10-29T22:50:27Z

🔒 Permission denied

Existing reviewers: click here to make jacobsky a reviewer

jacobsky · 2021-10-29T22:51:35Z

Odd, I'm listed as a reviewer on this PR, but bors doesn't seem to trust me 🤣

bors r+

bors · 2021-10-29T22:51:36Z

🔒 Permission denied

Existing reviewers: click here to make jacobsky a reviewer

jacobsky · 2021-10-29T22:52:28Z

bors r+

bors · 2021-10-29T22:52:29Z

🔒 Permission denied

Existing reviewers: click here to make jacobsky a reviewer

jacobsky · 2021-10-30T07:29:49Z

bors r+

bors · 2021-10-30T07:38:21Z

Build succeeded:

full-ci

Added some of the Transform2D methods from the Godot API to its Rust counterpart. The implementation uses rust, but closely follows the C++ codebase. Tests have been added to validate that calling the native transform2d API is equivalent to using the Rust methods.

821: Implement some methods of the Transform API r=Bromeon a=setzer22 Here are some more Transform methods. I followed the same strategy as for #791, where the implementation was done by translating the C++ code to Rust. I can also add some tests similar to the ones from the previous PR if you'd like 👍 Although the code for Transform is far simpler, because it delegates most of the implementation to `Vector3` and `Basis`. Co-authored-by: Setzer22 <jsanchezfsms@gmail.com>

setzer22 marked this pull request as draft October 5, 2021 18:24

Bromeon reviewed Oct 5, 2021

View reviewed changes

Bromeon added c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library labels Oct 5, 2021

This was linked to issues Oct 5, 2021

Implement missing Basis and Transform methods #290

Closed

Transform.xform() and Transform.xform_inv() methods are missing #719

Closed

jacobsky suggested changes Oct 5, 2021

View reviewed changes

setzer22 added a commit to setzer22/godot-rust that referenced this pull request Oct 9, 2021

PR godot-rust#791. Address review comments

4190568

jacobsky reviewed Oct 9, 2021

View reviewed changes

setzer22 added a commit to setzer22/godot-rust that referenced this pull request Oct 9, 2021

PR godot-rust#791 Initial testing strategy

e020eb0

Bromeon reviewed Oct 10, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Oct 10, 2021

Try #791:

2787a37

setzer22 mentioned this pull request Oct 11, 2021

Fixed _input being falsely detected when a Godot GUI element is on top of the other element. setzer22/godot-egui#9

Merged

bors bot added a commit that referenced this pull request Oct 14, 2021

Try #791:

3950d64

Bromeon reviewed Oct 14, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Oct 14, 2021

Try #791:

c5f1d25

bors bot added a commit that referenced this pull request Oct 14, 2021

Try #791:

91e9985

setzer22 marked this pull request as ready for review October 20, 2021 16:20

bors bot added a commit that referenced this pull request Oct 25, 2021

Try #791:

a3878d9

Bromeon reviewed Oct 25, 2021

View reviewed changes

setzer22 added a commit to setzer22/godot-rust that referenced this pull request Oct 25, 2021

PR godot-rust#791 Address review feedback.

0038317

setzer22 force-pushed the feat/transform2d-impls branch from 0038317 to fca5011 Compare October 26, 2021 08:02

setzer22 changed the title ~~[WIP] Implement some of the Transform2D API in rust~~ Implement some of the Transform2D API in rust Oct 26, 2021

jacobsky approved these changes Oct 29, 2021

View reviewed changes

jacobsky self-requested a review October 29, 2021 22:50

jacobsky approved these changes Oct 29, 2021

View reviewed changes

bors bot merged commit c24925a into godot-rust:master Oct 30, 2021

setzer22 mentioned this pull request Nov 23, 2021

Implement some methods of the Transform API #821

Merged

Implement some of the Transform2D API in rust #791

Implement some of the Transform2D API in rust #791

Conversation

setzer22 commented Oct 5, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobsky left a comment

Choose a reason for hiding this comment

setzer22 commented Oct 9, 2021

jacobsky 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

Choose a reason for hiding this comment

setzer22 Oct 9, 2021 • edited

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

setzer22 commented Oct 9, 2021

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

Bromeon commented Oct 10, 2021

bors bot commented Oct 10, 2021

try

Bromeon commented Oct 14, 2021

bors bot commented Oct 14, 2021

try

setzer22 commented Oct 14, 2021

setzer22 commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Oct 14, 2021

bors bot commented Oct 14, 2021

try

Bromeon commented Oct 14, 2021

bors bot commented Oct 14, 2021

try

setzer22 commented Oct 20, 2021

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

setzer22 Oct 25, 2021 • edited

Choose a reason for hiding this comment

bors bot commented Oct 25, 2021

try

setzer22 commented Oct 25, 2021

setzer22 commented Oct 26, 2021

jacobsky commented Oct 29, 2021

bors bot commented Oct 29, 2021

jacobsky commented Oct 29, 2021

bors bot commented Oct 29, 2021

jacobsky commented Oct 29, 2021

bors bot commented Oct 29, 2021

jacobsky commented Oct 29, 2021

bors bot commented Oct 29, 2021

jacobsky commented Oct 30, 2021

bors bot commented Oct 30, 2021

Bromeon left a comment •

edited

jacobsky left a comment •

edited

setzer22 Oct 9, 2021 •

edited

setzer22 Oct 25, 2021 •

edited