Add `reparented_to` method to `GlobalTransform` by nicopap · Pull Request #4891 · bevyengine/bevy

nicopap · 2022-06-01T13:39:08Z

Objective

Finding the proper transform to make an entity a child of another
without its global transform changing is tricky and error prone. It
requires some knowledge of how bevy computes and propagates
GlobalTransform through the hierarchy.

Solution

This introduces a way to find such a transform. The method is only
implemented on GlobalTransform since we expect it to be the most
common use-case.

Changelog

Add reparented_to method to GlobalTransform

alice-i-cecile

Excellent docs that really help motivate the use case.

crates/bevy_transform/src/components/global_transform.rs

nicopap · 2022-06-02T07:13:18Z

Looks like the check-doc CI check failed spuriously. I'd suggest relaunching the CI.

alice-i-cecile · 2022-06-02T14:06:01Z

bors try

bors · 2022-06-02T14:24:05Z

try

Build succeeded:

HackerFoo · 2022-06-03T22:26:42Z

Dividing a transform is the same as multiplying by the inverse. I find this intuitive, because an inverse "undoes" a transform, e.g. if a transform converts model space coordinates to world space, than its inverse converts world space coordinates to model space. Furthermore, it's easy to understand where the inverted transform needs to be applied (left or right), because if you can put a transform next to its inverse, they cancel out (i.e. T * T^-1 = I, where T^-1 is the inverse of T and I is the identity transform.) So inverses can be used to peel away transforms one-by-one from either the left or right of a chain of transforms.

Unfortunately, the TRS representation cannot be inverted if the scale is not uniform.

HackerFoo · 2022-06-03T22:29:42Z

crates/bevy_transform/src/components/global_transform.rs


+    /// Divide `self` with `transform`, this is the reciprocal of [`Self::mul_transform`].
+    ///
+    /// `t2 * t1 / t2 == t1` Note that transforms are not commutative, meaning that


This is confusing because the divisor is on the left, but it applies the inverse on the left. This will cause a lot of confusion. Furthermore, there should be two division operators, but Div can't represent this.

I'll be honest and admit I'm a bit confused. I've tried making sense of it on paper and my understanding is that if an inverse existed on the domain of transforms then it would be identical to (t2 * t1) * t2^-1. Am I wrong? How is that confusing? It does mirror the real numbers division (ie: a / b == a * b^-1)

Also why should there be two division operations?

Personally (not speaking for @HackerFoo) I would not define an inverse like this.

I'd instead define it given an operation (*, 1), then an inverse for a given x, y would be such that x * y = 1. Then / can be defined as * by the inverse. Hence I'd expect a * (b / b) = a * 1 = a.

For a well-formed (*, 1), of course a * (b / b) = (a * b) / b = (b * a) / b discontinuities non-withstanding which is the relationship that you have put.

However, the substitution steps steps above rely on the fact that (*, 1) is both commutative and associative and the precedence rules between * and / don't matter. Even without commutativity and associativity I'd very much expect that t1 * (t2 / t2) == t1 * 1 should hold (where 1 is the identity matrix).

Finally, I'd expect a second division operation for scalar division of matrices.

Thank you for the explanation. I see. So maybe it would be better to have a different name, for example "difference" or "unparent"? This way, not only it doesn't lead to confusion, but it hints at potential usage.

I think a different name would be good but I'm at a loss at what it should be called.

Of the names you've suggested "difference" feels more correct to me, but I think I'd have to formalize why I think that.

Regardless, I don't think that it is necessarily a problem that the name is not perfect, and I would rather that it's merged for use (with an imperfect name) and then the name finalized before an actual release.

nicopap · 2022-06-17T07:35:35Z

@yilinwei @HackerFoo I've changed the name of the "unparenting" method from div_transform to unparent_from and removed the impl Div for it. I think it not only fixes your legitimate concerns, but it also makes it much easier to understand how to use it!

HackerFoo · 2022-06-18T06:23:29Z

I was going to ask how this improves on multiplying by the inverse, then I realized that Transform is lacking an inverse method. I have implemented inversion for Transforms, but it cannot work with nonuniform scale.

If #4379 is merged, then this would be solved, and we can just use Affine3A's inverse method.

nicopap · 2022-06-18T07:41:26Z

I was going to ask how this improves on multiplying by the inverse, then I realized that Transform is lacking an inverse method. I have implemented inversion for Transforms, but it cannot work with nonuniform scale.

