MeshcatVisualizer: Tweaks to support caching mesh geometry on the zmqserver #13971
Conversation
There was a problem hiding this comment.
+@gizatt for feature review, please.
Reviewable status: LGTM missing from assignee gizatt, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @gizatt)
|
A quick thought - |
|
I don't think we have that problem here. I'm hashing the actual contents of the mesh file, not the name. |
There was a problem hiding this comment.
Nice, this seems really useful. Would have helped with my carrot cutting demo too! One thing about the source_name deletion, otherwise looks good.
Reviewed 3 of 3 files at r1.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee gizatt, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake)
bindings/pydrake/systems/meshcat_visualizer.py, line 247 at r1 (raw file):
frames_opacity, axis_length and axis_radius are the opacity, length and radius of the coordinate axes to be drawn. delete_prefix_on_load: Specifies whether we should deleting the
Nit "deleting" -> delete
bindings/pydrake/systems/meshcat_visualizer.py, line 386 at r1 (raw file):
for i in range(load_robot_msg.num_links): link = load_robot_msg.link[i] [source_name, frame_name] = self._parse_name(link.name)
Nit Might as well change source_name to _ if we're not using it?
bindings/pydrake/systems/meshcat_visualizer.py, line 398 at r1 (raw file):
if meshcat_geom is not None: cur_vis = ( self.vis[self.prefix][source_name][str(link.robot_num)]
I'm trying to convince myself that this isn't going to break multi-robot visualization -- seems fine for a single MBP hooked up to the scene graph, but can you hook up multiple? (I don't know that kind of scene graph usage well enough.)
bindings/pydrake/systems/meshcat_visualizer.py, line 431 at r1 (raw file):
# SceneGraph currently sets the name in PoseBundle as # "get_source_name::frame_name". [source_name, frame_name] = self._parse_name(
Nit Likewise, source_name no longer used.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee gizatt, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake and @SeanCurtis-TRI)
bindings/pydrake/systems/meshcat_visualizer.py, line 398 at r1 (raw file):
Previously, gizatt (Greg Izatt) wrote…
I'm trying to convince myself that this isn't going to break multi-robot visualization -- seems fine for a single MBP hooked up to the scene graph, but can you hook up multiple? (I don't know that kind of scene graph usage well enough.)
You are awesome for asking. ;-)
This is certainly not needed for the case of multiple robots loaded into the same MBP. For an example of that (with MeshcatVisualizer on master), you can see this screenshot. I've loaded the schunk twice.
The source_id is just the source ID of the multibodyplant. i pulled it out because it was getting incremented on every re-simulation (the kernel stayed alive, but i made a fresh mbp/scenegraph). as you can see in the image, the link.robot_num gives one level of protection. But one is also not allowed to give the same name to two model instances, so even the string name must be unique. (here I was commanded by the parser to name them differently). If i didn't name them explicitly, I'd get the error
This model already contains a model instance named 'Schunk_Gripper'. Model instance names must be unique within a given model. which comes from MultibodyTree.
I actually think we could remove the link.robot_num completely from this tree, as it contributes nothing (and now that I know more about meshcat, it feels inefficient).
I suppose removing source_name could cause problems if a SceneGraph were to have multiple sources registered, and they all registered the same frame_name and the same link.robot_num. Our current implementation is all pretty intertwined with the MBP/SceneGraph because it's still using the load_robot_draw message; which we hope to purge in the not-so-distant future.
@SeanCurtis-TRI -- what do you think? Removing the source_name here seems a lot easier than trying to find a more general mechanism for scenegraphs to have frame names that are unique within a scenegraph, but repeatable across multiple instances of scenegraph that are created from the same code?
(i'll resolve the rest of your comments once we decide on this)
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee gizatt, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @gizatt and @RussTedrake)
bindings/pydrake/systems/meshcat_visualizer.py, line 398 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
You are awesome for asking. ;-)
This is certainly not needed for the case of multiple robots loaded into the same MBP. For an example of that (with
MeshcatVisualizeron master), you can see this screenshot. I've loaded the schunk twice.
The source_id is just the source ID of the multibodyplant. i pulled it out because it was getting incremented on every re-simulation (the kernel stayed alive, but i made a fresh mbp/scenegraph). as you can see in the image, the
link.robot_numgives one level of protection. But one is also not allowed to give the same name to two model instances, so even the string name must be unique. (here I was commanded by the parser to name them differently). If i didn't name them explicitly, I'd get the error
This model already contains a model instance named 'Schunk_Gripper'. Model instance names must be unique within a given model.which comes fromMultibodyTree.I actually think we could remove the
link.robot_numcompletely from this tree, as it contributes nothing (and now that I know more about meshcat, it feels inefficient).I suppose removing
source_namecould cause problems if aSceneGraphwere to have multiple sources registered, and they all registered the sameframe_nameand the samelink.robot_num. Our current implementation is all pretty intertwined with the MBP/SceneGraph because it's still using the load_robot_draw message; which we hope to purge in the not-so-distant future.@SeanCurtis-TRI -- what do you think? Removing the
source_namehere seems a lot easier than trying to find a more general mechanism for scenegraphs to have frame names that are unique within a scenegraph, but repeatable across multiple instances of scenegraph that are created from the same code?(i'll resolve the rest of your comments once we decide on this)
To be perfectly frank, I didn't totally follow this. I strongly suspect everything gets better when the work is done directly from the QueryObject and SceneGraphInspector (rather than sniffing LCM messages).
For example, sources have names. While you are correct, re-instantiating an MBP and re-registering as a source in the same process will create an incremented source id, both MBP's could have the same source name and that would give you the kind of continuity you crave.
So, I suspect you're polishing the wrong rock. Having meshcat directly pull everything it actually wants to know from the data available in SceneGraph will give you the best solution.
|
Do you think that I'm not trying to polish a rock. I'm trying to make a minimal change that will dramatically improve the performance for people working on colab. The immediate question is: can we accept the proposed change (despite the potential caveat of multiple registered sources declaring exactly the same link names)? Then open an issue to upgrade to QueryObject? |
There was a problem hiding this comment.
@EricCousineau-TRI 's RViz visualizer uses just the QueryObject from an input port to handle initialization and updating. I think everything a visualizer would care about is available (even if the APIs aren't sweetened for that application).
My "warning" (if that's what it is) is to not go too far down the rabbit hole in coaxing the last subtle nuance out of the current implementation. A quick change for a solid gain is fully justified (particularly since we are not currently dedicating any resources to replacing this implementation with a QueryObject-based one). But as we worry about more subtleties and details in this implementation, that's probably the sign that it's time to prioritize the other activity (with the desired feature set fully in mind, of course).
Reviewable status: 4 unresolved discussions, LGTM missing from assignee gizatt, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @gizatt and @RussTedrake)
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake and @SeanCurtis-TRI)
bindings/pydrake/systems/meshcat_visualizer.py, line 398 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
To be perfectly frank, I didn't totally follow this. I strongly suspect everything gets better when the work is done directly from the
QueryObjectandSceneGraphInspector(rather than sniffing LCM messages).For example, sources have names. While you are correct, re-instantiating an MBP and re-registering as a source in the same process will create an incremented source id, both MBP's could have the same source name and that would give you the kind of continuity you crave.
So, I suspect you're polishing the wrong rock. Having meshcat directly pull everything it actually wants to know from the data available in SceneGraph will give you the best solution.
The source_name being used in the current version of the visualizer is the actual source name -- but if you're going through MBP's RegisterAsSourceForSceneGraph (here), it doesn't supply a source name when registering the MBP, so it defaults to using the source id. Eventually, that ought to get fixed (by passing in the system name, maybe), but I think that's out of scope for now.
And agreed in general that it's long past time to do a proper rewrite of this!
From f2f discussion, I'm on board with just removing the source name to get this out of the way, as long as an issue documenting it is open. I've poked around for simple alternatives, but short of just having a flat hierarchy that just uses the link index in the load robot message (which would be super ugly / I haven't thought all the way through), it seems like a reasonable option to get this working for the class.
There was a problem hiding this comment.
+@EricCousineau-TRI for platform review, given the understanding that Greg mentioned
Reviewable status: 4 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI, @RussTedrake, and @SeanCurtis-TRI)
|
#10482 has mention of porting I've filed meshcat-dev/meshcat-python#76 about the resource pool convo. |
There was a problem hiding this comment.
First pass done, just some high-level comments. PTAL
Reviewed 1 of 3 files at r1.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @RussTedrake and @SeanCurtis-TRI)
a discussion (no related file):
nit Can you post an example notebook of where you'd be using this?
Per comment below, it's unclear to me how users should resolve issues when they see overlapping scene visualizations.
bindings/pydrake/systems/meshcat_visualizer.py, line 252 at r1 (raw file):
simulation. False allows for the possibility of caching object meshes on the zmqserver/clients, to avoid repeatedly downloading meshes over the websockets link.
nit It's unclear what users should do when the run into the edge case of not having a clean slate.
Can you describe the failure mode of not deleting the prefix, and tell users how they should fix it?
e.g. "If you set delete_prefix_on_load=True and try to visualize two (phsyically) different scene graphs in the same notebook, you will see the two scenes overlap. To fix this, you should either set this to True or restart your serve by if they run two different simulations in the same notebook"
bindings/pydrake/systems/meshcat_visualizer.py, line 411 at r1 (raw file):
[frame_name][str(j)]) # Make the uuid's deterministic for mesh geometry, to # support caching at the zmqserver.
nit I had to look at meshcat-pythons source to confirm that it seems OK(ish) to override the uuid field.
Can a comment be made to that effect?
# N.B. It is fine to override the UUID as `meschat-python` does not necessarily rely on the objects all having a unique UUID.
or something like that?
bindings/pydrake/systems/meshcat_visualizer.py, line 412 at r1 (raw file):
# Make the uuid's deterministic for mesh geometry, to # support caching at the zmqserver. if isinstance(meshcat_geom, meshcat.geometry.MeshGeometry):
Given the above comment, can you check what happens if you visualize two IIWAs in the same scene?
I'm sure it will hit the caching code across to visualizations, but am curious to see if it also helps (or hurts) visualizing multiple instances of the same geometry.
I see that you've visualized two WSG grippers, but so it seems like it shouldn't hurt, so it'd be nice to see if this then helps that case? (at the cost of the scene overlap)
My guess is that Three.js will use just one mesh across all instances when visualizing, but meshcat-python will still send over the bytes, even though they're unused.
There was a problem hiding this comment.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @RussTedrake and @SeanCurtis-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can you post an example notebook of where you'd be using this?
Per comment below, it's unclear to me how users should resolve issues when they see overlapping scene visualizations.
(and the reason I'd like to see the example notebook is to see if you're using a fixed URL, or letting it start a new server... in which case, I dunno how caching works...)
|
the example that motivated it was the jacobian pseudo-inverse controller here: https://colab.research.google.com/github/RussTedrake/manipulation/blob/master/pick.ipynb#scrollTo=6v-EGfoI3y6V my PR is the first time that the uuid will be the same for the same mesh files pushed twice. so there is a chance that three.js might cache them now, but would not have before, i think? it's possible we could add those smarts to the zmqserver, but it would be nontrivial, because zmqserver doesn't actually unpack any of the messages... it would need to to implement this. |
Ah, that looks awesome - thank you!!! And sounds good on the ZMQ / Three.js side. I think evidence points to no failures, aside from unintentonial overlays, so need to confirm what Three.js actually does atm. |
There was a problem hiding this comment.
Reviewable status: 7 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @RussTedrake and @SeanCurtis-TRI)
bindings/pydrake/systems/meshcat_visualizer.py, line 411 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit I had to look at
meshcat-pythons source to confirm that it seems OK(ish) to override theuuidfield.Can a comment be made to that effect?
# N.B. It is fine to override the UUID as `meschat-python` does not necessarily rely on the objects all having a unique UUID.or something like that?
FWIW A better re-wording (given other convos):
# N.B. We deem it fine to override the UUID because (at present) the meshcat server + three.js will not
# be bothered by us changing it. Additionally, this means multiple (identical) geometries may have
# the same UUID, but we also find that this does not pose an issue at present.
bindings/pydrake/systems/meshcat_visualizer.py, line 412 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Given the above comment, can you check what happens if you visualize two IIWAs in the same scene?
I'm sure it will hit the caching code across to visualizations, but am curious to see if it also helps (or hurts) visualizing multiple instances of the same geometry.
I see that you've visualized two WSG grippers, but so it seems like it shouldn't hurt, so it'd be nice to see if this then helps that case? (at the cost of the scene overlap)
My guess is that Three.js will use just one mesh across all instances when visualizing, but
meshcat-pythonwill still send over the bytes, even though they're unused.
OK Per statement above, marking myself as satisfied here.
… server Adds an argument to the constructor to avoid deleting the entire tree on every run. Sets uuids deterministically based on the mesh file so that they can be recognized as identical on the server. See meshcat-dev/meshcat-python#75 This makes a *dramatic* improvement in the workflow of using meshcat on colab (at least as we use it in drake), because we don't have to repeatedly download large meshfiles from repeated simulations.
RussTedrake
force-pushed
the
meshcat_cache_meshes
branch
from
720bf8f to
45546c7
Compare
There was a problem hiding this comment.
fwiw -- the "separate zmq_url for each sim" is done only in the very first notebook in my course notes... just to make it super simple for people. (it also automatically opens the window). Every other notebook so far uses a single meshcat server for the duration of the notebook.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @gizatt)
bindings/pydrake/systems/meshcat_visualizer.py, line 247 at r1 (raw file):
Previously, gizatt (Greg Izatt) wrote…
Nit "deleting" -> delete
Done.
bindings/pydrake/systems/meshcat_visualizer.py, line 252 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit It's unclear what users should do when the run into the edge case of not having a clean slate.
Can you describe the failure mode of not deleting the prefix, and tell users how they should fix it?
e.g. "If you setdelete_prefix_on_load=Trueand try to visualize two (phsyically) different scene graphs in the same notebook, you will see the two scenes overlap. To fix this, you should either set this to True or restart your serve by if they run two different simulations in the same notebook"
Done. Added a comment, and a helper method.
bindings/pydrake/systems/meshcat_visualizer.py, line 386 at r1 (raw file):
Previously, gizatt (Greg Izatt) wrote…
Nit Might as well change
source_nameto_if we're not using it?
Done.
bindings/pydrake/systems/meshcat_visualizer.py, line 398 at r1 (raw file):
Previously, gizatt (Greg Izatt) wrote…
The
source_namebeing used in the current version of the visualizer is the actual source name -- but if you're going through MBP'sRegisterAsSourceForSceneGraph(here), it doesn't supply a source name when registering the MBP, so it defaults to using the source id. Eventually, that ought to get fixed (by passing in the system name, maybe), but I think that's out of scope for now.And agreed in general that it's long past time to do a proper rewrite of this!
From f2f discussion, I'm on board with just removing the source name to get this out of the way, as long as an issue documenting it is open. I've poked around for simple alternatives, but short of just having a flat hierarchy that just uses the link index in the load robot message (which would be super ugly / I haven't thought all the way through), it seems like a reasonable option to get this working for the class.
Done. (I think we're all happy now)
bindings/pydrake/systems/meshcat_visualizer.py, line 411 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
FWIW A better re-wording (given other convos):
# N.B. We deem it fine to override the UUID because (at present) the meshcat server + three.js will not # be bothered by us changing it. Additionally, this means multiple (identical) geometries may have # the same UUID, but we also find that this does not pose an issue at present.
Done.
bindings/pydrake/systems/meshcat_visualizer.py, line 431 at r1 (raw file):
Previously, gizatt (Greg Izatt) wrote…
Nit Likewise, source_name no longer used.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:complete! all discussions resolved, LGTM from assignees EricCousineau-TRI(platform),gizatt
Adds an argument to the constructor to avoid deleting the entire tree on every run.
Sets uuids deterministically based on the mesh file so that they can be recognized as identical on the server.
See meshcat-dev/meshcat-python#75
This makes a dramatic improvement in the workflow of using meshcat on colab (at least as we use it in drake), because we don't have to repeatedly download large meshfiles from repeated simulations.
This change is