Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rmodels] Initial work to correctly handle the node hierarchy in a glTF file #4037

Merged
merged 4 commits into from
Jun 22, 2024

Conversation

paulmelis
Copy link
Contributor

@paulmelis paulmelis commented Jun 4, 2024

Having to use glTF files without node hierarchies and without transformations applied to the meshes is really a big limitation. Also, the current handling is not really correct, as it completely ignores the node hierarchy and thus might lead to meshes not getting instanced more than once.

This patch adds using the nodes in the glTF file to drive the process of creating meshes, instead of only processing the list of meshes and ignoring the node hierarchy. Transforms defined in the nodes (including parent-child effects) are applied to the vertices/normals/tangents of the meshes in the leafs. A separate raylib Mesh is generated for each leaf node (and still generates one Mesh per primitive in a glTF mesh).

Static meshes with more complex hierarchy in terms of parenting and object transforms seem to work fine in my tests.

Haven't tried any glTF files with animations yet, but those are almost certainly broken. It's also the part where I don't know off-hand how to handle that, so any tips on what is currently going on with respect to bones and animation are appreciated.

Btw, scenes in the glTF files are also ignored (which can contain subsets of nodes, and thus meshes), but that's something I ignore here as well.

@raysan5
Copy link
Owner

raysan5 commented Jun 4, 2024

@paulmelis Thank you very much for working on this feature! It's a nice addition to the library!

About glTF animation, I did not implemented it but you can check LoadModelAnimationsGLTF(); in raylib side, it uses a Bone system to define skeleton, where every vertex is assigned a maximum of 4-bones influence. Then there is a Transform matrix for every bone for every key-frame. There is no interpolation implemented, animation just consists of transforming on CPU side all affected vertices by every Bone group and re-uploading transformed vertices data to GPU.

No GPU-skinning is supported due to limitations on some platforms supported by raylib.

@paulmelis
Copy link
Contributor Author

Am I reading

raylib/src/rmodels.c

Lines 5513 to 5519 in e1379af

model.meshes[meshIndex].animVertices = RL_CALLOC(model.meshes[meshIndex].vertexCount*3, sizeof(float));
memcpy(model.meshes[meshIndex].animVertices, model.meshes[meshIndex].vertices, model.meshes[meshIndex].vertexCount*3*sizeof(float));
model.meshes[meshIndex].animNormals = RL_CALLOC(model.meshes[meshIndex].vertexCount*3, sizeof(float));
if (model.meshes[meshIndex].normals != NULL)
{
memcpy(model.meshes[meshIndex].animNormals, model.meshes[meshIndex].normals, model.meshes[meshIndex].vertexCount*3*sizeof(float));
}
correctly in that animVertices and animNormals are always allocated, regardless if any animation is present? That potentially doubles memory usage for mesh vertices and normals.

@raysan5
Copy link
Owner

raysan5 commented Jun 4, 2024

that animVertices and animNormals are always allocated, regardless if any animation is present? That potentially doubles memory usage for mesh vertices and normals.

@paulmelis Mmmh... it seem so...

@paulmelis
Copy link
Contributor Author

I updated the animation data part as well. The robot animation in examples/shaders_shadowmap still seems to work.

@raysan5
Copy link
Owner

raysan5 commented Jun 5, 2024

I updated the animation data part as well. The robot animation in examples/shaders_shadowmap still seems to work.

Nice! Thanks for the review! Just let me know when this improvement is ready for review and merge!

@raysan5 raysan5 changed the title Initial work to correctly handle the node hierarchy in a glTF file [rmodels] Initial work to correctly handle the node hierarchy in a glTF file Jun 5, 2024
@raysan5 raysan5 added help needed - please! I need help with this issue and removed help needed - please! I need help with this issue labels Jun 5, 2024
@paulmelis
Copy link
Contributor Author

I've tested it with a few glTF models, including with more complex hierarchies. I saw no weird behaviour. I'd say it's ready for going into "beta testing" state

