-
Notifications
You must be signed in to change notification settings - Fork 220
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
clean up render window resize logic #716
base: rolling
Are you sure you want to change the base?
Conversation
rviz_rendering/src/rviz_rendering/objects/point_cloud_renderable.cpp
Outdated
Show resolved
Hide resolved
@@ -152,18 +154,18 @@ ToString(const EnumType & enumValue) | |||
return QString("%1::%2").arg(enumName).arg(static_cast<int>(enumValue)); | |||
} | |||
|
|||
void RenderWindow::resizeEvent(QResizeEvent * resize_event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is a virtual function from the parent QWindow. https://doc.qt.io/qt-5/qwindow.html#resizeEvent
we dont need to use the event param passed in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid an unused parameter warning, we can use Q_UNUSED or just write (void)resize_event
.
@@ -260,13 +260,8 @@ RenderWindowImpl::resize(size_t width, size_t height) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
width
and height
are unused
RenderWindowImpl::resize(size_t /*width*/, size_t /*height*/)
rviz_default_plugins/src/rviz_default_plugins/displays/pointcloud/point_cloud_common.cpp
Outdated
Show resolved
Hide resolved
auto width = parent_->width(); | ||
// auto width = ogre_render_window_->getWidth() ? ogre_render_window_->getWidth() : 100; | ||
auto height = parent_->height(); | ||
// auto height = ogre_render_window_->getHeight() ? ogre_render_window_->getHeight() : 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just remove these commented lines? I'm not sure why they were there to begin with.
…oud/point_cloud_common.cpp Co-authored-by: Jacob Perron <[email protected]>
to give some context: we are running into an issue rendering pointclouds in rviz and rviz2 related to the render window height. this PR is a result of me doing a deep dive over a few days to investigate that issue. To be clear this PR does not solve that open issue. |
@flynneva I've tested the changes locally, and didn't notice any change in behavior so I'm happy to accept this change. Could you resolve the outstanding comments above and rebase on |
@jacobperron sure! I'll try and get to this today |
cleaned up some of the logic for when rviz render window gets resized.
this should make it easier to diagnose whats going on over at #1508