Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a few transform convenience methods #2055

Closed

Conversation

Benjamin-L
Copy link

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 alice-i-cecile added A-Core Common functionality for all bevy apps C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Apr 30, 2021
/// homogeneous coordinates.
#[inline]
pub fn transform_point(&self, point: Vec3) -> Vec3 {
self.scale * (self.rotation * point)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does translation get applied here?


/// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we swap the comments / math here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@NathanSWard
Copy link
Contributor

NathanSWard commented Apr 30, 2021

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

@Benjamin-L
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Member

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

@Benjamin-L
Copy link
Author

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
Copy link
Member

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

@Benjamin-L
Copy link
Author

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

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

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
Copy link
Author

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
Copy link
Member

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

@Benjamin-L
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

@alice-i-cecile
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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

@alice-i-cecile
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

@Benjamin-L
Copy link
Author

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
Copy link
Member

alice-i-cecile commented May 1, 2021

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
Copy link
Author

Benjamin-L commented May 1, 2021

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
Copy link
Member

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
Copy link
Contributor

mtsr commented May 1, 2021

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

@Benjamin-L
Copy link
Author

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

@cart
Copy link
Member

cart commented May 1, 2021

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

@cart
Copy link
Member

cart commented May 5, 2021

Wider gamedev ecosystem Transform survey: #1755 (comment)

I think we should stick with the current SRT impl

@mockersf 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
Copy link
Member

@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
Copy link
Member

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

@alice-i-cecile 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants