Changing bone euler rotation sets non-normalized quaternion

[romlok]

Tested versions

Reproducible in:

• v4.3.stable.official [77dcf97]

System information

Godot v4.3.stable - Debian GNU/Linux trixie/sid trixie - Wayland - Vulkan (Forward+) - dedicated AMD Radeon RX 7600 (RADV NAVI33) - AMD Ryzen 5 7600 6-Core Processor (12 Threads)

Issue description

If one has a model rigged with a Skeleton3D, and changes a bone's rotation by typing values in the temporary Euler inputs, the resultant quaternion will often be non-normalized.
While this appears to work just fine on the skeleton's pose, it will cause an error (and reset of bone rotation) if animation keyframes are played using such a non-normalized quaternion:

AnimationPlayer: 'AnimationPlayer', Animation: '', 3D Rotation Track: 'characterMedium/Root/Skeleton3D:RightUpLeg' contains unnormalized Quaternion key.
The end quaternion (-0.968029, 0.072789, 0.005684, 0.237784) must be normalized.

This also happens if you manually alter the values of the quaternion's inputs. The quaternion ends up non-normalized and fails during animations, but works fine for statically posing the skeleton.

As a possible workaround, if you mouse-drag the euler values (click-hold on the x|y|z label, and drag the mouse left or right to gradually change the values), instead of manually entering values, this seems to cause the resultant quaternion to be normalized.

Though TBH I'm not sure if the issue here should be "editing bone rotation can produce non-normalized quaternions" or "bone animation tracks reject non-normalized quaternions, even though they work for static poses". 😕

Steps to reproduce

The attached MRP includes a rigged model and an AnimationPlayer. The included animation sets the rotation for both legs, but only the left leg works.

You can experiment with ways of changing the rotation value of the RightUpLeg bone, setting its keyframe in the animation, and then seeing if the animation plays.

Minimal reproduction project (MRP)

animate-quaternion.zip

[VicooDi]

Hello would this be good for a first contribution? I intend on giving it a try.
I'd assume the solution would be to find a way to normalize the quaternion upon the conversion from euler since that seems to be the intended behaviour.
please do share if you have any input or advise, thank you.

[TokageItLab]

@VicooDi Thank you for your applicant. Yes, that's generally true, so you are free to proceed with your work.

One thing to note is that you need to determine where the conversion between Euler and Quaternion is broken.

The conversion from Euler to Quaternion is a mathematical process, so if it is broken there, then it is a problem with the Math class. However, judging from the issue description, the problem only occurs with direct typing number, not spin box dragging, so it could be a precision issue or a spin box update timing issue.

[VicooDi]

after a lot of testing..
Tested on :
Godot v4.3stable - Win11 - Vulkan(Forward+) - AMD Ryzen 7 7700 integrated GPU
Findings :
for me this problem occurs in the opposite case which is when the values are edited via quaternion and when using the temporary euler I get a normalized quaternion be it by direct value or draging

• using temp euler :
  
  this is normalized properly
• using quaternion drag & direct value respectfully :
  
  
  these are clearly not normalized values
• an example using transform rotation quaternion from another object :
  
  be it by dragging or direct value input the values are always clamped so that the quaternion is normalized

the last example is how I think this should be working and how it already does in the rest of engines functionality.

as for animations I haven't found any problem in any case be it normalized or not.
Conclusiion :

• I don't think the issue is exactly the conversion but the normalization of the quaternion itself upon changing the value and specifically when dealing with bones, this is odd since I'd assume they'd have the same ruleset and share a lot of the same code with other rotation quaternions in the engine.
• this will probably need further testing.

[VicooDi]

fixed it for now

although there is on problem which is when you input all values in the quaternion to 0 it causes an error from the math library, you are not supposed to do this anyway and this does no harm so I've decided to ignore it.

this does make the section related to the bone in the mesh disappear while the quaternions values are set to 0, but as I said a quaternion is not supposed to be used this way and the second you change the values it gets back to normal so there is no harm.

this would still need a bit more testing and a second opinion..

[TokageItLab]

using quaternion drag & direct value respectfully :

This is the intended behavior. Because you are intentionally setting an un-normalized quaternion. Judging from the description, it is not relevant to the issue as the problem is with editing temporary euler.

What needs to be fixed here is to fix any cases where normalization was not performed somewhere on the editor side when the temporary euler was changed. Then, if there are no cases of non-normalization, there must be some misunderstanding in the issue description and this issue should be closed.

