Added recursive subgraph parsing, box3d shape, graphics items now immediately parented #87

EliteMasterEric · 2017-06-01T15:21:00Z

This commit includes several features:

Added recursive subgraph parsing for nodes and edges (required for namespace nesting in rqt_graph)
Added QGraphicsBox3dItem to render nodes as a 'box3d' dot shape. Group nodes (such as /action_topics in rqt_graph) should use this shape to represent that multiple nodes are being rendered as one.
Graphics items now immediately parented to provided scene to prevent a crash bug. Scene defaults to none for compatibility.

…ediately parented

dirk-thomas · 2017-07-06T21:57:34Z

With #72 being merged can you please update this PR to be based on the current default branch. Thank you.

EliteMasterEric · 2017-07-07T17:45:35Z

@fmauch, it appears we implemented recursive subgraph parsing in parallel with each other. Since your implementation is a bit cleaner, I fixed my code to use yours, and both the unit test and my pull request for rqt_graph are functioning correctly with these changes.

I'd like for you and @dirk-thomas to look at my pull request as it is now and provide feedback as necessary.

dirk-thomas · 2017-07-18T20:22:52Z

qt_dotgraph/src/qt_dotgraph/dot_to_qt.py

        return node_item

-    def addEdgeItem(self, edge, nodes, edges, highlight_level, same_label_siblings=False):
+    def addEdgeItem(self, edge, nodes, edges, highlight_level, scene=None, same_label_siblings=False):


Please add the new keyword argument scene at the end of the signature. That will avoid breaking any code using the current signature without specifying the keyword explicitly.

Same below for dotcode_to_qt_items.

dirk-thomas · 2017-07-18T20:24:19Z

qt_dotgraph/src/qt_dotgraph/dot_to_qt.py

        return nodes, edges

-    def parse_nodes(self, graph, highlight_level):
+    def parse_nodes(self, graph, highlight_level, scene):


Can the new argument be an optional keyword argument (for the same compatibility reason mentioned above).

All callers should pass the keyword argument by explicitly passing the keyword.

Same for parse_edges below.

dirk-thomas · 2017-07-18T20:25:13Z

qt_dotgraph/src/qt_dotgraph/edge_item.py

