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

Fixed segfault caused by OgreRTShaderSystem #563

Merged
merged 3 commits into from
Feb 28, 2022

Conversation

WilliamLewww
Copy link
Contributor

🦟 Fixed segfault caused by OgreRTShaderSystem

Fixes Segfault in

Summary

Fixes the segfault that occurs after running any ign-gazebo test that uses OGRE1.

The removal of the this->Fini() is safe because OgreRTShaderSystem's Fini() method is already being ran in OgreRenderEngine:
https://github.com/ignitionrobotics/ign-rendering/blob/e0ed1b1732e654dab8084fc7821df05b4da7c759/ogre/src/OgreRenderEngine.cc#L108

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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Feb 18, 2022
@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #563 (f110893) into main (063510c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #563   +/-   ##
=======================================
  Coverage   53.13%   53.14%           
=======================================
  Files         213      213           
  Lines       21217    21219    +2     
=======================================
+ Hits        11273    11276    +3     
+ Misses       9944     9943    -1     
Impacted Files Coverage Δ
ogre/src/OgreRTShaderSystem.cc 43.54% <100.00%> (+0.41%) ⬆️
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 100.00% <0.00%> (+3.33%) ⬆️

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 063510c...f110893. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Feb 19, 2022

I'm testing this branch with the integration test in gazebosim/gz-sim#1215. This fixes the segfault for me. However I think the Fini function is also not being called by OgreRenderEngine because when I tried adding a print statement, I don't see it printed when running the test.

This leads me to think that it's a threading issue. The OgreRTShaderSystem is created in the rendering thread but destroyed when it goes out of scope in the test's main thread. For now, we can do something like the Ogre2RayQuery class and add a check in the OgreRTShaderSystem destructor to make sure that this function is only called in the thread it's created in. Something like:

if (std::this_thread::get_id() == this->dataPtr->threadId)
  this->Fini();

A more involved fix is to make sure we destroy the render engine in ign-gazebo's sensors system properly in the RenderThread but that's needs a bit more work and testing.

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@iche033 iche033 merged commit 1baa935 into gazebosim:main Feb 28, 2022
@WilliamLewww WilliamLewww mentioned this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants