New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Meshcat can handle Mesh(foo.gltf) #19635
Meshcat can handle Mesh(foo.gltf) #19635
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+(status: do not merge)
This PR depends on meshcat-dev/meshcat#144 to be fully functional. This makes it so Drake can throw embedded .gltf files, but the other PR is necessary so that meshcat can catch them. Once both PRs have merged, meshcat_manual_test.cc
swaps out the boring green cube with a new blue cube (shown here):
The newly added blue cube is a compact model whose textures are 32x32 to keep it small but still sufficient to underscore what is possible.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", missing label for release notes (waiting on @SeanCurtis-TRI)
2191d3a
to
bc73ef4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-(status: do not review)
You can test it by calling:
bazel run //geometry:meshcat_manual_test
The old green box has been replaced with the box shown above. All other aspects of the test should still work as advertised.
+@jwnimmer-tri for feature review, please.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+(release notes: feature)
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 6 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
geometry/meshcat.cc
line 404 at r1 (raw file):
matrix(1, 1) = mesh.scale(); matrix(2, 2) = mesh.scale(); } else { // not obj or no mtllib.
bitrot
Suggestion:
// not obj (or no mtllib) nor gltf
geometry/render/test/meshes/cube.gltf
line 2 at r1 (raw file):
{ "asset":{
nit All of the other gltf files in Anzu and Drake use spaces instead of tabs. Could we use spaces in this one for consistency?
geometry/test/meshcat_manual_test.cc
line 208 at r1 (raw file):
animation.SetTransform(40, "sphere", RigidTransformd(sphere_home)); std::cout << "- the blue box should spin clockwise about the +z axis.\n";
nit This is a tiny bit ambiguous now. There are two blue boxes, albeit one we call a "box" vs "cube" for whether or not the diameters are uniform.
Maybe "leftmost blue box" is sufficient?
Ditto below near "should have disappeared".
geometry/test/meshcat_manual_test.cc
line 398 at r1 (raw file):
<< html_filename << "\nOpen that location in your browser now and confirm that " "the iiwa is visible and the animation plays."
This isn't working for me. (The camera is zoomed into the iiwa base with no ability to pan/zoom, but the animation player does work.)
I'll see if it's also broken on master for me.
geometry/test/meshcat_test.cc
line 300 at r1 (raw file):
"gltf", Mesh(FindResourceOrThrow( "drake/geometry/render/test/meshes/cube.gltf"), .25));
nit GSG https://drake.mit.edu/styleguide/cppguide.html#Floating_Literals
Suggestion:
0.25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
geometry/test/meshcat_manual_test.cc
line 398 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This isn't working for me. (The camera is zoomed into the iiwa base with no ability to pan/zoom, but the animation player does work.)
I'll see if it's also broken on master for me.
Weird. It's passing now both on this branch (and of course on master). I tried a few different things and nothing was able to repro the problem. I guess it was a browser fluke.
cd4216b
to
89d7bfc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also reversed the order of the commits. The intention is to mark this as curated so that the meshcat dependency gets updated in its own commit.
Or would that be adversarial to release engineering? Is the true solution to update meshcat in its own PR and then to merge this one after the fact?
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
geometry/render/test/meshes/cube.gltf
line 2 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit All of the other gltf files in Anzu and Drake use spaces instead of tabs. Could we use spaces in this one for consistency?
I'm somewhat disinclined to do so. Those were generated by VTK, this was generated by blender. Unless we want to create an auto-formatter for our ascii gltf files, I'd say we have to let it go. It doesn't seem reasonable for the author to force one generator to look like another.
geometry/test/meshcat_manual_test.cc
line 208 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit This is a tiny bit ambiguous now. There are two blue boxes, albeit one we call a "box" vs "cube" for whether or not the diameters are uniform.
Maybe "leftmost blue box" is sufficient?
Ditto below near "should have disappeared".
I pushed the cube back to being green. Seemed simpler that way.
geometry/test/meshcat_manual_test.cc
line 398 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Weird. It's passing now both on this branch (and of course on master). I tried a few different things and nothing was able to repro the problem. I guess it was a browser fluke.
Next time, I'd recommend sacrificing a chicken before trying. I can give you one of mine.
geometry/test/meshcat_test.cc
line 300 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit GSG https://drake.mit.edu/styleguide/cppguide.html#Floating_Literals
Fixed throughout this file and meshcat_manual_test.cc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reviewers, the blue box is now green again.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also reversed the order of the commits. The intention is to mark this as curated so that the meshcat dependency gets updated in its own commit.
Right. If this is merged as "curated" ideally the upgrade is first and the new feature is second. Physically its the same either way, but this order might be slightly clearer for buildcops that they would need to revert either the second commit or both commits; they shouldn't revert just the first commit on its own.
Or would that be adversarial to release engineering? Is the true solution to update meshcat in its own PR and then to merge this one after the fact?
Release engineer doesn't care either way, the most people who care are buildcops. Since both commits require meshcat_manual_test inspection, it's probably smoothest to keep them in the same PR.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
geometry/test/meshcat_manual_test.cc
line 208 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I pushed the cube back to being green. Seemed simpler that way.
Great!
geometry/test/meshcat_test.cc
line 300 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Fixed throughout this file and
meshcat_manual_test.cc
.
The global fix was incomplete (I found about a dozen more missed), and anyway I object to mixing this much unrelated churn into a feature PR that also upgrades an external (i.e., one that's more at risk of revert wars than usual).
I took the liberty of forking the cleanups into their own PR, which I hope to merge during the day today:
#19645
89d7bfc
to
7398f50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
geometry/test/meshcat_test.cc
line 300 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The global fix was incomplete (I found about a dozen more missed), and anyway I object to mixing this much unrelated churn into a feature PR that also upgrades an external (i.e., one that's more at risk of revert wars than usual).
I took the liberty of forking the cleanups into their own PR, which I hope to merge during the day today:
#19645
D'oh! Sloppy, sloppy, sloppy. Thanks for breaking it out. It makes this PR much happier. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
This change requires both .glTF preliminary PR trains to merge first: - General glTF support (from which this code comes) - Meshcat specific support (RobotLocomotion#19635)
Meshcat passes embedded .gltf files along to the meshcat session. This includes an incidental recentering of the objects created in the meshcat_manual_test.cc. This depends on updates to meshcat itself.
7398f50
to
9748fb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-(status: do not merge)
Meshcat has now updated (see meshcat-dev/meshcat#144). This is ready to get through the process.
Ready for full feature review, please.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@sherm1 for platform review or delegation, please.
Reviewed 4 of 4 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Platform
I ran meshcat_manual_test and followed the instructions -- everything looked great.
Reviewed 1 of 4 files at r2, 1 of 3 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)
Meshcat passes embedded .gltf files along to the meshcat session. This includes an incidental recentering of the objects created in the meshcat_manual_test.cc.
This change is