Fix OSX crash (fix #1531) #1532
Conversation
There was a problem hiding this comment.
Thanks for the detailed analysis in #1531 and your attempt to fix the issue. Did you gain more insight into the root cause of the race condition? I agree with the cherry-pick of ros2/rviz#319, but I'm not yet convinced about the remaining changes.
| delete render_panel_; | ||
| delete screen_rect_; | ||
| removeAndDestroyChildNode(img_scene_node_->getParentSceneNode(), img_scene_node_); | ||
| } | ||
| } | ||
|
|
||
| void ImageDisplay::preRenderTargetUpdate(const Ogre::RenderTargetEvent& /*evt*/) | ||
| { | ||
| if (has_run_once_) |
There was a problem hiding this comment.
This seems to be at the core of this PR: You need to ignore the first rendering of the scene.
But what is the root of this race condition? Why does it help to drop the first rendering (only)?
There was a problem hiding this comment.
Unfortunately I didn't get to the root of the race condition. I don't understand the rviz code and the OSX specifics well enough I'm afraid.
| camera_scene_manager_ = Ogre::Root::getSingleton().createSceneManager(Ogre::ST_GENERIC, ss.str()); | ||
| } | ||
|
|
||
| bg_scene_node_ = camera_scene_manager_->getRootSceneNode()->createChildSceneNode(); | ||
| fg_scene_node_ = camera_scene_manager_->getRootSceneNode()->createChildSceneNode(); |
There was a problem hiding this comment.
Why do you create a separate SceneManager? Isn't this rendering an empty scene - only comprising the camera image, but not the default rviz scene anymore?
There was a problem hiding this comment.
If I just use the standard SceneManager, rviz crashes when adding a second camera display. I took the idea of having a separate SceneManager from the image display code where this exact code is being used. Again though I'm not sure about the internals; I agree that it should work just using the default SceneManager but I don't understand the code well enough to make this work. On a general note, quite a bit of code is duplicated between the image and camera displays and could probably be abstracted away.
|
Ping @rhaschke - I'd love to see this PR being merged. As previously mentioned I unfortunately don't have in depth knowledge of rviz and can't really explain why the fixes in this PR are required, but in my opinion a workaround a bug is better than having a bug. |
|
As this bug is not reproducible on Linux, I'm hesitating to integrate such a workaround without understanding its root cause.
|
|
Many thanks for getting back to me so quickly @rhaschke! There are multiple fixes included in this pull request:
I hope this helps. Maybe @botteroa-si has more insights (author of ros2/rviz#319)? |
|
Thanks for this summary. This will hopefully help understanding the problem. However, I will not have time to look into this in the next few weeks. I'm sorry. |
|
For all OSX users who observe this crash: There is now a patched version available in https://github.com/RoboStack/ros-noetic (see https://medium.com/robostack/cross-platform-conda-packages-for-ros-fa1974fd1de3) |
Description
Rviz still crashes when adding a second Camera display for whatever reason, I hope the maintainers can help me in pointing me towards the potential reason of that crash.(see 3b67770)I know that the fixes currently are quite messy, but the best I can do with my very limited knowledge of rviz. Someone else might know how to properly fix things.