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

Added nested subnamespaces and more options to hide or group topics. #13

Merged
merged 9 commits into from May 22, 2018

Conversation

Projects
None yet
2 participants
@MasterEric
Copy link
Contributor

commented Aug 2, 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 pull request aims to solve these problems with several changes, including:

  • Added nested subnamespaces. The "Group Namespaces" checkbox has been replaced with a spin box, allowing users to enter the number of levels of grouping they desire.
  • Grouped topics (such as action topics) now display as 3D boxes.
  • Added a checkbox allowing the grouping of topics added by tf2 (from ros/geometry2) (i.e. /tf and /tf_static), in a manner similar to action topics.
  • Added a checkbox allowing the hiding of topics added by tf2 (from ros/geometry2) (i.e. /tf and /tf_static).
  • Added a checkbox allowing the grouping of topics added by image_transport (from ros-perception/image_common) (i.e. /, /compressed, /theora, and /compressedDepth), in a manner similar to action topics.
  • Added a checkbox allowing the hiding of topics added by dynamic_reconfigure (from ros/dynamic_reconfigure) (i.e. /parameter_descriptions and /parameter_updates).

These changes, originally implemented in #9, have been split into six separate commits, with formatting changes removed, for ease of review.

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

Note that the Hide section of the toolbar has been shifted down to a new line, and margins have been changed as the creation of a new line caused spacing problems.

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.
Attachments(1).zip

MasterEric added some commits Aug 2, 2017

Added nested subnamespaces.
The "Group Namespaces" checkbox has been replaced with a numeric spinner, where the user can specify the desired layers of depth.
Added a checkbox to hide topics added by ros/dynamic_reconfigure.
Parameter topics include /parameter_descriptions and /parameter_updates.

@MasterEric MasterEric changed the title Added nested subnamespaces. Added nested subnamespaces and more options to hide or group topics. Aug 2, 2017

@MasterEric

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2017

@dirk-thomas These changes have gone 22 days since they have been revised and split into separate requests with no response. Overall, they provide significant improvements to the user experience and I would like them to be implemented as soon as possible.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Oct 24, 2017

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

@MasterEric The qt_gui_core PR still has pending comments and this PR can't be merged before the other one has been.

@dirk-thomas
Copy link
Member

left a comment

The new feature looks very nice on more complex graphs 👍

I added some comments inline - mostly nitpicks.

Beside them the toolbar looks a bit off for me:

  • the spacing between the three horizontal layouts is not equal
  • the second and third "row" seems to be indented (not sure if that was the case before already?)
  • the "highlight" and "fit" label in the second "row" is not aligned with the other options in that "row"

rqt_graph_toolbar

@@ -420,6 +428,136 @@ def _accumulate_action_topics(self, nodes_in, edges_in, node_connections):
nodes.remove(n)
return nodes, edges, action_nodes

def _populate_node_graph(self, cluster_namespaces_level, node_list, dotcode_factory, dotgraph, rank, orientation, simplify):
namespace_clusters = {}
if (cluster_namespaces_level > 0):

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

Please remove parenthesis around the condition.

Same below.

if (cluster_namespaces_level > 0):
for node in node_list:
if (str(node.strip()).count('/') > 2):
for i in range(2, min(2+cluster_namespaces_level, len(node.strip().split('/')))):

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

Please add spaces around operators.

Same below.

for n in removal_nodes:
if n in nodes:
nodes.remove(n)
if len(tf_topic_edges_in) == 0 and len(tf_topic_edges_out) == 0:

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

More Pythonic: if not tf_topic_edges_in and not tf_topic_edges_out:

parent_namespace = '/'.join(node.strip().split('/')[:i-1])
if namespace not in namespace_clusters:
if parent_namespace == '':
namespace_clusters[namespace] = dotcode_factory.add_subgraph_to_graph(dotgraph, namespace, rank=rank, rankdir=orientation, simplify=simplify)

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

Please remove trailing whitespace.

