Skip to content

Fix transform inversion to handle scaling correctly #1823

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

Conversation

killerdevildog
Copy link

  • Transform2D::invert() now uses proper determinant-based inversion instead of simple element swapping
  • Transform3D::invert() now uses basis.invert() instead of basis.transpose()
  • Both methods now handle matrices with scaling, rotation, and translation correctly
  • Removes FIXME comments about rotation-only assumption
  • Makes invert() equivalent to affine_invert() for consistency

Fixes: Transform scaling assumptions in invert() methods

- Transform2D::invert() now uses proper determinant-based inversion instead of simple element swapping
- Transform3D::invert() now uses basis.invert() instead of basis.transpose()
- Both methods now handle matrices with scaling, rotation, and translation correctly
- Removes FIXME comments about rotation-only assumption
- Makes invert() equivalent to affine_invert() for consistency

Fixes: Transform scaling assumptions in invert() methods
@killerdevildog killerdevildog requested a review from a team as a code owner July 21, 2025 16:52
@Ivorforce
Copy link
Member

This would be a regression from godotengine/godot#100209, cc @Flarkk.

Note that the problem has to be fixed upstream, with a PR to https://github.com/godotengine/godot. This repository only tracks the changes and syncs occasionally.

@Flarkk
Copy link
Contributor

Flarkk commented Jul 21, 2025

Makes invert() equivalent to affine_invert() for consistency

invert() and affine_invert() are different on purpose. This is to save computation time when the matrix is a pure rotation matrix with no scaling, which is a very common case.

That being said, it is true that this can lead to confusion, even though it is properly documented. There has been a few discussions opened on this topic over the past years (for example godotengine/godot#39433).

Instead of this PR, the best path forward would be to :

  1. Expose your perspective on one of these discussions, and/or open a new proposal
  2. If there is enough consensus, propose an implementation by opening a PR on the godot repository
  3. Lastly open a twin PR on godot-cpp and link it to the godot PR

This would be a regression from godotengine/godot#100209

I don't think it's related to godotengine/godot#100209. It didn't change anything about inverses.

@Ivorforce
Copy link
Member

I don't think it's related to godotengine/godot#100209. It didn't change anything about inverses.

You're absolutely right, seems like I mistook the file being changed. Sorry for the tag! (Though I'm glad I did anyway, since you explained the situation so well :) )

invert() and affine_invert() are different on purpose. This is to save computation time when the matrix is a pure rotation matrix with no scaling, which is a very common case.
That being said, it is true that this can lead to confusion, even though it is properly documented. There has been a few discussions opened on this topic over the past years (for example godotengine/godot#39433).

Perhaps this is a problem with there not being proper documentation for the codebase itself again. I see the comments in the implementations, but that is usually not the first place I'd check for info like this.
The header might be more appropriate, but it's also likely to be overlooked. The best way to fix this problem might be to rename invert to invert_rotation, and add a new function invert that always does correct rotation. Or from another perspective, add the fix and re-add the old function under a new name.

Thanks again for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants