Fix: Reversed models when skeletons exist #375
Conversation
|
@luca-della-vedova, |
|
@luca-della-vedova , |
onurtore
changed the title
onurtore
force-pushed
the
otore19/skeleton_fix
branch
from
b0a606f to
6696399
Compare
onurtore
changed the title
| @@ -489,6 +530,9 @@ Mesh *AssimpLoader::Load(const std::string &_filename) | |||
| // TODO parse different skeletons and merge them | |||
| this->dataPtr->RecursiveSkeletonCreate(rootNode->mChildren[childIdx], rootSkelNode, rootTransform); | |||
| } | |||
| rootSkelNode = rootSkelNode->Child(0); //We dont need scene node and adding will create inverse model | |||
There was a problem hiding this comment.
Does not look safe to me. But if root-node does not have just one child after refined then something we never seen before happened. Planning to add some additional check around this, at least it will be easier to detect if problem occurs during development.
There was a problem hiding this comment.
Agree, I think this is a good time to start investigating the TODO above about meshes with multiple skeletons.
A classic example we have been using internally is the one below, where the human and the cane have two separate skeletons that are merged in the "legacy" ColladaLoader.
This model segfaults both with this PR and without and by looking at the stdout debug output from assimp it seems it might actually be an assimp parsing issue since it reports a lot of errors about bones not being found.
It seems then that the fix might once again be on the assimp side, then if this approach works for that edge case I think we can keep this approach until we find a case for which it doesn't work.
There was a problem hiding this comment.
When I try to load the mesh with the assimp PR you created, the whole Gazebo justs stucks and prints malloc(): invalid size (unsorted)), and if I dont use that PR, then we see this errors related to bones not being found. Furthermore, when I use the old colladaLoader, then again I see the Malloc error as well... So maybe it is an error not related to loaders? Should we find another dae file with multiple skeletons?
There was a problem hiding this comment.
@luca-della-vedova,
I can change the solution from removing the parent node to adding an identity transformation matrix if there is non exist?
There was a problem hiding this comment.
When I try to load the mesh with the assimp PR you created, the whole Gazebo justs stucks and prints malloc(): invalid size (unsorted)), and if I dont use that PR, then we see this errors related to bones not being found. Furthermore, when I use the old colladaLoader, then again I see the Malloc error as well... So maybe it is an error not related to loaders? Should we find another dae file with multiple skeletons?
That's a bit strange, the model works (or at least used to work under Fortress) with the default collada loader, I wonder if the issue is somewhere else? Can you check that it works with a binary Fortress installation just to be safe?
I can change the solution from removing the parent node to adding an identity transformation matrix if there is non exist?
That makes me feel a bit better, but I thought the transformation is always populated and is an identity for Scene? Is that not the case?
There was a problem hiding this comment.
Friendly ping for this @onurtore, I think we can merge it after this change and start to look into opening a PR against main
There was a problem hiding this comment.
Hi @luca-della-vedova ,
Adding inverse bind transform to the scene brings back the animation segmentation fault. So now I am not sure whether should we add that or not.
Btw, mesh works with Fortress however, I am porting everthing to Fortress to find whether assimp works with this mesh or not.
I was planning to test new mesh file to give us a final idea about this method.
There was a problem hiding this comment.
Hi @luca-della-vedova,
Good news, this mesh also loaded with assimp in Fortress. The only problem was related to texture as you can see below.
I am in favour of merge. The next think I will do is the divide the CreateActor() function into subparts. I tested the createActor function with the static analysis tools and dividing into subparts looks like good idea (You can see some stats below.) This will also help me to again go-over the animation segmentation error and will give an idea about whether we can change it to adding inverse bind transform to scene node rather than removing it.
Cyclomatic Complexity: 36
#Lines: 302
There was a problem hiding this comment.
@luca-della-vedova ,
Assigning child and setting parent null creates segmentation error in Edifice demo.
There was a problem hiding this comment.
Yea I'm not sure how stable this solution is. The issue is not in adding a child and setting a parent to null but rather accessing the hardcoded element at index 0 in cases in which the root node does not have any children (which happens very often for simple meshes).
I added the "if" statement to only execute this block when there are animations in the scene which mitigates the issue, since scene with animations will most likely have a number of nodes, but it would be good to remove the hardcoded value or make it safer
|
@luca-della-vedova, |
onurtore
force-pushed
the
otore19/skeleton_fix
branch
from
6696399 to
5fb25c8
Compare
| @@ -489,6 +530,9 @@ Mesh *AssimpLoader::Load(const std::string &_filename) | |||
| // TODO parse different skeletons and merge them | |||
| this->dataPtr->RecursiveSkeletonCreate(rootNode->mChildren[childIdx], rootSkelNode, rootTransform); | |||
| } | |||
| rootSkelNode = rootSkelNode->Child(0); //We dont need scene node and adding will create inverse model | |||
There was a problem hiding this comment.
Agree, I think this is a good time to start investigating the TODO above about meshes with multiple skeletons.
A classic example we have been using internally is the one below, where the human and the cane have two separate skeletons that are merged in the "legacy" ColladaLoader.
This model segfaults both with this PR and without and by looking at the stdout debug output from assimp it seems it might actually be an assimp parsing issue since it reports a lot of errors about bones not being found.
It seems then that the fix might once again be on the assimp side, then if this approach works for that edge case I think we can keep this approach until we find a case for which it doesn't work.
|
Hi @ahcorde, |
onurtore
force-pushed
the
otore19/skeleton_fix
branch
from
0cd7bc4 to
4ebb28f
Compare
|
Assimp PR I created for the missing bones merged. I wasnt too effective last weekend (I had graduation ceremony) but now continue my work. |
|
Sometimes the Gazebo stucks like below, is this happening to you?
When try to click buttons it says:
|
|
@luca-della-vedova , Maybe after fixing the meshes with multiple skeletons it is time to ask OSRF' internal team to use this branch for test their meshes or share their meshes with us to test ? We can create a flag for using assimp loader during compilation for the initial one. Furthermore, maybe we can start doing memory and speed test? |
I'd be happy to see some profiling results! I think a way to really push the confidence in the implementation would be to add unit tests for new formats (for the minimum implementation of just using Up to you which task you want to tackle first! |
Signed-off by: Onur Berk Töre <[email protected]> Co-authored-by: Luca Della Vedova <[email protected]>
Creating skeleton from Assimp nodes ends up with reversed models/weird arms, because some of the nodes does not have corresponding bones. Correspondence problems creates bones without inverse transform. While utilizing removeEmptyBones flag to false solves the issue of weird arm problem, the reversed model problem still exists. However, this problem does not originate from Assimp, the problem arise because we are trying to add nodes that are not related to bones. These nodes also not defined as joints in the original dae files so Assimp not loading them as bone is logical and correct behaviour.
To solve this issue, this code traverses the bone structure, and stores the name of the bones. These bone names used during skeleton creation to pass the nodes without corresponding bones while preserving the possible transformations. Finally, scene node removed from skeleton.