Introduces remove scene node env var, renames func

[onurtore]

🦟 Bug fix

Fixes #393

Summary

Assimp creates additional scene node for the skeleton, this commit introduces a flag for removing that. Users also
allowed to set this flag through environment variable. Furthermore renames a previous func to fix its naming.

This commit does not implement the actual operation but prepares the necessary env var. The actual base implementation already inside #410, and will move from there.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[codecov]

Codecov Report

Merging #424 (6d5f755) into main (b0001bf) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

❗ Current head 6d5f755 differs from pull request most recent head 6702594. Consider uploading reports for the commit 6702594 to get more accurate results

@@            Coverage Diff             @@
##             main     #424      +/-   ##
==========================================
- Coverage   81.07%   81.00%   -0.07%     
==========================================
  Files          79       79              
  Lines        9679     9687       +8     
==========================================
  Hits         7847     7847              
- Misses       1832     1840       +8     

Impacted Files	Coverage Δ
graphics/src/AssimpLoader.cc	86.59% <0.00%> (-2.59%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[mjcarroll]

@onurtore and @luca-della-vedova what is the status of this (and the other few Assimp-related PRs?

[onurtore]

Hi @mjcarroll,
There is one major integration problem, and the whole process kinda stuck. The problem arises as segmentation fault during loading of certain sdf files with actors (One example is actor.sdf) with Assimp. This happens because:

Current integration of the Assimp library creates additional root node for the Gazebo skeleton. This root node generated by Assimp and can not be found in the file that user trying to load and creates segmentation faults during loading of actor.sdf. I will refer to this node as root node, and the actual root node which is the root node defined in the file (actor.sdf) as actual root node.

Segmentation fault happens during the XDisplacement check in src/rendering/SceneManager.cc during actor creation. Related code snippet is here. What happens is, Gazebo tries to access rootNode animation, however the rootNode does not have any animation because it is not the actual root node, it is a dummy root node generated by assimp.

To solve this, I suggest to remove this node by just assigning root node to it is child (PR [here])(#410). I am aware this sound dangerous, as it stated by @luca-della-vedova below:

I wonder if instead of removing the root and hardcoding a "root is equal to the first child" logic we could just fix the scene node by assigning it a dummy animation that is just an identity (zero translation, unit quaternion) for the whole time, so children are preserved and their animations unchanged.

I'm not familiar enough with actor animations and exporting software to know whether the hardcoding 0 would have any side effects or not but it feels a bit dangerous if a root scene node was to have more than one child for any reason.

Luca suggest adding dummy animation because if root node have more than one actual root node,
assigning root to one of its actual root node might be problematic as we disregard some critical information. In that case, dummy animation needs to have the actual roots node properties, otherwise XDisplacement check condition inside the actor creation will be wrong. However, if there is a possibility of more than one child, then which child we are going to use for the dummy animation, that is a question.

As an alternative solution, I created a PR where we check not just the root node, but all the skeleton nodes for XDisplacement (PR), This way we can sure whether there is XDisplacement or not by checking all the nodes. However, this might increase the number of positives as it stated by @luca-della-vedova :

Did you try to use this function on an actual animated model that has no X displacement (i.e. this)?
My concern is that this change will return much more positives. For example if there is even one node that has a pose which has a X in its last keyframe that is slightly different from the first keyframe (i.e. the hand bone for a person looking at their phone while being on the spot) this function might return true while previously we would have considered that false since the root was not moving.

So with this new information, I do not have any alternative. I suggest to assign the root node to its child, because I do not see any cases where dummy root node have more than one real child. However creating a PR for this functionality will make at least one test fails because we created a specific test for checking dummy root node. And no one accepts former intern's PR with failed test cases. (Just a joke :D)

Inaction is also a possible choice for this problem, because Assimp will use a new skeleton class in future, and this will remove the dummy root node, however we dont know when will this be available. And will require significant re-writing of the code.

I am open to any advise or a roadmap from you and @luca-della-vedova for this problem, and can try implement this in my spare time (Just got an job, so it might be little slow, however I would love to implement the final PRs for the functionality because this was my GSoC project.)

There are some more functionalities planned as well. One is replacing the bvhAnimation loader with assimp but they are relatively small compared to this.

[onurtore]

Hi @mjcarroll,
Friendly ping. An answer would be really good.

[luca-della-vedova]

Hello!

I agree that it is a hairy issue which might not have a full answer, since creating the dummy root node will fix the segfault but break the interpolate_x behavior, while removing the node (as here) will preserve interpolate_x but break multiple skeleton tests (and maybe others? I can't remember the details of the failing CI run on the PR that made this the default behavior).

I think the option of adding an environment variable might seem to help but it's just a somewhat obscure option to keep track of for a very narrow edge case, specifically:

• If a user wants to use a mesh that relies on interpolate_x, they should set this environment variable, what makes it even worse right now is that the only mesh that does is actually a collada mesh in the actor.sdf world, which by default wouldn't use this loader anyway. So this fix would help preserve the original behavior if the user was to set two environment variables, one to force using assimp and one to remove the root node. At this point the user might as well not set any and keep using the default ColladaLoader and it would be just fine.
• If a user doesn't need that specific mesh, this change is not really needed.
• Worst of all, if a user wanted to create a world with both the animated mesh above and other meshes, the behavior might be unpredictable since the user must remove the root node but specifying that flag puts us in a code path where several other assimp unit tests fail, so it's unclear what the world would look like.

So overall my 2c is that I would go for the "noop" approach, as much as I would have personally liked pushing for a single loader to use for all the asset formats it seems that might not happen for some time because of issues like this, so we can just stick with using the other loaders for previously supported formats, while revisiting regularly the state of upstream assimp and testing with the environment variable to force using assimp for all the mesh formats, to determine whether we want to make it a new default behavior.

[azeey]

@luca-della-vedova after reading your comment, my understanding is that we should close this PR and keep using the old Collada loader for all dae files. Is that the case? If so, do we need to revert/modify #393?

[luca-della-vedova]

@luca-della-vedova after reading your comment, my understanding is that we should close this PR and keep using the old Collada loader for all dae files. Is that the case? If so, do we need to revert/modify #393?

Hi!

Yea agree with the approach. #393 does not override the previous behavior and we still use the collada loader for all dae files so there is no need to revert anything. I'll close this then