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.

Sign up for GitHub

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

Jump to bottom

Fix OSX crash (fix #1531) #1532

Open
wants to merge 5 commits into
base: noetic-devel
Choose a base branch
from
Open

Fix OSX crash (fix #1531) #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c0a605
Fix OSX crash
Tobias-Fischer Jul 27, 2020
3b67770
Fix Camera Display on OSX
Tobias-Fischer Jul 27, 2020
2da4afa
Remove debug
Tobias-Fischer Jul 27, 2020
f3c6342
Make format check happy
Tobias-Fischer Jul 27, 2020
3eb8c1e
Update camera_display.cpp
Tobias-Fischer Jan 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions src/rviz/default_plugin/camera_display.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include <OgreRectangle2D.h>
#include <OgreRenderSystem.h>
#include <OgreRenderWindow.h>
#include <OgreRoot.h>
#include <OgreSceneManager.h>
#include <OgreSceneNode.h>
#include <OgreTextureManager.h>
Expand Down Expand Up @@ -102,6 +103,8 @@ CameraDisplay::CameraDisplay()
SLOT(forceRender()));
zoom_property_->setMin(0.00001);
zoom_property_->setMax(100000);

has_run_once_ = false;
}

CameraDisplay::~CameraDisplay()
Expand All @@ -127,8 +130,15 @@ void CameraDisplay::onInitialize()
{
ImageDisplayBase::onInitialize();

bg_scene_node_ = scene_node_->createChildSceneNode();
fg_scene_node_ = scene_node_->createChildSceneNode();
{
static uint32_t count = 0;
std::stringstream ss;
ss << "CameraDisplay" << count++;
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();
Comment on lines +137 to +141
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


{
static int count = 0;
Expand Down Expand Up @@ -189,6 +199,7 @@ void CameraDisplay::onInitialize()
render_panel_->getRenderWindow()->setActive(false);
render_panel_->resize(640, 480);
render_panel_->initialize(context_->getSceneManager(), context_);
// render_panel_->initialize(camera_scene_manager_, context_);

setAssociatedWidget(render_panel_);

Expand All @@ -212,8 +223,16 @@ void CameraDisplay::onInitialize()
void CameraDisplay::preRenderTargetUpdate(const Ogre::RenderTargetEvent& /*evt*/)
{
QString image_position = image_position_property_->getString();
bg_scene_node_->setVisible(caminfo_ok_ && (image_position == BACKGROUND || image_position == BOTH));
fg_scene_node_->setVisible(caminfo_ok_ && (image_position == OVERLAY || image_position == BOTH));

if (has_run_once_)
{
fg_scene_node_->setVisible(caminfo_ok_ && (image_position == OVERLAY || image_position == BOTH));
bg_scene_node_->setVisible(caminfo_ok_ && (image_position == BACKGROUND || image_position == BOTH));
}
else
{
has_run_once_ = true;
}

// set view flags on all displays
visibility_property_->update();
Expand Down
4 changes: 4 additions & 0 deletions src/rviz/default_plugin/camera_display.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ class CameraDisplay : public ImageDisplayBase, public Ogre::RenderTargetListener
ROSImageTexture texture_;
RenderPanel* render_panel_;

Ogre::SceneManager* camera_scene_manager_;

private Q_SLOTS:
void forceRender();
void updateAlpha();
Expand Down Expand Up @@ -135,6 +137,8 @@ private Q_SLOTS:
bool force_render_;

uint32_t vis_bit_;

bool has_run_once_;
};

} // namespace rviz
Expand Down
24 changes: 23 additions & 1 deletion src/rviz/default_plugin/image_display.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ ImageDisplay::ImageDisplay() : ImageDisplayBase(), texture_()
this, SLOT(updateNormalizeOptions()));

got_float_image_ = false;
has_run_once_ = false;
}

void ImageDisplay::onInitialize()
Expand Down Expand Up @@ -114,12 +115,13 @@ void ImageDisplay::onInitialize()
screen_rect_->setBoundingBox(aabInf);
setMaterial(*screen_rect_, material_);
img_scene_node_->attachObject(screen_rect_);
img_scene_node_->setVisible(false);
}

