Add Python bindings for drake_ros_viz package.

[hidmic]

Spin off from #2. Built on top of #23.

• Add tests for python bindings.

---

This change is 

[gbiggs]

Based on the API used in the sames in PR #39, this PR provides sufficient Python bindings to be useful. I think this is ready for review and merge once #23 goes in.

[sloretz]

I think this PR looks good, but it could use a test. I would suggest something high level like a test that creates a Drake Diagram including the RVizVisualizer system in Python with a subscriber, and checks to make sure the subscriber receives at least one message.

Reviewable status: 0 of 63 files reviewed, all discussions resolved

[gbiggs]

There's a sample of using the bindings in #39. I'll adapt that to an automatable test.

[gbiggs]

I've pushed a somewhat-complex integration test that shows the Python bindings work end-to-end. I think that when we get the C++ tests done and testing individual marker types, this test should be simplified considerably, but it will do for now.

[sloretz]

Reviewed 58 of 60 files at r8, 1 of 8 files at r12, 1 of 2 files at r14, 1 of 3 files at r19, 1 of 1 files at r20, 6 of 6 files at r21, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hidmic)

---

drake_ros_viz/test/test_python_bindings.py line 38 at r21 (raw file):

class ManagedSubscription:

WaitForTopics is almost usable here, but it doesn't seem to give a way to get the message that was received on the topic. It would be nice to upstream this class to launch_testing_ros in the future. Mind making a ticket on this repo about upstreaming it?

---

drake_ros_viz/test/test_python_bindings.py line 43 at r21 (raw file):

            topic_name='/scene_markers/visual',
            required_message_count=1):
        rclpy.init()

I'd recommend doing this in a (Python) context manager so the default rclpy.Contextwill be shut down even when the test fails

class ManagedSubscription:

def __init__(self):
   self._context = rclpy.Context()

def __enter__(self):
    self._context.init()
    # ... create subscription and node ...
    return self

def __exit__(self, t, v, tb):
    self._context.try_shutdown()

# ...


def test_receive_visual_marker_array():
    with ManagedSubscription() as managed_subscription:
        # ... test stuff here

---

drake_ros_viz/test/test_python_bindings.py line 72 at r21 (raw file):

        executor.add_node(self._node)
        while self.continue_spinning(start_time, timeout):
            executor.spin_once(timeout_sec=1)

Alternative implementation: spinning thread target is executor.spin_until_future_complete, and the Future is completed by callback setting the result of the future to the message received. The advantage is it removes the up-to 1 second delay of this loop exiting.

---

drake_ros_viz/test/test_python_bindings.py line 91 at r21 (raw file):

class System:

System seems a little confusing with Drake systems in this context, but I don't have a better name.

---

drake_ros_viz/test/test_python_bindings.py line 153 at r21 (raw file):

    # Dissect and check some important values from a marker
    marker = received_messages[0].markers[1]
    assert marker.ns == 'Source_19'

How stable is Source_19? If we're unsure, maybe this could be relaxed to check it's a non-empty string.

[sloretz]

Reviewed 1 of 1 files at r23, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hidmic)

---

drake_ros_viz/test/test_python_bindings.py line 38 at r21 (raw file):

Previously, sloretz (Shane Loretz) wrote…

WaitForTopics is almost usable here, but it doesn't seem to give a way to get the message that was received on the topic. It would be nice to upstream this class to launch_testing_ros in the future. Mind making a ticket on this repo about upstreaming it?

#80

---

drake_ros_viz/test/test_python_bindings.py line 91 at r21 (raw file):

Previously, sloretz (Shane Loretz) wrote…

System seems a little confusing with Drake systems in this context, but I don't have a better name.

a4a6000

[sloretz]

I pushed a commit to get the test passing on CI. The stability of Source_19 is my only concern, but I'm willing to merge first and see if it becomes a problem later. If CI is green this

Reviewed 1 of 1 files at r24, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hidmic)

[sloretz]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hidmic)

---

drake_ros_viz/test/test_python_bindings.py line 169 at r24 (raw file):

        # There should be 23 markers in the array
        assert len(rx_messages[0].markers) == 24

