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

Multilayer namespace nesting, grouping for tf and image topics, hiding for tf and dynamic reconfigure #9

Closed
wants to merge 1 commit into from

Conversation

EliteMasterEric
Copy link
Contributor

@EliteMasterEric EliteMasterEric commented Jun 1, 2017

rqt_graph has had a long-standing problem; in real use cases, graphs quickly become unwieldy and unreadable, with dynamic reconfigure's parameter topics everywhere, and don't even start with images. This large commit includes a number of feature improvements designed to combat this, listed below:

  • Added nested subnamespaces; the "Group Namespaces" checkbox is now a numeric spinbox, which specifies the number of layers of depth desired.
  • Allowed the grouping of tf topics (/tf and /tf_static).
  • Allowed the hiding of tf topics.
  • Allowed the grouping of image topics (/, /compressed, /theora, and /compressedDepth), similar to action topics.
  • Allowed the hiding of dynamic reconfigure topics (/parameter_descriptions and /parameter_updates).
  • Grouped topics (tf, action topics, and image topics) display as a 3d box when grouped.
  • UI changes; reduced some margins, added checkboxes for above options, moved "Hide" section to a new line to prevent the interface being too wide.

These changes are dependent on pull request ros-visualization/qt_gui_core#87.

This pull request resolves issues #3 and #4.

Three files are attached to this pull request; the launch file I used for testing, a before picture, and an after picture, displaying what rqt_graph looks like before and after the changes.

before
after-2
Attachments.zip

@dirk-thomas
Copy link
Contributor

Thank you for improving this.

The problem with the size of this PR is that it make the review process unnecessary difficult. Can you please either split the different unrelated features into separate PRs or at least into separate commits. That will make it more manageable to review the individual pieces.

Also please avoid any unrelated changes (like https://github.com/ros-visualization/rqt_graph/pull/9/files#diff-9d45525c1c11e3b0e12360b5e5ac47f9R67) since they only make the patch bigger and less readable. If you care about cleanup up existing code like this (which would be great) please do so in a separate PR.

@EliteMasterEric
Copy link
Contributor Author

EliteMasterEric commented Aug 1, 2017

@dirk-thomas The splitting of this pull request into constituent parts may take a bit of time; for the moment, however, I will be closing this pull request and creating individual ones for different features, since, as you mention, that will be easier to maintain in the long run.

Some of the unrelated changes like minor formatting differences and small padding changes in RqtGraph.ui are due to the software used when creating the pull request; these will be minimized as much as possible in the new requests.

EDIT: These changes are now re-implemented and cleaned up in #13

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