Fix Transform2D from_rotation_translation_scale creating a rotated origin

[rand0m-cloud]

This should close #915. This PR fixes a bug in the Transform2D constructor where the position was rotated around the origin.

[rand0m-cloud]

Oh well, its been a rough time debugging this issue. Feels like I was building the integration test and sometimes failing to copy the native library over in my testing. Anyways, this actually appears to be intended behavior. Playing with the integration test shows Godot also returns the same transform when using rotated. I disagree with that behavior, but its not a bug.

The PR has been updated to address the bug with Transform2D::from_rotation_translation_scale.

[Bromeon]

Thanks for addressing this!

Would your fixed version be equivalent to

Self::IDENTITY
    .translated(translation)
    .scaled(scale)
    .rotated(rotation)

And if yes, should we maybe rename the constructor to from_translated_scaled_rotated (and leave the old one deprecated, for compatibility)?

[rand0m-cloud]

No and this would be breaking change on from_rotation_translation_scale. The problem statement is I want to create a Transform with a given position and basis. As the constructor works currently, setting the rotation value to anything other than zero will mutate the position I gave it. (Setting the scale to anything other than one would also mutate the position.)

From the issue:

let position = Vector2::new(250.0, 150.0);
let rotation = PI / 4.0;
let transform = Transform2D::from_rotation_translation_scale(position, rotation, Vector2::ONE);

The resulting transform will not have an origin of (250, 150) but rather a (70.71068, 282.8427) origin. The most concise way of achieving my goal seems to be:

let mut transform = Transform2D::IDENTITY.translated(position);
transform.set_rotation(rotation);

As a comparison, in scripting Transform2D(rotation, position) will retain the given position as the transform's origin.

I think depreciating and renaming the constructor will be ideal. An additional constructor should be made to allow users to create Transform2D like they can do in scripting.

[Bromeon]

I think depreciating and renaming the constructor will be ideal. An additional constructor should be made to allow users to create Transform2D like they can do in scripting.

For the 2D Transform2D class, I fully agree.

For the 3D Transform class, there is no GDScript equivalent of a constructor taking translation, rotation and scale. So my questions are:

1. Is there one particular order of translation/rotation/scale which is significantly more frequent than others?
2. If not, should we keep the constructor at all? Transform::IDENTITY.scaled(s).translated(t).rotated(r) is a bit longer, but much more explicit.
3. If we do keep it, the naming (now from_rotation_translation_scale) is important. It fulfills two functions:
   • Express which parameter does what
   • Possibly indicate the order how transforms are applied -- this is not necessarily true, but this may also cause confusion. Which is also a reason why the fluent API may be better than such a constructor.

[Bromeon]

Postponed to v0.10.2 🔜

[Bromeon]

@rand0m-cloud The questions I asked above are still open, could you maybe elaborate a common use case?

I plan to release godot-rust 0.11 in the next few days, so this would be the last chance for a breaking API change for a long time. Otherwise we'd need to live with deprecation.

[Bromeon]

bors r+

[bors[bot]]

Build succeeded:

• full-ci

[Bromeon]

Thanks a lot for this fix! 🚀