edges where the edges to tf topics have been removed, and
a map with all the connections to the resulting tf group node'''
removal_nodes = []
tf_nodes = {}

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

Assigned but never used?

edges,
node_connections,
hide_single_connection_topics,
hide_dead_end_topics)

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

Avoid arbitrary indentation (even though it was that way before your patch 😉). Either indent up to the open parenthesis or wrap directly after the parenthesis and indent all argument just one level / 4 spaces. The second option is preferred since it avoids the long indentation.

Same below.

namespace_clusters = self._populate_node_graph(cluster_namespaces_level, (nt_nodes or [])
+ [action_prefix + ACTION_TOPICS_SUFFIX for (action_prefix, _) in action_nodes.items()]
+ [image_prefix + IMAGE_TOPICS_SUFFIX for (image_prefix, _) in image_nodes.items()]
+ nn_nodes if nn_nodes is not None else [],

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

Please wrap after the open parenthesis and make the operators trailing the previous lines.

# cluster topics with same namespace
if (cluster_namespaces_level > 0 and
n.strip().count('/') > 1 and
len(n.strip().split('/')[1]) > 0):

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

Currently the indentation level doesn't allow to distinguish the condition from the block. Instead use:

if (
    cluster_namespaces_level > 0 and
    n.strip().count('/') > 1 and
    len(n.strip().split('/')[1]) > 0
):
    ...

Same below.

else:
namespace = '/'.join(n.strip().split('/')[:cluster_namespaces_level+1])
if namespace not in namespace_clusters:
print("Namespace '"+namespace+"' not found.")

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

When does this happen? Is that an error?

Same below.

This comment has been minimized.

Copy link
@MasterEric

MasterEric Nov 3, 2017

Author Contributor

There were cases where namespace was not in namespace_clusters during testing; this line will be removed.

print("Namespace '"+namespace+"' not found.")
self._add_topic_node_group('n'+n, dotcode_factory=dotcode_factory, dotgraph=namespace_clusters[namespace], quiet=quiet)
else:
self._add_topic_node_group('n'+n, dotcode_factory=dotcode_factory, dotgraph=dotgraph, quiet=quiet)

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Oct 30, 2017

Member

These three loops look partially very similar. Maybe the code would get shorter by refactoring some of this into a helper function?

Update: and a fourth time below.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented Oct 30, 2017

This repo only has a single master branch used for all currently active ROS distros (Indigo, Kinetic, Lunar). ros-visualization/qt_gui_core#87 on the other hand is targeting the kinetic-devel branch.

I would rather not create separate branches in this repo for this feature but that would require to backport the qt_gui_core PR to Indigo / Qt 4. Is that something you would want to do after the qt_gui_core PR has been merged into kinetic-devel?

@MasterEric

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2018

Hey @dirk-thomas,
I was able to get the changes in this PR working on Indigo by using the changes I made a PR for in ros-visualization/qt_gui_core#111. Please test them out and, if you are able to merge those changes, finally merge these major changes to rqt_graph that I am sure everyone will appreciate.

@MasterEric

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@dirk-thomas If the latest pull request ros-visualization/qt_gui_core#117 is approved promptly, do you think the changes to rqt_graph can be approved in time for the long-term supported Melodic release?

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented May 22, 2018

@ros-pull-request-builder retest this please

package.xml Outdated
@@ -23,7 +23,7 @@

<run_depend version_gte="0.2.19">python_qt_binding</run_depend>
<run_depend>python-rospkg</run_depend>
<run_depend>qt_dotgraph</run_depend>
<run_depend version_gte="0.3.8">qt_dotgraph</run_depend>

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas May 22, 2018

Member

This version dependency won't work for Indigo where the next patch release will be 0.2.x. I will remove it from the patch.

@dirk-thomas

This comment has been minimized.

Copy link
Member

commented May 22, 2018

With the qt_gui_core backport merged and released into Indigo this is good to go. I will make a new source release after the merge and release the patch version for Melodic.

Thank you for the contribution.

@dirk-thomas dirk-thomas merged commit e1945ec into ros-visualization:master May 22, 2018

4 checks passed

Ipr__rqt_graph__ubuntu_trusty_amd64 Build finished.
Details
Kpr__rqt_graph__ubuntu_xenial_amd64 Build finished.
Details
Lpr__rqt_graph__ubuntu_xenial_amd64 Build finished.
Details
Mpr__rqt_graph__ubuntu_bionic_amd64 Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.