@giovancris
Copy link

Hey @paulmelis great work on this!

I noticed that using this model https://github.com/KhronosGroup/glTF-Sample-Models/blob/main/2.0/CesiumMan/glTF-Binary/CesiumMan.glb we have some issues!

I had no time to debug this one, but I think it might be useful to report it here :)

@paulmelis
Copy link
Contributor Author

paulmelis commented Jun 9, 2024

I noticed that using this model https://github.com/KhronosGroup/glTF-Sample-Models/blob/main/2.0/CesiumMan/glTF-Binary/CesiumMan.glb we have some issues!

I had no time to debug this one, but I think it might be useful to report it here :)

It looks like this is an issue with LoadModelAnimations() in raylib. That code isn't touched by this pull request, though. So this particular file triggers a bug in the current raylib animation loading code, which I'm not familiar with. It's best to create a separate bug report for this case.

$ cat load_model_animations.c 
#include "raylib.h"

int main(void)
{
    InitWindow(1024, 512, "");

    int count;
    ModelAnimation* robotAnimations = LoadModelAnimations("CesiumMan.glb", &count);
}

$ gcc -o load_model_animations load_model_animations.c -lraylib
$ gdb ./load_model_animations
(gdb) run
Starting program: /home/melis/concepts/battlegroup-raylib/tests/load_model_animations 
...
INFO: Initializing raylib 5.1-dev                                                                                                                                                                                  
INFO: Platform backend: DESKTOP (GLFW)
...
INFO: FILEIO: [CesiumMan.glb] File loaded successfully

Program received signal SIGSEGV, Segmentation fault.
__strncpy_avx2 () at ../sysdeps/x86_64/multiarch/strncpy-avx2.S:85
85              VMOVU   (%rsi), %VMM(0)                                                                                                                                                                            
(gdb) bt
#0  __strncpy_avx2 () at ../sysdeps/x86_64/multiarch/strncpy-avx2.S:85
#1  0x00007ffff7cc3434 in LoadModelAnimations () from /usr/lib/libraylib.so.500
#2  0x000055555555519f in main () at load_model_animations.c:8

Edit: dug a bit deeper (in the fork for this pull request, hence the different line numbers), and it seems animData.name is a null pointer, causing the strncpy to fail. @raysan5 does this sound plausible? I don't know anything about how animation data is read by cgltf.

#1  0x00007ffff7cf9ae2 in LoadModelAnimationsGLTF (fileName=0x555555556005 "CesiumMan.glb", animCount=0x7fffffffdf4c) at /home/melis/c/raylib-paulmelis/src/rmodels.c:5851
5851	                strncpy(animations[i].name, animData.name, sizeof(animations[i].name));
(gdb) p animData
$1 = {name = 0x0, samplers = 0x555555e7b9f0, samplers_count = 57, channels = 0x555555e7aba0, channels_count = 57, extras = {start_offset = 0, end_offset = 0, data = 0x0}, extensions_count = 0, extensions = 0x0}
(gdb) p animData.name
$2 = 0x0

@raysan5
Copy link
Owner

raysan5 commented Jun 9, 2024

@paulmelis Afair, animation names support was added quite recently in a PR, probably not extensively tested.

EDIT: Just pushed a quick fix for this issue.

raysan5 added a commit that referenced this pull request Jun 9, 2024
@giovancris
Copy link

Hey yeah I saw this one but I just bypassed with NULL check!

I was referring to the wrong animation. I think we are having a problem with the worldMatrix we are using to transform the vertices, if you set this matrix to identity the animation is OK.

In the original code the animation for the this model looks good!

@paulmelis
Copy link
Contributor Author

paulmelis commented Jun 9, 2024

I was referring to the wrong animation. I think we are having a problem with the worldMatrix we are using to transform the vertices, if you set this matrix to identity the animation is OK.

Do you have more details (or a code snippet) to show what you mean, as it's not entirely clear to me which worldMatrix and in which context you refer to?

