Godot 4 breaks animated 3d models (Godot 3 to 4 conversion issue)

[Zireael07]

Godot version

4.0.2

System information

Linux Manjaro, Intel Kaby Lake

Issue description

Just open the attached project. It's a free CC-licensed minimal project that was advertised as, and indeed, working in 3.x.

For comparison's sake, a glb reexport of the character is also included. This one at least animates properly, but I haven't been able to get bone attachments and physical bones to work.

Steps to reproduce

See above

Minimal reproduction project

modeltest.zip

[Zireael07]

Still the case in 4.2.

I have an entire project that relies on this model in escn, and on bone attachments/physical bones. How do I unbreak that?

[Zireael07]

Tried nuking all the changes in Git (which brings the project in discussion back to 3.x), and re-upgrading. Same effect, the model is broken in exactly the same way.

[fire]

See detailed discussion on glb and lods on animated meshes. #84479

[Zireael07]

Note that GLB works (more or less, requires redoing all the attachments), it's the ESCN that doesn't.

[nikitalita]

This is due to Godot 4.x not having a compatibility layer for loading the old 3.x Skeleton and Animation resources (see #53689 for details):

• Skeletons had a Transform pose property in 3.x, which was broken out to Vector3 pose_position; Quaternion pose_rotation; Vector3 pose_scalein 4.x
• Animations used to have a transform track type, which was similarly broken out into position_3d, rotation_3d, and scale types

It wouldn't be too much effort to implement a compatibility layer here, and we already have a compatibility layer for the old 3.x Mesh format.

[nikitalita]

Skeleton pose conversion is easy enough to handle in _set():

Animations are going to be tricky, though; Animation tracks are stored ini style:

This means that, upon load, each of those indexed properties gets set, one at a time, which complicates things. We need to split the transformation track up into position, rotation, and scale tracks, and we can't arbitrarily add tracks in the middle of a load, since the index of the tracks will change. I'm not really sure how to handle this.

[nikitalita]

I have a WIP Solution up here: https://github.com/nikitalita/godot/tree/convert-escn

This converts all the animations, skeletons, and shaders into the new format.

Unfortunately, while all the resources in the escn load without errors now, the result comes out looking like this:

Trying to play any of the animations makes it look like this:

Banged my head against the wall for several hours trying to figure out why; no dice.

@fire , could you kindly take a look at this on my branch? I'm testing this with the repro project in the OP.

[fire]

Here's some notes from https://chat.godotengine.org about godot 3 to godot 4.

October 12, 2023
[aaronfranke]
aaronfranke7:17 PM

In Godot's Skeleton3D node, the value passed into set_bone_pose_rotation that results in an unposed bone is the bone's rest rotation. This goes contrary to my expectations, because I would expect to be able to pass a value of no rotation into set_bone_pose_rotation in order to have the bone be in the rest position, such that the bone's actual rotation is rest * pose. This is how it works in Blender. Same with set_bone_pose_position and set_bone_pose_scale. Is this a known issue? Is there a plan to change this behavior in the next compatibility breakage?

[Lyuma]
Lyuma11:06 PM
aaronfranke you have pointed out the one key difference between Godot 3 and Godot 4 😉

[Lyuma]
Lyuma11:14 PM
I can give a lot of reasons why the new Godot 4 behavior is more desirable:

First, while the user experience of being able to pass Quaternion(0,0,0,1) to reset the rotation to rest was really pleasant., it created a lot of extra math internally to the skeleton: every time a pose was evaluated, it was required to also multiply the rest pose matrices at each step, so this added more code and more CPU overhead.

Not all meshes have a rest pose. In particular, animation skeletons imported from Maya for example might not have a defined rest pose, or the rest poses of each bone might be identity. This disparity might be confusing to users. This point is less of an issue due to point (3) but I still agree with the principle that the rest pose should be merely advisory, not required for the math to work.

The import retargeter has created a standard skeleton with a frame of reference for all human bones which can be relied on. If I set %GeneralSkeleton:LeftUpperArm rotation to Quaternion(0.707, 0, 0.707, 0), it will behave the same on every mesh imported with the retargeter. Multiplying in the rest rotation would not assist at all in the understanding of the meaning of a particular pose.

Godot 4 separated the bone pose into separate position, rotation and scale components. While position (and possibly scale) of individual bones may differ between humanoid armatures, the rotations are standardized, and the hips position values are actually also standardized when the skeleton scale is applied. So there is not the same danger that existed in Godot 3 of accidentally overwriting the rest position when assigning the bone pose.

Anyway that's my mini litany. I could go on, but I think the decision in Godot 4 to de-incorporate rest from bone pose was very sound and has made skeletons far easier to work with.

[nikitalita]

@Zireael07 I now have it working; Your escn model imports fine and the animations play correctly now. Can you please test this to make sure that it works? https://github.com/nikitalita/godot/tree/convert-escn-2 (artifacts will show up here: https://github.com/nikitalita/godot/actions/runs/7304153146)

[fire]

@nikitalita did you make a pr for this?

[nikitalita]

I haven't made a PR yet because this involved changes to resource loading that probably won't be upstreamed in its current state. I had to add the ability to add special handlers for specific resource types to the loader, such that it forces the resource_format_text loader to load that resource as a MissingResource and then call the handler to instantiate the resource and set the properties.

[fire]

#87050 was merged, can people check if this resolves?

[nikitalita]

probably not, it doesn't fix the animations; I have to get #87106 into shape for merging for closing this

[Zireael07]

@nikitalita I somehow missed that comment that @ - me, are the artifacts still available?

[nikitalita]

https://github.com/nikitalita/godot/actions/runs/7501520724