I think it’s surprising that in rviz when you toggle checkboxes it deletes the visualization rather than making it invisible/visible. Turns out this is quite easy behavior to change, at least for some displays and I’m interested in doing so. However, everyone is used to it working the old way which I understand. So my questions are:
What are the use cases for “uncheck should delete”? Why might you want to delete as opposed to just make invisible?
Would you prefer visibility toggling, assuming you still had a way to delete
What should the UI be for delete if the main checkbox was changed to visibility?
Having a visibility toggle would be useful. Great idea! The UI might look e.g. similar to what Blender has, i.e. an eye icon next to the item (keeping the checkbox for deleting).
One more use-case for deleting is if you accidentally visualize a 30 Hz Realsense D435 pointcloud with decay set to a non-zero number. That will basically eat all your memory, and you want to have a way to get rid of that…
I like that it also un-subscribes. Because when connecting to a robot over a poor connection I typically disable the pointclouds.
Only hiding them would still mean a lot of processing and data streaming.
I recall wishing it would not reset the markers when unchecking the display, but there are certainly pros and cons.
Many years ago I added a CLEARALL function to the Marker ROS msg to allow you to reset the markers programatically, which I have sinced used extensively
Thanks for all the response, I’m starting to like the idea of either (1) a context menu on the display or (2) a new eye icon for doing visibility, and leaving the checkbox behavior unchanged.
API-wise, a display offers the methods onEnable() / onDisable(), which are currently linked to the checkbox, as well as a reset() method, which can be triggered globally for all displays via the Reset button in the footer. Most displays currently call reset() in their onDisable() method() as well, thus clearing the display.
Conceptually we have three different aspects to distinguish:
Toggling visibility
Toggling subscription
Resetting the display
While an additional eye-icon to toggle visibility seems appealing, it raises a major conceptual issue:
Introducing another toggling button (checkbox + eye icon) would allow toggling subscription and visibility separately. However, the Display base class doesn’t provide an appropriate API to toggle visibility (there is only onEnable and onDisable). Augmenting the existing API (e.g. introducing onShow and onHide) would invalidate all existing display plugins, which IMO should be avoided - at least in ROS1’s rviz.
Thus, from my point of view, introducing a new toggling button doesn’t make sense. Rather we should stick to the current praxis combining visibility and subscription toggling and linking them to the existing checkbox.
However, I support the idea of factoring out the reset trigger. This could be done by a context menu as originally suggested by @Peter_Mitrano or we could introduce a new reset-icon next to the checkbox, which would be more easily accessible than a context menu. Conceptually, this icon would be a push button triggering the reset, in contrast to a toggle button changing visibility.
Thanks for the thoughtful comment. Can you explain what you mean by “invalidate all existing display plugins?”. Perhaps you mean they would stop compiling because they wouldn’t be implementing these new pure virtual functions, in which case maybe we provide default/empty implementations in Display ? Or does even the existence of the new functions break something?
As for what you propose, let me make sure I understand. So we change the checkbox to do visibility + subscription (all display checkboxes should now behave like this? or just the left-most global one per display? what about sub-checkboxes like namespaces in Markers, or the MoveIt plugins?) And then we add a new context menu to the Dispay and put “reset” in there, which deletes all the markers.
This seems like a good compromise. I would prefer two separate ones if we can figure out how to make that work, but I’ll take the compromise over having to use my own source build of rviz all the time.
Introducing a pair of new virtual functions to handle toggling of an eye-icon, will leave all existing plugins with unimplemented methods. Providing an empty implementation isn’t very useful as well, as those plugins will not react to the eye-icon then. Finally, just introducing new virtual functions will invalidate the ABI and require recompiling all downstream plugins, which should be avoided within a release cycle.
Yes, my proposal is to toggle via the checkbox both visibility and subscription. This applies only for the top-most display-enabled checkbox. If we want to have similar visibility toggling for other checkboxes as well, the corresponding plugin can implement this, of course.
What do you mean with “you prefer two separate ones”? Two separate checkboxes to toggle subscription and visibility? And a 3rd button to reset?
Finally, just introducing new virtual functions will invalidate the ABI and require recompiling all downstream plugins, which should be avoided within a release cycle.
Totally agreed with this for ROS 1.
I will note, however, that we can change ABI for the ROS 2 version of RViz that is going to go into Galactic. If this is a feature we want going forward, we can make the change at GitHub - ros2/rviz: ROS 3D Robot Visualizer
I’ve started the implementation, and the first part of not clearing/resetting displays was easy. Now I want to modify the GUI and I’m totally lost (I’m not familiar with Qt’s Model/View framework). Here’s what I think I want to do:
Basically add a reset button next to each display. It would be good if individual display plugins could use this as well, like each namespace which right now has a checkbox would also have one of these reset buttons
I’ve figured out that BoolProperty somehow becomes a checkbox, and that I should probably change columnCount in property_tree_model.h to 3, but haven’t managed to get any gui changes to happen. Any help would be appreciated, or if you hate this design and would prefer a different UI for the “reset/clear” functionality, let me know.
UPDATE:
Here is the branch I’m currently working on: GitHub - UM-ARM-Lab/rviz at visibility2
I actually managed to implement this a different way, but I don’t like the UI. I’m starting to understand the architecture, and it segfaults in all sorts of cases.
You don’t want to add a new column to the model. According to your figure, both, the checkbox and the new reset button are displayed in the 2nd column. I think you should override the Display’s paint() method to paint the checkbox and the reset button: https://doc.qt.io/qt-5/modelview.html#3-4-delegates
Additionally, you would need to react to this “button” press by extending PropertyTreeWidget.
If you want to enable this behavior for other BoolProperties, I suggest introducing a new class ResetableBoolProperty:
Property → BoolProperty → ResetableBoolProperty → Display.
However, this will change ABI again.
That’s the drawback of outdated ROS1. New features will only go into ROS2
If we restrict the new feature to the display’s checkbox, I think, ABI can be maintained.
In any case, you can target the new feature for ROS2.
Totally understandable, its old software and pushing ROS2 is the right thing to do. I don’t think people would be happy if we change the display checkbox to no longer reset, but don’t provide a new way to reset. I haven’t thought of a way to add new reset functionality without changing ABI, but if you have ideas I’m all ears.
I don’t use ROS2 yet, so I can’t justify spending time on that right now, but when I do switch I will probably do so.
