GLTFDocument/GLTFState don't work correctly for skeletons in 4.2.1

Calandiel commented 8 months ago

Tested versions

Reproducible in 4.2.1.stable
Not reproducible in 4.1.3.stable

System information

Godot v4.2.1.stable.mono - Fedora Linux 38 (Workstation Edition) - Wayland - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 Laptop GPU () - 11th Gen Intel(R) Core(TM) i5-11400H @ 2.70GHz (12 Threads)

Issue description

Something about the behavior of append_from_file or get_godot_skeleton on GLTFDocument has changed between versions 4.1.3.stable and 4.2.1.stable.

The following script loads a gltf file, then gets a godot skeleton from the gltf state and prints the number of bones on it:

extends Node

@export_file() var path: String;

func _ready():
    var doc = GLTFDocument.new()
    var state = GLTFState.new()

    doc.append_from_file(path, state)

    var gltf_skeleton = state.skeletons[0]
    var godot_skeleton = gltf_skeleton.get_godot_skeleton() # Returns null in Godot 4.2.1 stable and an instance in Godot 4.1.3

    print(godot_skeleton.get_bone_count())

In 4.2.1.stable, get_godot_skeleton returns null, in 4.1.3, it returns a proper object that can be used (and the following line prints out the number of bones correctly, as can be verified with external programs such as Blender).

I've noticed other oddities (duplicating an instance of a node with blend shapes loaded with GLTFDocument/GLTFState doesn't work in 4.2.1.stable but does in 4.1.3). Let me know if I ought to open more issues related to them or if I should instead append more information to this issue (as I imagine they could have the same root cause).

Steps to reproduce

run the included project in 4.2.1.stable (example.tscn), note that the program fails to print the number of bones in the model
repeat with 4.1.3.stable, note that this time it works

Minimal reproduction project (MRP)

loadingtest.zip

fire commented 8 months ago

@aaronfranke Any ideas? Will probably need to git bisect to find the change.

Calandiel commented 8 months ago

When I was working on a workaround by writing a gltf parser from scratch myself (I only need a subset of all features), I noticed that the rendering server now checks for presence of tangents if normals are present (this wasn't the case before). The model I attached in the reproduction project has many blend shapes that don't store tangent data themselves, leading to the check for this data failing when iterating over blend shapes to add a surface to an array mesh.

Perhaps that's related to the root cause of the issue above?

Calandiel commented 8 months ago

It's caused by missing tangents. Calling append_from_file with a flag 8 forces recalculation of tangents and keeps everything functional. I think this behavior isn't necessarily unreasonable but the error message for it isn't very good.

I could potentially do change the error message myself for it if it's likely to get merged (as in, changing the error message is preferred over changing the gltf loading behavior). What do you think? @fire

fire commented 8 months ago

That sounds good.

aaronfranke commented 8 months ago

@Calandiel If the change to the behavior fixes a regression (skeletons not loading), I would prefer that. It doesn't make sense to me that missing tangents would cause skeletons to break.

Calandiel commented 8 months ago

@Calandiel If the change to the behavior fixes a regression (skeletons not loading), I would prefer that. It doesn't make sense to me that missing tangents would cause skeletons to break.

The issue with that is that I don't have much free time and it sounds like a lot more work than changing the error message

matheusmdx commented 4 months ago

Bisecting points to #80831 as the culprit:

aaronfranke commented 4 months ago

@matheusmdx Thanks for bisecting. Now that the cause is known, this actually makes perfect sense, and the new behavior is expected. Reasoning: If you run append_from_file, it is not expected to have any nodes generated yet. The nodes will only become generated after you run generate_scene, before which there is no skeleton. Furthermore, if you run generate_scene twice, get_godot_skeleton will return the skeleton from the latest generate_scene call.

In summary: You can't get the Godot skeleton if it hasn't been generated, and it was a bug in 4.1 and earlier to generate a skeleton node outside of the node generation step, so the code in OP was not "supposed to" have ever worked.

If you want the skeleton, run generate_scene to generate the scene with the skeleton in it.

matheusmdx commented 4 months ago

@aaronfranke Btw i did what you said an in fact that solves the issue, but when i use GLTFDocument.generate_scene the console spam lots of errors. Should we open an issue for this?

WARNING: Adding 'Body' as child to 'Skeleton3D' will make owner 'anime-humanoid' inconsistent. Consider unsetting the owner beforehand.
     at: add_child (scene/main/node.cpp:1575)
WARNING: Adding 'Face' as child to 'Skeleton3D' will make owner 'anime-humanoid' inconsistent. Consider unsetting the owner beforehand.
     at: add_child (scene/main/node.cpp:1575)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Index p_shape = 0 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 1 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 2 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 3 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
...
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 48 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 49 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 50 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Blend shape format must match the main array format for Vertex, Normal and Tangent arrays.
   at: (servers/rendering_server.cpp:1230)
ERROR: Condition "err != OK" is true.
   at: add_surface_from_arrays (scene/resources/mesh.cpp:1810)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_material (scene/resources/mesh.cpp:1931)
ERROR: Index p_idx = -1 is out of bounds (surfaces.size() = 0).
   at: surface_set_name (scene/resources/mesh.cpp:1951)
ERROR: Index p_shape = 0 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 1 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 2 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 3 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 4 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
...
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 176 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)
ERROR: Index p_shape = 177 is out of bounds ((int)mi->blend_weights.size() = 0).
   at: mesh_instance_set_blend_shape_weight (servers/rendering/renderer_rd/storage_rd/mesh_storage.cpp:920)

matheusmdx commented 4 months ago

Oh nevermind, i compiled the lastest version bd2300d77 and this spam didn't happen anymore, just the warnings.

aaronfranke commented 4 months ago

The only part that doesn't make sense is how normals/tangents would affect this as @Calandiel describes. Unless these are two separate bugs?

godotengine / godot