Fix non normalized quaternion for Skeleton3D

[VicooDi]

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: Changing bone euler rotation sets non-normalized quaternion #98090

[TokageItLab]

I recall that there is a problem in the inspector with normalization immediately after a set.

1. The phase (sign) can be inverted unintentionally

Quaternions can have phase inverted states for the same rotational state when they are static and not interpolated. Implicit normalization can cause sudden positive/negative inversions, which can break animation keys and editing by dragging. This is related to the nan issue you comment on in #98090 (comment).

2. If the moment you set one element, the other elements change on their own, you will not be able to enter a specific quaternion manually

Similar to above, but this means that it is necessary to temporarily accept an unnormalized quaternion to allow manual input.

For example, if you want to enter the value Quaternion(0.5, 0.0, 0.5, 0.0), the element changes when you enter 0.5 for x. If you change x to 0.5 and then change y, the x value changes again. This is an unacceptable problem.

3. The behavior is not consistent with Quaternion elsewhere

If you could prevent flipping the positive and negative, you might be able to do normalization immediately after input, but this would not be consistent with other places, and you might want it in other places. For example, when editing a key on the Rotation3DTrack.

Therefore, when implementing normalization, the Quaternion property should be hinted to perform normalization by PropertyHint, which will then perform normalization within the EditorPropertyQuaternion.

Although there are few cases where an unnormalized Quaternion is needed, it is necessary to accept the unnormalized state as well, since it is possible to have temporary unnormalized states during nlerp() and slerp() calculations. However, for Skeleton's pose, a broken Quaternion should not be set and the set should be aborted with an error.

---

Finally, the one part of the correct approaches is not to normalize the quaternion in the setter, but rather to generate an ERROR and not accept the broken quaternion.

This can coexist with the normalization of the EditorPropertyQuaternion, so the PR can be split into two:

1. Add hint, normalization and temporary Quaternion editor to EditorPropertyQuaternion
2. Make error when setting un-normalized quaternion in set_bone_pose_rotation()

However, note that the modification of the EditorPropertyQuaternion must be implemented first, since a temporary un-normalized quaternion for manual input must be allowed, without which manual input cannot be performed due to errors. Probably need to add an editor like temporary QuaternionEditor that can switch with temporary EulerEditor.

[TokageItLab]

Suggested change

	return Math::is_equal_approx(length_squared(), 1, (real_t)0.01/*UNIT_EPSILON*/); //use less epsilon // 0.01 is a temporary fix for precision errors
	return Math::is_equal_approx(length_squared(), 1, (real_t)UNIT_EPSILON); //use less epsilon

I feel that the precision threshold should not be changed, and if the precision is insufficient prior to this point, and I think it is possible to increase the precision by using double insteads real_t (float) within the calculation as commented in #98090 (comment).

[TokageItLab]

If the precision problem were corrected properly, these should not be necessary.

[VicooDi]

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

[TokageItLab]

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.

[VicooDi]

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!

[VicooDi]

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/math function in quaternion.cpp form:

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 know length_squared() is a lot better of an alternative to length() in terms of optimization that should not come at the cost of precision.

[TokageItLab]

This approach does not solve the problem of the quaternions being normalized on their own in Node3D and not being able to enter quaternions manually. In other words, one of the following two approaches must be chosen:

1. If we add a normalization button, we must remove the implicit normalization in Node3D

2. If we add the temporary Quaternion editor, we will need to add either implicit normalization or rejection of unnormalized quaternions due to error to Bone Pose

Since this is a pretty fundamental design issue, it seems to me that we need to have a discussion with @reduz about which approach to choose

In the case of 1, unintended normalization or errors may occur during processing unrelated directly to the quaternion set, so I personally prefer 2 as the safer choice.

At worst, I think it is possibly acceptable to change length_squared and length, but I still think there are causes elsewhere, such as the trigonometric calculations. length is very slightly more computationally expensive than length_squared (but due to the many locations where it is used, the performance degradation could be noticeable depending on the algorithm), and is not consistent with other linear arithmetic classes.

[VicooDi]

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