Fix non normalized quaternion for Skeleton3D
Fix for issue Changing bone euler rotation sets non-normalized quaternion #98090
I haven't received much advice while working on this issue so I went with the most straight forward solution, I have made more clarifications of what I did on the comment section of the issue and would appreciate any feedback if my solution is not sufficient.
- Bugsquad edit, fixes: https://github.com/godotengine/godot/issues/98090
to explain the new changes made :
Editor :
the changes to the files in /editor (editor_properties.cpp and editor_properties.h) boil down to adding a button to normalize the quaternion on demand, this solves the issue if the user need to make a non normalized quaternion while still offering the option for the user to "fix" the quaternion
video showcasing the change
the icon choice and positioning still need more opinions as this was made to showcase the idea itself.
core :
the change in /core/quaterninon.cpp was a temporary one made to fix the issue with the is_normalized() function as it is a bit "too precise" (more info here) this also is the solution to https://github.com/godotengine/godot/issues/98090, this is just a temporary fix and would nned to be changed later.
scene :
this should not even be considered a change, but the first commit's solution was removed and replaced with commented error message to be used in the future in case the user uses a unormalized quaternion, the reason this was commented was because right now it only clutters the output so I'm currently looking into a more elegant approach to this error.
this issue started out as me thinking it's just a simple conversion between euler to quaternion so I got a bit confused when it evolved into creating a new functionality so I made this commit mainly to get more feedback and opinions on this.
the change in /core/quaterninon.cpp was a temporary one made to fix the issue with the is_normalized() function as it is a bit "too precise"
I think it's not "too precise", but the precision seems to be lacking in the prior process as I commented above.
I think it's not "too precise", but the precision seems to be lacking in the prior process as I commented above.
yeah I'll definitely be looking into that as a solution for the next time, thanks for the idea!
Revision To Changes :
Editor :
- the button position was changed to underneath the quaternion and instead of an icon the button now has "Normalize" written on it so the user can immidetly tell it's for normalizing the quaternion, even if they are still confused on what it does if pressed a message is displayed on the output with "Quaternion Normalized", this action also supports undo and redo now.
new button "Normalize"
- the error message that appears when a non-normalized quaternion is made was changed into a warning as this action has become easily reversible now.
Core :
- as for the error with
is_normalized()I tried going with the playing around with types and conversion in multiple files to get a better precision but it didn't work, what worked though was changing the/core/mathfunction inquaternion.cppform:
bool Quaternion::is_normalized() const {
return Math::is_equal_approx(length_squared(), 1, (real_t)UNIT_EPSILON); //use less epsilon
}
to:
bool Quaternion::is_normalized() const {
return Math::is_equal_approx(length(), 1, (real_t)UNIT_EPSILON); //use less epsilon
}
- it seems that
length_squared()moved the number to be compared a bit too much from it's intended value, and although I knowlength_squared()is a lot better of an alternative tolength()in terms of optimization that should not come at the cost of precision.
@TokageItLab ,Yes this approach doesn't solve the issue with Node3D because it's supposed to solve the issue with skeleton3D but if we are to go forward with this implicit normalization will have to be removed from Node3D. but other than that I agree with everything else, we should get more opinions on this before going forward.