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

first pass at toggling visibility #1587

Open
wants to merge 1 commit into
base: noetic-devel
Choose a base branch
from

Conversation

PeterMitrano
Copy link

Description

Change the behavior of the Marker & MarkerArray checkboxes to only toggle visibility instead of deleting the markers. I feel like this is the behavior most people expect/prefer (see #379) I can document the change on the wiki if this is merged

Checklist

  • If you are addressing rendering issues, please provide:
    • Images of both, broken and fixed renderings.
    • Source code to reproduce the issue, e.g. a YAML or rosbag file with a MarkerArray msg.
  • If you are changing GUI, please include screenshots showing how things looked before and after.
  • Choose the proper target branch: latest release branch, for non-ABI-breaking changes, future release branch otherwise.
    Due to the lack of active maintainers, we cannot provide support for older release branches anymore.
  • Did you change how RViz works? Added new functionality? Do not forget to update the tutorials and/or documentation on the ROS wiki
  • While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers of RViz. Refer to the RViz Wiki for reviewing guidelines.

@rhaschke
Copy link
Contributor

Thanks for this contribution. But, to be honest, I'm not sure we should touch this behaviour in Rviz1. Actually, the standard behaviour of most (if not all) display plugins is to reset on disabling/enabling. If we change the behaviour, it should be changed consistently for all default plugins AND we should provide a method to actually reset the display as suggested in the almost 10-years old issue #379. You are welcome though to trigger a new discussion on discourse about this. If there is enough momentum to change the behaviour (and adapt all displays) we should do it.

@PeterMitrano
Copy link
Author

PeterMitrano commented Feb 10, 2021

Thanks for the quick response robert, I will post on the discourse to see what others think. I have a few different ideas on how this could be implemented so we might be able to find a middle ground

@rhaschke
Copy link
Contributor

Please send me a link to the discourse discussion when initiated. I like the idea of having context menus associated to the top-level display entries offering delete, rename, duplicate, reset, etc.

@PeterMitrano
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants