Transition / blend times in AnimationPlayer are reordered upon saving

[RobTheFiveNine]

Godot version

v4.0.1.stable.official [cacf499]

System information

Kubuntu 23.04

Issue description

When setting up an AnimationPlayer with several animations, it is possible to hit the "Animation" button to the left of the animation name / dropdown box and click "Edit Transitions..." which will let you create a blend time between the animations. The blend times of these animations are then stored in the tscn file like so:

blend_times = [&"RESET", &"RESET", 0.8, &"RESET", &"anim1", 0.8, &"RESET", &"anim3", 0.8, &"RESET", &"anim2", 0.8, &"anim1", &"RESET", 0.8, &"anim1", &"anim1", 0.8, &"anim1", &"anim3", 0.8, &"anim1", &"anim2", 0.8, &"anim3", &"RESET", 0.8, &"anim3", &"anim1", 0.8, &"anim3", &"anim3", 0.8, &"anim3", &"anim2", 0.8, &"anim2", &"RESET", 0.8, &"anim2", &"anim1", 0.8, &"anim2", &"anim3", 0.8, &"anim2", &"anim2", 0.8]

If an unrelated change is then made to the scene (for example, adding another child node), after saving, the blend times are then regenerated. Although the blend times remain correct, the order in the file is changed, which results in version control noise every time the file is saved.

An example of this can be seen on line 85 of the following diff: RobTheFiveNine/godot-animation-blend-reorder-bug@3ab62ba#diff-d3989be7149720fdbe62886f9bcc0644e7012d3c8b3f239c3d618e0760c307d0R85

index 2cffb18..e2771b9 100644
--- a/node_3d.tscn
+++ b/node_3d.tscn
@@ -2,9 +2,8 @@
 
 [sub_resource type="BoxMesh" id="BoxMesh_6clns"]
 
-[sub_resource type="Animation" id="Animation_mi6jd"]
-resource_name = "anim1"
-length = 3.0
+[sub_resource type="Animation" id="Animation_d1xbe"]
+length = 0.001
 tracks/0/type = "value"
 tracks/0/imported = false
 tracks/0/enabled = true
@@ -12,14 +11,15 @@ tracks/0/path = NodePath("CSGMesh3D:rotation")
 tracks/0/interp = 1
 tracks/0/loop_wrap = true
 tracks/0/keys = {
-"times": PackedFloat32Array(0, 1.5, 3),
-"transitions": PackedFloat32Array(1, 1, 1),
+"times": PackedFloat32Array(0),
+"transitions": PackedFloat32Array(1),
 "update": 0,
-"values": [Vector3(0, 0, 0), Vector3(3.14159, 0, 0), Vector3(0, 0, 0)]
+"values": [Vector3(0, 0, 0)]
 }
 
-[sub_resource type="Animation" id="Animation_d1xbe"]
-length = 0.001
+[sub_resource type="Animation" id="Animation_mi6jd"]
+resource_name = "anim1"
+length = 3.0
 tracks/0/type = "value"
 tracks/0/imported = false
 tracks/0/enabled = true
@@ -27,10 +27,10 @@ tracks/0/path = NodePath("CSGMesh3D:rotation")
 tracks/0/interp = 1
 tracks/0/loop_wrap = true
 tracks/0/keys = {
-"times": PackedFloat32Array(0),
-"transitions": PackedFloat32Array(1),
+"times": PackedFloat32Array(0, 1.5, 3),
+"transitions": PackedFloat32Array(1, 1, 1),
 "update": 0,
-"values": [Vector3(0, 0, 0)]
+"values": [Vector3(0, 0, 0), Vector3(3.14159, 0, 0), Vector3(0, 0, 0)]
 }
 
 [sub_resource type="Animation" id="Animation_2woje"]
@@ -82,4 +82,7 @@ mesh = SubResource("BoxMesh_6clns")
 libraries = {
 "": SubResource("AnimationLibrary_0p174")
 }
-blend_times = [&"RESET", &"RESET", 0.8, &"RESET", &"anim1", 0.8, &"RESET", &"anim3", 0.8, &"RESET", &"anim2", 0.8, &"anim1", &"RESET", 0.8, &"anim1", &"anim1", 0.8, &"anim1", &"anim3", 0.8, &"anim1", &"anim2", 0.8, &"anim3", &"RESET", 0.8, &"anim3", &"anim1", 0.8, &"anim3", &"anim3", 0.8, &"anim3", &"anim2", 0.8, &"anim2", &"RESET", 0.8, &"anim2", &"anim1", 0.8, &"anim2", &"anim3", 0.8, &"anim2", &"anim2", 0.8]
+blend_times = [&"RESET", &"RESET", 0.8, &"RESET", &"anim3", 0.8, &"RESET", &"anim2", 0.8, &"RESET", &"anim1", 0.8, &"anim3", &"RESET", 0.8, &"anim3", &"anim3", 0.8, &"anim3", &"anim2", 0.8, &"anim3", &"anim1", 0.8, &"anim2", &"RESET", 0.8, &"anim2", &"anim3", 0.8, &"anim2", &"anim2", 0.8, &"anim2", &"anim1", 0.8, &"anim1", &"RESET", 0.8, &"anim1", &"anim3", 0.8, &"anim1", &"anim2", 0.8, &"anim1", &"anim1", 0.8]
+
+[node name="Node3D" type="Node3D" parent="."]
+transform = Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 1.54048, 0, 0)

