pydrake meshcat: Fix name parsing bug #13018
Conversation
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @mpetersen94)
a discussion (no related file):
Is there a way you could recover the Gist from the Slack discussion?
I'm trying to look at it now, but it seems to have expired...
https://gist.github.com/mpetersen94/1dbdaa3bd9bdc255747322f71cb6a6dd
That, or could you recreate the problem and post a new Gist?
There was a problem hiding this comment.
+@EricCousineau-TRI for feature review
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @mpetersen94)
|
Yes I can! Here is a recreation of the gist from the slack discussion. |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Is there a way you could recover the Gist from the Slack discussion?
I'm trying to look at it now, but it seems to have expired...
https://gist.github.com/mpetersen94/1dbdaa3bd9bdc255747322f71cb6a6ddThat, or could you recreate the problem and post a new Gist?
Done. See github comment above.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @mpetersen94)
a discussion (no related file):
Previously, mpetersen94 (Mark Petersen) wrote…
Done. See github comment above.
Sorry for the delay! Repro'd on my system to get a sense. Comments below.
bindings/pydrake/systems/meshcat_visualizer.py, line 290 at r1 (raw file):
delim = "::" if delim not in name: default_source = "world"
I don't think world is appropriate. In your repro, it's associated with an effectively "unnamed" source, so I think a better named would be unknown or unnamed.
Can you use one of those?
There was a problem hiding this comment.
FTR - made repro commit here:
https://github.com/EricCousineau-TRI/drake/tree/33547a78ab3af118243b98b9b87935c891438648/tmp
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @mpetersen94)
bindings/pydrake/systems/meshcat_visualizer.py, line 291 at r1 (raw file):
if delim not in name: default_source = "world" return default_source, name
Also, is it possible for you to add a simple test to ensure this contract is maintained?
(e.g. adding test_custom_mbp_construction or what not to meshcat_visualizer_test?)
mpetersen94
force-pushed
the
fix/meshcat
branch
from
e28210a
to
fb3adef
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @EricCousineau-TRI)
bindings/pydrake/systems/meshcat_visualizer.py, line 290 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I don't think
worldis appropriate. In your repro, it's associated with an effectively "unnamed" source, so I think a better named would beunknownorunnamed.Can you use one of those?
Done.
bindings/pydrake/systems/meshcat_visualizer.py, line 291 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Also, is it possible for you to add a simple test to ensure this contract is maintained?
(e.g. addingtest_custom_mbp_constructionor what not tomeshcat_visualizer_test?)
Done, I think. Or were you thinking a more minimal test?
There was a problem hiding this comment.
+@ggould-tri for platform review, please.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform) (waiting on @EricCousineau-TRI, @ggould-tri, and @mpetersen94)
bindings/pydrake/systems/meshcat_visualizer.py, line 291 at r1 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
Done, I think. Or were you thinking a more minimal test?
OK Will comment in test itself.
bindings/pydrake/systems/test/meshcat_visualizer_test.py, line 166 at r2 (raw file):
simulator.AdvanceTo(.1) def test_anchored_geometry_parsing(self):
Yeah, it's kinda hard to get the true purpose of this test.
Perhaps test_custom_mbp_name_parsing, and perhaps avoid registering properties that aren't directly used in this test? (e.g. if it's just the name, make it absolutely minimal, or document why you need the properties?)
And I guess if everything is necessary, perhaps make it evident in a test docstring? e.g.
"""
Ensure that name parsing does not choke on custom-registered geometries.
"""
# - below, comment something like:
# Make a minimal example to ensure it is used by MeshcatVisualizer.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform) (waiting on @EricCousineau-TRI, @ggould-tri, and @mpetersen94)
bindings/pydrake/systems/test/meshcat_visualizer_test.py, line 166 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Yeah, it's kinda hard to get the true purpose of this test.
Perhaps
test_custom_mbp_name_parsing, and perhaps avoid registering properties that aren't directly used in this test? (e.g. if it's just the name, make it absolutely minimal, or document why you need the properties?)And I guess if everything is necessary, perhaps make it evident in a test docstring? e.g.
""" Ensure that name parsing does not choke on custom-registered geometries. """ # - below, comment something like: # Make a minimal example to ensure it is used by MeshcatVisualizer.
I don't want to put mbp in the name because I think this bug only pops up only when you are just using a SceneGraph. I'll try to pull out the most minimal representation that exposes this bug.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status: 2 unresolved discussions (waiting on @mpetersen94)
bindings/pydrake/systems/meshcat_visualizer.py, line 285 at r2 (raw file):
def _parse_name(self, name): # Parse name, split on the first (required) occurrence of `::` to get
minor: "required" is no longer correct here.
bindings/pydrake/systems/meshcat_visualizer.py, line 287 at r2 (raw file):
# Parse name, split on the first (required) occurrence of `::` to get # the source name, and let the rest be the frame name. # TODO(eric.cousineau): Remove name parsing once #9128 is resolved.
FYI @EricCousineau-TRI the referenced issue is resolved. Have an opinion on this?
bindings/pydrake/systems/meshcat_visualizer.py, line 291 at r2 (raw file):
if delim not in name: default_source = "unnamed" return default_source, name
FYI isn't the remainder of this function just name.split("::", 1)?
bindings/pydrake/systems/test/meshcat_visualizer_test.py, line 177 at r2 (raw file):
meshcat.get_input_port(0)) source_id = scene_graph.RegisterSource()
FYI the build error is the absence of any input port data for this SG source.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions (waiting on @mpetersen94)
bindings/pydrake/systems/meshcat_visualizer.py, line 287 at r2 (raw file):
Previously, ggould-tri wrote…
FYI @EricCousineau-TRI the referenced issue is resolved. Have an opinion on this?
Oh yeahhhhh. This was referencing the name hacks as a means to do hierarchy on the LCM-generated names from SG's visualization functions.
I think the most relevant action item is to reword:
TODO(eric.cousineau): Remove name parsing once this is reimplemented to use Shape introspection.
Mark, you OK with writing this in? I can push it if you like.
bindings/pydrake/systems/meshcat_visualizer.py, line 291 at r2 (raw file):
Previously, ggould-tri wrote…
FYI isn't the remainder of this function just
name.split("::", 1)?
OK Assuming there's only 1 occurrence of ::, yeah. Otherwise, maybe not?
Perhaps worth fixing as part of the non-LCM-based workflow?
bindings/pydrake/systems/test/meshcat_visualizer_test.py, line 166 at r2 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
I don't want to put mbp in the name because I think this bug only pops up only when you are just using a SceneGraph. I'll try to pull out the most minimal representation that exposes this bug.
I'm fine with mbp not being there, but having some indicator that it's for a name.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions (waiting on @EricCousineau-TRI and @mpetersen94)
bindings/pydrake/systems/meshcat_visualizer.py, line 287 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Oh yeahhhhh. This was referencing the name hacks as a means to do hierarchy on the LCM-generated names from SG's visualization functions.
I think the most relevant action item is to reword:
TODO(eric.cousineau): Remove name parsing once this is reimplemented to use Shape introspection.Mark, you OK with writing this in? I can push it if you like.
(dummy comment to make reviewable happy)
mpetersen94
force-pushed
the
fix/meshcat
branch
from
fb3adef
to
e02084c
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI and @ggould-tri)
bindings/pydrake/systems/meshcat_visualizer.py, line 285 at r2 (raw file):
Previously, ggould-tri wrote…
minor: "required" is no longer correct here.
Done.
bindings/pydrake/systems/meshcat_visualizer.py, line 287 at r2 (raw file):
Previously, ggould-tri wrote…
(dummy comment to make reviewable happy)
Done.
bindings/pydrake/systems/meshcat_visualizer.py, line 291 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
OK Assuming there's only 1 occurrence of
::, yeah. Otherwise, maybe not?
Perhaps worth fixing as part of the non-LCM-based workflow?
So no work needed for this PR right?
bindings/pydrake/systems/test/meshcat_visualizer_test.py, line 166 at r2 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I'm fine with
mbpnot being there, but having some indicator that it's for a name.
Done.
bindings/pydrake/systems/test/meshcat_visualizer_test.py, line 177 at r2 (raw file):
Previously, ggould-tri wrote…
FYI the build error is the absence of any input port data for this SG source.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status:complete! all discussions resolved, LGTM from assignees ggould-tri(platform),EricCousineau-TRI(platform)
bindings/pydrake/systems/meshcat_visualizer.py, line 291 at r2 (raw file):
Previously, mpetersen94 (Mark Petersen) wrote…
So no work needed for this PR right?
OK Yup, looks fine to me for now!
Since this assert has bitten me multiple times when I add geometry programmatically and try to visualize it in Meshcat, I wanted to propose a fix. This was the solution proposed in a slack discussion but given that the above TODO references a closed issue, there might be a better solution.
@EricCousineau-TRI for discussion/feature review.
This change is