[geometry] Establish Meshcat in C++ (step 4) #15670
Conversation
There was a problem hiding this comment.
+@SeanCurtis-TRI for feature review, please.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on @RussTedrake and @SeanCurtis-TRI)
a discussion (no related file):
Step 4 doesn't seem like it fully completes the issue, at least per this comment:
#13038 (comment)
Do you want to keep issue open for adding MeshcatVisualizerCpp bindings?
If not, could the issue be updated to reflect that C++ bindings won't be added?
There was a problem hiding this comment.
+@EricCousineau-TRI for platform review (will review after feature stamp)
Reviewable status: 1 unresolved discussion, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI, @RussTedrake, and @SeanCurtis-TRI)
RussTedrake
force-pushed
the
meshcat_visualizer
branch
from
7b6cc4c
to
0223f6b
Compare
|
a discussion (no related file): Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Nope. I only added Step 5 recently. But the issue title is Mechat in C++. This completes that. (plus the binding PR is ready once this lands) |
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)
a discussion (no related file):
Previously, RussTedrake (Russ Tedrake) wrote…
Nope. I only added Step 5 recently. But the issue title is Mechat in C++. This completes that. (plus the binding PR is ready once this lands)
OK SGTM - thanks for clarifying!
There was a problem hiding this comment.
First pass done -- the big topic is the relationship between MeshcatVisualizer and Meshcat, see my dedicated discussion below.
Reviewed 5 of 7 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 39 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @RussTedrake)
a discussion (no related file):
The whole raw-pointer vs shared_ptr thing is kind of ugly.
- It necessitates documentation about how to construct a version that is actually transmogrifiable.
- that mechanism is very counter-intuitive.
- It leads to complexity such that you have
AddToBuilderthat introduces visualizers that cannot be transmogrified. In other words, the most convenient way to add and connect a visualizer produces the most broken version. Alternatively, we double up the number ofAddToBuilderoverloads. - It's really not clear what workflow is being enabled here.
I have an alternative proposal:
Hypothesis: there is no application that needs both a MeshcatVisualizer and Meshcat that cannot instantiate the MeshcatVisualizer first. (Sorry for the double negative.) In other words, every application that requires both a MeshcatVisualizer and Meshcat can freely instantiate the MeshcatVisualizer first.
Implication:
- Simply have the
MeshcatVisualizercreate its ownMeshcatfrom a single constructor that only takes parameters. - Store it in a
shared_ptr. - Document that every
MeshcatVisualizerinstance is part of a "family" -- all instances that are scalar converted from a commonMeshcatVisualizerinstance. They all share the sameMeshcatinstance (and will modify the state of the same visualizer window). Generally, it is inadvisable to do so. - Keep the method that returns a mutable
Meshcatinstance so if there's anything else that needs aMeshcat, it can make use of it.- Alternatively, always return a
shared_ptrso that the whole ecosystem surroundingMeshcatworks the same. Everyone obviously and explicitly shares theMeshcatand no one has it destroyed out from under them.
- Alternatively, always return a
This simplifies the universe immensely. Some of the benefits:
- Much simpler interface.
- Much simpler documentation.
- This would allow adding two different visualizers to a single diagram -- one drawing illustration and one drawing proximity. That'd be slick. :)
So, what am I not seeing? What workflow have I failed to imagine? I know shared_ptr isn't common in drake, but it seems perfectly appropriate here. This certainly doesn't preclude the possibility of doing Meshcat stuff without a MeshcatVisualizer, so that workflow is not harmed (or even changed) in any way.
-- commits, line 3 at r2:
BTW Probably worth reversing these two lines.
The first line only means something to github, the second line is the one that matters to humans -- particularly someone compiling release notes.
geometry/BUILD.bazel, line 420 at r2 (raw file):
) drake_cc_library(
BTW I note that we've got interleaved library, binary, and test. Prior to the meshcat endeavors, they were separated by category.
However, I need to reorganize this file (I hate the fact that it's only partially alphabetized), so I can clean that up at the same time. I just wanted to point it out to you in case you hadn't noticed.
geometry/BUILD.bazel, line 426 at r2 (raw file):
deps = [ ":geometry_roles", ":geometry_version",
BTW You're explicitly declaring dependency on geometry_version, but you're not explicitly including it. So, you're getting the header by transitivity but not the dependency. Might be worth unifying it (in either direction). However, what's here doesn't break any rules, it's just...funny.
geometry/BUILD.bazel, line 433 at r2 (raw file):
"//math:geometric_transform", "//systems/framework:context", "//systems/framework:leaf_system",
nit: Probably want to declare the dependency on diagram_builder directly. The path by which you're getting it is pretty esoteric.
geometry/meshcat.cc, line 71 at r2 (raw file):
using WebSocket = uWS::WebSocket<kSsl, kIsServer, PerSocketData>; using MsgPackMap = std::map<std::string, msgpack::object>; constexpr static double kMaxBackPressure{50 * 1024 * 1024};
nit: A comment on units and why this value, please.
geometry/meshcat.cc, line 302 at r2 (raw file):
if (size > kMaxBackPressure) { throw std::runtime_error(fmt::format( "The meshfile at {} is too large for the current websocket setup. "
nit: This error message would be far more helpful if it came with guidance on what the user (whose simulation has just crashed) can do about it.
geometry/meshcat_visualizer.h, line 18 at r2 (raw file):
/** The set of parameters for configuring MeshcatVisualizer. */ struct MeshcatVisualizerParams {
nit: Let's pull this into its own file/build target. It will better enable configuring visualization via yaml such that it doesn't have to drag in the whole universe of geometry, just to be able to set some parameters.
(You'll note that DrakeVisualizerParams are in their own -- a recent change.)
geometry/meshcat_visualizer.h, line 19 at r2 (raw file):
BTW I recognize that this language comes more or less directly from DrakeVisualizer, but in hindsight, it's far too implementation detail heavy. Instead:
The duration (in simulation seconds) between attempts to update poses in the visualizer.
This is a statement that is equally true of both visualizers (and, generally, all visualizers of the same stamp).
geometry/meshcat_visualizer.h, line 30 at r2 (raw file):
/** A prefix to add to the path for all objects and transforms curated by the * MeshcatVisualizer. It can be an absolute path or relative path. See @ref
nit: Leading * is inconsistent with the rest of the file (and geometry).
geometry/meshcat_visualizer.h, line 31 at r2 (raw file):
/** A prefix to add to the path for all objects and transforms curated by the * MeshcatVisualizer. It can be an absolute path or relative path. See @ref * meshcat_path "Meshcat paths" for details. */
nit: I think there needs to be some explicit reconciliation between this prefix and the one in Meshcat -- does one win? Do they collaborate? Given that Meshcat is so public, it seems necessary that this should be clear.
geometry/meshcat_visualizer.h, line 35 at r2 (raw file):
/** Determines whether to send a Meschat::Delete("/prefix") message on an initialization event to remove any visualizations e.g. from a previous
nit: Indentation in consistent with the file (subsequent lines line up with the first * and not on the /.
geometry/meshcat_visualizer.h, line 38 at r2 (raw file):
BTW: On the same "hide implementation details" topic, it seems that this would be better as clear_on_initialization_event.
If
true, the visualizer will remove all objects under theprefix` folder on an initialization event.
I've left the idea of a prefix folder as that is very much a public UI aspect of visualization. But method invocations are not.
geometry/meshcat_visualizer.h, line 59 at r2 (raw file):
geometries with other roles. Only one role can be specified. See DrakeVisualizer which uses the same mechanisms for more details.
BTW: This system is unique in its dependence of what is arguably an externally initialized Meshcat instance. It's probably worth documenting here. Also worth emphasizing that the user could use that Meshcat instance to do extra fun and games with the visualization beyond just showing what's in SceneGraph. I feel it's a major feature well worth emphasizing.
geometry/meshcat_visualizer.h, line 60 at r2 (raw file):
DrakeVisualizer which uses the same mechanisms for more details. @tparam_nonsymbolic_scalar
btw This is typically the last thing.
geometry/meshcat_visualizer.h, line 62 at r2 (raw file):
@tparam_nonsymbolic_scalar Note that you must use the std::shared_pointer<Meschat> version of the
BTW Any particular you didn't use @note? Didn't want doxygen to underscore this?
geometry/meshcat_visualizer.h, line 70 at r2 (raw file):
DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN(MeshcatVisualizer) /** Creates an instance of %MeshcatVisualizer.
Does this constructor really have any value? If we eliminated this,the python side be unchanged. And the C++ side would require std::make_shared instead of simply building it on the heap. But is that so bad?
By eliminating this, we eliminate the distinction between a system that can be scalar converted and one that cannot be (and the rather unobvious trigger for that distinction to the user).
geometry/meshcat_visualizer.h, line 72 at r2 (raw file):
/** Creates an instance of %MeshcatVisualizer. @param meshcat A Meshcat instance, which must remain valid for the lifetime
nit: This issue goes away if the constructor does. But, if it stays, the justification for why this exists should be a developer note and not a user-facing API note.
geometry/meshcat_visualizer.h, line 73 at r2 (raw file):
@param meshcat A Meshcat instance, which must remain valid for the lifetime of this object. We do not offer to create an owned an
nit "...an owned an..."""
(But, the argument is this justification probably doesn't belong here.)
geometry/meshcat_visualizer.h, line 80 at r2 (raw file):
@param params The set of parameters to control this system's behavior. @throws std::exception if `params.publish_period <= 0`. @throws std::exception if `params.role == Role::kUnassigned`. */
nit: Erratic white spacing on the close of the commeint: */. It should be a single space between the last character and this (as long as it fits). It should only go to the next lie if it doesn't fit. Two spaces is right out.
geometry/meshcat_visualizer.h, line 86 at r2 (raw file):
/** Alternative constructor which takes a shared pointer to meshcat. This version must be used in order to support scalar conversion (e.g. ToAutoDiffXd()), because otherwise the lifetime requirements on a raw Meshcat
nit: Similar note here regarding developer documentation that has mistakenly ended up in user-facing documentation.
geometry/meshcat_visualizer.h, line 95 at r2 (raw file):
/** Scalar-converting copy constructor. See @ref system_scalar_conversion. It should only be used to convert _from_ double _to_ other scalar types. @throws std::exception if `other` does not *own* its Meshcats.
nit: s/Meshcats/Meshcat
geometry/meshcat_visualizer.h, line 95 at r2 (raw file):
I think the comment:
We do not offer to create an owned instance...
Precludes the possibility of using the word "own" here. But, again, it all goes away if the only way to construct this is with a shared_ptr.
geometry/meshcat_visualizer.h, line 102 at r2 (raw file):
/** Returns the Meshcat instance being used for visualization. This can be used to e.g. add additional visualizations to the same scene. */ Meshcat* meshcat() const { return meshcat_; }
nit: Presumably this can never return nullptr. So, it should return a reference.
I'm flexible on whether this were to be called mutable_meshcat() (I'd leave that to taste), but it's worth emphasizing that it is mutable in the documentation.
geometry/meshcat_visualizer.h, line 110 at r2 (raw file):
same prefix. Use MeshcatVisualizer::delete_prefix_on_initialization_event to determine whether this should be called on initialization. */ void DeletePrefix() const;
nit: This is the level of abstraction where I'm less swayed that lots of details of Meshcat should be bleeding through. I'd advocate Clear.
I don't mind it documenting what gets cleared in terms of the prefix. But I'd be a lot happier of the API itself minimized the Meshcat details in favor of more generic visualization concepts.
Admittedly, it's difficult to document this, because whatever prefix the visualizer uses, any other code can add to the same prefix and those visual elements will likewise disappear. You probably need a warning that suggests if someone adds to the same prefix, those elements will also be cleared.
geometry/meshcat_visualizer.h, line 119 at r2 (raw file):
nit: Another case where you are copying from a flawed source. This would really be better as:
Adds a MescatVisualizer and connects it to the given SceneGraph's.... etc.
(Same below...and then I'll go and clean up the DrakeVisualizer documentation to match after this merges.)
geometry/meshcat_visualizer.h, line 137 at r2 (raw file):
private: /* Helper constructor which exists solely to give a useful error message for
BTW "exists solely" seems to be betrayed by the fact that this is the fundamental constructor that all other constructors delegate to. It does more than just generate an error message - it is the constructor that does all of the configuration all the time. Admittedly, this would also go away if we change the whole raw vs shared thing.
geometry/meshcat_visualizer.h, line 161 at r2 (raw file):
/* Handles the initialization event. */ systems::EventStatus DeletePrefix(const systems::Context<T>&) const;
nit: This would be better named OnInitialization or some such thing.
geometry/meshcat_visualizer.h, line 171 at r2 (raw file):
mutable std::shared_ptr<Meshcat> owned_meshcat_{}; /* The version of the geometry that was last loaded (i.e., had a load message
nit: copypasta here. "load message sent".
geometry/meshcat_visualizer.cc, line 14 at r2 (raw file):
namespace geometry { template <typename T>
nit: Your definitions are in a different order than your declarations. I'd propose this private constructor should move down.
geometry/meshcat_visualizer.cc, line 37 at r2 (raw file):
} std::cout << "period = " << params_.publish_period << std::endl;
nit leftover debug
geometry/meshcat_visualizer.cc, line 107 at r2 (raw file):
if (set_objects) { SetObjects(query_object.inspector());
nit: This call to SetObjects should simply go inside the test for version. Being inside the body of that test is 100% correlated with being inside this one -- with no intervening code.
geometry/meshcat_visualizer.cc, line 107 at r2 (raw file):
if (set_objects) { SetObjects(query_object.inspector());
nit: A thought that occurred to me here that I'll confirm later.
I suspect that SetObjects doesn't delete anything. Therefore, if the change in geometry entails the removal of geometry, it will end up staying in the visualizer as zombie geometry.
Confirmed.
geometry/meshcat_visualizer.cc, line 122 at r2 (raw file):
for (FrameId frame_id : inspector.GetAllFrameIds()) { std::string frame_path =
You've done something here that I don't think was intentional.
SceneGraph supports arbitrary hierarchies of frames -- just like Meshcat. This code is arbitrarily flattening it so that regardless of what structure SceneGraph has, every frame becomes a child of the world (well, not strictly true -- the modification of the frame name into "model_name/body_name" gives one layer of hierarchy). I don't think you mean to do that. (DrakeVisualizer does that because it's part of the LCM message.) Meshcat` is different, deserves to be different, and shouldn't suffer from the same behavior (unless, of course we want to).
That said, it's worth noting that MBP also flattens the tree when it registers its bodies with SceneGraph. Even if you made this do the "right" thing vis a vis SceneGraph, you wouldn't be able to detect a difference if you're visualizing MBP.
However, by respecting SceneGraph's structure here, you'll get the right behavior, regardless of whether MBP changes in the future or we visualize other things.
(Fortunately, as far as correctness goes, we're good because SceneGraph requires globally unique frame names.)
geometry/meshcat_visualizer.cc, line 142 at r2 (raw file):
// when coming from MultibodyPlant. const std::string path = fmt::format("{}/{}", frame_path, geom_id.get_value());
nit: Geometries have names (I'm looking to kill the horrible geometry names that MBP sets.) Perhaps, at least, a TODO to use the names when they're not all cluttered up with "model::geometry"?
geometry/meshcat_visualizer.cc, line 144 at r2 (raw file):
fmt::format("{}/{}", frame_path, geom_id.get_value()); Rgba rgba = params_.default_color; if (params_.role == Role::kIllustration) {
I note that this behavior is a bit different from DrakeVisualizer. DrakeVisualizer does two things:
- Allows you to assign color to proximity geometry and have that appear.
- If we're visualizing some non-illustration geometry, but a given geometry also has an illustration role, the geometry will use the illustration color to visualize (no need to duplicate color declarations across roles).
This implementation doesn't offer either of those. All collision geometries must be colored homogeneously. We can enable the first feature very easily and simplify the code. From:
Rgba rgba = params_.default_color;
if (params_.role == Role::kIllustration) {
const IllustrationProperties* props =
inspector.GetIllustrationProperties(geom_id);
if (props && props->HasProperty("phong", "diffuse")) {
rgba = props->GetProperty<Rgba>("phong", "diffuse");
}
}to
const Rgba rgba = inspector.GetProperties(geom_id, params_.role)
->GetPropertyOrDefault("phong", "diffuse", params_.default_color);Even if you chose not to document this behavior in this PR, this is still a simpler way to articulate it than the current code.
The other option is, of course, to fully ape the DrakeVisualizer behavior in this regard.
geometry/test/meshcat_manual_test.cc, line 9 at r2 (raw file):
#include "drake/multibody/parsing/parser.h" #include "drake/multibody/plant/multibody_plant.h" #include "drake/systems/analysis/simulator.h"
BTW I really like this test. :) It's super convenient and is expanding nicely.
geometry/test/meshcat_manual_test.cc, line 108 at r2 (raw file):
simulator.set_target_realtime_rate(1.0); simulator.AdvanceTo(4.0);
BTW I'd say 90% of the time, when I run this binary, it fails to exit on its own and I have to hit Ctrl-C. Do you encounter this at all?
You might consider adding a
std::cout << "Exiting... (if it doesn't exit, feel free to hit Ctrl-C)\n";to just before the return.
geometry/test/meshcat_visualizer_test.cc, line 17 at r2 (raw file):
namespace { // The tests in this file require a dependency on MultibodyPlant. One could
nit: Can you document the choice to not include AutoDiffXd in here (beyond just scalar conversion). I've recently wrestled with a class that claimed scalar conversion, did, what it considered to be, a conversion and declared success. In fact, the conversion was incomplete and the converted instance was useless.
Also, probably worth building with AutoDiffXd.
geometry/test/meshcat_visualizer_test.cc, line 91 at r2 (raw file):
int num_periodic_events = 0; auto periodic_events = visualizer_->GetPeriodicEvents();
nit: An overly aggressive use of auto.
geometry/test/meshcat_visualizer_test.cc, line 94 at r2 (raw file):
for (const auto& data_and_vector : periodic_events) { for (const auto& event : data_and_vector.second) { if (event->get_trigger_type() == systems::TriggerType::kPeriodic) {
nit: This additional guard seems to be a test of the framework: "Does `GetPeriodicEvents possibly return events that don't have periodic triggers?" Probably worth just excising the test.
geometry/test/meshcat_visualizer_test.cc, line 105 at r2 (raw file):
TEST_F(MeshcatVisualizerWithIiwaTest, Roles) { MeshcatVisualizerParams params;
nit: If the code ignored params.role and just hard coded a particular role, this test would largely still pass. (Except for the exception.)
geometry/test/meshcat_visualizer_test.cc, line 157 at r2 (raw file):
EXPECT_FALSE(meshcat_.HasPath("/drake/visualizer/my_random_path")); // Repeat, but this time with delete prefix disable.
nit: s/disable/disabled.
geometry/test/meshcat_visualizer_test.cc, line 238 at r2 (raw file):
"MeshcatVisualizer can only be scalar converted if it owns.*"); auto autodiff = owned.ToAutoDiffXd(); autodiff->CreateDefaultContext();
nit: Is this really sufficient to show that transmogrification created a functional instance? (Again, burned by recent experience.)
|
a discussion (no related file): Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'm afraid I disagree with the hypothesis. The workflow that will be most important to me immediately, is one where in the first cell of my Jupyter notebook, I create a Meshcat object. I open the visualizer once. It persists for my entire notebook. In cells that follow, Diagrams containing MeshcatVisualizer come and they go. The Meshcat object is effectively a global. But I might have two of them (if I need two visualization windows). |
There was a problem hiding this comment.
Reviewable status: 39 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @RussTedrake)
a discussion (no related file):
Previously, RussTedrake (Russ Tedrake) wrote…
I'm afraid I disagree with the hypothesis.
The workflow that will be most important to me immediately, is one where in the first cell of my Jupyter notebook, I create a Meshcat object. I open the visualizer once. It persists for my entire notebook. In cells that follow, Diagrams containing MeshcatVisualizer come and they go. The Meshcat object is effectively a global. But I might have two of them (if I need two visualization windows).
That doesn't contradict my hypothesis. While you clearly want to have that workflow, tell me why you need to have that workflow? My hypothesis uses the word "requires" very intentionally.
I have to infer from your workflow that between creating your global Meshcat and instantiating a visualizer, you're planning on pushing things to the meshcat, right? (Which still doesn't preclude first creating the visualizer, getting the Meshcat from it and latter passing it to the DiagramBuilder.)
And on a separate note, the alternative solution that I deleted was that you have two constructors. One like documented above and one that takes a shared_ptr. Then it's still always transmogrifiable. In the case where all you want to do is visualize SceneGraph, you don't have to do anything extra.
My main gripe is still the weird modes for the system.
RussTedrake
force-pushed
the
meshcat_visualizer
branch
from
0223f6b
to
f669327
Compare
There was a problem hiding this comment.
Reviewable status: 15 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
That doesn't contradict my hypothesis. While you clearly want to have that workflow, tell me why you need to have that workflow? My hypothesis uses the word "requires" very intentionally.
I have to infer from your workflow that between creating your global
Meshcatand instantiating a visualizer, you're planning on pushing things to the meshcat, right? (Which still doesn't preclude first creating the visualizer, getting theMeshcatfrom it and latter passing it to theDiagramBuilder.)
And on a separate note, the alternative solution that I deleted was that you have two constructors. One like documented above and one that takes a shared_ptr. Then it's still always transmogrifiable. In the case where all you want to do is visualize
SceneGraph, you don't have to do anything extra.My main gripe is still the weird modes for the system.
I've removed the raw pointer version. I had considered that possibility already. I agree it's better.
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Probably worth reversing these two lines.
The first line only means something to github, the second line is the one that matters to humans -- particularly someone compiling release notes.
Done. (but the resolution is the more monumental of the two!)
geometry/BUILD.bazel, line 426 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW You're explicitly declaring dependency on
geometry_version, but you're not explicitly including it. So, you're getting the header by transitivity but not the dependency. Might be worth unifying it (in either direction). However, what's here doesn't break any rules, it's just...funny.
Done
geometry/BUILD.bazel, line 433 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Probably want to declare the dependency on
diagram_builderdirectly. The path by which you're getting it is pretty esoteric.
Done.
geometry/meshcat.cc, line 71 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: A comment on units and why this value, please.
Done.
geometry/meshcat.cc, line 302 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This error message would be far more helpful if it came with guidance on what the user (whose simulation has just crashed) can do about it.
Done.
geometry/meshcat_visualizer.h, line 18 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Let's pull this into its own file/build target. It will better enable configuring visualization via yaml such that it doesn't have to drag in the whole universe of geometry, just to be able to set some parameters.
(You'll note that
DrakeVisualizerParamsare in their own -- a recent change.)
Done.
geometry/meshcat_visualizer.h, line 19 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I recognize that this language comes more or less directly from
DrakeVisualizer, but in hindsight, it's far too implementation detail heavy. Instead:The duration (in simulation seconds) between attempts to update poses in the visualizer.
This is a statement that is equally true of both visualizers (and, generally, all visualizers of the same stamp).
Done.
geometry/meshcat_visualizer.h, line 30 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Leading
*is inconsistent with the rest of the file (and geometry).
Done. yes, my editor does this on autoformat... i normally catch them, but not always!
geometry/meshcat_visualizer.h, line 31 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: I think there needs to be some explicit reconciliation between this prefix and the one in
Meshcat-- does one win? Do they collaborate? Given thatMeshcatis so public, it seems necessary that this should be clear.
Done.
geometry/meshcat_visualizer.h, line 35 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Indentation in consistent with the file (subsequent lines line up with the first
*and not on the/.
Done.
geometry/meshcat_visualizer.h, line 59 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW: This system is unique in its dependence of what is arguably an externally initialized
Meshcatinstance. It's probably worth documenting here. Also worth emphasizing that the user could use thatMeshcatinstance to do extra fun and games with the visualization beyond just showing what's inSceneGraph. I feel it's a major feature well worth emphasizing.
Done.
geometry/meshcat_visualizer.h, line 60 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
btw This is typically the last thing.
Done
geometry/meshcat_visualizer.h, line 62 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Any particular you didn't use
@note? Didn't want doxygen to underscore this?
N/A
geometry/meshcat_visualizer.h, line 70 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Does this constructor really have any value? If we eliminated this,the python side be unchanged. And the C++ side would require
std::make_sharedinstead of simply building it on the heap. But is that so bad?By eliminating this, we eliminate the distinction between a system that can be scalar converted and one that cannot be (and the rather unobvious trigger for that distinction to the user).
Done.
geometry/meshcat_visualizer.h, line 72 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This issue goes away if the constructor does. But, if it stays, the justification for why this exists should be a developer note and not a user-facing API note.
Done.
geometry/meshcat_visualizer.h, line 73 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit "...an owned an..."""
(But, the argument is this justification probably doesn't belong here.)
Done
geometry/meshcat_visualizer.h, line 80 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Erratic white spacing on the close of the commeint:
*/. It should be a single space between the last character and this (as long as it fits). It should only go to the next lie if it doesn't fit. Two spaces is right out.
Done.
geometry/meshcat_visualizer.h, line 86 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Similar note here regarding developer documentation that has mistakenly ended up in user-facing documentation.
N/A
geometry/meshcat_visualizer.h, line 95 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: s/Meshcats/Meshcat
N/A
geometry/meshcat_visualizer.h, line 95 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I think the comment:
We do not offer to create an owned instance...
Precludes the possibility of using the word "own" here. But, again, it all goes away if the only way to construct this is with a
shared_ptr.
Done.
geometry/meshcat_visualizer.h, line 102 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Presumably this can never return
nullptr. So, it should return a reference.I'm flexible on whether this were to be called
mutable_meshcat()(I'd leave that to taste), but it's worth emphasizing that it is mutable in the documentation.
Done.
geometry/meshcat_visualizer.h, line 110 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This is the level of abstraction where I'm less swayed that lots of details of
Meshcatshould be bleeding through. I'd advocateClear.I don't mind it documenting what gets cleared in terms of the prefix. But I'd be a lot happier of the API itself minimized the Meshcat details in favor of more generic visualization concepts.
Admittedly, it's difficult to document this, because whatever prefix the visualizer uses, any other code can add to the same prefix and those visual elements will likewise disappear. You probably need a warning that suggests if someone adds to the same prefix, those elements will also be cleared.
There is history with this method/workflow in meshcat_visualizer python, and we do have "prefix" as a concept in the parameters, so I'm inclined to leave it.
geometry/meshcat_visualizer.h, line 119 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Another case where you are copying from a flawed source. This would really be better as:
Adds a MescatVisualizer and connects it to the given SceneGraph's.... etc.
(Same below...and then I'll go and clean up the
DrakeVisualizerdocumentation to match after this merges.)
Done.
geometry/meshcat_visualizer.h, line 137 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW "exists solely" seems to be betrayed by the fact that this is the fundamental constructor that all other constructors delegate to. It does more than just generate an error message - it is the constructor that does all of the configuration all the time. Admittedly, this would also go away if we change the whole raw vs shared thing.
N/A
geometry/meshcat_visualizer.h, line 161 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This would be better named
OnInitializationor some such thing.
Done.
geometry/meshcat_visualizer.h, line 171 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: copypasta here. "load message sent".
Done.
geometry/meshcat_visualizer.cc, line 14 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Your definitions are in a different order than your declarations. I'd propose this private constructor should move down.
N/A
geometry/meshcat_visualizer.cc, line 37 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit leftover debug
Done.
geometry/meshcat_visualizer.cc, line 107 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This call to
SetObjectsshould simply go inside the test for version. Being inside the body of that test is 100% correlated with being inside this one -- with no intervening code.
Done.
geometry/meshcat_visualizer.cc, line 107 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: A thought that occurred to me here that I'll confirm later.
I suspect that
SetObjectsdoesn't delete anything. Therefore, if the change in geometry entails the removal of geometry, it will end up staying in the visualizer as zombie geometry.
Confirmed.
Good catch. Resolved.
geometry/meshcat_visualizer.cc, line 122 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
You've done something here that I don't think was intentional.
SceneGraphsupports arbitrary hierarchies of frames -- just likeMeshcat. This code is arbitrarily flattening it so that regardless of what structureSceneGraphhas, every frame becomes a child of the world (well, not strictly true -- the modification of the frame name into "model_name/body_name" gives one layer of hierarchy). I don't think you mean to do that. (DrakeVisualizer does that because it's part of the LCM message.)Meshcat` is different, deserves to be different, and shouldn't suffer from the same behavior (unless, of course we want to).That said, it's worth noting that MBP also flattens the tree when it registers its bodies with SceneGraph. Even if you made this do the "right" thing vis a vis SceneGraph, you wouldn't be able to detect a difference if you're visualizing MBP.
However, by respecting SceneGraph's structure here, you'll get the right behavior, regardless of whether MBP changes in the future or we visualize other things.
(Fortunately, as far as correctness goes, we're good because
SceneGraphrequires globally unique frame names.)
I didn't realize that. So I should crawl through the frames instead of just frames with geometry?
geometry/meshcat_visualizer.cc, line 142 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Geometries have names (I'm looking to kill the horrible geometry names that MBP sets.) Perhaps, at least, a TODO to use the names when they're not all cluttered up with "model::geometry"?
That's basically what I had above.
geometry/meshcat_visualizer.cc, line 144 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I note that this behavior is a bit different from
DrakeVisualizer.DrakeVisualizerdoes two things:
- Allows you to assign color to proximity geometry and have that appear.
- If we're visualizing some non-illustration geometry, but a given geometry also has an illustration role, the geometry will use the illustration color to visualize (no need to duplicate color declarations across roles).
This implementation doesn't offer either of those. All collision geometries must be colored homogeneously. We can enable the first feature very easily and simplify the code. From:
Rgba rgba = params_.default_color; if (params_.role == Role::kIllustration) { const IllustrationProperties* props = inspector.GetIllustrationProperties(geom_id); if (props && props->HasProperty("phong", "diffuse")) { rgba = props->GetProperty<Rgba>("phong", "diffuse"); } }to
const Rgba rgba = inspector.GetProperties(geom_id, params_.role) ->GetPropertyOrDefault("phong", "diffuse", params_.default_color);Even if you chose not to document this behavior in this PR, this is still a simpler way to articulate it than the current code.
The other option is, of course, to fully ape the
DrakeVisualizerbehavior in this regard.
Done. Nice.
geometry/test/meshcat_manual_test.cc, line 108 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I'd say 90% of the time, when I run this binary, it fails to exit on its own and I have to hit Ctrl-C. Do you encounter this at all?
You might consider adding a
std::cout << "Exiting... (if it doesn't exit, feel free to hit Ctrl-C)\n";to just before the return.
Done.
geometry/test/meshcat_visualizer_test.cc, line 17 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Can you document the choice to not include AutoDiffXd in here (beyond just scalar conversion). I've recently wrestled with a class that claimed scalar conversion, did, what it considered to be, a conversion and declared success. In fact, the conversion was incomplete and the converted instance was useless.
Also, probably worth building with
AutoDiffXd.
I started building with AutoDiffXd directly, but it's a mess, since I can't parse into AutoDiffXd. So I have to build geometry manually which adds tons of bloat.
geometry/test/meshcat_visualizer_test.cc, line 91 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: An overly aggressive use of
auto.
It's used by every call to GetPeriodicEvents() in the codebase, including system.cc. The return value type is a mess.
geometry/test/meshcat_visualizer_test.cc, line 94 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This additional guard seems to be a test of the framework: "Does `GetPeriodicEvents possibly return events that don't have periodic triggers?" Probably worth just excising the test.
Done.
geometry/test/meshcat_visualizer_test.cc, line 105 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: If the code ignored
params.roleand just hard coded a particular role, this test would largely still pass. (Except for the exception.)
I know. I thought about it. But didn't come up with a reasonable way to do better. If it makes you feel any better, it did catch me on the fact that I had been using iiwa_no_collision.urdf.
geometry/test/meshcat_visualizer_test.cc, line 157 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: s/disable/disabled.
Done.
geometry/test/meshcat_visualizer_test.cc, line 238 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Is this really sufficient to show that transmogrification created a functional instance? (Again, burned by recent experience.)
Done. I've used the iiwa example and forced it to Publish. That should hit many of the bits.
RussTedrake
force-pushed
the
meshcat_visualizer
branch
3 times, most recently
from
398ac9f
to
53e346d
Compare
There was a problem hiding this comment.
Reviewable status: 15 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)
a discussion (no related file):
Previously, RussTedrake (Russ Tedrake) wrote…
I've removed the raw pointer version. I had considered that possibility already. I agree it's better.
To follow up on your first point... I really do have one Meshcat for the notebook, but MeshcatVisualizers come and go as I make and destroy Diagrams in different examples. And I wouldn't want the lifetime of Meshcat to end with the first MeshcatVisualizer ends. I really don't see how we could make MeshcatVisualizer the more primary object. If you wanted to offer a workflow (in a follow-up) where people who only want one Diagram / one MeshcatVisualizer don't have to make the separate Meshcat object first, that would be fine for me. But it's not my main workflow.
There was a problem hiding this comment.
Reviewable status: 15 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, RussTedrake (Russ Tedrake) wrote…
To follow up on your first point... I really do have one Meshcat for the notebook, but MeshcatVisualizers come and go as I make and destroy Diagrams in different examples. And I wouldn't want the lifetime of Meshcat to end with the first MeshcatVisualizer ends. I really don't see how we could make MeshcatVisualizer the more primary object. If you wanted to offer a workflow (in a follow-up) where people who only want one Diagram / one MeshcatVisualizer don't have to make the separate Meshcat object first, that would be fine for me. But it's not my main workflow.
Ah...the "MeshcatVisualizers come and go" is definitely the compelling point.
There was a problem hiding this comment.
One last lap, I believe.
Reviewed 5 of 6 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 14 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @RussTedrake)
geometry/meshcat.cc, line 71 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done.
(BTW the reason graphics cards have such huge memory is textures, not meshes. In the future, the textures may be the limiting factor).
geometry/meshcat.cc, line 302 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done.
The user can't actually increase the allowance, right? (Except by modifying and recompliing the code?)
geometry/meshcat_visualizer.h, line 30 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done. yes, my editor does this on autoformat... i normally catch them, but not always!
I have similar woes. I find as long as I catch the first one, it doesn't continue. But if I miss the first, chaos reigns supreme.
geometry/meshcat_visualizer.h, line 102 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done.
Again, I'd emphasize documenting that we know it's mutable -- especially as this is a const method. Without that, this looks like a typo/oversight.
That brings up the subject, what's the motivation for providing mutable access via a const instance? Returning something mutable on a const instance is extraordinary enough that I think an effort has to be made to justify (and that justification should be captured).
geometry/meshcat_visualizer.h, line 110 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
There is history with this method/workflow in meshcat_visualizer python, and we do have "prefix" as a concept in the parameters, so I'm inclined to leave it.
I'm marking myself satisfied, but "DeletePrefix" easily lends itself to be interpreted as setting the prefix to the empty string. (And all the other reasons I've said.) But I'm more willing to yield to author preference than I am to fight this battle. :)
geometry/meshcat_visualizer.h, line 38 at r4 (raw file):
(see @ref geometry_roles for more details). It can be configured to visualize geometries with other roles. Only one role can be specified. See DrakeVisualizer which uses the same mechanisms for more details.
One of the unique properties of this system is that each transmogrified version of an instance will communicate with the same Meshcat. That belongs in the class documentation.
geometry/meshcat_visualizer.h, line 54 at r4 (raw file):
@throws std::exception if `params.publish_period <= 0`. @throws std::exception if `params.role == Role::kUnassigned`. */ explicit MeshcatVisualizer(const std::shared_ptr<Meshcat>& meshcat,
nit pass-by-value with a move for the meshcat argument.
geometry/meshcat_visualizer.h, line 131 at r4 (raw file):
nit:
"...last set in Meshcat, by this instance. Because the underlying
Meshcatis shared, this visualizer has no guarantees that theMeshcatstate correlates with this value.
Let's be clear about the potential conflicts this design is inviting.
geometry/meshcat_visualizer.cc, line 122 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
I didn't realize that. So I should crawl through the frames instead of just frames with geometry?
The API is insufficient to support walking the tree. Please put in a TODO (with my name) to correct that when SceneGraphInspector's API supports it.
geometry/meshcat_visualizer.cc, line 142 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
That's basically what I had above.
I'm not sure what you're saying.
My point is that your "geometry" name is going to look like "model/body/7". I'm saying that every geometry has a name as well. It ultimately comes from the sdf/urdf file (or via the API). That's information you're not using.
The second thought is that MBP registers geometry with obnoxious names that I intend to change. :) It has the name "model_instance::geometry_name". You've handled the "model_instance::frame_name" up above to turn the body name into a meaningful path based on model instance. The exact same mapping doesn't make as much sense for geometry.
So, we have three options:
- Continue just using numbers for geometries.
- Use the geometry name as SceneGraph knows them (as ugly as they might be), waiting for the names to get better in the source.
- Post-process the SceneGraph-supplied names (assuming they all come from MBP) to split on "::" and throw out the model instance name.
geometry/meshcat_visualizer.cc, line 144 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done. Nice.
Did you explicitly decide to not document this added feature (certainly an option I can support -- I just wasn't sure if it was by intent or by oversight)? Otherwise, you should document in the class that when visualizing any role, that the visualizer will respect the presence of ("phong", "diffuse") property to set the color of the geometry (e.g. for custom coloring proximity geometry).
geometry/meshcat_visualizer.cc, line 57 at r4 (raw file):
const MeshcatVisualizer<T>& MeshcatVisualizer<T>::AddToBuilder( systems::DiagramBuilder<T>* builder, const SceneGraph<T>& scene_graph, const std::shared_ptr<Meshcat>& meshcat, MeshcatVisualizerParams params) {
nit You should probably make these pass-by-value with a move.
geometry/meshcat_visualizer.cc, line 93 at r4 (raw file):
void MeshcatVisualizer<T>::SetObjects( const SceneGraphInspector<T>& inspector) const { // Frames registered previously that are not set again here should be deleted.
BTW This is good insurance. Ironically, removal/addition of frames is not currently supported (as it changes the semantics of the port and requires coordination between two systems). It's the addition/removal of geometries that is currently supported.
Nevertheless, this is forward looking code.
geometry/meshcat_visualizer.cc, line 95 at r4 (raw file):
// Frames registered previously that are not set again here should be deleted. std::map <FrameId, std::string> frames_to_delete{}; dynamic_frames_.swap(frames_to_delete);
BTW Niiiice. :)
geometry/meshcat_visualizer.cc, line 114 at r4 (raw file):
} for (GeometryId geom_id : inspector.GetGeometries(frame_id, params_.role)) {
nit: A persistent frame can gain/lose geometries. This will not handle the case where geometries disappear.
geometry/meshcat_visualizer_params.h, line 11 at r4 (raw file):
namespace geometry { /** The set of parameters for configuring MeshcatVisualizer. */
nit: I know, I know. But sometimes two spaces before */ sometimes one. Let's make it one.
geometry/meshcat_visualizer_params.h, line 25 at r4 (raw file):
nit: "add its own prefix" is ambiguous. How about,
If relative, this
prefixwill be appended to theMeshcatprefix.
geometry/test/meshcat_manual_test.cc, line 108 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done.
Did you mean to add both newlines? "\n" and std:;endl?
geometry/test/meshcat_visualizer_test.cc, line 17 at r2 (raw file):
The go ahead and say something like:
We're intentionally not building MeshcatVisualizer directly. Parsing is not supported and populating the MBP is more work than its worth. The scalar-converted instance is tested and that provides sufficient evidence for the validity of the type.
Again, the goal is to make it clear that the absence of something is designed and not forgotten.
geometry/test/meshcat_visualizer_test.cc, line 91 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
It's used by every call to GetPeriodicEvents() in the codebase, including system.cc. The return value type is a mess.
I imagine it is. It's ironic, the fact that it masks a really complex type is used as the justification despite the fact that the guidance is to only use it when type is obvious. Still, it's moot point now.
geometry/test/meshcat_visualizer_test.cc, line 105 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
I know. I thought about it. But didn't come up with a reasonable way to do better. If it makes you feel any better, it did catch me on the fact that I had been using iiwa_no_collision.urdf.
If you know your test is dissatisfying in some way, you should document it. Otherwise, it suggests the wrong thing to future readers.
One way you could test it, if you really wanted, is to assign different names to the geometries and look for those geometries in the path.
geometry/test/meshcat_visualizer_test.cc, line 162 at r4 (raw file):
nit: This deserves a note explaining why you're calling Publish -- i.e., what is its purpose and the argument that it achieves it. E.g.
We're doing a smoke test to make sure the scalar copy converter is implemented correctly. By publishing on the converted diagram, we're making sure this doesn't "blow up". We consider this to be sufficient evidence of correctness.
RussTedrake
force-pushed
the
meshcat_visualizer
branch
from
53e346d
to
3801155
Compare
There was a problem hiding this comment.
Done. PTAL.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Ah...the "MeshcatVisualizers come and go" is definitely the compelling point.
Done.
geometry/meshcat.cc, line 302 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
The user can't actually increase the allowance, right? (Except by modifying and recompliing the code?)
Correct.
geometry/meshcat_visualizer.h, line 102 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Again, I'd emphasize documenting that we know it's mutable -- especially as this is a
constmethod. Without that, this looks like a typo/oversight.That brings up the subject, what's the motivation for providing mutable access via a const instance? Returning something mutable on a const instance is extraordinary enough that I think an effort has to be made to justify (and that justification should be captured).
Done.
geometry/meshcat_visualizer.h, line 110 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'm marking myself satisfied, but "DeletePrefix" easily lends itself to be interpreted as setting the prefix to the empty string. (And all the other reasons I've said.) But I'm more willing to yield to author preference than I am to fight this battle. :)
I see your point. I've changed it to Delete() which is similar enough for people to find, and consistent with the nomenclature of Meshcat.
geometry/meshcat_visualizer.h, line 38 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
One of the unique properties of this system is that each transmogrified version of an instance will communicate with the same
Meshcat. That belongs in the class documentation.
Done. I've added a sentence. But I disagree on the uniqueness. Even if a transmogrified DrakeVisualizer creates its own new DrakeLcm, from the user's perspective all of the geometry is still showing up in the same visualizer (and load messages will erase the old content, etc).
geometry/meshcat_visualizer.h, line 54 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit pass-by-value with a move for the
meshcatargument.
Done.
geometry/meshcat_visualizer.h, line 131 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit:
"...last set in Meshcat, by this instance. Because the underlying
Meshcatis shared, this visualizer has no guarantees that theMeshcatstate correlates with this value.Let's be clear about the potential conflicts this design is inviting.
Done.
geometry/meshcat_visualizer.cc, line 122 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
The API is insufficient to support walking the tree. Please put in a TODO (with my name) to correct that when SceneGraphInspector's API supports it.
Done.
geometry/meshcat_visualizer.cc, line 142 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'm not sure what you're saying.
My point is that your "geometry" name is going to look like "model/body/7". I'm saying that every geometry has a name as well. It ultimately comes from the sdf/urdf file (or via the API). That's information you're not using.
The second thought is that MBP registers geometry with obnoxious names that I intend to change. :) It has the name "model_instance::geometry_name". You've handled the "model_instance::frame_name" up above to turn the body name into a meaningful path based on model instance. The exact same mapping doesn't make as much sense for geometry.
So, we have three options:
- Continue just using numbers for geometries.
- Use the geometry name as SceneGraph knows them (as ugly as they might be), waiting for the names to get better in the source.
- Post-process the SceneGraph-supplied names (assuming they all come from MBP) to split on "::" and throw out the model instance name.
It's not just model_instance::geometry_name. It often also has a pointer address (as the comment says), presumably because urdfs very often don't specify a name for the geometry (iiwa14_ does not). I did try initially to parse it by converting '::', but felt that such parsing code might be brittle. I do not like using the ids, but still don't see a better solution.
geometry/meshcat_visualizer.cc, line 144 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Did you explicitly decide to not document this added feature (certainly an option I can support -- I just wasn't sure if it was by intent or by oversight)? Otherwise, you should document in the class that when visualizing any role, that the visualizer will respect the presence of ("phong", "diffuse") property to set the color of the geometry (e.g. for custom coloring proximity geometry).
I think it's far too much detail for a class documentation.
geometry/meshcat_visualizer.cc, line 57 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit You should probably make these pass-by-value with a move.
Done.
geometry/meshcat_visualizer.cc, line 93 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW This is good insurance. Ironically, removal/addition of frames is not currently supported (as it changes the semantics of the port and requires coordination between two systems). It's the addition/removal of geometries that is currently supported.
Nevertheless, this is forward looking code.
Understood. I still think the comment is correct. I delete frames here. I started adding to the comment that "Based on the current implementation of SceneGraph, this would only happen if a frame sent before no longer has geometry with the specified role associated with it ..." and then realized that these details definitely don't belong here (and may become stale). So I'm happiest leaving it as is.
geometry/meshcat_visualizer.cc, line 114 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: A persistent frame can gain/lose geometries. This will not handle the case where geometries disappear.
Done. Good catch.
geometry/meshcat_visualizer_params.h, line 25 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: "add its own prefix" is ambiguous. How about,
If relative, this
prefixwill be appended to theMeshcatprefix.
Done.
geometry/test/meshcat_manual_test.cc, line 108 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Did you mean to add both newlines? "\n" and
std:;endl?
Done.
geometry/test/meshcat_visualizer_test.cc, line 17 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
The go ahead and say something like:
We're intentionally not building MeshcatVisualizer directly. Parsing is not supported and populating the MBP is more work than its worth. The scalar-converted instance is tested and that provides sufficient evidence for the validity of the type.
Again, the goal is to make it clear that the absence of something is designed and not forgotten.
Done.
geometry/test/meshcat_visualizer_test.cc, line 105 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
If you know your test is dissatisfying in some way, you should document it. Otherwise, it suggests the wrong thing to future readers.
One way you could test it, if you really wanted, is to assign different names to the geometries and look for those geometries in the path.
Done. I've found another way and added the test.
Note: GetFrameByName() also feels like a missing API in SceneGraphInspector. I chose to go through the MBP path instead of writing the search through all inspector frames myself.
geometry/test/meshcat_visualizer_test.cc, line 162 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This deserves a note explaining why you're calling
Publish-- i.e., what is its purpose and the argument that it achieves it. E.g.We're doing a smoke test to make sure the scalar copy converter is implemented correctly. By publishing on the converted diagram, we're making sure this doesn't "blow up". We consider this to be sufficient evidence of correctness.
Done.
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1, 4 of 6 files at r5.
Reviewable status: 10 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI, @RussTedrake, and @SeanCurtis-TRI)
geometry/meshcat.cc, line 309 at r4 (raw file):
"The meshfile at {} is too large for the current websocket setup. " "Size {} is greater than the max backpressure {}. You will either " "need to reduce the size of your mesh, or increase the allowance",
nit "increase the allowance" only seems achievable by changing the source code.
Consider mention this in error message, allowing it to be adjusted, or reference issue for TODO about throttling send rate?
geometry/meshcat_visualizer.cc, line 25 at r4 (raw file):
throw std::runtime_error( "MeshcatVisualizer cannot be used for geometries with the " "Role::kUnassigned value. Please choose proximity, perception, or "
nit Error message explicitly references enum for kUnassigned, but does not explicitly reference kProximity, kPerception, or kIllustration.
Consider using explicit references for all enums for consistency.
geometry/meshcat_visualizer.cc, line 115 at r5 (raw file):
size_t pos = 0; while ((pos = frame_path.find("::", pos)) != std::string::npos) { frame_path.replace(pos++, 2, "/");
BTW This may be another case for @calderpg-tri's point on #15560.
(And if anything, at least we should have a better StringReplace function.)
geometry/test/meshcat_manual_test.cc, line 1 at r5 (raw file):
#include "drake/common/find_resource.h"
Working: Testing locally.
geometry/test/meshcat_manual_test.cc, line 109 at r5 (raw file):
simulator.AdvanceTo(4.0); std::cout << "Exiting... (if it doesn't exit, feel free to hit Ctrl-C)"
nit This behavior is a tad bit odd, and I'm not sure if this will bite users in the future - e.g. doing parallel rollouts that use this workflow, and then they have to code up weird logic to kill workers w/ hanging simulator processes.
Is it worth a TODO / issue to investigate?
Or, if the recommendation is "avoid this workflow if you really need your process to end at a desired time", can that be written down?
geometry/test/meshcat_visualizer_test.cc, line 1 at r5 (raw file):
#include "drake/geometry/meshcat_visualizer.h"
nit There is no negative coverage for throwing if publish_period <= 0.
Consider adding that test case; my recommendation would be to move kUnassigned-check from Roles test into something like a InvalidParam test case.
geometry/test/meshcat_visualizer_test.cc, line 94 at r5 (raw file):
TEST_F(MeshcatVisualizerWithIiwaTest, Roles) { MeshcatVisualizerParams params; for (Role role : {Role::kProximity, Role::kIllustration, Role::kPerception}) {
nit It was not immediately clear to me what the condition was for iiwa_link_7 that would make this test pass.
Can you add a comment to that effect?
e.g. "This test expects iiwa_link_7 in our URDF file to have geometry for all three roles."
geometry/test/meshcat_visualizer_test.cc, line 236 at r5 (raw file):
}
BTW Extra blank two lines.
There was a problem hiding this comment.
Reviewable status: 9 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @RussTedrake and @SeanCurtis-TRI)
geometry/test/meshcat_manual_test.cc, line 1 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: Testing locally.
Done. Steps executed as expected!
geometry/test/meshcat_manual_test.cc, line 109 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit This behavior is a tad bit odd, and I'm not sure if this will bite users in the future - e.g. doing parallel rollouts that use this workflow, and then they have to code up weird logic to kill workers w/ hanging simulator processes.
Is it worth a TODO / issue to investigate?
Or, if the recommendation is "avoid this workflow if you really need your process to end at a desired time", can that be written down?
Running with gdb bazel-bin/geometry/meshcat_manual_test, it seems to hang on ~Meshcat, specifically at std::thread::join().
Fixes would be:
- Figure out how to make
uWS::Appget a signal to shut down. - Make the
std::threadrun in daemon mode, if possible.
There was a problem hiding this comment.
Reviewable status: 9 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @RussTedrake and @SeanCurtis-TRI)
geometry/test/meshcat_manual_test.cc, line 109 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Running with
gdb bazel-bin/geometry/meshcat_manual_test, it seems to hang on~Meshcat, specifically atstd::thread::join().Fixes would be:
- Figure out how to make
uWS::Appget a signal to shut down.- Make the
std::threadrun in daemon mode, if possible.
From looking at code, it seems like ~WebSocketPublisher is closing the socket per instructions here:
https://github.com/uNetworking/uWebSockets/blob/6048d026d/misc/READMORE.md#apprun-and-fallthrough
However, something is still getting hung up. Perhaps just a TODO in that section of code, around loop_->defer(/* close socket */)?
There was a problem hiding this comment.
Reviewable status: 9 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @RussTedrake and @SeanCurtis-TRI)
geometry/test/meshcat_manual_test.cc, line 1 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Done. Steps executed as expected!
Also, filed minor aesthetic issue: meshcat-dev/meshcat#93
RussTedrake
force-pushed
the
meshcat_visualizer
branch
from
3801155
to
1a815ac
Compare
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)
geometry/meshcat.cc, line 309 at r4 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit "increase the allowance" only seems achievable by changing the source code.
Consider mention this in error message, allowing it to be adjusted, or reference issue for TODO about throttling send rate?
uh... the error message says "modify the code".??
geometry/meshcat_visualizer.cc, line 25 at r4 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Error message explicitly references enum for
kUnassigned, but does not explicitly referencekProximity,kPerception, orkIllustration.Consider using explicit references for all enums for consistency.
Done.
geometry/meshcat_visualizer.cc, line 115 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW This may be another case for @calderpg-tri's point on #15560.
(And if anything, at least we should have a better
StringReplacefunction.)
i don't think calder commented on #15560? in any case, I don't think there is anything here for me...?
geometry/test/meshcat_manual_test.cc, line 109 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
From looking at code, it seems like
~WebSocketPublisheris closing the socket per instructions here:
https://github.com/uNetworking/uWebSockets/blob/6048d026d/misc/READMORE.md#apprun-and-fallthroughHowever, something is still getting hung up. Perhaps just a TODO in that section of code, around
loop_->defer(/* close socket */)?
Done. I've resolved it.
geometry/test/meshcat_visualizer_test.cc, line 1 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit There is no negative coverage for throwing if
publish_period <= 0.Consider adding that test case; my recommendation would be to move
kUnassigned-check fromRolestest into something like aInvalidParamtest case.
I had a test that confirmed publish_period was set correctly, but sean asked me to remove it. I don't throw on publish_period <=0, i drake_demand it.
geometry/test/meshcat_visualizer_test.cc, line 94 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit It was not immediately clear to me what the condition was for
iiwa_link_7that would make this test pass.Can you add a comment to that effect?
e.g. "This test expectsiiwa_link_7in our URDF file to have geometry for all three roles."
Done. it doesn't actually require that. i've added a comment.
There was a problem hiding this comment.
LGTM for platform, pending feature approval.
Reviewed 1 of 6 files at r3, 2 of 6 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @SeanCurtis-TRI)
geometry/meshcat.cc, line 309 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
uh... the error message says "modify the code".??
OK Ah, missed that. I had reviewed r4, not when it was introduced in r5.
geometry/meshcat_visualizer.cc, line 115 at r5 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
i don't think calder commented on #15560? in any case, I don't think there is anything here for me...?
Sorry, typo; meant #15660
geometry/test/meshcat_manual_test.cc, line 109 at r5 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done. I've resolved it.
OK Sweet! Confirmed it no longer hangs, with or without connecting to HTTP server (i.e., opening link in web browser).
geometry/test/meshcat_visualizer_test.cc, line 1 at r5 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
I had a test that confirmed publish_period was set correctly, but sean asked me to remove it. I don't throw on publish_period <=0, i drake_demand it.
OK Ah, missed that.
geometry/test/meshcat_visualizer_test.cc, line 94 at r5 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done. it doesn't actually require that. i've added a comment.
OK Ah, gotcha.
So if the IIWA URDF geometry were to be updated to have an illustration geometry, but not a perception geometry, this test would still pass.
There was a problem hiding this comment.
I'm not quite done with the "return mutable meshcat from const visualizer" topic. And clarification of a miscommunication that I hadn't even realized happened yesterday (re: unit test on setting publish period).
Reviewed 2 of 6 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @RussTedrake)
geometry/meshcat.cc, line 359 at r6 (raw file):
~WebSocketPublisher() { DRAKE_DEMAND(std::this_thread::get_id() == main_thread_id_); loop_->defer([this]() {
BTW Huzzah!
geometry/meshcat_visualizer.h, line 102 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done.
My follow up question is not answered.
Why mutable access from a const instance? There's some workflow I can't imagine where code only has access to the const system, but still needs to modify visualization. Why can't it have mutable access? Life gets much simpler if that were the case.
Arguing that the thing you're returning must be non-const to do anything interesting is an argument for why someone wants a mutable Meshcat. It is not an argument for why they can get it from a const MeshcatVisualizer. I'm still waiting to hear that justification.
geometry/meshcat_visualizer.h, line 38 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done. I've added a sentence. But I disagree on the uniqueness. Even if a transmogrified DrakeVisualizer creates its own new DrakeLcm, from the user's perspective all of the geometry is still showing up in the same visualizer (and load messages will erase the old content, etc).
Agreed. Not unique. The unique thing is that it can also be changed by things that aren't MeshcatVisualizer.
geometry/meshcat_visualizer.h, line 131 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done.
BTW By truncating it, you've left it as an exercise to the reader to infer why you've emphasized "by this instance". But I'll defer to you.
geometry/meshcat_visualizer.cc, line 142 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
It's not just model_instance::geometry_name. It often also has a pointer address (as the comment says), presumably because urdfs very often don't specify a name for the geometry (iiwa14_ does not). I did try initially to parse it by converting '::', but felt that such parsing code might be brittle. I do not like using the ids, but still don't see a better solution.
Aaaah. Sounds good.
geometry/meshcat_visualizer.cc, line 144 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
I think it's far too much detail for a class documentation.
I couldn't disagree more.
I can accept the argument that it's a feature that you don't care to publish.
But if it's a feature that you want users to benefit from, class documentation is the only place it can go.
But since I'm not assuming you want to publish the feature, I'm marking myself as satisfied.
geometry/meshcat_visualizer.cc, line 93 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Understood. I still think the comment is correct. I delete frames here. I started adding to the comment that "Based on the current implementation of SceneGraph, this would only happen if a frame sent before no longer has geometry with the specified role associated with it ..." and then realized that these details definitely don't belong here (and may become stale). So I'm happiest leaving it as is.
Oh, I agree. I wasn't suggesting you change the code -- it was merely a prefix to my subsequent comment about it not being enough to account for what can change in versions.
geometry/meshcat_visualizer.cc, line 100 at r6 (raw file):
// deleted. std::map <GeometryId, std::string> geometries_to_delete{}; geometries_.swap(geometries_to_delete);
nit: I don't see any calls to geometries_to_delete.erase() which should happen when a geometry is added to geometries_.
geometry/test/meshcat_visualizer_test.cc, line 94 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done.
Aaaaah! Now I realize why this test was deleted. Miscommunication!
I didn't mean nuke this whole test. I meant, "nuke the single line I was commenting on. There's no point in testing event->get_trigger_type() == systems::TrigerType::kPeriodic. You've asked for GetPeriodicEvents(), we can assume that the framework is sufficiently correct that that function returns events with the right trigger.
Otherwise, this test should be kept. I didn't stop to think about what it's deletion meant in my follow up pass. But the statement, "I had a test, but sean asked me to remove it" set off multiple alarms in my mind. So, I went digging and realize what this miscommunication was. This test should definitely come back.
geometry/test/meshcat_visualizer_test.cc, line 1 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
OK Ah, missed that.
I realize there was a miscommunication on that test. I missed it then, but te "sean asked me to remove it" sounded completely unfamiliar. I didn't want you to remove the whole test -- just the if statement conditioning your subsequent queries.
The test should definitely be here. The conversation is on revision 3 if it gets lost.
geometry/test/meshcat_visualizer_test.cc, line 29 at r6 (raw file):
// We're intentionally not building MeshcatVisualizer<AutoDiffXd> directly. // Parsing with AutoDiffXd is not supported and populating the MBP is more work // than its worth. The scalar-converted instance is tested and that provides
nit: I seem to have subconscious that seeks to introduce defects by having typos in recommended text that gets copied verbatim.
s/its/it's
Adds MeshcatVisualizer Resolves RobotLocomotion#13038
RussTedrake
force-pushed
the
meshcat_visualizer
branch
from
1a815ac
to
08cc2e9
Compare
There was a problem hiding this comment.
Done. PTAL.
Reviewable status: LGTM missing from assignees SeanCurtis-TRI(platform),EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)
geometry/meshcat_visualizer.h, line 102 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
My follow up question is not answered.
Why mutable access from a const instance? There's some workflow I can't imagine where code only has access to the const system, but still needs to modify visualization. Why can't it have mutable access? Life gets much simpler if that were the case.
Arguing that the thing you're returning must be non-const to do anything interesting is an argument for why someone wants a mutable
Meshcat. It is not an argument for why they can get it from a constMeshcatVisualizer. I'm still waiting to hear that justification.
I've nuked the method entirely. Return a const meshcat is not useful for anything. I don't have a use case that demands that I get my meshcat out of the visualizer.
geometry/meshcat_visualizer.h, line 131 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW By truncating it, you've left it as an exercise to the reader to infer why you've emphasized "by this instance". But I'll defer to you.
Done.
geometry/meshcat_visualizer.cc, line 100 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: I don't see any calls to
geometries_to_delete.erase()which should happen when a geometry is added togeometries_.
Done. Yikes. Good catch. Thanks.
geometry/test/meshcat_visualizer_test.cc, line 94 at r2 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Aaaaah! Now I realize why this test was deleted. Miscommunication!
I didn't mean nuke this whole test. I meant, "nuke the single line I was commenting on. There's no point in testing
event->get_trigger_type() == systems::TrigerType::kPeriodic. You've asked forGetPeriodicEvents(), we can assume that the framework is sufficiently correct that that function returns events with the right trigger.Otherwise, this test should be kept. I didn't stop to think about what it's deletion meant in my follow up pass. But the statement, "I had a test, but sean asked me to remove it" set off multiple alarms in my mind. So, I went digging and realize what this miscommunication was. This test should definitely come back.
hilarious. to be fair, you did use the words "Probably worth just excising the test."
geometry/test/meshcat_visualizer_test.cc, line 1 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I realize there was a miscommunication on that test. I missed it then, but te "sean asked me to remove it" sounded completely unfamiliar. I didn't want you to remove the whole test -- just the if statement conditioning your subsequent queries.
The test should definitely be here. The conversation is on revision 3 if it gets lost.
Done.
geometry/test/meshcat_visualizer_test.cc, line 29 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: I seem to have subconscious that seeks to introduce defects by having typos in recommended text that gets copied verbatim.
s/its/it's
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @RussTedrake)
geometry/test/meshcat_visualizer_test.cc, line 94 at r2 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
hilarious. to be fair, you did use the words "Probably worth just excising the test."
Oh. Absolutely! This one is on me. The ambiguity in the original comment coupled with the lack of consideration in the result. I'm just glad it's back.
There was a problem hiding this comment.
@EricCousineau-TRI 's gave an LGTM above (without tagging
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @RussTedrake)
Resolves #13038
Adds MeshcatVisualizer
This change is