-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Fix Node3D.set_global_rotation()
resetting node scale.
#90584
Fix Node3D.set_global_rotation()
resetting node scale.
#90584
Conversation
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.
LGTM
Node3D.set_global_rotation()
resetting node scale.
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.
@aaronfranke I think this is correct?
This should be valid to fix, but I wouldn't cherry pick it as it at least mildly breaks compatibility, people might very well rely on it resetting scale and this would break their code |
Agreed, cherry picking it back into 4.2 is dangerous as only a patch-level fix for this reason. |
a4e6d4b
to
7b07540
Compare
I would lean towards cherry-picking this. It is really weird for setting the rotation to change the scale. I would assume most users are using this method with unscaled objects, or else they would've run into this bug before. |
7b07540
to
dd97ff4
Compare
This was caused because the basis for the target global transform was entirely recreated. This process did not account for the scale of the basis of the current global transform. This PR amends this by scaling the recreated basis for the global transform by the current global scale of the Node3D. Note that this scaling has to be done from the current global scale and not from the local scale of the Node3D, otherwise issues would arise if parents of this Node3D would be scaled.
This bug was discovered before 4.0 released so it's absolutely been run into, the report in question is from march last year, so just after 4.0, and there was a report before that with less details from January of the same year, so I don't think this is necessarily unnoticed |
Thanks! And congrats for your first merged Godot contribution 🎉 |
Fixes #74162.
The error was caused because the basis for the target global transform was entirely recreated. This process did not account for the scale of the basis of the current global transform.
This PR amends this by scaling the recreated basis for the global transform by the current global scale of the Node3D. Note that this scaling has to be done from the current global scale and not from the local scale of the Node3D.
This is important, otherwise a scaled parent of the Node3D would cause incorrect results on overwriting the global transform basis.