Add a few transform convenience methods #2055

Benjamin-L · 2021-04-30T02:49:17Z

Converting transforms into a matrix in order to operate on vectors or compute the inverse is poor ergonomics, and these operations can be implemented easily as methods in Transform and GlobalTransform. I think the name transform_vector is a bit confusing, especially given mul_vec3 exists. If anybody has a better suggestion that would be great!

Previously it was possible to convert a transform to a 4x4 matrix and then invert it, but inverting the transform in-place is both faster and more convenient.

alice-i-cecile · 2021-04-30T02:56:49Z

crates/bevy_transform/src/components/global_transform.rs

+    /// homogeneous coordinates.
+    #[inline]
+    pub fn transform_point(&self, point: Vec3) -> Vec3 {
+        self.scale * (self.rotation * point)


How does translation get applied here?

alice-i-cecile · 2021-04-30T02:57:15Z

crates/bevy_transform/src/components/global_transform.rs

+
+    /// Returns the result of applying this [`GlobalTransform`] to a [`Vec3`] interpreted as a vector.
+    ///
+    /// This applies rotation and scale, but not translation. It's the equivalent of using w=0 in


Did we swap the comments / math here?

Yeah... I sure did. lol. I'll fix that

alice-i-cecile · 2021-04-30T03:00:38Z

crates/bevy_transform/src/components/global_transform.rs

+    /// Returns the result of applying this [`GlobalTransform`] to a [`Vec3`] interpreted as a point.
+    ///
+    /// This applies rotation, scale, and translation. It's the equivalent of using w=1 in
+    /// homogeneous coordinates.


Could you link an appropriate explanatory article on homogenous coordinates as part of this? It's pretty dense for beginners.

https://www.tomdalling.com/blog/modern-opengl/explaining-homogenous-coordinates-and-projective-geometry/ looks quite nice.

alice-i-cecile · 2021-04-30T03:01:05Z

crates/bevy_transform/src/components/transform.rs

+    /// Returns the inverse of this transform.
+    ///
+    /// The inverse consists of applying the inverse rotation, scale, and
+    /// translation in reverse order.


I think spelling out A, then B, then C is clearer than "reverse order".

alice-i-cecile · 2021-04-30T03:02:25Z

crates/bevy_transform/src/components/transform.rs

+    /// This applies rotation, scale, and translation. It's the equivalent of using w=1 in
+    /// homogeneous coordinates.
+    #[inline]
+    pub fn transform_point(&self, point: Vec3) -> Vec3 {


Does having two functions of the same name but different function arguments work in Rust?

rust doesn't have function overloading, so it should not work.

One of the functions is on Transform, the other on GlobalTransform.

NathanSWard · 2021-04-30T03:42:10Z

Thanks for the PR! I would also like to see some tests added to ensue these functions have the correct behavior 😄

Benjamin-L · 2021-04-30T06:58:17Z

Is there a preference between doc-tests vs. putting the tests in their own functions? I'm gonna add doc-tests for now, but I can change that if you want me to.

mockersf · 2021-04-30T08:34:25Z

doc tests are when the test is useful to show usage in the doc. if it's actual tests, they should live in #[test] instead so that they won't be visible in the doc

Benjamin-L · 2021-05-01T01:20:02Z

While writing unit tests I discovered that the existing matrix methods are wrong. The Mat4::from_scale_rotation_translation function applies scale first, then rotation, then translation. The bevy Transform applies rotation first, then scale, then translation. The result is that the behavior of matrices from the Transform::compute_matrix method are not consistent with the transform they were generated from.

At this point we have a choice. We can either modify Transform::compute_matrix to use the RST (rotate, scale, translate) ordering, or we can modify the entire Transform type to use SRT. I'm actually in favor of the latter, because SRT is far more common, and I personally have run into several errors in my code using bevy because I assumed that scale was applied before rotation. Either option is a breaking change, but modifying the actual behavior of the Transform struct would probably be more significant. I would be in favor of doing this as soon as possible, while there aren't as many users. It's also worth noting that the inconsistency only matters with nonuniform scale. If scale is uniform the rotation and translation operations are commutative.

alice-i-cecile · 2021-05-01T01:22:18Z

I heavily prefer SRT as well and agree with your reasoning. If this change is implemented, please note it in the migration guide due to the serious impact on users.

alice-i-cecile · 2021-05-01T01:23:06Z

(also sorry @cart, CI approval on this one would be nice too)

Benjamin-L · 2021-05-01T01:26:44Z

I could make a separate PR to switch to SRT, or I could make it part of this one. If we do a separate PR I'll probably wait for it to get merged before pushing the tests for this, since they depend on the transform and matrix behavior being consistent.

alice-i-cecile · 2021-05-01T01:28:37Z

@Benjamin-L, I think a separate PR is better: it'll keep the scope smaller for review and keep this PR uncontroversial.

Benjamin-L · 2021-05-01T01:29:18Z

Alright, I'll mark this one as WIP then since it won't be complete until the other one is done.

Benjamin-L · 2021-05-01T02:13:05Z

Another discovery: the transform composition method is also broken, even for uniform scaling. I'm gonna use RST ordering, since that's what the code currently does, but this applies for SRT as well.

t⃗ = Translation component (a vector)
s⃗ = scale component (a vector, multiplied component-wise)
R = rotation component (a matrix, it's a quaternion in the real implementation but whatever)

For a transform tₙ(x):

tₙ(x) = Rₙ s⃗ₙ x + t⃗ₙ

The correct way to compose transformations looks like this:

t₂(x) = t₁(t₀(x))
t₂(x) = R₁ s⃗₁ (R₀ s⃗₀ x + t⃗₀) + t⃗₁
      = R₁ s⃗₁ R₀ s⃗₀ x + R₁ s⃗₁  t⃗₀ + t⃗₁
      = (R₁ R₀) ((R₀⁻¹ s⃗₁) s⃗₀) x + R₁ s⃗₁  t⃗₀ + t⃗₁
R₂ = R₁ R₀
s⃗₂ = (R₀⁻¹ s⃗₁) s⃗₀
t⃗₂ = R₁ s⃗₁  t⃗₀ + t⃗₁

What the current code does is:

R₂ = R₁ R₀
s⃗₂ = s⃗₁ s⃗₀
t⃗₂ = t⃗₀ + t⃗₁

This needs to get fixed regardless of how we resolve the discrepancy between transform.compute_matrix() * vec and transform * vec. I think it probably belongs in a third PR that will be less controversial than the SRT vs RST change, but still controversial because it is a subtle breaking change.

Benjamin-L · 2021-05-01T02:16:04Z

Actually, there's also an argument for doing both of those changes as part of the same PR: I want to add a bunch of unit tests that will catch this type of thing in the future. The unit tests won't pass until both problems are fixed.

alice-i-cecile · 2021-05-01T02:31:49Z

Nice find! Thanks a ton. I like both of those fixes in the same PR so then our unit tests can pass.

Benjamin-L · 2021-05-01T03:20:06Z

What are your thoughts on renaming the mul_vec3 function to transform_point in this PR, instead of keeping both around. If we're gonna be making breaking changes before the next version anyway, I think the name transform_vec3 is confusing and does a bad job of communicating to users that they need to consider whether the Vec3 they are transforming is a point or a vector.

alice-i-cecile · 2021-05-01T03:32:03Z

What are your thoughts on renaming the mul_vec3 function to transform_point in this PR, instead of keeping both around.

I think this is a good change. I much prefer focusing on use cases rather than the raw math, and we should try to avoid redundant approaches like this where possible. Adding a doc alias would help with migration pain though :)

Benjamin-L · 2021-05-01T03:33:31Z

Deprecating the old one instead of removing it would also be an option for less migration pain.

alice-i-cecile · 2021-05-01T03:35:28Z

Deprecating the old one instead of removing it would also be an option for less migration pain.

Ha, I think deprecating functionality violates our Bevy ethos at this stage ;) I'd rather have pointers to the fix than worry about managing deprecation at this stage, especially since nothing else in the engine uses a deprecation strategy yet.

(You're fully right that this is the right solution for more mature libraries)

Benjamin-L · 2021-05-01T03:36:05Z

I much prefer focusing on use cases rather than the raw math

Honestly, if I was designing this system from scratch I would make the distinction between points and vectors at the type level. I've done this in most of the graphics-related things I've worked on and it has caught a ton of bugs. It does come at the cost of some increased API friction when you do need to convert between the two though, and it might be too late to consider it at this point.

alice-i-cecile · 2021-05-01T03:37:12Z

I much prefer focusing on use cases rather than the raw math

Honestly, if I was designing this system from scratch I would make the distinction between points and vectors at the type level. I've done this in most of the graphics-related things I've worked on and it has caught a ton of bugs. It does come at the cost of some increased API friction when you do need to convert between the two though, and it might be too late to consider it at this point.

Have you seen @aevyrie's geometric primitives RFC. This sort of thing is exactly the intent of that work, and would make an excellent comment to include there.

Benjamin-L · 2021-05-01T03:39:10Z

Ooh, I did not see that RFC before. It's pretty much exactly what I had in mind.

alice-i-cecile · 2021-05-01T03:47:01Z

Once that RFC is implemented, would it make sense to move some of these convenience methods to those structs or traits instead? It makes more sense to me than them being methods of the Transform itself TBH, just because I tend to think of methods as transforming the object itself, rather than applying the object to something else.

Benjamin-L · 2021-05-01T03:49:32Z

I think having methods that perform an action on a different object in the context of the self object is pretty normal, but I do think we should add Mul<Point> and Mul<Direction> impls. I guess I will also say that if I was looking for the methods to apply a transform to a point or direction without prior knowledge about the API, the Transform object is probably the first place I would look.

alice-i-cecile · 2021-05-01T03:51:41Z

The Mul impls seem like a nice compromise; that sounds good to me.

Benjamin-L · 2021-05-01T03:55:55Z

Hmm, maybe it makes sense to have a more general Transformable trait. I just realized that it would make sense to apply a transform to several of the other types added in the RFC. Going even farther with this, maybe we want to make a distinction between isometries, similiarities, and affine transformations. You can apply an isometry or similarity to a circle or arc, and the result will still be a circle/arc, but you can't do the same with an arbitrary affine transformation because of the nonuniform scale.

alice-i-cecile · 2021-05-01T04:00:51Z

Hmm, maybe it makes sense to have a more general Transformable trait

I think I agree with you, but this deserves to be a comment on the RFC linked or an RFC of its own :p

Benjamin-L · 2021-05-01T04:07:21Z

So... I just realized something. There's a reason that the order of application for Transform is RST. SRT representation is actually not closed under composition, because the result of composing arbitrary transforms with nonuniform scale can have shear, which cannot be captured by the SRT representation. This makes me a little sad, because RST is kinda unintuitive, but it does make the decision of how to resolve the discrepancy between applying a transform directly vs converting it to a matrix easy, because we only have one option.

alice-i-cecile · 2021-05-01T04:09:16Z

There's a reason that the order of application for Transform is RST. SRT representation is actually not closed under composition, because the result of composing arbitrary transforms with nonuniform scale can have shear, which cannot be captured by the SRT representation.

Ah, you're right. Well, that makes the decision easy. We should try to document both the ordering and why it's needed carefully then.

mtsr · 2021-05-01T04:21:12Z

Some of the math errors would also be solved by #1755, I think.

Benjamin-L · 2021-05-01T04:44:17Z

Oh, yes they would... It might be a good idea to wait on a decision on that issue before continuing with this.

cart · 2021-05-01T19:09:01Z

Yeah I'm cool with making the relevant changes in a single pr if it means adding tests for general transform op correctness.

cart · 2021-05-05T18:39:58Z

Wider gamedev ecosystem Transform survey: #1755 (comment)

I think we should stick with the current SRT impl

alice-i-cecile · 2022-04-25T14:05:41Z

@Benjamin-L, can you comment in #2373? We've since relicensed to MIT + Apache, and I'd like to either ensure that this can be picked up or close it out.

alice-i-cecile · 2022-05-02T13:03:05Z

Closing this out as abandoned. Do not reuse this code unless the relicensing is resolved.

Benjamin-L added 2 commits April 29, 2021 19:45

added transform inverse method

b01b074

Previously it was possible to convert a transform to a 4x4 matrix and then invert it, but inverting the transform in-place is both faster and more convenient.

added transform_point and transform_vector methods

980cf52

alice-i-cecile added A-Core Common functionality for all bevy apps C-Usability A simple quality-of-life change that makes Bevy easier to use labels Apr 30, 2021

alice-i-cecile reviewed Apr 30, 2021

View reviewed changes

Benjamin-L marked this pull request as draft May 1, 2021 01:30

Benjamin-L mentioned this pull request May 1, 2021

Revert transform back to using Mat4 #1755

Open

mockersf added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 16, 2021

alice-i-cecile closed this May 2, 2022

alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label May 2, 2022

Add a few transform convenience methods #2055

Add a few transform convenience methods #2055

Conversation

Benjamin-L commented Apr 30, 2021

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

NathanSWard commented Apr 30, 2021 • edited Loading

Benjamin-L commented Apr 30, 2021

mockersf commented Apr 30, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021

alice-i-cecile commented May 1, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021

Benjamin-L commented May 1, 2021

Benjamin-L commented May 1, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021

Benjamin-L commented May 1, 2021

alice-i-cecile commented May 1, 2021 • edited Loading

Benjamin-L commented May 1, 2021 • edited Loading

alice-i-cecile commented May 1, 2021

mtsr commented May 1, 2021

Benjamin-L commented May 1, 2021

cart commented May 1, 2021

cart commented May 5, 2021

alice-i-cecile commented Apr 25, 2022

alice-i-cecile commented May 2, 2022

NathanSWard commented Apr 30, 2021 •

edited

Loading

alice-i-cecile commented May 1, 2021 •

edited

Loading

Benjamin-L commented May 1, 2021 •

edited

Loading