-
-
Notifications
You must be signed in to change notification settings - Fork 222
Implement XformInv<...>
for Transform2D
, Transform3D
, Basis
.
#1082
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
base: master
Are you sure you want to change the base?
Implement XformInv<...>
for Transform2D
, Transform3D
, Basis
.
#1082
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1082 |
Godot supports it, but is it intuitive? Mathematically, you cannot multiply a 2x1 column vector with a 2x2 matrix. Since there is already the more explicit |
To elaborate a bit on my answer... The risk I see is in situations like these: let rotate: Transform3D = ...;
let rotate2: Transform3D = ...;
let rotated: Vector3 = ...;
let transform = rotate2 * rotate * rotated; Now someone wants to change order of rotations, but the refactor happens at 3:43am: let transform = rotate2 * rotated * rotate; Code compiles, everything seems fine. The developer commits, doesn't test... next day another developer spends 2h debugging because some object is in the wrong place, and that code looks fine at first. Of course it's an artificial scenario, and better variable names would help, but still, this class of issues doesn't exist with a named function. It's also much easier to recognize inverse-multiply if it doesn't look like regular matrix multiplication. There is also more nuance here. Godot docs:
So Maybe also noteworthy is that GDScript defines a lot of operators. Which is fine, with its focus on rapid prototyping and convenience rather than type-safety. For Rust, a lot of these operators would be considered weakly typed or straight unintuitive; I mean what's Point being, just because GDScript offers an operator doesn't mean we have to expose it as an operator, too. We should definitely expose the functionality for completeness. In obscure cases, In this case, we should probably add a named method, like it was the case in Godot 3 (I couldn't find why this was changed, might be interesting). Transform3D::xform_inv(self, v: Vector3) -> Vector3 This operation also exists for some other matrix-like types, such as Godot also uses an direct implementation and not |
I do agree, and I would be even more strict here – In general, I would propose creating methods
Methods has been removed with this PR, with comment stating that they are too complex and operators should be used instead 🤔 : godotengine/godot#42780 |
Why does it not make sense? There are also very practical reasons to support chaining of it: rotation * scale * vector
// vs.
(rotaton * scale).xform(vector)
Good call about I would definitely keep
Thanks for looking this up! (Note to self: need to expand It's interesting that they found |
Because it multiplies component of a transform – basis – with vector and adds origin afterwards. I.e. – it performs xform, instead of matrix multiplication, while docs state that both Transform2D and Transform3D are matrices. Transforms are ( therefore my_transform3d.basis * my_vector3 + my_transform3d.origin == my_transform3d * my_vector3;
my_transform2d.basis_xform(my_vector2) + my_transform2d.origin == my_transform2d * my_vector2; Godot docs explicitly state that I have no idea how common this kind of operator overload is outside Godot 🤔 RE: exposing
|
You're right, I glanced over this part. The mathematical operation that includes translation would be a 4x4 matrix, multiplied by a 4x1 vector: But since the last row are constant values, they don't need to be stored. So game developers usually use 3x4 matrices. This representation is indeed quite common in gamedev: |
I think most Rust libraries dont overload It'd be a breaking change but it may make sense to just remove them in our library too. |
Alright, then it would make sense to do the following:
|
Good point -- that's probably the reason why it doesn't exist in Godot for 3D transforms. Maybe we should briefly mention this in
Oh, interesting. I wonder if the design choice here was -- since those libraries offer many different matrix types -- to only implement
This one is interesting, I'm not sure about removing |
So how should we proceed here? To cover all operations, we'd have:
So, new APIs (✨) would be:
Note that "Apply inverse" could already today be done with Maybe the complete table above could be documented in a section # Vector transforms at the end of both |
d5efdcc
to
7ed6dc9
Compare
Mul<Transform...>
for Vector2
, Vector3
Xform<...>
for Transform2D
, Transform3D
, Basis
.
Xform<...>
for Transform2D
, Transform3D
, Basis
.XformInv<...>
for Transform2D
, Transform3D
, Basis
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
I was wondering about XformInv
being a trait instead of inherent methods, but seeing that it's also implemented for Aabb
/Plane
/Rect2
, it makes sense as we can't overload methods.
Does it maybe make sense for XformInv<T>
to have the supertrait Mul<T>
? That way, we can ensure symmetry by requiring the "xform" operation to be implemented, too.
godot-core/src/builtin/basis.rs
Outdated
/// Inversely transforms given [`Vector3`] by this basis, | ||
/// under the assumption that the basis is orthonormal (i.e. rotation/reflection is fine, scaling/skew is not). | ||
/// | ||
/// `basis.xform_inv(vector)` is equivalent to `basis.transposed() * vector`. See [`Basis::transposed()`]. | ||
/// | ||
/// For transforming by inverse of a non-orthonormal basis (e.g. with scaling) `basis.inverse() * vector` can be used instead. See [`Basis::inverse()`]. | ||
/// | ||
/// _Godot equivalent: `vector * basis`_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this comes from Godot docs on Vector3 * Basis
, but we should probably elaborate a bit better here and not assume implicit math knowledge:
- It's non-obvious why the equivalent is
basis.transposed() * vector
and notbasis.inverse() * vector
. We should explain that under the constraints, it's equivalent to both, and the inverse is the transpose for orthogonal matrices. - Note that the matrix can be orthogonal, but the basis can be orthonormal (see math.se).
Generally I find it a bit confusing that this isn't using the inverse and doesn't undo the scaling. I guess we should follow Godot, but is it that useful in practice? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included extended info why is that + once again linked docs to Godot Basis (since it explains all these concepts in the first paragraph or so) + included a link for Matrices&Transform tutorial (doesn't touch this topic directly but links to further materials).
I guess we should follow Godot, but is it that useful in practice?
honestly no idea – the only way to make sure would be analysis of repos/polling the users. For now I would stick to godot implementation, especially since we are doing so already (with Mul<T>
).
/// | Operation | Transform2D | Notes | | ||
/// |--------------------------------|--------------------------------|-------------------------------------| | ||
/// | Apply | transform * v | Supports [`Rect2`] and [`Vector2`]. | | ||
/// | Apply inverse | transform.xform_inv(v) | Supports [`Rect2`] and [`Vector2`]. | | ||
/// | Apply, no translate | transform.basis_xform(v) | Supports [`Vector2`]. | | ||
/// | Apply inverse, no translate | transform.basis_xform_inv(v) | Supports [`Vector2`]. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice to include this table! 👍
The 2nd column should use backticks for monospace style.
assert_eq_approx!( | ||
TEST_TRANSFORM_ORTHONORMAL.xform_inv(vec), | ||
vec.to_variant() | ||
.evaluate( | ||
&TEST_TRANSFORM_ORTHONORMAL.to_variant(), | ||
VariantOperator::MULTIPLY | ||
) | ||
.unwrap() | ||
.to::<Vector2>(), | ||
"operator: Transform2D * Vector2" | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use local variable definitions instead of mega-asserts 🙂
The
.to_variant()
.evaluate(&input.to_variant(), VariantOperator::MULTIPLY)
.unwrap()
.to::<T>(),
is also repeated many times and could be extracted into a multiply_variant
(or so) method.
* file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
*/ | ||
|
||
/// Applying inverse transforms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never includes scaling or translation, right? Might be worth mentioning... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly it does 🙃 - for Plane https://docs.godotengine.org/en/stable/classes/class_plane.html#class-plane-operator-mul-transform3d.
- Create new trait `XformInv<...>`. - Implement `XformInv<Rect2/Vector2>` for `Transform2D` and `Transform2D::basis_xform_inv`. - Implement `XformInv<Aabb/Plane/Vector3>` for `Transform3D` and `XformInv<Vector3>` for `Basis`. - Document transform/basis operations.
- Fix test (use `TEST_TRANSFORM_ORTHONORMAL` instead of `TEST_TRANSFORM` to test against some sane operation, not garbage)
- Post-CR fixes.
882f3f9
to
e6d7c23
Compare
Implements
Mul<Transform...>
forVector2
,Vector3
.We already support
Transform * Vector
so the reverse should be supported as well. Tests are translated 1:1 from Godot (the implementation haven't really changed for 12 years and I don't expect to see any changes here anytime soon).Godot Impl
Multiplying Godot Vector x Transform results in performing XFormInv.
Initially I did it with macro, but it is not worth it.