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

Do not block visualization manager updates when opening the display panel dialog #795

Merged

Conversation

ivanpauno
Copy link
Member

Fixes #793.

…anel dialog

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the bug Something isn't working label Nov 4, 2021
@ivanpauno ivanpauno requested a review from jacobperron November 4, 2021 14:38
@ivanpauno ivanpauno self-assigned this Nov 4, 2021
@clalancette
Copy link
Contributor

In theory I'm on board with this, but you might want to have a look at ros-visualization/rviz#631, ros-visualization/rviz#1606, ros-visualization/rviz#1639, and ros-visualization/rviz#1641 where similar things have been tossed around in the original RViz for years. Your suggested solution is different, so it may solve the problems, but we should do some testing of the other problems reported.

@ivanpauno
Copy link
Member Author

According to #601, the idea of stopping the world when openning a dialog was intentional.
I don't see how that solves the reported problem:

This fixes the problem that rviz becomes effectively unusable when the update/render cycle takes too long (e.g. while showing high-frequency point cloud data).

was the idea to open a dialog when rviz gets unusable so you can remove the display causing the issue?

If that was the intention, it looks like a really weird idea.
The solution applied to noetic looks better, though I would like to test ros-visualization/rviz#1641.

@clalancette
Copy link
Contributor

was the idea to open a dialog when rviz gets unusable so you can remove the display causing the issue?

Good question; I don't know :).

If that was the intention, it looks like a really weird idea.

But yeah, I agree, it seems weird.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Member Author

I've (partially) reverted ros-visualization/rviz#631.
I've tried to reproduce the issue reported in ros-visualization/rviz#1641 and I couldn't.

I think that deleting this stopUpdate()/startUpdate() calls is fine.
If this reintroduces a problem, blocking updates while a dialog is open is probably not the right solution and we will have to figure out something else (or if that's the right solution, maybe we want to be spinning a ros executor somewhere else).

@ivanpauno
Copy link
Member Author

@jacobperron friendly ping

Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

lgtm

@ivanpauno
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit 0c149d9 into ros2 Nov 19, 2021
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/dont-stop-updating-when-opening-display-dialog branch November 19, 2021 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ROS callbacks not processed if Qt dialog is open
3 participants