jkuhlmann / cgltf

:diamond_shape_with_a_dot_inside: Single-file glTF 2.0 loader and writer written in C99
MIT License
1.42k stars 135 forks source link

Node parent sanity check prevents loading files created with UniGLTF-1.27 #222

Closed WickedSmoke closed 1 year ago

WickedSmoke commented 1 year ago

Parse fails on files generated with UniGLTF-1.27 because data->scenes[i].nodes[j]->parent is not NULL at line 6659 in cgltf_fixup_pointers().

The following is the JSON scene graph, which should be valid as glTF doesn't declare parents, only children. The loader should see a scene with a single node and ignore the unreferenced "tmpParent".

    "nodes": [  
        {       
            "children": [ 1 ],
            "name": "tmpParent",
            "rotation": [ 0, 0, 0, 1 ],
            "scale": [ 1, 1, 1 ],
            "translation": [ 0, 0, 0 ]
        },
        {
            "mesh": 0,
            "name": "fountainCenter",
            "rotation": [ 0, -0.707106769, 0, 0.7071068 ],
            "scale": [ 1, 1, 1 ],
            "translation": [ 0, 0, 0 ]
        }
    ],
    "scene": 0,
    "scenes": [ { "nodes": [ 1 ] } ]
zeux commented 1 year ago

This scene graph structure fails validation with:

            {
                "code": "SCENE_NON_ROOT_NODE",
                "message": "Node 1 is not a root node.",
                "severity": 0,
                "pointer": "/scenes/0/nodes/0"
            },

Relevant spec language:

glTF 2.0 assets MAY contain zero or more scenes, the set of visual objects to render. Scenes are defined in a scenes array. All nodes listed in scene.nodes array MUST be root nodes, i.e., they MUST NOT be listed in a node.children array of any node. The same root node MAY appear in multiple scenes.

I recommend reporting this as a UniGLTF bug.

WickedSmoke commented 1 year ago

I'm not a user of that exporter but there are assets floating around the net that do. I'll just be disabling that check in my version of cgltf.h so that I can load models.

WickedSmoke commented 1 year ago

Adding a new error code that indicates "the data was loaded OK, but it does not strictly follow spec." would be good here. Something that would help users know why the data isn't loading without having to crack open a debugger as I did. An even more specific error (invalid scene root node) might be better.

zeux commented 1 year ago

We already return "cgltf_result_invalid_gltf" for this which has the broad meaning you suggest. When the spec is violated, the resulting data may be internally inconsistent, for example for this specific issue, obviously we return from the function early so the data isn't really usable, but if we didn't, the parent links would be invalid for some nodes as the code doesn't necessarily select the "right" parent in case the file violates the invariants.

There are a lot of different error conditions for why the spec could be violated. In general I think the expectation is that the loaders return the error if the file isn't conformant (when that prevents correct operation - like it does in this case, because we can't establish the correct "parent" link otherwise!), and further diagnostics can be retrieved by running the official glTF validator.

WickedSmoke commented 1 year ago

Thanks for the explanation. I'm just starting to use this format so I wasn't aware of gltf_validator. It does look like the issue is fixed in newer versions of UniGLTF.