The only change made in this sample project was to add a Node3D child object to the root node, but the order of the blend times changes and makes the version control flag more changes.

The same issue can also be triggered by simply hitting CTRL+S on an unchanged scene, doing this can result in the blend_times property being changed, as can be seen in this commit: RobTheFiveNine/godot-animation-blend-reorder-bug@806b2b4

index e2771b9..6e4fd74 100644
--- a/node_3d.tscn
+++ b/node_3d.tscn
@@ -82,7 +82,7 @@ mesh = SubResource("BoxMesh_6clns")
 libraries = {
 "": SubResource("AnimationLibrary_0p174")
 }
-blend_times = [&"RESET", &"RESET", 0.8, &"RESET", &"anim3", 0.8, &"RESET", &"anim2", 0.8, &"RESET", &"anim1", 0.8, &"anim3", &"RESET", 0.8, &"anim3", &"anim3", 0.8, &"anim3", &"anim2", 0.8, &"anim3", &"anim1", 0.8, &"anim2", &"RESET", 0.8, &"anim2", &"anim3", 0.8, &"anim2", &"anim2", 0.8, &"anim2", &"anim1", 0.8, &"anim1", &"RESET", 0.8, &"anim1", &"anim3", 0.8, &"anim1", &"anim2", 0.8, &"anim1", &"anim1", 0.8]
+blend_times = [&"RESET", &"RESET", 0.8, &"RESET", &"anim2", 0.8, &"RESET", &"anim1", 0.8, &"RESET", &"anim3", 0.8, &"anim2", &"RESET", 0.8, &"anim2", &"anim2", 0.8, &"anim2", &"anim1", 0.8, &"anim2", &"anim3", 0.8, &"anim1", &"RESET", 0.8, &"anim1", &"anim2", 0.8, &"anim1", &"anim1", 0.8, &"anim1", &"anim3", 0.8, &"anim3", &"RESET", 0.8, &"anim3", &"anim2", 0.8, &"anim3", &"anim1", 0.8, &"anim3", &"anim3", 0.8]
 
 [node name="Node3D" type="Node3D" parent="."]
 transform = Transform3D(1, 0, 0, 0, 1, 0, 0, 0, 1, 1.54048, 0, 0)

Steps to reproduce

1. Create a new project and add an AnimationPlayer which has several animations
2. Setup blend times on each animation
3. Commit the project to a version control repo
4. Press CTRL + S on the unchanged scene or add a new node to it and check the diff in the repository (sometimes it may take a few attempts as it seems to sporadically use a different order when saving the blend times)

Minimal reproduction project

https://github.com/RobTheFiveNine/godot-animation-blend-reorder-bug

[RobTheFiveNine]

I've noticed there is a similar issue with mesh libraries. I have a project that has a MeshLibrary saved to a tres file and it sporadically regenerates the subresource IDs and causing unnecessary version control noise like so:

@@ -106,7 +106,7 @@ data = {
 }
 
 [sub_resource type="ImageTexture" id="ImageTexture_kh4xs"]
-image = SubResource("Image_f12pa")
+image = SubResource("Image_mp7od")
 
 [sub_resource type="BoxShape3D" id="BoxShape3D_0kusy"]
 size = Vector3(1.12986, 1.00339, 0.87913)

[JacobMillner]

I'm also seeing this bug in 4.2.1.

[JacobMillner]

I think I found where the bug lives, I will hopefully have a fix PR up soon.

[JacobMillner]

Ok so I see that when the animation_player tries to order the blend_times BlendKeys here https://github.com/godotengine/godot/blob/master/scene/animation/animation_player.cpp#L82 it's putting them in a sim-random order.

I believe the problem is that the it uses the BlendKeys < operator to sort, and this compares pointers which is non-deterministic, seen here https://github.com/godotengine/godot/blob/master/scene/animation/animation_player.h#L97-L101

This matches the behavior where each time I open godot and a scene with blend_times it re-orderes them but doesn't do it on each save. You have to open and close the app.

My proposed fix is to just cast the pointers as strings on BlendKeys < operator before the comparison so when sorting it, it is in lexicographical order and deterministic.

bool operator<(const BlendKey &bk) const {
   		if (String(from) == String(bk.from)) {
   			return String(to) < String(bk.to);
   		} else {
   			return String(from) < String(bk.from);
   		}
   	}

I tested this locally and it did the trick - no re-ordering each time I open the scene in godot.

Any thoughts? This seems kind of hacky but i'm not sure of a better way without affecting other parts of the program.

Edit: I may be able to use get_name instead of String() - i'll test that tonight