@@ -127,6 +127,7 @@ def __init__(self, highlight_level, spline, label_center, label, from_node, to_n
            self._arrow.setAcceptHoverEvents(True)

        self._path = QGraphicsPathItem()
+        self._path = QGraphicsPathItem(parent)


Should the line before be removed?

dirk-thomas · 2017-07-18T20:26:30Z

qt_dotgraph/src/qt_dotgraph/node_item.py

+        elif shape == 'box3d':
+            self._graphics_item = QGraphicsBox3dItem(bounding_box)
+        else:
+            #raise ValueError("Invalid shape '"+shape+"'")


Please remove the commented code line.

dirk-thomas · 2017-07-18T20:26:56Z

qt_dotgraph/src/qt_dotgraph/node_item.py

+            self._graphics_item = QGraphicsBox3dItem(bounding_box)
+        else:
+            #raise ValueError("Invalid shape '"+shape+"'")
+            print("Invalid shape '"+shape+"', defaulting to ellipse")


Please use print("Invalid shape '%s', defaulting to ellipse" % shape).

Also print the error message to stderr:

print('...', file=sys.stderr)

This will require the following at the top of the file:

from __future__ import print_function import sys

dirk-thomas · 2017-07-18T20:29:49Z

qt_dotgraph/src/qt_dotgraph/dot_shapes.py

+		painter.drawLine(self._bounding_box.topRight().x(),
+						 self._bounding_box.topRight().y(),
+						 self._bounding_box.topRight().x(),
+						 self._bounding_box.bottomRight().y() - self._bounding_box.height() * 0.1);


Please use four space indentation and a newline at the end of the file to match the code style of the existing code.

EliteMasterEric · 2017-08-01T20:30:53Z

@dirk-thomas I have made the requested changes. I'll start work on splitting the related ros-visualization/rqt_graph#9

Fix

dirk-thomas · 2017-10-09T21:22:52Z

@mastereric This comment is still pending.

EliteMasterEric · 2017-10-30T20:47:09Z

@dirk-thomas Sorry for the delay in resolving the issues in your comment. The changes have been completed, please review them when you have time.

dirk-thomas

Beside a few nitpicks this looks good to be merged.

dirk-thomas · 2017-10-30T21:58:49Z

qt_dotgraph/src/qt_dotgraph/dot_shapes.py

@@ -0,0 +1,44 @@
+from python_qt_binding.QtCore import QRectF
+from python_qt_binding.QtGui import QColor


Import not used.

dirk-thomas · 2017-10-30T22:01:09Z

qt_dotgraph/src/qt_dotgraph/dot_shapes.py

+        rectangle = QRectF(self._bounding_box.topLeft().x(),
+                         self._bounding_box.topLeft().y() + self._bounding_box.height() * 0.1,
+                         self._bounding_box.width() - self._bounding_box.height() * 0.1,
+                         self._bounding_box.height() - self._bounding_box.height() * 0.1)


Indentation doesn't align with parenthesis.

dirk-thomas · 2017-10-30T22:03:58Z

qt_dotgraph/src/qt_dotgraph/dot_to_qt.py

        """Recursively searches all nodes inside the graph and all subgraphs."""
        # let pydot imitate pygraphviz api
        graph.nodes_iter = graph.get_node_list
        graph.subgraphs_iter = graph.get_subgraph_list

        nodes = {}
+        edges = {}


Assigned but never used?

dirk-thomas · 2017-10-30T22:05:46Z

qt_dotgraph/src/qt_dotgraph/node_item.py

@@ -75,6 +77,17 @@ def __init__(self, highlight_level, bounding_box, label, shape, color=None, pare

        self.hovershape = None

+    def parse_shape(self, shape, bounding_box):
+        if shape == 'box' or shape == 'rect' or shape == 'rectangle':


Instead: if shape in ('box', 'rect', 'rectangle'):

Similar below.

EliteMasterEric · 2017-11-03T16:46:49Z

Hey @dirk-thomas,
Thank you for the work you've done in reviewing this code! Having someone else look at a pull request is really helpful in finding the oversights and unused variables from older iterations of code.

I've posted revisions to your latest set of nitpicks, and hope this code can be merged soon. After that, I'll look into pushing these revisions to lunar-devel as well.

P.S. It was cool meeting you back at ROSCon 2017!

dirk-thomas · 2017-11-03T17:13:34Z

qt_dotgraph/src/qt_dotgraph/node_item.py

+            self._graphics_item = QGraphicsRectItem(bounding_box)
+        elif shape in ('ellipse', 'oval', 'circle'):
+            self._graphics_item = QGraphicsEllipseItem(bounding_box)
+        elif shape in ('box3d'):


This line is a bit tricky. With a single item in the parenthesis Python doesn't interpret that as a list. So share='box' would match. It needs to be: elif shape in ('box3d', ):. I will apply the change directly.

dirk-thomas · 2017-11-03T17:20:38Z

I've posted revisions to your latest set of nitpicks, and hope this code can be merged soon.

I just committed a minor logic fix in 8180a48 Once CI has passed I will merge this. Thank you for your effort on this nice feature!

After that, I'll look into pushing these revisions to lunar-devel as well.

This repo doesn't have a Lunar-specific branch. Kinetic and Lunar are using the same code from the kinetic-devel branch. So once this is merged I can make a release into Kinetic and Lunar.

For the pending PR downstream you might want to make sure to add a minimum version for the dependency on qt_dotgraph. The upcoming release will be version 0.3.8. That will make sure that when users update the plugin requiring the changes here are also getting the latest version of qt_dotgraph.

P.S. It was cool meeting you back at ROSCon 2017!

It is always great to add a face to the GitHub username 😉

EliteMasterEric · 2017-11-03T17:53:40Z

After that, I'll look into pushing these revisions to lunar-devel as well.

Looks like I misread one of your comments; the changes here need to be backported (to a new branch ros-visualization/qt_gui_core:indigo-devel), not forward-ported, right?

dirk-thomas · 2017-11-03T18:02:02Z

Oh right, since the plugin using this actually has a single branch across all ROS distros. The only downside is that we won't be able to specify a version dependency then since qt_dotgraph has different versions in Indigo and Kinetic.

EliteMasterEric · 2017-11-03T18:45:29Z

Specifying a version dependency seems important, since the new version of rqt_graph depends on this new version of qt_gui_core.

Could we branch the current repo from where the last indigo version was made and create an indigo-devel branch? This makes sense; because indigo uses a different version of qt, it needs a different codebase. Kinetic and Lunar can stay together.

dirk-thomas · 2017-11-03T19:05:32Z

The branch in this repo used for Indigo is groovy-devel. If you port this patch to that branch we can release new minor versions to make sure that both the release for Indigo as well as Kinetic and Lunar are higher than what was available before. If we rev Indigo from 0.2.32 to 0.4.0 we can actually use a version dependency >= 0.3.8 (which is the version from the Kinetic branch). That numbering is not very intuitive but should work.

As a next step it would be good if you create a branch from the groovy-devel branch, cherry-pick the change from this PR (363d837), and then add additional commits to it to make it work with Qt 4. That PR can then be merged and we can make a new Indigo release (with the weird version increment).

EliteMasterEric · 2017-11-27T20:45:01Z

@dirk-thomas Sorry for the delay in this, I cherry picked my commit to the groovy-devel branch as a new pull request #108.

Added recursive subgraph parsing, box3d shape, graphics items now imm…

1223742

…ediately parented

EliteMasterEric mentioned this pull request Jun 2, 2017

Multilayer namespace nesting, grouping for tf and image topics, hiding for tf and dynamic reconfigure ros-visualization/rqt_graph#9

Closed

EliteMasterEric mentioned this pull request Jun 21, 2017

parse subgraphs recursively #72

Merged

Merged with fmauch's branch.

b39e9b3

dirk-thomas reviewed Jul 18, 2017

View reviewed changes

dirk-thomas added the enhancement label Jul 18, 2017

Merge remote-tracking branch 'main/kinetic-devel' into kinetic-devel

6fdf57d

Method signature fixes and formatting changes

885ed74

Fix

EliteMasterEric mentioned this pull request Aug 24, 2017

Added nested subnamespaces and more options to hide or group topics. ros-visualization/rqt_graph#13

Merged

Removed semicolons; replaced tabs with spaces; added newline to end.

80a6c3a

dirk-thomas reviewed Oct 30, 2017

View reviewed changes

Latest set of minor fixes.

c01a78d

dirk-thomas reviewed Nov 3, 2017

View reviewed changes

fix condition

8180a48

dirk-thomas merged commit 363d837 into ros-visualization:kinetic-devel Nov 3, 2017

EliteMasterEric mentioned this pull request Nov 27, 2017

Cherry picked recursive subgraph parsing to indigo #108

Closed

EliteMasterEric mentioned this pull request Feb 26, 2018

Backport kinetic patches to groovy-devel #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added recursive subgraph parsing, box3d shape, graphics items now immediately parented #87

Added recursive subgraph parsing, box3d shape, graphics items now immediately parented #87

EliteMasterEric commented Jun 1, 2017

dirk-thomas commented Jul 6, 2017

EliteMasterEric commented Jul 7, 2017

dirk-thomas Jul 18, 2017

dirk-thomas Jul 18, 2017

dirk-thomas Jul 18, 2017

dirk-thomas Jul 18, 2017

dirk-thomas Jul 18, 2017

dirk-thomas Jul 18, 2017

dirk-thomas Jul 18, 2017

EliteMasterEric commented Aug 1, 2017

dirk-thomas commented Oct 9, 2017 •

edited

Loading

EliteMasterEric commented Oct 30, 2017

dirk-thomas left a comment

dirk-thomas Oct 30, 2017

dirk-thomas Oct 30, 2017

dirk-thomas Oct 30, 2017

dirk-thomas Oct 30, 2017

EliteMasterEric commented Nov 3, 2017

dirk-thomas Nov 3, 2017

dirk-thomas commented Nov 3, 2017

EliteMasterEric commented Nov 3, 2017

dirk-thomas commented Nov 3, 2017

EliteMasterEric commented Nov 3, 2017

dirk-thomas commented Nov 3, 2017

EliteMasterEric commented Nov 27, 2017

		@@ -0,0 +1,44 @@
		from python_qt_binding.QtCore import QRectF
		from python_qt_binding.QtGui import QColor

Added recursive subgraph parsing, box3d shape, graphics items now immediately parented #87

Added recursive subgraph parsing, box3d shape, graphics items now immediately parented #87

Conversation

EliteMasterEric commented Jun 1, 2017

dirk-thomas commented Jul 6, 2017

EliteMasterEric commented Jul 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliteMasterEric commented Aug 1, 2017

dirk-thomas commented Oct 9, 2017 • edited Loading

EliteMasterEric commented Oct 30, 2017

dirk-thomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EliteMasterEric commented Nov 3, 2017

Choose a reason for hiding this comment

dirk-thomas commented Nov 3, 2017

EliteMasterEric commented Nov 3, 2017

dirk-thomas commented Nov 3, 2017

EliteMasterEric commented Nov 3, 2017

dirk-thomas commented Nov 3, 2017

EliteMasterEric commented Nov 27, 2017

dirk-thomas commented Oct 9, 2017 •

edited

Loading