Hmm, looks like this changed.

   6:             # There should be 23 markers in the array
  6: >           assert len(rx_messages[0].markers) == 24
  6: E           AssertionError: assert 25 == 24
  6: E            +  where 25 = len([visualization_msgs.msg.Marker(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_i...anosec=0), frame_locked=True, points=[], colors=[], text='', mesh_resource='', mesh_use_embedded_materials=False), ...])
  6: E            +    where [visualization_msgs.msg.Marker(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_i...anosec=0), frame_locked=True, points=[], colors=[], text='', mesh_resource='', mesh_use_embedded_materials=False), ...] = visualization_msgs.msg.MarkerArray(markers=[visualization_msgs.msg.Marker(header=std_msgs.msg.Header(stamp=builtin_int...e:///opt/drake/share/drake/manipulation/models/iiwa_description/iiwa7/link_5.obj', mesh_use_embedded_materials=False)]).markers

I suspect this is an upstream drake change, because the number of markers is 24 with f4005300353b44120cd23b0804282cff228bd0d1 and 25 with 98aa0fe342337ff759ca0bda0fc316e94294946f, but I don't know where in between the change happened.

I think we could relax the check to "2 or more".

[sloretz]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hidmic)

---

drake_ros_viz/test/test_python_bindings.py line 169 at r24 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Hmm, looks like this changed.

   6:             # There should be 23 markers in the array
  6: >           assert len(rx_messages[0].markers) == 24
  6: E           AssertionError: assert 25 == 24
  6: E            +  where 25 = len([visualization_msgs.msg.Marker(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_i...anosec=0), frame_locked=True, points=[], colors=[], text='', mesh_resource='', mesh_use_embedded_materials=False), ...])
  6: E            +    where [visualization_msgs.msg.Marker(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_i...anosec=0), frame_locked=True, points=[], colors=[], text='', mesh_resource='', mesh_use_embedded_materials=False), ...] = visualization_msgs.msg.MarkerArray(markers=[visualization_msgs.msg.Marker(header=std_msgs.msg.Header(stamp=builtin_int...e:///opt/drake/share/drake/manipulation/models/iiwa_description/iiwa7/link_5.obj', mesh_use_embedded_materials=False)]).markers

I suspect this is an upstream drake change, because the number of markers is 24 with f4005300353b44120cd23b0804282cff228bd0d1 and 25 with 98aa0fe342337ff759ca0bda0fc316e94294946f, but I don't know where in between the change happened.

I think we could relax the check to "2 or more".

git bisect says RobotLocomotion/drake@b298869 is the commit that makes the number of markers go from 24 to 25, but I don't understand why.

[sloretz]

Reviewable status: 67 of 68 files reviewed, all discussions resolved (waiting on @sloretz)

---

drake_ros_viz/test/test_python_bindings.py line 153 at r21 (raw file):

Previously, sloretz (Shane Loretz) wrote…

How stable is Source_19? If we're unsure, maybe this could be relaxed to check it's a non-empty string.

Relaxed this check in 77171c4

---

drake_ros_viz/test/test_python_bindings.py line 169 at r24 (raw file):

Previously, sloretz (Shane Loretz) wrote…

git bisect says RobotLocomotion/drake@b298869 is the commit that makes the number of markers go from 24 to 25, but I don't understand why.

Relaxed this check in 77171c4

[sloretz]

Reviewable status: 67 of 68 files reviewed, all discussions resolved (waiting on @sloretz)

[sloretz]

Reviewed 1 of 1 files at r25, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hidmic)

[gbiggs]

Reviewed 1 of 1 files at r20.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hidmic)

---

drake_ros_viz/test/test_python_bindings.py line 38 at r21 (raw file):

Previously, sloretz (Shane Loretz) wrote…

WaitForTopics is almost usable here, but it doesn't seem to give a way to get the message that was received on the topic. It would be nice to upstream this class to launch_testing_ros in the future. Mind making a ticket on this repo about upstreaming it?

Issue added here. There's a similar class in C++ in the rosbag2 repository that I plan on copying for the C++ integration tests for drake_ros_viz; that could also be upstreamed, I think.

---

drake_ros_viz/test/test_python_bindings.py line 72 at r21 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Alternative implementation: spinning thread target is executor.spin_until_future_complete, and the Future is completed by callback setting the result of the future to the message received. The advantage is it removes the up-to 1 second delay of this loop exiting.

I have no experience with futures in Python. I initially intended to use them to implement this rather than the busy-loop, but when I looked into them they seemed tightly tied to async Python, and everything I read implied that I would have to make every function from main() down async for a future to work.

Do you have a good reference or sample I can look at to understand how to use a future here?

---

drake_ros_viz/test/test_python_bindings.py line 91 at r21 (raw file):

Previously, sloretz (Shane Loretz) wrote…

System seems a little confusing with Drake systems in this context, but I don't have a better name.

I agree that it's not a great name. I renamed it to DrakeTestSystem in a4a6000, which is slightly better, I think.

---

drake_ros_viz/test/test_python_bindings.py line 153 at r21 (raw file):

Previously, sloretz (Shane Loretz) wrote…

Relaxed this check in 77171c4

To be completely honest, I don't know. This is the downside of using a complex system for the integration test rather than a simple system where we have complete control over ordering.