It solves it in the sense it stops pretending to be an inverse or a division. Since you rightly pointed out yourself, with the current transform implementation, there is no such thing. It is misleading and confusing to call it div or inverse, so we call it by what it is actually supposed to, or what it's useful for in its limited use case. Which is to find a transform B from transforms self and A such as self = A * B.

And now I realize the name doesn't convey that at all, in fact it conveys the opposite!

We could wait until #4379, in which transform has a natural inverse and therefore division. But until then, we would be missing an important and useful functionality. And as you pointed out, division is a direct and simple replacement to the unparent_from operation, so it would be easy to migrate that method to the new representation.

Finding the proper transform to make an entity a child of another without its global transform changing is tricky and error prone. It requires some knowledge of how bevy computes and propagates `GlobalTransform` through the hierarchy. This introduces a way to find such a transform. The method is only implemented on `GlobalTransform` since we expect it to be the most common use-case.

alice-i-cecile

I think this is useful and clear. Much happier with a more specific name.

HackerFoo

There needs to be a test that shows that this operation can work with nonuniform scale, but I know that there is no way to implement this without changing the representation of Transform.

A simple example is a scale of Vec3::new(1., 2., 1.) rotated around X by pi/4 radians. There is simply no way to represent this in SRT, and so multiplication, division (which would be rotation in the opposite direction), and inversion (which must change the order of scale and rotation, hence rotating the scale) are all incomplete.

crates/bevy_transform/src/components/global_transform.rs

HackerFoo · 2022-07-02T21:02:46Z

If this operation is useful, I'd be okay with it if it asserts that the scale of both inputs are uniform, which makes them independent of rotation, and documentation that explains why.

HackerFoo · 2022-07-02T21:15:04Z

I can see why it should seem possible that you can "undo" each transformation in the reverse order they have been applied, but the problem is that we can't even safely apply a chain of transformations if there is nonuniform scale in any of the transforms applied. I believe Mul should not be implemented for Transform, because it is incomplete.

nicopap · 2022-07-03T10:00:56Z

crates/bevy_transform/src/components/global_transform.rs

+    fn transform_equal(left: GlobalTransform, right: Transform) -> bool {
+        left.scale.abs_diff_eq(right.scale, 0.001)
+            && left.translation.abs_diff_eq(right.translation, 0.001)
+            && left.rotation.angle_between(right.rotation) < 0.0001
+    }
+
+    #[test]
+    fn reparented_to_transform_identity() {
+        fn reparent_to_same(t1: GlobalTransform, t2: GlobalTransform) -> Transform {
+            t2.mul_transform(t1.into()).reparented_to(t2)
+        }
+        let t1 = GlobalTransform {
+            translation: Vec3::new(1034.0, 34.0, -1324.34),
+            rotation: Quat::from_euler(XYZ, 1.0, 0.9, 2.1),
+            scale: Vec3::new(1.0, 2.345, 0.0),
+        };
+        let t2 = GlobalTransform {
+            translation: Vec3::new(0.0, -54.493, 324.34),
+            rotation: Quat::from_euler(XYZ, 1.9, 0.3, 3.0),
+            scale: Vec3::new(3.0, 1.345, 0.9),
+        };
+        let retransformed = reparent_to_same(t1, t2);
+        assert!(
+            transform_equal(t1, retransformed),
+            "t1:{t1:#?} retransformed:{retransformed:#?}"
+        );
+    }
+    #[test]
+    fn reparented_usecase() {
+        let t1 = GlobalTransform {
+            translation: Vec3::new(1034.0, 34.0, -1324.34),
+            rotation: Quat::from_euler(XYZ, 0.8, 1.9, 2.1),
+            scale: Vec3::new(-1.0, -2.3, 10.9),
+        };
+        let t2 = GlobalTransform {
+            translation: Vec3::new(28.0, -54.493, 324.34),
+            rotation: Quat::from_euler(XYZ, 0.0, 3.1, 0.1),
+            scale: Vec3::new(3.0, -1.345, 0.9),
+        };
+        // goal: find `X` such as `t2 * X = t1`
+        let reparented = t1.reparented_to(t2);
+        let t1_prime = t2 * reparented;
+        assert!(
+            transform_equal(t1, t1_prime.into()),
+            "t1:{t1:#?} t1_prime:{t1_prime:#?}"
+        );
+    }