@giovancris
Copy link

Hey @paulmelis sure!

I was using the example 'shaders_shadowmap.c'! Just change the path supplied to LoadModel and LoadModelAnimation from
Model robot = LoadModel("resources/models/robot.glb");
to
Model robot = LoadModel("path/to/test_models/CesiumMan.glb");

Same thing for LoadModelAnimation change it from
ModelAnimation* robotAnimations = LoadModelAnimations("resources/models/robot.glb", &animCount);
to
ModelAnimation* robotAnimations = LoadModelAnimations("rpath/to/test_models/CesiumMan.glb", &animCount);

run the example and you can see that the animation is wrong.

Screen.Recording.2024-06-10.at.09.16.26.mov

Regarding the worldMatrix I am referring to the matrix we are using at line 5104 file rmodels.c

`cgltf_float worldTransform[16];
cgltf_node_transform_world(node, worldTransform);

        Matrix worldMatrix = {
            worldTransform[0], worldTransform[4], worldTransform[8], worldTransform[12],
            worldTransform[1], worldTransform[5], worldTransform[9], worldTransform[13],
            worldTransform[2], worldTransform[6], worldTransform[10], worldTransform[14],
            worldTransform[3], worldTransform[7], worldTransform[11], worldTransform[15]
        };`

I tried to set this matrix to identity and the animation comes back to the correct behavior!

Screen.Recording.2024-06-10.at.09.29.17.mov

@paulmelis
Copy link
Contributor Author

paulmelis commented Jun 10, 2024

So I'm still not sure if what you're seeing is fully caused by my changes to the glTF handling. When I re-export CesiumMan.glb from Blender and load it into current raylib (so without my patches for glTF) I'm seeing the same distortions and weird movement. And similar for some other models (e.g. https://sketchfab.com/3d-models/model-47a-loggerhead-sea-turtle-c438e81e796d41d9a6ae4cc147ef8d4f).

There's also different aspects that influence each other:

  • Current glTF import (without these patches) is more or less broken, as it ignores the node hierarchy in the glTF file, leading to incorrect mesh transforms and instancing. This impacts all use of more complex glTF files (i.e. beyond a single mesh in a file), regardless if they contain animation data. The reason CesiumMan.glb actually worked so far was that it only contains a single mesh, which gets deformed correctly through the bone hierarchy.
  • Current import of glTF animations also does not appear to be correct in all cases (my examples above)
  • Raylib in itself does not support a true node hierarchy when it comes to meshes/models.
  • Due to the previous point the patches in this pull request work around this limitation by transforming all mesh data in leaf nodes by their net local-to-world transform. That's actually quite a hack, but it's about the only thing possible to get complex glTF models to import correctly (as long as there's no node hierarchies in Raylib). It's also causing some of the issues you're seeing with animated models.

I'm actually wondering if it is even possible to correctly support models with complex hierarchies and having animation data, without some form of node hierarchy.

@giovancris
Copy link

Ah OK! I see, and thank you for the explanation :)

@MrScautHD
Copy link
Contributor

AMAZING WORK!

@raysan5
Copy link
Owner

raysan5 commented Jun 22, 2024

@paulmelis Is this PR ready for merge (despite the current limitations with nodes-hierarchy)?

@paulmelis
Copy link
Contributor Author

paulmelis commented Jun 22, 2024

Hi @raysan5, I'd say it's a definite improvement for importing glTF models, as you no longer need to flatten the hierarchy in the file. I don't fully know about the impact on animated models, but I suspect current support isn't perfect either (i.e. #4055). So I would suggest indeed merging this and then we can look at improving animation support on top of it later.

@raysan5 raysan5 merged commit d582bec into raysan5:master Jun 22, 2024
14 checks passed
@raysan5
Copy link
Owner

raysan5 commented Jun 22, 2024

@paulmelis Merged! Thank you very much for all the effort put on this improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants