I would like confirmation of approving on the direction in addition to mine. Probably @lawnjelly ?

@TokageItLab If this is to address the problem of non-normalized quats, I would tend to think #98090 (comment) is the best approach to fix (and I think you basically alluded to this in your quote I made there).

These other approaches seem like some kind of bodge to avoid fixing the inspector to deal with these normalized values properly. But maybe I'm misunderstanding.

(The need for more precision in the inspector for Quats than regular floats may also be an issue that warrants the changes in this PR, but at the moment the PR text seems to emphasize fixing the normalization problem rather than lack of precision for input. Maybe it's just the "fixes" section on the PR notes that is misleading?)

@lawnjelly The essence and cause of the problem is that when normalized values are entered into the inspector, as shown in script #106335, they are no longer normalized due to rounding(snapping).

extends Node3D

func _process(delta: float) -> void:
	var q: Quaternion = Quaternion(randf_range(-1, 1), randf_range(-1, 1), randf_range(-1, 1), randf_range(-1, 1)).normalized()
	q = Quaternion(snappedf(q.x, 0.001), snappedf(q.y, 0.001), snappedf(q.z, 0.001), snappedf(q.w, 0.001))

	if !q.is_normalized(): # <- Frequently detecting unnormalized
		print(str(q))

I don't know if it is possible in the EditorInspector to separate the displayed value from the internal value. Is there another similar place to do this? I cannot figure out how to keep the inspector value separate from the object immediately, after the value input to the inspector is internally normalized and set to the object. I think we need to change the fundamental behavior of the inspector when object is updated for that.

One particular problem is that internal normalization can cause other elements than the spin box you are operating to change internally, causing the current value and the inspector's value to be far apart. That approach would work for a single float spinbox, but it is not a good approach for vectors or matrices IMO.

Also, in the case of sequential input of normalized elements one at a time, if internal normalization is performed on each input, the final input will not match the value that was originally intended. This means that the user will never be able to enter arbitrary values into Quaternion. This is what I said during the #98308 (review) (case 2) review.

This means that we need to be very careful about the timing of internal normalization. For example, normalization should not occur when moving from the x SpinBox of a Quaternion property to the y SpinBox. But we do not know how to distinguish this from when you go from the x SpinBox of a Quaternion property to focus on another property and then back to the y SpinBox of a Quaternion property.

So I think we basically need to keep the values using this PR approach, without internal normalization. In other words, the only thing that can be internally asynchronous is precision.

Having sorted out the problem above, I am more convinced that it is currently difficult to avoid the problem except by this PR approach. Thanks @lawnjelly, the opinion has been helpful in sorting out them.

If we were to take the internal normalization approach, we would need to implement a temporary editor for Quaternion, which also seems like overkill.

So I would judge that this PR can be merged.

I think we need to change the fundamental behavior of the inspector when object is updated for that.

Well, yes, that's what I'm suggesting.
If you aren't confident in this area, you could ask some of the more editor orientated contributors if they can help?

Also, in the case of sequential input of normalized elements one at a time, if internal normalization is performed on each input, the final input will not match the value that was originally intended. This means that the user will never be able to enter arbitrary values into Quaternion. This is what I said during the #98308 (review) (case 2) review.

I didn't write anything about normalizing editor elements one element at a time.

You explicitly cannot do this for any class that is normalized, because it would prevent entering anything sensible in the editor. What I was suggesting is that the internal quat is normalized, the editor quat is a different variable. If you re-read what I wrote in the original comment hopefully this will make sense.

After all, currently the editor Quaternion is supposed to immediately reflect the value returned by the object after it is set to the object, so we need to change the editor fundamental behavior, but which is not an easy task since the scope of influence is quite large.

Also, a second PR is needed to implement the internal normalization after changing the editor fundamental behavior. It is probably ideal, but definitely not something that can be done in time for 4.5 at this stage, no matter who does it. I will try to write a proposal later.

So I consider this PR to be fine as a tentative bugfix. If in the future we include the editor Quaternion separation and internal normalization, there should be no compatibility concerns with the project itself although the editor input workflow for user may bit change.

Sure, if this fixes some issues on general editing of quaternions due to giving more accuracy etc than that is fine to proceed too.

I'm only questioning whether this actually does fix #98090 , or whether it actually addressed normalization of quats at all.

#98090 is definitely fixed by this PR. Since EulerEditor will input its composed normalized Quaternion value into QuaternionEditor but the problem occurs due to snapping in the spinbox.

editor_properties.cpp

void EditorPropertyQuaternion::_custom_value_changed(double val) {
	edit_euler.x = euler[0]->get_value();
	edit_euler.y = euler[1]->get_value();
	edit_euler.z = euler[2]->get_value();

	Vector3 v;
	v.x = Math::deg_to_rad(edit_euler.x);
	v.y = Math::deg_to_rad(edit_euler.y);
	v.z = Math::deg_to_rad(edit_euler.z);

	Quaternion temp_q = Quaternion::from_euler(v);
	spin[0]->set_value_no_signal(temp_q.x); // Will be snapped and can be broken.
	spin[1]->set_value_no_signal(temp_q.y); // Will be snapped and can be broken.
	spin[2]->set_value_no_signal(temp_q.z); // Will be snapped and can be broken.
	spin[3]->set_value_no_signal(temp_q.w); // Will be snapped and can be broken.
	_value_changed(-1, "");
}

EulerEditor - (NormalizedQuaternion) -> QuaternionEditorSpinBox - (SnappedBrokenQuaternion) -> Object = cause Error

It also means that current QuaternionEditor, for example, if you just look at the keys of an imported animation RotationTrack may be fine, but if you directly edit the Quaternion key values and enter the same values for appearance (by snapping), it may break. I remember there was a similar problem #64340 before #98090, but I think the cause is the same.

Applying the EulerEditor values directly to the object may also solve #98090, but this PR is more generic because it also addresses the problem that copying and pasting normalized Quaternion values will break them. In the future, maybe the EulerEditor values should be sent to the internal Quaternion.

Can you update the pull request so it's rebased?

@lawnjelly This issue is still unresolved, and it prevents Quaternion keyframe editing (setting some specific Euler values) on the Godot. Unless you intend to block this and you implement your proposed solution, I believe this PR should be merged. Can you approve it?

I wasn't intending to block this PR, but just spur some questioning on best way of editing normalized values (to prevent problems like the linked issue).

From memory the problem is, if you make the inspector value the "current always normalized master value", then it makes the usability very bad if you want to e.g. type in a specific quaternion (or vector3 etc).

Afaik in this PR you are trying to make the inspector "master value" more accurate here, to make it more likely to be normalized.

My thoughts are, that in a way it is irrelevant whether the inspector value is normalized (that's just a display for easy editing). Only the internal value needs to be normalized, so decouple the two. I'm not keen to approve something which I'm not yet convinced is a good approach.

It might be a good idea to bring it up in a rendering(?) meeting to get approval / thoughts, as a lot of the rendering guys are familiar with quaternions / normalization issues (and more familiar with 4.x than myself), and this is relevant not just for quaternions.

@lawnjelly This issue is simpler than you think, it occurs because the precision of the Quaternion values entered into the Inspector is insufficient, causing this to manifest in the scene.

Quaternions have a temporary EulerEditor, but this is what you use to input values into the Quaternion Inspector, and it suffers from this precision issue. Even if a user enters a Quaternion calculated on a calculator, the same problem which the precision is insufficient and an error occurs.

From memory the problem is, if you make the inspector value the "current always normalized master value", then it makes the usability very bad if you want to e.g. type in a specific quaternion (or vector3 etc).

So, since this operation is currently impossible with some values, there's little point in worrying about it. Moreover, a temporary EulerEditor already exists to avoid the need to directly modify the values of QuaternionEditor elements, but certain values are unavailable due to precision issues on Quaternion elements side.

In other words, this will occur unless a decision is made to add normalize() to all setters of the Quaternion property (However, that will undoubtedly cause performance issues). Alternatively, it could be resolved by making the value displayed in the inspector different from the internal value within the inspector (With the current Inspector, that is impossible and always the same). However, as @aaronfranke mentioned above, that would be a major change. The implementation is likely to be more complex than you imagine.

Therefore, the simplest solution currently is just to increase the inspector's precision and allow pasting values calculated with a calculator, as proposed in this PR. I believe separating internal inspector values from external values can be addressed as a follow-up later (and considering the possibility that it could also be applied to properties that want to use doubles other than quaternions, the scope of the review becomes larger), so I argue that prioritizing a solution for the current error-induced unusable state is more important.

To reconfirm the reproducibility of the issue, it will be demonstrated that the problem occurs within the IK demo:

Project is here

hand_c2 track key value set x:11.9 as euler:

If the value is set x:12.0 as euler, then convert to Quaternion normalization and set it into QuternionEditor, finally precision is lost and normalization breaks by the QuternionEditor inspector:

hand_c2 track will no longer be playable:

Thanks!

Cherry-picked for 4.6.1.