Backport #188: Fix crash when using BVH animations

[adlarkin]

Signed-off-by: Ashton Larkin ashton@openrobotics.org

🦟 Bug fix

Related to gazebosim/gz-sim#647

Summary

This is a backport of #188. Once this is merged, we should no longer see actors crash at launch (the ign-common3 branch applies to both Citadel and Dome).

To test this, run ign gazebo -r actor.sdf and make sure no crash occurs.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• 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

[codecov]

Codecov Report

Merging #199 (5186393) into ign-common3 (f1c8c4a) will decrease coverage by 0.19%.
The diff coverage is 45.83%.

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

@@               Coverage Diff               @@
##           ign-common3     #199      +/-   ##
===============================================
- Coverage        75.20%   75.00%   -0.20%     
===============================================
  Files               73       72       -1     
  Lines            10307    10290      -17     
===============================================
- Hits              7751     7718      -33     
- Misses            2556     2572      +16     

Impacted Files	Coverage Δ
graphics/src/SkeletonAnimation.cc	40.74% <33.33%> (-1.73%)	⬇️
graphics/src/Skeleton.cc	42.17% <50.00%> (-0.43%)	⬇️
graphics/src/Animation.cc	91.56% <66.66%> (-0.34%)	⬇️
graphics/src/Image.cc	65.00% <0.00%> (-2.09%)	⬇️
graphics/include/ignition/common/Image.hh

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1c8c4a...ee6fe52. Read the comment docs.

[adlarkin]

I tried testing this by running ign gazebo -r actor.sdf. I'm not sure why, but this does not seem to fix the actor crashing on citadel or dome. For citadel, the simulation starts, but then crashes after a few seconds. For dome, it crashes on launch and sometimes shows this error (other times, it shows a long backtrace similar to the backtrace in gazebosim/gz-sim#647):

terminate called after throwing an instance of 'std::logic_error'
  what():  basic_string::_M_construct null not valid

I'm not sure why this is happening. I'll need to look into it some more, unless someone else knows what's going on here?

[mjcarroll]

@osrf-jenkins retest this please

[chapulina]

I spent some time debugging the crashes on Citadel but the more I dig, the uglier it gets. I have added some safety checks on the following 2 branches:

• 5186393
• gazebosim/gz-sim@30c10c0

My favorite one is the std::map that ends up with a size larger than its max_size 🤷‍♀️

---

I suspect that the best course of action right now should be to revert #136, because as the documentation says, the Skeleton doesn't own its Animations, so it shouldn't be deleting them. I don't know what is supposed to be deleting them though, so I'm not sure what's the correct way to avoid a memory leak. Maybe there was never a proper way to delete it.

What's best, a memory leak, or a segfault? ⚖️

https://github.com/ignitionrobotics/ign-common/blob/b7ac4ed8329d50aee7d8c3e92cdf9b307aef7c0b/graphics/include/ignition/common/Skeleton.hh#L135-L138

[adlarkin]

@chapulina I believe I may have fixed the issue 🤞 since ign-common3 still uses a raw impl pointer, we need to do a deep copy here, or else we are just copying the pointer, which eventually goes out of scope. ign-common4 uses the new utils impl ptr, so that's why this wasn't needed in #188. I have added an assignment operator in a3983b6, and that seems to fix the crash for me.

More information about this can be found in #136 (comment).

[iche033]

this function should not be merged forward to ign-common4 right? Can you add a note as a reminder?

[adlarkin]

Added: af8b84a

Let me know if there's a better way/format of adding a note like this. I don't think it should be a TODO since nothing really needs to be "done", and I also assume it shouldn't be a doxygen comment since this isn't relevant for user documentation (it's more of just an internal developer note).

[iche033]

looks good to me.

[chapulina]

Niiice! It works for me with the assignment operator 😃