[geometry] Establishes Meshcat in C++ (step 1) #15618
Conversation
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake)
setup/mac/binary_distribution/Brewfile, line 24 at r1 (raw file):
brew 'nlopt' brew 'numpy' brew 'msgpack'
@BetsyMcPhail I will need to bump the build servers (ubuntu, too) to get msgpack. We can wait until the reviewers ok my usage of this external; but wanted to give you a heads up that it is coming.
There was a problem hiding this comment.
+@IanTheEngineer for feature review, please.
BTW -- CI won't pass until the msgpack dependency is updated on the build servers. I have tested on mac locally (in addition to ubuntu bionic).
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @IanTheEngineer)
|
@drake-jenkins-bot linux-bionic-unprovisioned-gcc-bazel-experimental-release please per https://drake.mit.edu/jenkins.html#updating-installation-prerequisites |
There was a problem hiding this comment.
For reference, the direct links inside the #13038 thread -
Design Decisions: #13038 (comment)
PR Strategy: #13038 (comment)
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @IanTheEngineer)
There was a problem hiding this comment.
First review pass done, and successfully tested locally. This looks great, Russ. I like the design you've put forth here. The new dependencies themselves do seem rather light, well licensed, and well maintained. I just have a few minor discussion points below.
Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee IanTheEngineer, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake)
geometry/dev/meshcat.cc, line 4 at r1 (raw file):
Quoted 6 lines of code…
#include <future> #include <string> #include <thread> #include <unordered_map> #include <utility> #include <App.h>
Nit: You already included the above dependencies in the meshcat.h file. I don't mind if you prefer leaving them here, but I believe they can be removed.
geometry/dev/meshcat.cc, line 45 at r1 (raw file):
void Meshcat::join_websocket_thread() { websocket_thread_.join(); }
FYI, It took me a minute to realize that this function is "snake_case" because the function is basically an accessor function providing access to the underlying thread join() functionality. Am I following this logic correctly?
geometry/dev/meshcat.cc, line 113 at r1 (raw file):
LIBUS_LISTEN_EXCLUSIVE_PORT
How do we determine this port is exclusively owned by the WebSocket app? I am wondering if anything needs to be done to explicitly claim ownership over the port, as LIBUS_LISTEN_DEFAULT is the non-exclusive default listening option.
geometry/dev/meshcat.cc, line 122 at r1 (raw file):
port++ <= kMaxPort
FYI, if I am reading it correctly, this means we can only have 50 instances of Meshcat XD.
geometry/dev/meshcat_demo.cc, line 7 at r1 (raw file):
bazel run //geometry:meshcat_demo
This should be bazel run //geometry/dev:meshcat_demo.
geometry/dev/meshcat_demo.cc, line 25 at r1 (raw file):
// Note: the grid doesn't actually disappear (bug in meshcat), but the // checkbox is unchecked.
FYI: I could reproduce this bug. It gave me pause until I found your note here :) Perhaps give a nod to this bug above in your test description as well.
geometry/dev/meshcat_demo.cc, line 30 at r1 (raw file):
meshcat.join_websocket_thread();
Nit: Even though it's not strictly necessary as the OS will clean it up, this example would be "more correct" if you also invoked meshcat2.join_websocket_thread(); to wait for the second meshcat instance thread to rejoin you the main thread.
setup/ubuntu/source_distribution/packages-bionic.txt, line 24 at r1 (raw file):
liblz4-dev liblzma-dev libmsgpack-dev
FYI, (though perhaps this is quite obvious) apt install'ing libmsgpack-dev or running the setup script with this branch checked out is required to test this PR.
There was a problem hiding this comment.
+@jwnimmer-tri for platform review per schedule.
dev bits since they don't need platform review.
Reviewed 17 of 21 files at r1, all commit messages.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee IanTheEngineer (waiting on @RussTedrake)
-- commits, line 4 at r1 ([raw file](https://github.com/robotlocomotion/drake/blob/07e80e97602947088cab4adfea31a4adb4e5ecd8/-- commits#L4)):
nit Typo on in
doc/BUILD.bazel, line 57 at r1 (raw file):
"site.webmanifest", ], visibility = ["//visibility:public"],
FYI I oscillated back and forth about whether this should be "public" or @drake//:__subpackages__, but ended up with this being okay as-is. I guess it doesn't much hurt to allow downstream software to re-use our icons in their build system?
geometry/dev/BUILD.bazel, line 10 at r1 (raw file):
load("//tools/lint:lint.bzl", "add_lint_tests") package(default_visibility = ["//visibility:public"])
nit Code in dev folders should have scoped visibility, not public. That would be "//visibility:private" nominally, but making it visible to a list of other dev folders within Drake would be fine as well.
geometry/dev/BUILD.bazel, line 21 at r1 (raw file):
"@meshcat//:dist/main.min.js", ], linkopts = ["-pthread"],
nit Since our meshcat.h and meshcat.cc files don't use pthread, this seems like a lost dependency of our of our externals, so should be added there, not here.
Indeed, looking at the Makefile for uWebSockets, it looks like it needs -pthread, so this line should move into the @uwebsockets//:uwebsockets target instead of here.
geometry/dev/meshcat.h, line 9 at r1 (raw file):
#include <utility> #include <App.h>
FYI For the future --
Per #7451, we do not want to add new library dependencies to our installed headers. Therefore, if #include <App.h> stays here, then meshcat.h cannot be installed, and Drake headers that include meshcat.h also cannot be installed. Only *.cc files could use this code.
If we need to provide public header access to class Meshcat, then the uWS stuff will need to move into a pimpl.
tools/workspace/default.bzl, line 53 at r1 (raw file):
load("@drake//tools/workspace/meshcat:repository.bzl", "meshcat_repository") load("@drake//tools/workspace/meshcat_python:repository.bzl", "meshcat_python_repository") # noqa load("@drake//tools/workspace/msgpack:repository.bzl", "msgpack_repository")
nit Keep the list of externals alpha-sorted (move this after "mosek").
tools/workspace/default.bzl, line 201 at r1 (raw file):
meshcat_python_repository(name = "meshcat_python", mirrors = mirrors) if "msgpack" not in excludes: msgpack_repository(name = "msgpack")
nit Keep the list of externals alpha-sorted (move this after "mosek").
tools/workspace/usockets/package.BUILD.bazel, line 21 at r1 (raw file):
"src/*.h", "src/internal/*.h", "src/internal/**/*.h",
nit It's not necessarily a problem either way, but instead of the prior three lines of headers, I think we could say ...
"src/**/*.h"
... on its own, instead, since there does not appear to be anything that we need to exclude or avoid.
Note that the *.c stuff is correct as-written, because it matches the upstream Makefile.
There was a problem hiding this comment.
Reviewable status: 11 unresolved discussions, LGTM missing from assignee IanTheEngineer (waiting on @RussTedrake)
geometry/dev/meshcat.cc, line 45 at r1 (raw file):
btw: Per styleguide: https://drake.mit.edu/styleguide/cppguide.html#Function_Names
Such methods should have a time complexity of O(1) and be less than 5 lines long.
Surely a thread join method is not O(1)?
RussTedrake
force-pushed
the
meshcat_uwebsockets
branch
from
07e80e9
to
f4a6f95
Compare
This is the first of a series of PRs that will provide Meshcat as a visualizer in C++. The design and PR strategy is documented in RobotLocomotion#13038. This is the first PR: Meshcat proof of life. Starts the server, demonstrates that clients can connect, and just sends one type of message to show that data can flow. Reviewers can focus on the build system and websocket server details. The dependencies added here are all quite lightweight, and properly licensed. And at the end of the PR train, we will likely want to deprecate meshcat-python, and eventually remove the pip dependencies that come along with it.
RussTedrake
force-pushed
the
meshcat_uwebsockets
branch
from
f4a6f95
to
37fd417
Compare
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer and @jwnimmer-tri)
-- commits, line 4 at r1 ([raw file](https://github.com/robotlocomotion/drake/blob/07e80e97602947088cab4adfea31a4adb4e5ecd8/-- commits#L4)):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Typo
on in
Done.
doc/BUILD.bazel, line 57 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
FYI I oscillated back and forth about whether this should be "public" or
@drake//:__subpackages__, but ended up with this being okay as-is. I guess it doesn't much hurt to allow downstream software to re-use our icons in their build system?
I went through the same thought process. I'm happy with either.
tools/workspace/default.bzl, line 53 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Keep the list of externals alpha-sorted (move this after "mosek").
Done.
tools/workspace/default.bzl, line 201 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Keep the list of externals alpha-sorted (move this after "mosek").
Done.
tools/workspace/usockets/package.BUILD.bazel, line 21 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit It's not necessarily a problem either way, but instead of the prior three lines of headers, I think we could say ...
"src/**/*.h"... on its own, instead, since there does not appear to be anything that we need to exclude or avoid.
Note that the
*.cstuff is correct as-written, because it matches the upstreamMakefile.
Done.
geometry/dev/BUILD.bazel, line 10 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Code in
devfolders should have scoped visibility, not public. That would be"//visibility:private"nominally, but making it visible to a list of other dev folders within Drake would be fine as well.
Done.
geometry/dev/BUILD.bazel, line 21 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Since our
meshcat.handmeshcat.ccfiles don't use pthread, this seems like a lost dependency of our of our externals, so should be added there, not here.Indeed, looking at the
MakefileforuWebSockets, it looks like it needs-pthread, so this line should move into the@uwebsockets//:uwebsocketstarget instead of here.
Done.
geometry/dev/meshcat.h, line 9 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
FYI For the future --
Per #7451, we do not want to add new library dependencies to our installed headers. Therefore, if
#include <App.h>stays here, thenmeshcat.hcannot be installed, and Drake headers that includemeshcat.halso cannot be installed. Only*.ccfiles could use this code.If we need to provide public header access to
class Meshcat, then the uWS stuff will need to move into a pimpl.
Done. Implemented the PIMPL. Good call.
geometry/dev/meshcat.cc, line 4 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
#include <future> #include <string> #include <thread> #include <unordered_map> #include <utility> #include <App.h>Nit: You already included the above dependencies in the
meshcat.hfile. I don't mind if you prefer leaving them here, but I believe they can be removed.
Done. The PIMPL allowed me to remove them from the header.
geometry/dev/meshcat.cc, line 45 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
btw: Per styleguide: https://drake.mit.edu/styleguide/cppguide.html#Function_Names
Such methods should have a time complexity of O(1) and be less than 5 lines long.
Surely a thread join method is not O(1)?
Done.
geometry/dev/meshcat.cc, line 113 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
LIBUS_LISTEN_EXCLUSIVE_PORTHow do we determine this port is exclusively owned by the WebSocket app? I am wondering if anything needs to be done to explicitly claim ownership over the port, as LIBUS_LISTEN_DEFAULT is the non-exclusive default listening option.
Passing this allows me to ensure that multiple instances of Meshcat will not listen on the same port. I don't know how to protect from some other application silently listening on that port, and would think that protection is out of scope?
geometry/dev/meshcat.cc, line 122 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
port++ <= kMaxPortFYI, if I am reading it correctly, this means we can only have 50 instances of Meshcat XD.
Yes. I've made it 7099, since that's the number of ports that the docker instances open up for meshcat (but it's all pretty arbitrary, i guess... I've rarely needed more than 2!). I've added a comment in the constructor, which I think is the right place since we may offer passing a port number as a constructor option in the future.
geometry/dev/meshcat_demo.cc, line 7 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
bazel run //geometry:meshcat_demoThis should be
bazel run //geometry/dev:meshcat_demo.
Done. Good catch. But hopefully we'll move it back soon!
geometry/dev/meshcat_demo.cc, line 25 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
// Note: the grid doesn't actually disappear (bug in meshcat), but the // checkbox is unchecked.FYI: I could reproduce this bug. It gave me pause until I found your note here :) Perhaps give a nod to this bug above in your test description as well.
Done. Turns out the bug was mine. I've fixed it.
geometry/dev/meshcat_demo.cc, line 30 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
meshcat.join_websocket_thread();Nit: Even though it's not strictly necessary as the OS will clean it up, this example would be "more correct" if you also invoked
meshcat2.join_websocket_thread();to wait for the second meshcat instance thread to rejoin you the main thread.
Done.
There was a problem hiding this comment.
Reviewed 1 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer)
|
@drake-jenkins-bot linux-bionic-unprovisioned-gcc-bazel-experimental-release please +@BetsyMcPhail for review, and to update the CI instances. |
There was a problem hiding this comment.
Linux images have been updated (as always, keep in mind it may take awhile for Jenkins to use new images)
Reviewable status: 3 unresolved discussions, LGTM missing from assignees BetsyMcPhail,IanTheEngineer (waiting on @BetsyMcPhail and @IanTheEngineer)
There was a problem hiding this comment.
Mac images are also updated.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee IanTheEngineer (waiting on @BetsyMcPhail and @IanTheEngineer)
|
@drake-jenkins-bot retest this please |
|
@drake-jenkins-bot mac-catalina-clang-bazel-experimental-release please It looks like CI is passing so far, which means that @IanTheEngineer the only thing blocking this PR now is your |
There was a problem hiding this comment.
I quite like the PIMPL implementation to hide the external dependencies. I think overall things look good now to proceed. Thanks for the changes Russ!
Reviewed 4 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),BetsyMcPhail,IanTheEngineer (waiting on @BetsyMcPhail and @IanTheEngineer)
geometry/dev/meshcat.cc, line 113 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Passing this allows me to ensure that multiple instances of Meshcat will not listen on the same port. I don't know how to protect from some other application silently listening on that port, and would think that protection is out of scope?
Ah, I read this the other way : that you were meant to guarantee this socket was available. It makes more sense that you're signalling to the app to only grab this port if it's currently free.
geometry/dev/meshcat.cc, line 75 at r3 (raw file):
// Note: Must pass path and property by value because they will go out of // scope.
BTW: Good find here. I would hazard a guess this was the bug preventing meshcat's grid from being disabled in the test.
There was a problem hiding this comment.
Reviewed 1 of 5 files at r2.
Reviewable status:
This is the first of a series of PRs that will provide Meshcat as a visualizer in C++. The design and PR strategy is documented on in #13038.
This is the first PR:
Meshcat proof of life. Starts the server, demonstrates that clients can connect, and just sends one type of message to show that data can flow. Reviewers can focus on the build system and websocket server details.
The dependencies added here are all quite lightweight, and properly licensed. And at the end of the PR train, we will likely want to deprecate meshcat-python, and eventually remove the pip dependencies that come along with it.
This change is