render_panel_ = new RenderPanel();
render_panel_->getRenderWindow()->addListener(this);
render_panel_->getRenderWindow()->setAutoUpdated(false);
render_panel_->getRenderWindow()->setActive(false);

render_panel_->resize(640, 480);
render_panel_->initialize(img_scene_manager_, context_);

Expand All @@ -136,12 +138,32 @@ ImageDisplay::~ImageDisplay()
{
if (initialized())
{
render_panel_->getRenderWindow()->removeListener(this);

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_)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{
img_scene_node_->setVisible(true);
}
else
{
has_run_once_ = true;
img_scene_node_->setVisible(false);
}
}

void ImageDisplay::postRenderTargetUpdate(const Ogre::RenderTargetEvent& /*evt*/)
{
img_scene_node_->setVisible(false);
}

void ImageDisplay::onEnable()
{
ImageDisplayBase::subscribe();
Expand Down
8 changes: 7 additions & 1 deletion src/rviz/default_plugin/image_display.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ namespace rviz
* \class ImageDisplay
*
*/
class ImageDisplay : public ImageDisplayBase
class ImageDisplay : public ImageDisplayBase, public Ogre::RenderTargetListener
{
Q_OBJECT
public:
Expand All @@ -71,6 +71,10 @@ class ImageDisplay : public ImageDisplayBase
void update(float wall_dt, float ros_dt) override;
void reset() override;

// Overrides from Ogre::RenderTargetListener
void preRenderTargetUpdate(const Ogre::RenderTargetEvent& evt) override;
void postRenderTargetUpdate(const Ogre::RenderTargetEvent& evt) override;

public Q_SLOTS:
virtual void updateNormalizeOptions();

Expand Down Expand Up @@ -98,6 +102,8 @@ public Q_SLOTS:
FloatProperty* max_property_;
IntProperty* median_buffer_size_property_;
bool got_float_image_;

bool has_run_once_;
};

} // namespace rviz
Expand Down
3 changes: 2 additions & 1 deletion src/rviz/ogre_helpers/render_system.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,7 @@ Ogre::RenderWindow* RenderSystem::makeRenderWindow(WindowIDType window_id,
// Created a non-stereo window. Discard it and try again (below)
// without the stereo parameter.
ogre_root_->detachRenderTarget(window);
window->destroy();
ogre_root_->destroyRenderTarget(window);
window = nullptr;
stream << "x";
is_stereo = false;
Expand Down Expand Up @@ -480,6 +480,7 @@ Ogre::RenderWindow* RenderSystem::tryMakeRenderWindow(const std::string& name,
if (x_baddrawable_error)
{
ogre_root_->detachRenderTarget(window);
ogre_root_->destroyRenderTarget(window);
window = nullptr;
x_baddrawable_error = false;
}
Expand Down
7 changes: 7 additions & 0 deletions src/rviz/render_panel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

#include <OgreSceneManager.h>
#include <OgreCamera.h>
#include <OgreRenderWindow.h>

#include <rviz/display.h>
#include <rviz/view_controller.h>
Expand Down Expand Up @@ -100,6 +101,12 @@ void RenderPanel::leaveEvent(QEvent* /*event*/)
}
}

void RenderPanel::resizeEvent(QResizeEvent* event)
{
QWidget::resizeEvent(event);
render_window_->windowMovedOrResized();
}

void RenderPanel::onRenderWindowMouseEvents(QMouseEvent* event)
{
int last_x = mouse_x_;
Expand Down
2 changes: 2 additions & 0 deletions src/rviz/render_panel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ class RenderPanel : public QtOgreRenderWindow, public Ogre::SceneManager::Listen
/// Called when any mouse event happens inside the render window
void onRenderWindowMouseEvents(QMouseEvent* event);

void resizeEvent(QResizeEvent* event) override;

// QWidget mouse events all get sent to onRenderWindowMouseEvents().
// QMouseEvent.type() distinguishes them later.
void mouseMoveEvent(QMouseEvent* event) override
Expand Down