[geometry] Establish Meshcat in C++ (step 3) #15647
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)
|
Related to #13038. |
RussTedrake
force-pushed
the
meshcat_api
branch
3 times, most recently
from
dfe98c6
to
ce776dd
Compare
There was a problem hiding this comment.
First pass done, looks great! Just want to resolve some confusion on my end about how clearing the tree works in Python vs here. Also starting build on my end to try it out.
Reviewed 8 of 11 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee gizatt, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake)
geometry/meshcat.h, line 61 at r2 (raw file):
/** Deletes the object at the given path as well as all of its children. Deleting a path that does not exist in the current tree does nothing. Deleting the root node (`path` == "" or "/") does nothing.
How do you clear the whole tree? In python I'm used to just using vis.delete(), which I assumed was deleting the root node -- I've found it really convenient.
geometry/meshcat.cc, line 157 at r2 (raw file):
if (path.empty()) { // To match javascript, we don't delete the empty path. return;
But in meshcat-python, if the path to delete is empty, the whole scene tree is deleted (by replacing with a new empty scene tree). https://github.com/rdeits/meshcat-python/blob/46fe99c3044f788c84d45d2c70c0483be38f80b6/src/meshcat/servers/zmqserver.py#L295 Agreed the Javascript side doesn't want the empty path deleted either (it'd throw an error)... but I'm confused if the Python functionality is maintained.
geometry/meshcat.cc, line 369 at r2 (raw file):
material.uuid = uuids::to_string(uuid_generator()); material.type = "MeshPhongMaterial"; material.color = static_cast<int>(255 * rgba.r()) * 256 * 256 +
nit The bit shifting operator (<<) might be more legible for byte packing.
geometry/meshcat_types.h, line 32 at r2 (raw file):
std::string uuid{}; std::string type{}; int color{229 * 256 * 256 + 229 * 256 + 229};
nit Same as other comment, bit shifting might be more legible
geometry/test/meshcat_manual_test.cc, line 32 at r2 (raw file):
meshcat.SetObject("/meshcat/ellipsoid", Ellipsoid(.25, .25, .5), Rgba(1., 0, 1, 1));
Could add a test of alpha handling in here to make sure it goes through OK?
geometry/test/meshcat_test.cc, line 129 at r2 (raw file):
// Deleting a random string does nothing. meshcat.Delete("/meshcat/bad"); // Using a trailing slash acts the same as a random string.
Minor unclear comment -- i.e. deleting with trailing slash does not delete anything. Maybe a little unintuitive, but matches meshcat-python.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee gizatt, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake)
geometry/meshcat.h, line 61 at r2 (raw file):
Previously, gizatt (Greg Izatt) wrote…
How do you clear the whole tree? In python I'm used to just using vis.delete(), which I assumed was deleting the root node -- I've found it really convenient.
Ah, Python's visualizer keeps a root "meshcat" node around, and all paths you supply are relative to that. So vis.delete() is calling delete on a root node of the scene. The functionality written here makes sense; but maybe an additional delete-all-root-nodes could be useful (but fine if out of scope for this PR).
geometry/meshcat.cc, line 157 at r2 (raw file):
Previously, gizatt (Greg Izatt) wrote…
But in meshcat-python, if the path to delete is empty, the whole scene tree is deleted (by replacing with a new empty scene tree). https://github.com/rdeits/meshcat-python/blob/46fe99c3044f788c84d45d2c70c0483be38f80b6/src/meshcat/servers/zmqserver.py#L295 Agreed the Javascript side doesn't want the empty path deleted either (it'd throw an error)... but I'm confused if the Python functionality is maintained.
Still confused about what that Python code is doing but I think the code here is correct / shouldn't hold up this PR.
RussTedrake
force-pushed
the
meshcat_api
branch
from
ce776dd
to
241406e
Compare
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee gizatt, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @gizatt)
geometry/meshcat.h, line 61 at r2 (raw file):
Previously, gizatt (Greg Izatt) wrote…
Ah, Python's visualizer keeps a root "meshcat" node around, and all paths you supply are relative to that. So
vis.delete()is calling delete on a root node of the scene. The functionality written here makes sense; but maybe an additional delete-all-root-nodes could be useful (but fine if out of scope for this PR).
Right. The missing concept is actually the default prefix. In meshcat-python, if you specify a path "dir1/transform" without the leading "/", then it uses the path "/meshcat/dir1/transform". Calling delete("/") has no effect, but vis.delete() == vis.delete("/meshcat"). I'm planning to add that concept here, too... I just worried that the PR was getting big so didn't do it yet!
geometry/meshcat.cc, line 157 at r2 (raw file):
Previously, gizatt (Greg Izatt) wrote…
Still confused about what that Python code is doing but I think the code here is correct / shouldn't hold up this PR.
answered above.
geometry/meshcat.cc, line 369 at r2 (raw file):
Previously, gizatt (Greg Izatt) wrote…
nit The bit shifting operator (<<) might be more legible for byte packing.
Done. (I had that at one point... I agree it's better)
geometry/meshcat_types.h, line 32 at r2 (raw file):
Previously, gizatt (Greg Izatt) wrote…
nit Same as other comment, bit shifting might be more legible
Done.
geometry/test/meshcat_manual_test.cc, line 32 at r2 (raw file):
Previously, gizatt (Greg Izatt) wrote…
Could add a test of alpha handling in here to make sure it goes through OK?
Done. Great idea!
geometry/test/meshcat_test.cc, line 129 at r2 (raw file):
Previously, gizatt (Greg Izatt) wrote…
Minor unclear comment -- i.e. deleting with trailing slash does not delete anything. Maybe a little unintuitive, but matches meshcat-python.
Done.
There was a problem hiding this comment.
Alpha looks good, and default prefix stuff definitely makes sense to defer. 
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake)
There was a problem hiding this comment.
Thanks Greg.
+@SeanCurtis-TRI for platform review, please.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @SeanCurtis-TRI)
RussTedrake
force-pushed
the
meshcat_api
branch
3 times, most recently
from
fe4bb30
to
0982a31
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @gizatt, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
I attempted to run meshcat_manual_test and failed.
INFO: Repository msgpack instantiated at:
/home/seancurtis/code/drake/WORKSPACE:10:22: in <toplevel>
/home/seancurtis/code/drake/tools/workspace/default.bzl:317:29: in add_default_workspace
/home/seancurtis/code/drake/tools/workspace/default.bzl:206:27: in add_default_repositories
/home/seancurtis/code/drake/tools/workspace/msgpack/repository.bzl:14:26: in msgpack_repository
Repository rule pkg_config_repository defined at:
/home/seancurtis/code/drake/tools/workspace/pkg_config.bzl:285:40: in <toplevel>
ERROR: An error occurred during the fetch of repository 'msgpack':
Traceback (most recent call last):
File "/home/seancurtis/code/drake/tools/workspace/pkg_config.bzl", line 279, column 13, in _impl
fail("Unable to complete pkg-config setup for " +
Error in fail: Unable to complete pkg-config setup for @msgpack repository: error 1 from [/usr/bin/pkg-config, "msgpack"]:
ERROR: Error fetching repository: Traceback (most recent call last):
File "/home/seancurtis/code/drake/tools/workspace/pkg_config.bzl", line 279, column 13, in _impl
fail("Unable to complete pkg-config setup for " +
Error in fail: Unable to complete pkg-config setup for @msgpack repository: error 1 from [/usr/bin/pkg-config, "msgpack"]: Any idea why I'd stumble at the first gate?
geometry/test/meshcat_manual_test.cc, line 43 at r4 (raw file):
Open up your browser to the URL above. - The background should be off (white).\n";
nit: Not clear why "white" is in parentheses.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @gizatt, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I attempted to run
meshcat_manual_testand failed.INFO: Repository msgpack instantiated at: /home/seancurtis/code/drake/WORKSPACE:10:22: in <toplevel> /home/seancurtis/code/drake/tools/workspace/default.bzl:317:29: in add_default_workspace /home/seancurtis/code/drake/tools/workspace/default.bzl:206:27: in add_default_repositories /home/seancurtis/code/drake/tools/workspace/msgpack/repository.bzl:14:26: in msgpack_repository Repository rule pkg_config_repository defined at: /home/seancurtis/code/drake/tools/workspace/pkg_config.bzl:285:40: in <toplevel> ERROR: An error occurred during the fetch of repository 'msgpack': Traceback (most recent call last): File "/home/seancurtis/code/drake/tools/workspace/pkg_config.bzl", line 279, column 13, in _impl fail("Unable to complete pkg-config setup for " + Error in fail: Unable to complete pkg-config setup for @msgpack repository: error 1 from [/usr/bin/pkg-config, "msgpack"]: ERROR: Error fetching repository: Traceback (most recent call last): File "/home/seancurtis/code/drake/tools/workspace/pkg_config.bzl", line 279, column 13, in _impl fail("Unable to complete pkg-config setup for " + Error in fail: Unable to complete pkg-config setup for @msgpack repository: error 1 from [/usr/bin/pkg-config, "msgpack"]:Any idea why I'd stumble at the first gate?
Possibly due to pkg_config_paths = ["/usr/local/opt/msgpack/lib/pkgconfig"], in msgpack/repository.bzl.
Is there some readme I missed about being able to run this?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @gizatt, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Possibly due to
pkg_config_paths = ["/usr/local/opt/msgpack/lib/pkgconfig"],inmsgpack/repository.bzl.Is there some readme I missed about being able to run this?
Yep. It's been so long since I've had to run install_prereqs I didn't even think about it.
|
a discussion (no related file): Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Right. Just run the install_prereqs. |
There was a problem hiding this comment.
Reviewed new changes to add default prefix -- looks good, with one documentation nit.
Reviewed 1 of 3 files at r2, 5 of 5 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @RussTedrake)
geometry/meshcat.cc, line 620 at r4 (raw file):
} std::string FullPath(std::string_view path) const {
nit Could use brief explanatory comment that this assembles prefix and path (depending on details of each), or name change to something obvious.
There was a problem hiding this comment.
Reviewed 1 of 5 files at r4, all commit messages.
Reviewable status: 19 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @RussTedrake)
a discussion (no related file):
The colors are all super saturated. It seems like there's no specularity and probably an ambient light cranked up pretty high. It leaves the objects looking very flat and monochromatic.
geometry/meshcat.h, line 19 at r4 (raw file):
Users can navigate their browser to the hosted URL to visualize the Meshcat scene. Note that, unlike many visualizers, one cannot open the visualizer until this server is running.
BTW I have a number of notes regarding path semantics that would probably organically evaporate if we had a nice treatise up here explaining the details. But I've called out the examples where I think the lack of clarity is leading to ambiguity.
geometry/meshcat.h, line 38 at r4 (raw file):
std::string ws_url() const; /** Returns the prefix that gets added to any path that does not begin with
nit: This isn't strictly true. The default prefix is drake. But what is added to any path that does not begin with / is /drake. Which gets returned?
geometry/meshcat.h, line 43 at r4 (raw file):
/** Sets the prefix that gets added to any path that does not begin with '/'. Any leading or trailing '/' on `prefix` will be removed. */
BTW And the prefix "my/deep/prefix" is considered valid, right?
geometry/meshcat.h, line 47 at r4 (raw file):
/** Sets the 3D object at a given path in the scene tree. @param path a "/"-separated string indicating the path in the scene tree.
BTW Meta-note: Do you really want to use the term "separated" as opposed to "delimited" or some such thing? "Separated"
geometry/meshcat.h, line 51 at r4 (raw file):
@param rgba an Rgba that specifies the (solid) color of the object. */ void SetObject(std::string_view path, const Shape& shape,
nit: Based on the comments on SetProperty, you might consider returning the "object" string and define its relationship between the "group" name that is given as a parameter and the name of the object itself.
geometry/meshcat.h, line 61 at r4 (raw file):
"/foo/robots/HAL9000". @param path a "/"-separated string indicating the path in the scene tree. @param X_PathParent the relative transform from the path to it's immediate
nit: This is a very surprising name. For example, assume I have the object O with path /foo. It's parent is the world. Traditionally, we'd think about defining that as X_WO (pose of O w.r.t. W). However, you have that implicitly defined here as X_OW where O is "path" and W is parent.
The poses you have in meshcat_manual_test.cc seem to go the other way (e.g. sphere with pose <-2, 0, 0> is the sphere's origin in the world frame: X_WS).
Based on this, it seems to me that it's only the name of the variable that is flawed.
geometry/meshcat.h, line 72 at r4 (raw file):
Deleting the root node "/" does nothing. @param path a "/"-separated string indicating the path in the scene tree. */
nit: Per our f2f, I'm unclear about the semantics of what the "/" path means and what it effect it should have. Expectation is that it clears the world.
geometry/meshcat.h, line 81 at r4 (raw file):
will turn off the background. Note: Meshcat appends an extra path element with the name <object> to every
As I read this, if I call SetObject("my/path", ...); I can't use the same "my/path" string to set properties? Is that what this comment means?
Presumably this also applies to SetTransform? Can I set the transfrom of the object or just its "group" that it is in?
Alternatively, can we set properties on anything that doesn't end in ""? Does making this detail public help?
geometry/meshcat.h, line 115 at r4 (raw file):
// set_control. /** @name (Advanced)
BTW Have you considered:
a) Private with friend access?
b) Excluding these from rendered doxygen?
geometry/meshcat.cc, line 72 at r4 (raw file):
While this is a valid effort, I think it's a bridge too far. The foundational statement in question:
structs should be used for passive objects that carry data.
Your "helpers" aren't helpers, they are full pieces of logic that are deserving of unit tests themselves. You've got members (e.g., children that can't really be meaningfully interacted with directly, but only through operator[] and at(). You even change type representation (the container has unique_ptr<Element> but your methods return Element&. The Send() method is the final nail in the struct coffin -- that's not part of a "passive object". This is definitely no longer a struct.
However, looking at how this class is used below, I feel you lose very little in making the change to class.
geometry/meshcat.cc, line 87 at r4 (raw file):
// the `path`. Adds new elements if they do not exist. Comparable to // std::map::operator[]. SceneTreeElement& operator[](std::string_view path) {
BTW I almost wonder if all of this wouldn't benefit if you had a formal concept of ScenePath and wrapped up all of the string manipulation there. As it is, you seem to have a lot of string manipulation in walking this graph.
geometry/meshcat.cc, line 88 at r4 (raw file):
// std::map::operator[]. SceneTreeElement& operator[](std::string_view path) { if (!path.empty() && path.front() == '/') {
BTW: You're paying this cost at each level of recursion although you only need it at the initial call -- each recursive call skips the terminating slash, so (barring strings that look like "a//weird//path", you aren't getting a leading slash in recursion. Again, more weight in favor of a ScenePath class that can really focus in on validating paths and breaking them apart, etc.
geometry/meshcat.cc, line 122 at r4 (raw file):
std::string name(path.substr(0, loc)); auto child = children.find(name); DRAKE_THROW_UNLESS(child != children.end());
nit: This should be a proper throw with a message. If the end user asks for about a bad path, this line isn't going to provide much insight. Admittedly, located here the best message they can get is whatever portion of the original path deviated from reality. But that's still better.
Alternatively, you might consider returning a pointer that may be null and allow the original caller to throw the message where they can include the full, user-provided path.
geometry/meshcat.cc, line 277 at r4 (raw file):
internal::GeometryData& geometry = lumped.geometries[0]; // TODO(russt): Use filename as uuid, and avoid resending meshes unless
BTW If you use the file name, you should also capture the file modification date so we don't fall into the drake_visualizer trap of needing to restart the visualizer every time an obj file changes. Probably worth adding that to the TODO.
geometry/meshcat.cc, line 296 at r4 (raw file):
} int size = input.tellg();
Is this simply dumping the ascii obj contents into the websocket? If so, could you document that here, it is otherwise not obvious.
geometry/meshcat.cc, line 359 at r4 (raw file):
void set_prefix(std::string_view prefix) { DRAKE_DEMAND(std::this_thread::get_id() == main_thread_id_); if (!prefix.empty() && prefix.front() == '/') {
nit: These should be while loops to handle ///oops///.
geometry/meshcat.cc, line 368 at r4 (raw file):
} void SetObject(std::string_view path, const Shape& shape, const Rgba& rgba) {
Checkpoint for myself
geometry/test/meshcat_manual_test.cc, line 44 at r4 (raw file):
- The background should be off (white).\n"; - From left to right along the x axis, you should see:
BTW For the record, when I launched this, the default camera positions was looking down the -x axis. Therefore, there was no "left to right", it was back to front.
geometry/test/meshcat_manual_test.cc, line 48 at r4 (raw file):
- a green cylinder (with the long axis in z) - a pink semi-transparent ellipsoid (long axis in z) - a blue box (long axis in z)
nit: Your blue box's long axis is not in the z. It's in the y. We've got a mapping problem.
RussTedrake
force-pushed
the
meshcat_api
branch
from
0982a31
to
0ffc214
Compare
|
geometry/meshcat.h, line 19 at r4 (raw file): Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Let's agree on this before I move on. I've written up the proposed semantics for this PR... I'll remove the set/get prefix for now so that we can lock in the basic semantics without dealing with that small additional complexity. Let me know what you think? |
There was a problem hiding this comment.
Reviewed 1 of 5 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 26 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @RussTedrake)
geometry/meshcat.h, line 19 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Let's agree on this before I move on. I've written up the proposed semantics for this PR... I'll remove the set/get prefix for now so that we can lock in the basic semantics without dealing with that small additional complexity. Let me know what you think?
On the whole, I think it's good. As I put my devil's advocated hat on, I can come up with a number of questions. I'm marking myself satisfied here so that those specific comments can each serve their unique purpose without dragging this along.
geometry/meshcat.h, line 81 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
As I read this, if I call
SetObject("my/path", ...);I can't use the same "my/path" string to set properties? Is that what this comment means?Presumably this also applies to
SetTransform? Can I set the transfrom of the object or just its "group" that it is in?Alternatively, can we set properties on anything that doesn't end in ""? Does making this detail public help?
(The
""in that previous comment was supposed to be"<object>", burned my markdown.)Also, I've since learned I can apply transforms to
my/pathbut properties must be assigned tomy/path/<object>. Is it worth simply appending<object>under the hood as part of the API? That way the path the used inSetObjectis the same path they'd use inSetPropertyand this confusion clears up.
geometry/meshcat.h, line 14 at r5 (raw file):
namespace geometry { /** Provides an interface to Meshcat (https://github.com/rdeits/meshcat).nit
%Meshcat-- in this case, you don't want it to be a link to the class.
geometry/meshcat.h, line 20 at r5 (raw file):
scene. Note that, unlike many visualizers, one cannot open the visualizer until this server is running.BTW Use of
@sectionhas interesting layout implications. The "section" is bigger than the summary, the initial paragraph, and just about everything else.
geometry/meshcat.h, line 21 at r5 (raw file):
this server is running. @section meshcat_path Meshcat paths and the scene treeI think there needs to be some explicit mention that adding objects is a bit different. The "path" behaves more like directories and the object added gets a generated name. This matters because I need to use that generated name if I intend to tweak/delete that geometry (and not the whole folder its in).
geometry/meshcat.h, line 38 at r5 (raw file):
Do we allow it to be relative to anything other than the working directory listed below? I'd suggest a bit of elaboration:A path that does not begin with "/" is treated as a relative path to the current working directory.
(By the way, if we bring bag "set prefix", that is essentially the
cdcommand -- by setting it to/drake/thing/foothen any relative path will be in that current working directory.)
geometry/meshcat.h, line 39 at r5 (raw file):
without modification. - A path that *does not* begin with "/" is treated as a relative path. - The "working directory" is fixed to be "/drake" in the current implementation.Is there any guidance about the advisability of providing absolute paths that don't include
/drake? It seems there's a basic implication that that's a bad thing, but they are fully empowered to do whatever they want. Are there any implications? Is there a benefit to have the working directory set?
geometry/meshcat.h, line 41 at r5 (raw file):
- The "working directory" is fixed to be "/drake" in the current implementation. So any relative path "foo" will be treated as "/drake/foo". - Delete("/foo") will remove all objects, transforms, and properties in "/foo"BTW
Delete()becomes a nice hyperlink to the function. If you swapped:
Delete("/foo")for@ref Delete "Delete(\"/foo\")", then you'll also get a nice hyperlink for that reference as well. The downside is that it's not as smoothly readable in code. I leave it up to you how much you might want hyperlinks in the rendered docs.Same for
Delete("")andDelete("/drake").
geometry/meshcat.h, line 43 at r5 (raw file):
- Delete("/foo") will remove all objects, transforms, and properties in "/foo" and its children from the scene tree. - Delete() is equivalent to Delete("") or Delete("/drake").BTW The semantics of this last is a bit regrettable. Up to this point, there's a reasonable correlation with file path operations. But this one breaks down -- deleting the current working directory, while in it.
Given the strong analogy with
filesystem, I feel that every deviation is a potential source for confusion.This is just a passing thought. I'm not sure how to resolve this (if we can), but would suspect it's somewhere in the direction of limiting the number of equivalent ways of doing one thing, particularly if some of the overloads go against the "file system" paradigm.
geometry/meshcat.h, line 46 at r5 (raw file):
It is considered good practice to add your geometry using relative paths, so that Delete() can clear all of the geometry from the scene.BTW If this is the justification for the
/drakeprefix, I don't think I'm sold.Delete()without arguments feels a bit shady. It somehow feels like it'd be better explicit by method name or by argument `Delete("*") (as a special argument). Just thinking aloud here.
geometry/meshcat.h, line 50 at r5 (raw file):
The root directory contains a number of elements that are set up automatically in the browser. These include "/Background", "/Lights", "/Grid", and "/Cameras". See the meshcat documentation for their details.Is there a ready link for this documentation?
geometry/meshcat.h, line 51 at r5 (raw file):
in the browser. These include "/Background", "/Lights", "/Grid", and "/Cameras". See the meshcat documentation for their details. - You can modify these elements, create new lights/cameras, and even deleteIs this something we want to enforce? Geometries can only be added to
/drake? Lights, cameras, etc. can only be added to/? That might provide a clean division between "visualized content" and "visualized configuration".Alternatively, I can imagine segregating those things that
Meshcatconfigures on its own and those things the user sets up via calls toMeshcat's public API. Of course, that leaves the question open, where do user configured lights go? Should they persist if I delete the geometry in the scene? What if I modify the properties of one ofMeshcat's lights? etc.I can also imagine that with this string-based, it would be easy to say "Lights" instead of "/Lights" and, depending on the operation, it might easily become a no-op, or have unintended consequences.
All of this is just thinking aloud.
geometry/meshcat.h, line 79 at r5 (raw file):
@param rgba an Rgba that specifies the (solid) color of the object. */ void SetObject(std::string_view path, const Shape& shape,What happens if I assign to different shapes to the same path?
geometry/meshcat.cc, line 122 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This should be a proper throw with a message. If the end user asks for about a bad path, this line isn't going to provide much insight. Admittedly, located here the best message they can get is whatever portion of the original
pathdeviated from reality. But that's still better.Alternatively, you might consider returning a pointer that may be null and allow the original caller to throw the message where they can include the full, user-provided path.
FTR If you end up doing that, you can eliminate the
Containsmethod entirely --At()would provide the full functionality as reported by its return type.
geometry/meshcat.cc, line 369 at r5 (raw file):
void SetObject(std::string_view path, const Shape& shape, const Rgba& rgba) { DRAKE_DEMAND(std::this_thread::get_id() == main_thread_id_);BTW Is the name
main_thread_is_a bit misleading? It's whichever thread theMeshcatinstance was created in.We should probably be clear in the documentation of
Meshcat, that calls to its API that configure the world should all be performed in the same thread in which the instance was created.
geometry/meshcat.cc, line 473 at r5 (raw file):
data.value = value; std::stringstream message;BTW It's not obvious to me, but why can't the message packing go inside the lambda? Then you could simply do this:
loop_->defer([this, prop_data = std::move(data)]() { std::stringstream message; msgpack::pack(message, data); app->publish("all", message.str(), uWS::OpCode::BINARY, false); SceneTreeElement& e = scene_tree_root_[data.path]; e.properties[prop_data.property] = message.str(); });This limits the number of strings you're copying because the path and property strings would simply be moved into the lambda.
geometry/meshcat.cc, line 478 at r5 (raw file):
// Note: pass all temporaries by value. loop_->defer( [this, app = app_, path = data.path, property, msg = message.str()]() {nit: (Meta) the abbreviation
msgis not style-guide compliant. (I know, I've recently been burned on this exact same abbreviation.)
geometry/meshcat.cc, line 621 at r5 (raw file):
std::string FullPath(std::string_view path) const { DRAKE_DEMAND(std::this_thread::get_id() == main_thread_id_);This guard makes great sense if we actually attempt to write anything to the meshcat web socket -- but do we need it just to do string mangling?
And if that's the case, better as
staticrather thanconst?
geometry/meshcat.cc, line 622 at r5 (raw file):
std::string FullPath(std::string_view path) const { DRAKE_DEMAND(std::this_thread::get_id() == main_thread_id_); if (static_cast<int>(path.size()) > 1 && path.back() == '/') {nit
static_castisn't necessary here.
RussTedrake
force-pushed
the
meshcat_api
branch
from
0ffc214
to
f8dd04d
Compare
There was a problem hiding this comment.
Reviewable status: 16 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @gizatt and @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
The colors are all super saturated. It seems like there's no specularity and probably an ambient light cranked up pretty high. It leaves the objects looking very flat and monochromatic.
Not sure what to do about it here.. would of course love your help to improve it. (perhaps in a follow-up?)
geometry/meshcat.h, line 38 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This isn't strictly true. The default prefix is
drake. But what is added to any path that does not begin with/is/drake. Which gets returned?
N/A
geometry/meshcat.h, line 43 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW And the prefix "my/deep/prefix" is considered valid, right?
N/A
geometry/meshcat.h, line 47 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Meta-note: Do you really want to use the term "separated" as opposed to "delimited" or some such thing? "Separated"
it's copied right out of the meshcat documentation. but i guess i can change it here.
geometry/meshcat.h, line 51 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Based on the comments on
SetProperty, you might consider returning the "object" string and define its relationship between the "group" name that is given as a parameter and the name of the object itself.
Done. I actually implemented a version that returned the absolute_path, but by the time I had updated the tests, etc, I decided I didn't like it. It deviates too much from the other patterns. I've returned to simply documenting the idea for now.
For the record, in all of my uses of meshcat, I have never referenced the "" field directly. It's nice that I can, but it's not common.
geometry/meshcat.h, line 61 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This is a very surprising name. For example, assume I have the object O with path
/foo. It's parent is the world. Traditionally, we'd think about defining that asX_WO(pose of O w.r.t. W). However, you have that implicitly defined here asX_OWwhere O is "path" and W is parent.The poses you have in
meshcat_manual_test.ccseem to go the other way (e.g. sphere with pose <-2, 0, 0> is the sphere's origin in the world frame: X_WS).Based on this, it seems to me that it's only the name of the variable that is flawed.
Oops. You're right. I somehow crossed my wires here!
geometry/meshcat.h, line 72 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Per our f2f, I'm unclear about the semantics of what the "/" path means and what it effect it should have. Expectation is that it clears the world.
I believe this has been sufficiently addressed / discussed.
geometry/meshcat.h, line 81 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
(The
""in that previous comment was supposed to be"<object>", burned my markdown.)Also, I've since learned I can apply transforms to
my/pathbut properties must be assigned tomy/path/<object>. Is it worth simply appending<object>under the hood as part of the API? That way the path the used inSetObjectis the same path they'd use inSetPropertyand this confusion clears up.
No. You can set properties to either the path (e.g. "visible" = "false" will hide all objects in this path including children) or to the object itself.
geometry/meshcat.h, line 115 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Have you considered:
a) Private with friend access?
b) Excluding these from rendered doxygen?
I considered (a), but have multiple test sites and thought it was too cumbersome. I will do (b).
geometry/meshcat.h, line 20 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Use of
@sectionhas interesting layout implications. The "section" is bigger than the summary, the initial paragraph, and just about everything else.
You do realize that I followed your example from SceneGraph, which renders the same> I'll mark myself satisfied.
geometry/meshcat.h, line 21 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I think there needs to be some explicit mention that adding objects is a bit different. The "path" behaves more like directories and the object added gets a generated name. This matters because I need to use that generated name if I intend to tweak/delete that geometry (and not the whole folder its in).
Done.
geometry/meshcat.h, line 38 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Do we allow it to be relative to anything other than the working directory listed below? I'd suggest a bit of elaboration:
A path that does not begin with "/" is treated as a relative path to the current working directory.
(By the way, if we bring bag "set prefix", that is essentially the
cdcommand -- by setting it to/drake/thing/foothen any relative path will be in that current working directory.)
Done (updating the comment). The analogy to cd was not lost on me. It also made me realize that it's a potential can of worms, which is why I've pulled it out of this PR.
geometry/meshcat.h, line 39 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Is there any guidance about the advisability of providing absolute paths that don't include
/drake? It seems there's a basic implication that that's a bad thing, but they are fully empowered to do whatever they want. Are there any implications? Is there a benefit to have the working directory set?
I started writing even more last night, but then removed it. More documentation is not always better (I think there are a lot of great details in our documentation that nobody ever reads because it's too verbose).
To answer your question, it's fine if you want to send all of your geometry to /sean/, therefore effectively making it your pwd. The only difference is that Delete() won't delete it.
This is what makes Delete("/") so tricky. It's tempting to support it, but consider the details. One fact is that if I send that command verbatim over the websocket to javascript, it gets silently ignored. I can't change that on this PR. I could choose to implement something different on the c++ meshcat side, but again it gets subtle. The most reasonable option would probably be:
- For the root node children, I send
Delete("/child1")...Delete("/childN)"messages to the client, and then simply remove them from my client side tree. - If there is an object or transform on the root node tree, I would send
Delete("/<object>")and/orSetTransform("/", Identity(), respectively. - If there are properties on the root tree (and there may well be), then I would not send anything to the server, but only remove the properties from the local tree. That means that the properties I've set before would not change properties on newly connected clients. But it would not unset properties on the connected clients, because to do that I would have to reason about the default property values in javascript and that, I fear, will lead to brittleness and heartache.
So I've decided to say instead that Delete("/") is forbidden, which, again, matches the decision made by the javascript meshcat.
geometry/meshcat.h, line 41 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW
Delete()becomes a nice hyperlink to the function. If you swapped:
Delete("/foo")for@ref Delete "Delete(\"/foo\")", then you'll also get a nice hyperlink for that reference as well. The downside is that it's not as smoothly readable in code. I leave it up to you how much you might want hyperlinks in the rendered docs.Same for
Delete("")andDelete("/drake").
I'm aware, and consciously chose this path.
geometry/meshcat.h, line 43 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW The semantics of this last is a bit regrettable. Up to this point, there's a reasonable correlation with file path operations. But this one breaks down -- deleting the current working directory, while in it.
Given the strong analogy with
filesystem, I feel that every deviation is a potential source for confusion.This is just a passing thought. I'm not sure how to resolve this (if we can), but would suspect it's somewhere in the direction of limiting the number of equivalent ways of doing one thing, particularly if some of the overloads go against the "file system" paradigm.
As @gizatt pointed out earlier in this PR, using meshcat.Delete() to feel like you have effectively reset the state of the visualizer is a very useful concept. And I would say it's almost the entire reason for the prefix concept.
There is a mismatch between the filesystem concept and the javascript meshcat tree. I'm not fixing that here. You perceive the cost of differing from the filesystem concept, but I'm more worried about the cost of differing from the meshcat tree concept. People love meshcat with it's current implementation and have been able to reason about differences from filesystem. People will continue to use meshcat from other languages, and I don't want to invent something entirely new. And anything I do to send multiple/different commands than the user spells explicitly will lead to brittleness as meshcat continues to evolve.
I also believe that the current meshcat concepts are not arbitrary, but were reasoned out carefully and have been somewhat battle tested. You and I are unlikely to do better reinventing things from scratch.
geometry/meshcat.h, line 46 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW If this is the justification for the
/drakeprefix, I don't think I'm sold.Delete()without arguments feels a bit shady. It somehow feels like it'd be better explicit by method name or by argument `Delete("*") (as a special argument). Just thinking aloud here.
It's the main justification. It also help keep the tree in the gui a bit more organized by default.
There are only a few relevant pieces of code in javascript. First, the path string gets split into an array:
https://github.com/rdeits/meshcat/blob/bf523ecd3662d64b2320e17fc319d91249f7b82b/src/index.js#L1079
The filter command means "/foo//bar" == "/foo/bar". "/drake/" == "/drake". And "/" returns an path array of length 0. Deleting a path with length 0 leads to a console message, but for the user it appears silently ignored.
https://github.com/rdeits/meshcat/blob/master/src/index.js#L936
Then the actual deletion happens here:
https://github.com/rdeits/meshcat/blob/master/src/index.js#L379
Again, your proposal sounds reasonable, but I think it leads to problems. By our rules Delete("") converts to Delete("/drake/") == Delete("/drake") so it should delete that directory. Not doing so would be too big of a gap from the meshcat javascript implementation, I think.
I could maybe see supporting "/foo/" on our side, which would be equivalent to "/foo" in every case except "/" which could do the more complicated work I described above. But this is adding more ways to do the same thing, not less, so goes against the principle you stated above.
geometry/meshcat.h, line 50 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Is there a ready link for this documentation?
Done. It was just above. I've added it here, too.
geometry/meshcat.h, line 51 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Is this something we want to enforce? Geometries can only be added to
/drake? Lights, cameras, etc. can only be added to/? That might provide a clean division between "visualized content" and "visualized configuration".Alternatively, I can imagine segregating those things that
Meshcatconfigures on its own and those things the user sets up via calls toMeshcat's public API. Of course, that leaves the question open, where do user configured lights go? Should they persist if I delete the geometry in the scene? What if I modify the properties of one ofMeshcat's lights? etc.I can also imagine that with this string-based, it would be easy to say "Lights" instead of "/Lights" and, depending on the operation, it might easily become a no-op, or have unintended consequences.
All of this is just thinking aloud.
I hear you. But I am not writing a visualizer in this PR. I am writing a wrapper to send messages to an existing visualizer. These are questions about the visualizer itself.
geometry/meshcat.h, line 79 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
What happens if I assign to different shapes to the same path?
Done.
geometry/meshcat.cc, line 72 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
While this is a valid effort, I think it's a bridge too far. The foundational statement in question:
structs should be used for passive objects that carry data.
Your "helpers" aren't helpers, they are full pieces of logic that are deserving of unit tests themselves. You've got members (e.g.,
childrenthat can't really be meaningfully interacted with directly, but only throughoperator[]andat(). You even change type representation (the container hasunique_ptr<Element>but your methods returnElement&. TheSend()method is the final nail in the struct coffin -- that's not part of a "passive object". This is definitely no longer a struct.However, looking at how this class is used below, I feel you lose very little in making the change to class.
Done. I wasn't worried about losing anything. Just the silliness of having to add access methods.
geometry/meshcat.cc, line 88 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW: You're paying this cost at each level of recursion although you only need it at the initial call -- each recursive call skips the terminating slash, so (barring strings that look like "a//weird//path", you aren't getting a leading slash in recursion. Again, more weight in favor of a
ScenePathclass that can really focus in on validating paths and breaking them apart, etc.
As described above, the javascript handles "//a//weird//path", so my intention was to provide the same support here. I've changed it to a while loop, though.
geometry/meshcat.cc, line 122 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
FTR If you end up doing that, you can eliminate the
Containsmethod entirely --At()would provide the full functionality as reported by its return type.
That's a good change... although I've changed to Find instead to keep some parity with std::map. In fact, it removes the need to throw completely.
geometry/meshcat.cc, line 277 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW If you use the file name, you should also capture the file modification date so we don't fall into the
drake_visualizertrap of needing to restart the visualizer every time an obj file changes. Probably worth adding that to the TODO.
Good call. I actually used the file contents in the python meshcat_visualizer for exactly this reason. I had simply forgotten for the moment.
geometry/meshcat.cc, line 296 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Is this simply dumping the ascii obj contents into the websocket? If so, could you document that here, it is otherwise not obvious.
Done.
geometry/meshcat.cc, line 359 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: These should be
whileloops to handle///oops///.
Done.
geometry/meshcat.cc, line 620 at r4 (raw file):
Previously, gizatt (Greg Izatt) wrote…
nit Could use brief explanatory comment that this assembles prefix and path (depending on details of each), or name change to something obvious.
Done. Thought it turned into a small document.
geometry/meshcat.cc, line 369 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Is the name
main_thread_is_a bit misleading? It's whichever thread theMeshcatinstance was created in.We should probably be clear in the documentation of
Meshcat, that calls to its API that configure the world should all be performed in the same thread in which the instance was created.
Done. Added a comment to the variable declaration, and a note in the public class documentation.
geometry/meshcat.cc, line 473 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW It's not obvious to me, but why can't the message packing go inside the lambda? Then you could simply do this:
loop_->defer([this, prop_data = std::move(data)]() { std::stringstream message; msgpack::pack(message, data); app->publish("all", message.str(), uWS::OpCode::BINARY, false); SceneTreeElement& e = scene_tree_root_[data.path]; e.properties[prop_data.property] = message.str(); });This limits the number of strings you're copying because the path and property strings would simply be moved into the lambda.
I like that! As a principle, I've been trying to keep any heavy lifting outside of the websocket loop (so that it can be responsive to incoming connections/messages). But avoiding copying the data, especially when it's a meshfile, seems like a massive win.
geometry/meshcat.cc, line 478 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: (Meta) the abbreviation
msgis not style-guide compliant. (I know, I've recently been burned on this exact same abbreviation.)
Done.
geometry/meshcat.cc, line 621 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
This guard makes great sense if we actually attempt to write anything to the meshcat web socket -- but do we need it just to do string mangling?
And if that's the case, better as
staticrather thanconst?
Done. I had it this way because it touches prefix_, which must only be consumed in main. But now (and perhaps forever), I can mark prefix_ const, so that removes this concern.
geometry/meshcat.cc, line 622 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit
static_castisn't necessary here.
Done.
geometry/test/meshcat_manual_test.cc, line 48 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Your blue box's long axis is not in the z. It's in the y. We've got a mapping problem.
Done. Indeed. From https://threejs.org/docs/index.html?q=BoxGeom#api/en/geometries/BoxGeometry we have
height — Height; that is, the length of the edges parallel to the Y axis.
depth — Depth; that is, the length of the edges parallel to the Z axis.
I had already fixed the cylinder, and don't see others that will be problematic.
|
a discussion (no related file): Previously, RussTedrake (Russ Tedrake) wrote…
(I suspect you would say the same thing about boxes, etc, drawn in meshcat python?). perhaps i've exaggerated the effect here by turning off the background? |
There was a problem hiding this comment.
Reviewable status: 17 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @gizatt, @RussTedrake, and @SeanCurtis-TRI)
a discussion (no related file):
pardon my ignorance @RussTedrake, will this new feature enable the 2D viz we were talking about last week? thanks.
|
@amcastro-tri — yes. One of the ways that i do planar visualization is to
out meshcat into orthographic mode. Check out the 2d teleop example in the
intro chapter of my manipulation notes.
I will be adding the SetPlanar method here soon, but after this one lands
the capability is technically there
…On Tue, Aug 24 2021 at 10:20 AM, Alejandro Castro ***@***.***> wrote:
***@***.**** commented on this pull request.
*Reviewable <https://reviewable.io/reviews/robotlocomotion/drake/15647>*
status: 17 unresolved discussions, LGTM missing from assignee
SeanCurtis-TRI(platform) (waiting on @gizatt <https://github.com/gizatt>,
@RussTedrake <https://github.com/RussTedrake>, and @SeanCurtis-TRI
<https://github.com/SeanCurtis-TRI>)
*a discussion
<https://reviewable.io/reviews/robotlocomotion/drake/15647#-MhsWw0e9KYniWr6QVyW:-MhsWw0e9KYniWr6QVyX:b-z7igpd>
(no related file):*
pardon my ignorance @RussTedrake <https://github.com/RussTedrake>, will
this new feature enable the 2D viz we were talking about last week? thanks.
------------------------------
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15647 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRE2NHMNBBAHIVMZKL25TDT6OTB7ANCNFSM5CSXQMXA>
.
|
There was a problem hiding this comment.
Full pass done
Reviewed 5 of 11 files at r1, 1 of 3 files at r2, 5 of 5 files at r6, all commit messages.
Reviewable status: 16 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @RussTedrake)
a discussion (no related file):
Previously, RussTedrake (Russ Tedrake) wrote…
(I suspect you would say the same thing about boxes, etc, drawn in meshcat python?). perhaps i've exaggerated the effect here by turning off the background?
I'll go ahead and look at it myself. :)
a discussion (no related file):
@RussTedrake your comments have informed in something I had failed to infer on my own: Meshcat is not a visualizer that happens to be implemented via meshcat (a la RenderEngineVtk); it is the injection of Meshcat into Drake. As such, it is designed to be a thin wrapper on the APIs and concepts implemented in meshcat. That's a very different lens for me. My lack of familiarity with the details of meshcat precluded my ability to discern what was "Just part of meshcat" and what was Drake's contribution/abstraction.
As I readjust my sailing direction, accordingly, I feel it shifts the purpose/burden of the documentation:
- We should be very clear about the "thin wrapper-ness" of the class with the design goal that familiarity with
meshcatshould imply familiarity withMeshcat. That is the class's purpose. - We should also be very clear where we've added any kind of arbitrary abstraction (e.g.,
/drakethat isn't vanillameshcat). - Finally, the documentation burden should primarily consist of links to
meshcatdocumentation, where possible. Where it isn't possible (or incomplete), we should shore it up but indicate that where we are merely elaborating on undocumentedmeshcatbehavior (again, in contrast with Drake-side design decisions).
geometry/BUILD.bazel, line 353 at r6 (raw file):
"exclude_from_package", ], visibility = ["//:__subpackages__"],
nit: The purpose of subpackage availability isn't clear to me.
geometry/meshcat.h, line 47 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
it's copied right out of the meshcat documentation. but i guess i can change it here.
I strongly regret making this comment. Again, with the thin-wrapper argument, using the meshcat language is a positive boon. Changing this to delimited is a step backwards from that perspective. Sorry about that. You can leave it as is, or change it back. I completely defer to you.
geometry/meshcat.h, line 81 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
No. You can set properties to either the path (e.g. "visible" = "false" will hide all objects in this path including children) or to the object itself.
Riiight. Hence the example you gave of hiding "/Background".
geometry/meshcat.h, line 20 at r5 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
You do realize that I followed your example from SceneGraph, which renders the same> I'll mark myself satisfied.
Now you've learned the folly of following a fool. Your justification is certainly sufficient to make yourself satisfied; it just means I'll have to include SceneGraph in my list of classes whose layout torments me.
geometry/meshcat.h, line 38 at r5 (raw file):
In retrospect, given my improved understanding of your intent, my recommendation that it be relative to the "current working directory" was misguided. That is a concept that doesn't belong here (as we have no intent to support it). It would've been better as:
A path that does not being with "/" is treated as a path relative to the "/drake" container.
(If "container" is the right word. I'm still really unclear on what to call the various types of things in meshcat.)
Alternatively, if we want to maintain the analogy of "working directory" in the documentation (as in the next document), we really would just need to excise the word "current". We could replace it with "default".
geometry/meshcat.h, line 39 at r5 (raw file):
There are two topics in this conversation:
- Advice on workflow
- Semantics of
Delete().
On the first, I think we can compactly communicate the following:
Recommended workflow
For convenience,
Meshcatfosters a work flow in which all user-created objects created in Drake are contained in the "/drake" folder. Objects added with a relative path are placed in the "/drake" folder for you. The benefits are:
- It's simple to distinguish between user objects and "infrastructure" objects in the visualizer.
- All user objects can easily be cleared by a single, parameter-free call to
Delete().You are welcome to use absolute paths to organize your data, but the burden on tracking and cleaning them up lie on you.
On the second, we can just end the conversation with the appeal to "thin wrapper". We're just making the known semantics of meshcat available to C++ code. Done. If that's the way the "delete" operation works in meshcat, that's the way it works here.
geometry/meshcat.h, line 43 at r5 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
As @gizatt pointed out earlier in this PR, using
meshcat.Delete()to feel like you have effectively reset the state of the visualizer is a very useful concept. And I would say it's almost the entire reason for theprefixconcept.There is a mismatch between the filesystem concept and the javascript meshcat tree. I'm not fixing that here. You perceive the cost of differing from the filesystem concept, but I'm more worried about the cost of differing from the meshcat tree concept. People love meshcat with it's current implementation and have been able to reason about differences from filesystem. People will continue to use meshcat from other languages, and I don't want to invent something entirely new. And anything I do to send multiple/different commands than the user spells explicitly will lead to brittleness as meshcat continues to evolve.
I also believe that the current meshcat concepts are not arbitrary, but were reasoned out carefully and have been somewhat battle tested. You and I are unlikely to do better reinventing things from scratch.
I'm closing for this for the "thin-wrapper" argument made above: it simply wraps meshcat functionality as intended.
geometry/meshcat.h, line 46 at r5 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
It's the main justification. It also help keep the tree in the gui a bit more organized by default.
There are only a few relevant pieces of code in javascript. First, the path string gets split into an array:
https://github.com/rdeits/meshcat/blob/bf523ecd3662d64b2320e17fc319d91249f7b82b/src/index.js#L1079
The filter command means "/foo//bar" == "/foo/bar". "/drake/" == "/drake". And "/" returns an path array of length 0. Deleting a path with length 0 leads to a console message, but for the user it appears silently ignored.
https://github.com/rdeits/meshcat/blob/master/src/index.js#L936Then the actual deletion happens here:
https://github.com/rdeits/meshcat/blob/master/src/index.js#L379Again, your proposal sounds reasonable, but I think it leads to problems. By our rules Delete("") converts to Delete("/drake/") == Delete("/drake") so it should delete that directory. Not doing so would be too big of a gap from the meshcat javascript implementation, I think.
I could maybe see supporting "/foo/" on our side, which would be equivalent to "/foo" in every case except "/" which could do the more complicated work I described above. But this is adding more ways to do the same thing, not less, so goes against the principle you stated above.
I'm closing for this for the "thin-wrapper" argument made above: it simply wraps meshcat functionality as intended. (See my note above on "recommended workflow".
geometry/meshcat.h, line 50 at r5 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Done. It was just above. I've added it here, too.
I was hoping for something more specific. The text here says the link contains "their details". That link gives me very little information about details. What properties can I set? What values are meaningful, etc. That documentation says that lights exist, and things like /Lights?DiretionalLight and /Lights/AmbientLight are "useful paths", but I don't even know how to change the intensity of the ambient light. So, a link with limited value relative to the stated purpose.
geometry/meshcat.h, line 35 at r6 (raw file):
Elements of the tree are referenced programmatically by a "/"-delimited string indicating the object's **path** in the scene tree. An object at path "/foo/bar"
BTW: With a larger view the term "object" now seems sadly overloaded. Calling SetObject("my/path", ...) and SetObject("my/path/more", ...) creates two objects (in the meshcat nomenclature). The proper path to those objects are "my/path/" and "my/path/more/". However, "my/path" and "my/path/more" are separate things that likewise have properties. Are they also "objects"? Overloading "object" to mean both "my/path" and "my/path/" is unfortunate.
I presume meshcat doesn't discuss the path semantics explicitly (hence the generated documentation here rather than a link).
geometry/meshcat.h, line 108 at r6 (raw file):
parent. */ void SetTransform(std::string_view path,
The unit tests on this caught me completely by surprise. I had apparently glossed over the details in the .cc file.
My initial interpretation of this was that it's a way to pose the object I've added. I.e.,:
meshcat.SetObject(my_path, my_shape, my_color);
meshcat.SetTransform(my_path + "/<object>", X_PG);Here is what surprised me:
- This will create a "thing" at the given path if it doesn't currently exist.
- The thing created is not the same as what is created in calling
SetObject-- i.e., it will have different properties. - I can set properties on the thing created by this method, but the path to that thing is the same as the path I passed here (in stark contrast to SetObject).
- If I call
SetTransform(my_path + "/<object>", X_PG)without having calledSetObject(my_path,...), I will get a "transform" object that is named like an "object" object but isn't.
Presumably, this is all common knowledge for meshcat users, right? I assume it's not actually documented and there is nothing to reference?
geometry/meshcat.cc, line 88 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
As described above, the javascript handles "//a//weird//path", so my intention was to provide the same support here. I've changed it to a while loop, though.
Good point. I'd considered the "///my/path///" option but forgot about the internal duplications. :)
geometry/meshcat.cc, line 122 at r4 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
That's a good change... although I've changed to
Findinstead to keep some parity with std::map. In fact, it removes the need to throw completely.
I agree. Find is much better.
geometry/meshcat.cc, line 75 at r6 (raw file):
// Member access methods (object_, transform_, and properties_ should be // effectively public). const std::optional<std::string>& object() const { return object_; }
BTW As I read this code, the way object() and transform() are used, when they are std::nullopt, you use the empty string. You could just make these strings that default to the empty string value and use it blindly.
geometry/meshcat.cc, line 403 at r6 (raw file):
void SetTransform(std::string_view path, const RigidTransformd& X_PathParent) {
nit: You'll want to a more global search and replace on X_PathParent.
geometry/meshcat.cc, line 463 at r6 (raw file):
// Note: pass all temporaries by value. loop_->defer([this, data = std::move(data)]() {
btw: I would be unsurprised if gcc complains about variable shadowing. But if this works, huzzah! Nowadays, I go out of my way to keep my names different because of the times gcc has barked at me.
geometry/meshcat.cc, line 466 at r6 (raw file):
std::stringstream message_stream; msgpack::pack(message_stream, data); std::string message = message_stream.str();
BTW Good call on this. I was overly generous in my assumption of how stringstream::str() worked. It definitely returns a copy. I like this (particularly with the accompanying move).
geometry/shape_specification.h, line 358 at r6 (raw file):
... void ImplementGeometry(const Sphere& sphere, void* data) override { DRAKE_ASSERT(data != nullptr);
BTW Thanks!
geometry/test/meshcat_test.cc, line 99 at r6 (raw file):
GTEST_TEST(MeshcatTest, SetTransform) { Meshcat meshcat; EXPECT_FALSE(meshcat.HasPath("transform"));
BTW Calling this object "frame" (instead of "transform") might be less misleading. The phrase meshcat.SetTransform("transform", X_PP) might be confusing to a user that comes looking to the test for examples of exercising the API. Why does "transform" appear twice? Is this like a property? Etc.
Same note applies to the path name "transform" below.
Follow up -- you can probably dismiss this out of hand. This comment was born out of not understanding that a transform is both a node in the graph as well as a property of a node. I'm leaving it here so you can see that I was surprised, but I now assume that this is merely importing the meshcat design via the thin-wrapper.
geometry/test/meshcat_test.cc, line 101 at r6 (raw file):
EXPECT_FALSE(meshcat.HasPath("transform")); EXPECT_TRUE(meshcat.GetPackedTransform("transform").empty()); const RigidTransformd X_PathParent{math::RollPitchYawd(.5, .26, -3),
nit: You have some X_PathParent relics left over here.
geometry/test/meshcat_test.cc, line 119 at r6 (raw file):
// Ok to delete an empty tree. meshcat.Delete(); EXPECT_FALSE(meshcat.HasPath(""));
nit: This implies that "/drake" doesn't exist until someone adds something. Is that what the user should expect? Or should we simply add "/drake" as part of the constructor? I guess the flipside of that is that Delete() would leave "/drake" intact (which we're not doing).
So, the fact that it doesn't initially exist and gets removed after Delete() is consistent. But given that the whole /drake folder is an abstraction implemented in our wrapper, we actually do have a choice to decide what its lifespan is.
If we stick with this behavior, we should tweak the documentation about the /drake container to make it clear that it won't exist until they call SetTransform or SetObject with a relative path (or with an absolute path that starts with /drake).
There was a problem hiding this comment.
Reviewable status: 16 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @RussTedrake)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'll go ahead and look at it myself. :)
BTW I would've thought that
meshcat.SetProperty("/Lights/AmbientLight", "intensity", 0.1);
would work. I added it to meshcat_manula_test.cc and it made no difference at all. That's obviously not getting broadcast appropriately. I'll leave it there for you.
RussTedrake
force-pushed
the
meshcat_api
branch
from
f8dd04d
to
17ac4f2
Compare
There was a problem hiding this comment.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @amcastro-tri, @gizatt, and @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I would've thought that
meshcat.SetProperty("/Lights/AmbientLight", "intensity", 0.1);would work. I added it to
meshcat_manula_test.ccand it made no difference at all. That's obviously not getting broadcast appropriately. I'll leave it there for you.
I almost don't want to tell you the fix.
meshcat.SetProperty("/Lights/AmbientLight/<object>", "intensity", 0.1);
:gulp: It would have been obvious if you had looked at the tree in the gui.
a discussion (no related file):
Previously, amcastro-tri (Alejandro Castro) wrote…
pardon my ignorance @RussTedrake, will this new feature enable the 2D viz we were talking about last week? thanks.
Yes. One of the ways that I visualize things in 2D is using meshcat with the camera set to orthographic mode. You can see an example if you run the 2D teleop in https://colab.research.google.com/github/RussTedrake/manipulation/blob/master/intro.ipynb .
Once this PR lands it will be possible to do that in c++. A forthcoming PR will add some sugar to make it easy.
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
@RussTedrake your comments have informed in something I had failed to infer on my own:
Meshcatis not a visualizer that happens to be implemented viameshcat(a laRenderEngineVtk); it is the injection ofMeshcatinto Drake. As such, it is designed to be a thin wrapper on the APIs and concepts implemented inmeshcat. That's a very different lens for me. My lack of familiarity with the details ofmeshcatprecluded my ability to discern what was "Just part ofmeshcat" and what was Drake's contribution/abstraction.As I readjust my sailing direction, accordingly, I feel it shifts the purpose/burden of the documentation:
- We should be very clear about the "thin wrapper-ness" of the class with the design goal that familiarity with
meshcatshould imply familiarity withMeshcat. That is the class's purpose.- We should also be very clear where we've added any kind of arbitrary abstraction (e.g.,
/drakethat isn't vanillameshcat).- Finally, the documentation burden should primarily consist of links to
meshcatdocumentation, where possible. Where it isn't possible (or incomplete), we should shore it up but indicate that where we are merely elaborating on undocumentedmeshcatbehavior (again, in contrast with Drake-side design decisions).
Ok.
geometry/BUILD.bazel, line 353 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: The purpose of subpackage availability isn't clear to me.
I guess you're right. I want to avoid installing the headers, but I guess it's ok to use them internally. (I'm not sure if bazel is protecting us from another installed header doing a "#include" on this header...?)
geometry/meshcat.h, line 47 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I strongly regret making this comment. Again, with the thin-wrapper argument, using the meshcat language is a positive boon. Changing this to delimited is a step backwards from that perspective. Sorry about that. You can leave it as is, or change it back. I completely defer to you.
I think it's fine.
geometry/meshcat.h, line 38 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
In retrospect, given my improved understanding of your intent, my recommendation that it be relative to the "current working directory" was misguided. That is a concept that doesn't belong here (as we have no intent to support it). It would've been better as:
A path that does not being with "/" is treated as a path relative to the "/drake" container.
(If "container" is the right word. I'm still really unclear on what to call the various types of things in meshcat.)
Alternatively, if we want to maintain the analogy of "working directory" in the documentation (as in the next document), we really would just need to excise the word "current". We could replace it with "default".
Done. Changed "current" to "default"
geometry/meshcat.h, line 39 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
There are two topics in this conversation:
- Advice on workflow
- Semantics of
Delete().On the first, I think we can compactly communicate the following:
Recommended workflow
For convenience,
Meshcatfosters a work flow in which all user-created objects created in Drake are contained in the "/drake" folder. Objects added with a relative path are placed in the "/drake" folder for you. The benefits are:
- It's simple to distinguish between user objects and "infrastructure" objects in the visualizer.
- All user objects can easily be cleared by a single, parameter-free call to
Delete().You are welcome to use absolute paths to organize your data, but the burden on tracking and cleaning them up lie on you.
On the second, we can just end the conversation with the appeal to "thin wrapper". We're just making the known semantics of meshcat available to C++ code. Done. If that's the way the "delete" operation works in meshcat, that's the way it works here.
Done.
geometry/meshcat.h, line 50 at r5 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I was hoping for something more specific. The text here says the link contains "their details". That link gives me very little information about details. What properties can I set? What values are meaningful, etc. That documentation says that lights exist, and things like
/Lights?DiretionalLightand/Lights/AmbientLightare "useful paths", but I don't even know how to change the intensity of the ambient light. So, a link with limited value relative to the stated purpose.
I agree that the meshcat documentation is thin. I did into three.js when I need to know more. I've added the second link, and reworded so that the implication is not that "all details will be found if you click on this link".
geometry/meshcat.h, line 35 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW: With a larger view the term "object" now seems sadly overloaded. Calling SetObject("my/path", ...) and SetObject("my/path/more", ...) creates two objects (in the meshcat nomenclature). The proper path to those objects are "my/path/" and "my/path/more/". However, "my/path" and "my/path/more" are separate things that likewise have properties. Are they also "objects"? Overloading "object" to mean both "my/path" and "my/path/" is unfortunate.
I presume meshcat doesn't discuss the path semantics explicitly (hence the generated documentation here rather than a link).
This paragraph is actually taken almost verbatim from the meshcat docs. (but it represents essentially the entirety of the discussion that the docs provide about paths), except to callout the "/" trick under SetObject.
Again, SetObject is a meshcat term. And I actually think it's appropriate here. I expect that we will have many SetObject calls with different typed arguments that generate different types of Objects.
geometry/meshcat.h, line 108 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
The unit tests on this caught me completely by surprise. I had apparently glossed over the details in the .cc file.
My initial interpretation of this was that it's a way to pose the object I've added. I.e.,:
meshcat.SetObject(my_path, my_shape, my_color); meshcat.SetTransform(my_path + "/<object>", X_PG);Here is what surprised me:
- This will create a "thing" at the given path if it doesn't currently exist.
- The thing created is not the same as what is created in calling
SetObject-- i.e., it will have different properties.- I can set properties on the thing created by this method, but the path to that thing is the same as the path I passed here (in stark contrast to SetObject).
- If I call
SetTransform(my_path + "/<object>", X_PG)without having calledSetObject(my_path,...), I will get a "transform" object that is named like an "object" object but isn't.Presumably, this is all common knowledge for meshcat users, right? I assume it's not actually documented and there is nothing to reference?
I believe your statements are all correct, and that we have already referenced the relevant documentation.
geometry/meshcat.cc, line 75 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW As I read this code, the way
object()andtransform()are used, when they arestd::nullopt, you use the empty string. You could just make these strings that default to the empty string value and use it blindly.I used std::optional so that I could avoid sending unnecessary messages over the websocket.
geometry/meshcat.cc, line 403 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: You'll want to a more global search and replace on
X_PathParent.Done.
geometry/test/meshcat_test.cc, line 99 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Calling this object "frame" (instead of "transform") might be less misleading. The phrase
meshcat.SetTransform("transform", X_PP)might be confusing to a user that comes looking to the test for examples of exercising the API. Why does "transform" appear twice? Is this like a property? Etc.Same note applies to the path name "transform" below.
Follow up -- you can probably dismiss this out of hand. This comment was born out of not understanding that a transform is both a node in the graph as well as a property of a node. I'm leaving it here so you can see that I was surprised, but I now assume that this is merely importing the meshcat design via the thin-wrapper.
Done. I've changed them to frame throughout.
geometry/test/meshcat_test.cc, line 119 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: This implies that
"/drake"doesn't exist until someone adds something. Is that what the user should expect? Or should we simply add "/drake" as part of the constructor? I guess the flipside of that is thatDelete()would leave "/drake" intact (which we're not doing).So, the fact that it doesn't initially exist and gets removed after
Delete()is consistent. But given that the whole/drakefolder is an abstraction implemented in our wrapper, we actually do have a choice to decide what its lifespan is.If we stick with this behavior, we should tweak the documentation about the
/drakecontainer to make it clear that it won't exist until they callSetTransformorSetObjectwith a relative path (or with an absolute path that starts with/drake).I think it's a detail that would just pollute the documentation. The existence or not of that path is irrelevant from the perspective of the user. If it exists and is empty, then no messages are sent to the client. If it does not exist and someone adds to it, then it will come into existence. The only way that it's existence can be tested is with this method which we've left out of the public documentation.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri and @RussTedrake)
a discussion (no related file):
Previously, RussTedrake (Russ Tedrake) wrote…
I almost don't want to tell you the fix.
meshcat.SetProperty("/Lights/AmbientLight/<object>", "intensity", 0.1);:gulp: It would have been obvious if you had looked at the tree in the gui.
That's on me. I tinkered with the viewer and still didn't register the <object> . :-P
geometry/meshcat.cc, line 75 at r6 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
I used std::optional so that I could avoid sending unnecessary messages over the websocket.
That would ideally be captured in a comment so future readers of the code don't make the same "optimization" I just made.
There was a problem hiding this comment.
I'm slightly inclined to forward this to the on-call platform reviewer. I feel like I heavily got into feature stuff and may have underserved the platform side. But I'll defer to you.
Reviewable status: 2 unresolved discussions (waiting on @amcastro-tri and @RussTedrake)
Implements the most essential elements of the Meshcat API: - Added SceneTree data structure. - Added stduuid (to create the uuid's required by meshcat) - Added SetObject from Shape, Rgba - Supports all shapes that meshcat_visualizer.py supports (more, in fact) - Does not support texture maps yet. - Added SetTransform - Added Delete - Added SetProperty<double> - Added HasPath, GetPacked* methods, primarily for testing. This PR is > 500 lines, but a significant portion of this commit is relatively boilerplate management of the msgpack data structures used to communicate with the javascript client. Updated the meshcat_manual_test to step the user through the checks. Updated meschat_test to hammer on the new API.
RussTedrake
force-pushed
the
meshcat_api
branch
from
17ac4f2
to
172280d
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion (waiting on @amcastro-tri and @SeanCurtis-TRI)
geometry/meshcat.cc, line 75 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
That would ideally be captured in a comment so future readers of the code don't make the same "optimization" I just made.
DOne.
There was a problem hiding this comment.
I think Greg represents a good feature reviewer, who knows Meshcat better than any other drake-dev. And I'm quite confident in the thoroughness of your review.
Reviewable status: 1 unresolved discussion (waiting on @amcastro-tri and @SeanCurtis-TRI)
a discussion (no related file):
Previously, RussTedrake (Russ Tedrake) wrote…
Yes. One of the ways that I visualize things in 2D is using meshcat with the camera set to orthographic mode. You can see an example if you run the 2D teleop in https://colab.research.google.com/github/RussTedrake/manipulation/blob/master/intro.ipynb .
Once this PR lands it will be possible to do that in c++. A forthcoming PR will add some sugar to make it easy.
Done.
Implements the most essential elements of the Meshcat API:
This PR is > 500 lines, but a significant portion of this commit is relatively boilerplate management of the msgpack data structures used to communicate with the javascript client.
Updated the meshcat_manual_test to step the user through the checks.
Updated meschat_test to hammer on the new API.
This change is