To summarize:

• If an unnormalized Quaternion is generated when the Temporary Euler is changed, it is a bug
• It is not a bug that pose breaks when you intentionally set an unnormalized Quaternion
  • This is because SkeletonAPI now rejects unnormalized quaternions by Godot 4.0 specification changing

[VicooDi]

in that case this issue needs more testing, as I stated previously for my test cases temporary euler had no error or unintentional behaviour and animations worked properly.
the only possible problem would be the quaternion normalization and since that's intended behaviour then there seems to be no issue

[TokageItLab]

As I noted in #98308 (review), the main problem with implicit normalization immediately after set is that you will not be able to manually set an arbitrary quaternion from the inspector. So, even if we do normalize somewhere, we will need to change the EditorPropertyQuaternion to accept a temporary quaternion.

[romlok]

as for animations I haven't found any problem in any case be it normalized or not.

Do you mean that for you, when opening the MRP and selecting the "new_animation" animation, both of the model's legs raise and there are no errors in the output pane? 🤯

This is what happens for me:

2024-10-19.09-32-55.mp4

---

I did some more testing to double check myself, and used a script in the MRP to log in real-time whether the rotation quaternion was normalised or not.

MRP scene script

@tool
extends Node3D

@onready var skeleton := $characterMedium/Root/Skeleton3D as Skeleton3D
var prev_quat : Quaternion

func _process(_delta: float) -> void:
	var bone_id := skeleton.find_bone("RightUpLeg")
	var current_quat := skeleton.get_bone_pose_rotation(bone_id)
	if prev_quat and prev_quat.is_equal_approx(current_quat):
		return
	print("Rot changed ", current_quat, " normalized? ", current_quat.is_normalized())
	prev_quat = current_quat

It seems that it's specific Euler combinations which result in a non-normalized quaternion, and the initial angle I wanted ((30, 0, 0)) just happened to be one that triggered this when entered. It can happen when mousedragging the inputs too, just not very often:

2024-10-19.10-00-15-00.00.00.279-00.00.32.502.mp4

---

As I noted in #98308 (review), the main problem with implicit normalization immediately after set is that you will not be able to manually set an arbitrary quaternion from the inspector. So, even if we do normalize somewhere, we will need to change the EditorPropertyQuaternion to accept a temporary quaternion.

My immediate thought was that normalization should be an explicit choice by the user. So perhaps a warning in the inspector for non-normalized values, plus a lil' button to manually normalize it. But that seems like a separate feature proposal!

[VicooDi]

@romlok you are right, it seems I haven't noticed the error, but the whole thing seems to just be a another precision issue.

the quaternion in question is indeed normalized, just off by a small margin which should not cause a problem

we can tell the error comes from quaternion.cpp core file here
and checking the file at core\math\quaternion.cpp we find these lines

114 #ifdef MATH_CHECKS
115 	ERR_FAIL_COND_V_MSG(!is_normalized(), Quaternion(), "The start quaternion " + operator String() + " must be normalized.");
116 	ERR_FAIL_COND_V_MSG(!p_to.is_normalized(), Quaternion(), "The end quaternion " + p_to.operator String() + " must be normalized.");
117 #endif
118 ...

we can speculate the culprit behind this is is_normalized()
and changing it to something more lenient like this solves the issue and both legs rise :

114 #ifdef MATH_CHECKS
115	ERR_FAIL_COND_V_MSG(length() - 1.0 > 0.1, Quaternion(), "The start quaternion " + operator String() + " must be normalized.");
116 	ERR_FAIL_COND_V_MSG(!p_to.length() - 1.0 > 0.1, Quaternion(), "The end quaternion " + p_to.operator String() + " must be normalized.");
117 #endif
118 ...

but fundamentally the issue seems to be that the function is_normalized() in quaternion.cpp is a bit too precise since it caused issues like this in other instances
@TokageItLab Do you have an idea of what should be done in cases like this?
I'd assume a new issue about ìs-normalized()'s precision would be made but I honestly don't know what should be done.

---

My immediate thought was that normalization should be an explicit choice by the user. So perhaps a warning in the inspector for non-normalized values, plus a lil' button to manually normalize it. But that seems like a separate feature proposal!

that does seem like the best approach and the one we're decided on! some UI that helps user decide on whether to use a normalized or a non normalized quaternion.