@HackerFoo Do you think those new tests are sufficient? Seems the transform_equal does test equality correctly now. I'll admit, the numerical accuracy sucks (only precision to the thousandth) and we might be able to get better accuracy if I dig up my numerical analysis books, but it's fine for the job now I think. I even used negative values and 0 for scale in reparented_usecase to check that the reparenting works even for admittedly silly and probably invalid values of scale.

nicopap · 2022-07-16T14:34:12Z

Superseded by #4379. Though I still think the example code in this PR is a good hint for the user. However, it might be better suited for a project like the cheat book. I'll close this for now.

nicopap force-pushed the add-transform-div branch from af699e1 to a5b1453 Compare June 1, 2022 13:44

alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Transform Translations, rotations and scales labels Jun 1, 2022

alice-i-cecile approved these changes Jun 1, 2022

View reviewed changes

mockersf reviewed Jun 1, 2022

View reviewed changes

crates/bevy_transform/src/components/global_transform.rs Outdated Show resolved Hide resolved

nicopap force-pushed the add-transform-div branch from a5b1453 to 751a195 Compare June 2, 2022 07:10

nicopap requested a review from mockersf June 2, 2022 07:10

bors bot added a commit that referenced this pull request Jun 2, 2022

Try #4891:

42365fc

nicopap mentioned this pull request Jun 3, 2022

Bug in the Mul impl for Transform and GlobalTransform #4910

Open

HackerFoo suggested changes Jun 3, 2022

View reviewed changes

alice-i-cecile added the X-Needs-SME This type of work requires an SME to approve it. label Jun 3, 2022

nicopap force-pushed the add-transform-div branch from 751a195 to 0d1449f Compare June 17, 2022 07:33

nicopap changed the title ~~Add div_transform method to GlobalTransform~~ Add unparent_from method to GlobalTransform Jun 17, 2022

nicopap force-pushed the add-transform-div branch 2 times, most recently from dfd16fc to 7978ecb Compare June 17, 2022 07:48

nicopap force-pushed the add-transform-div branch from 7978ecb to cce1f3c Compare June 18, 2022 07:50

nicopap changed the title ~~Add unparent_from method to GlobalTransform~~ Add reparented_to method to GlobalTransform Jun 18, 2022

alice-i-cecile self-requested a review June 28, 2022 16:23

alice-i-cecile approved these changes Jun 28, 2022

View reviewed changes

nicopap requested a review from HackerFoo June 28, 2022 18:55

HackerFoo suggested changes Jul 2, 2022

View reviewed changes

crates/bevy_transform/src/components/global_transform.rs Show resolved Hide resolved

crates/bevy_transform/src/components/global_transform.rs Outdated Show resolved Hide resolved

alice-i-cecile removed the X-Needs-SME This type of work requires an SME to approve it. label Jul 2, 2022

nicopap requested a review from HackerFoo July 3, 2022 09:49

Add reparented_to tests

f277b46

nicopap force-pushed the add-transform-div branch from 58cd7fd to f277b46 Compare July 3, 2022 10:00

nicopap commented Jul 3, 2022

View reviewed changes

nicopap closed this Jul 16, 2022

nicopap mentioned this pull request Dec 24, 2022

[Merged by Bors] - Add a reparented_to method to GlobalTransform #7020

Closed

nicopap deleted the add-transform-div branch September 5, 2023 15:45

Uh oh!

Conversation

nicopap commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicopap commented Jun 2, 2022

Uh oh!

alice-i-cecile commented Jun 2, 2022

Uh oh!

bors bot commented Jun 2, 2022

try

Uh oh!

HackerFoo commented Jun 3, 2022

Uh oh!

HackerFoo Jun 3, 2022

Choose a reason for hiding this comment

Uh oh!

nicopap Jun 4, 2022

Choose a reason for hiding this comment

Uh oh!

yilinwei Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

nicopap Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

yilinwei Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

nicopap commented Jun 17, 2022

Uh oh!

HackerFoo commented Jun 18, 2022

Uh oh!

nicopap commented Jun 18, 2022

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

HackerFoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HackerFoo commented Jul 2, 2022

Uh oh!

HackerFoo commented Jul 2, 2022

Uh oh!

nicopap Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

nicopap commented Jul 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nicopap commented Jun 1, 2022 •

edited

Loading