Implement some methods of the Transform API

[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.

[Bromeon]

Thank you!

This actually made me notice that we have quite a few inconsistencies in our geometric APIs:

• Transform2D and Quat have an IDENTITY constant, while Basis has an identity() function. We should probably go for the constant.
• Default is implemented for some geometric types but not all. Do we have a good use case for generic programming?
• Self vs. struct name inside impl.

No need to fix anything outside Transform here, I can gladly take care of that.

bors try

[Bromeon]

Any particular reason why this deviates from the GDScript method signature? In particular, using eye instead of &self. Probably it's more intuitive when one is mostly interested creating a new coordinate system from scratch (including translation + rotation, but not scale)? In that case, a constructor name like from_position_direction() or so might be more appropriate.

But generally it seems to me that in Godot, it's more idiomatic to construct custom transforms by chaining operations, rather than specialized constructors. Would this method be equivalent to the following?

Transform::IDENTITY
   .translated(eye)
   .looking_at(target, up) // assuming canonical impl

[Bromeon]

Same decision would also affect the already present translate(origin) vs. translated(&self, origin).

[setzer22]

Any particular reason why this deviates from the GDScript method signature?

The main reason is that I copied this from a utility method in my code that used these arguments. But I think it makes more sense to make it consistent with the API 👍

Same decision would also affect the already present translate(origin) vs. translated(&self, origin)

Indeed. I didn't want to change that because it was already there so it would become a breaking change. But since you're mentioning it I went ahead and created the following methods:

• from_position, renamed from the old translate.
• translate, which takes &mut self and modifies in-place
• translated, which takes &self and returns a modified transform

I think this is closer to the other APIs.

[Bromeon]

translated, which takes &self and returns a modified transform

This one definitely makes sense.

I'm not 100% sure about the others -- I would tend to go for the minimal API (close to Godot). Because if we have translate(), we would also need rotate() and scale(), and do the same for Transform2D. The same applies to from_position(), which can currently be expressed as IDENTITY.translated(v).

I'm not categorically against convenience functions, but for now I'd stay with the building blocks and encourage the "chain different transforms" pattern.

[bors]

try

Build succeeded:

• full-ci

[jacobsky]

Looks good to me. I have nothing to add that bromeon hasn't already mentioned.

[Bromeon]

Most looks good, see comment above.

Maybe we could add a constructor from_axis_origin() instead, consistent with Transform2D::from_axis_origin() and the GDScript constructor.

[setzer22]

@Bromeon I removed some of the redundant constructors to simplify the API and added a new from_axis_origin constructor as you suggested. I also realized Transform does not implement Mul, so I added an implementation for that, since it's a very convenient thing to have.

Let me know when this is ready to merge and I'll squash all those commits on my branch.

[Bromeon]

Thanks, looks good now! Also thanks for the multiplication!

If you squash the commits, it should be ready 🙂
bors try

[bors]

try

Build succeeded:

• full-ci

[Bromeon]

Thank you 🙂

bors r+

[bors]

Build succeeded:

• full-ci