Skip to content

Conversation

@WawasCode
Copy link
Contributor

@WawasCode WawasCode commented Oct 26, 2023

Refer to the comments in meshcat.html for more information on usage.
How it works:

  • Access https://localhost:7000?webxr=ar to enable AR.
  • Access https://localhost:7000?webxr=vr to enable VR.
  • Append &controller=on to the URL to visualize the headset controllers in immersive mode.

This change is Reviewable

@SeanCurtis-TRI SeanCurtis-TRI self-assigned this Oct 26, 2023
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@SeanCurtis-TRI for feature review

cc @jwnimmer-tri The intent of this is to provide some access to meshcat's VR. It's not an official feature. But if you know the url parameter, you can turn it on in your meshcat session. This seems the simplest way to make it available and see how it plays with Drake with the minimum of engineering. I'd appreciate your thoughts as well.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @WawasCode)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the query param is fine.

I'm not a fan of the setup instructions being buried in an HTML comment, and anyway Drake must not recommend system-wide config file changes in any case (so the instructions are wrong). My vote would be to land the query string feature, without any https mumbo jumbo, in the first pass.

Downstream projects that need https can write their own README for how to set it up.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @WawasCode)


geometry/meshcat.html line 253 at r1 (raw file):

    // Function to retrieve URL parameters.
    function getURLParameter(name) {

BTW Surely there's a canonical way to do with without arcane regexs? StackOvervflow seems to have several better options, or see how we already parse reconnect_ms as a query param around line 160, above.

@jwnimmer-tri
Copy link
Collaborator

For context -- the current nginx support in _geometry_extra.py is for use with Deepnote only, where Drake has full ownership over the runtime environment inside that Docker container. (And FYI that port forwarding already uses https, provided by Deepnote with the Deepnote certificate.)

Setting up nginx in other circumstances in more complicated, because Drake is not allowed to presume that it has full ownership of the runtime environment. So Drake would need to do something like launch the web proxy as a Python subprocess without changing any files in /etc and without using systemd services. In my experience, that might be somewhat difficult.

@WawasCode WawasCode force-pushed the webxr-html branch 2 times, most recently from e99a5d9 to afcd1e0 Compare October 26, 2023 17:32
Copy link
Contributor Author

@WawasCode WawasCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Maybe I'll find a way to do this without changing any files outside of Drake's environment. I imagine that the user will still have to provide a valid SSL certificate. I have removed the comment block for now.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @WawasCode)


geometry/meshcat.html line 253 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Surely there's a canonical way to do with without arcane regexs? StackOvervflow seems to have several better options, or see how we already parse reconnect_ms as a query param around line 160, above.

Thanks for pointing that out. Done.

Copy link
Contributor

@wrangelvid wrangelvid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you can still access webxr via the localhost without https. However, my desktop computer doesn't support webxr so I gave ngrok a try. Calling ngrok http 7000 created a https url hosted by ngrok that is accessible from any other device.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @WawasCode)

@SeanCurtis-TRI SeanCurtis-TRI changed the title Enabel webxr with html argument. Enable webxr with html argument Oct 26, 2023
@SeanCurtis-TRI SeanCurtis-TRI added the release notes: none This pull request should not be mentioned in the release notes label Oct 26, 2023
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@jwnimmer-tri I'm adding you as reviewer as well given comments and the like. Between the two of us we'll have the review covered.

+(release notes: none) -- this is a secret thing for now. It'll give us an easy means to tinker with VR and see if it's worth finishing the support from Drake's Meshcat side. So, we won't advertise it.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)


-- commits line 2 at r2:
nit: You'll want to fix this typo (I've edited the PR title, you can steal it from there).


-- commits line 3 at r2:
As the comments are now gone, let's put some more in the comment. In fact, I think if you simply copied the text you put in the PR description, that would be sufficient.


tools/workspace/meshcat/repository.bzl line 13 at r2 (raw file):

        drake/tools/workspace/meshcat/README.md for details.
        """,
        commit = "c788d692d62bf63408be3a127a6288c9845b4325",

@jwnimmer-tri What's the preferred mechanism to flag this so that a release engineer can recognize that meschat has been bumped?

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... support from Drake's Meshcat side.

What is still unfinished? A little documentation somewhere that explains the query string? If so, why not just do that in this same PR, and then call it done instead of secret?

The http thing does not seem like it needs to be MVP, so that's no reason to awit.

Or do you mean having functions on Meshcat itself to turn on VR modes, without the query string? If so, then we need to talk about the feature arc here, since having both mechanisms is a problem. Anyway, a C++ API for these seems wrong to me too. So maybe we're just lacking focusing about what the design arc here is supposed to be.

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)


-- commits line 3 at r2:

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

As the comments are now gone, let's put some more in the comment. In fact, I think if you simply copied the text you put in the PR description, that would be sufficient.

If this is supposed to be "secret", why are we forcing the author to put any details in the commit message? What is the defect, exactly?


tools/workspace/meshcat/repository.bzl line 13 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

@jwnimmer-tri What's the preferred mechanism to flag this so that a release engineer can recognize that meschat has been bumped?

The commit message should say "Upgrade meshcat to latest commit" in its body somewhere.

Really, the bump should have been a separate pull request, and we need to do meshcat_manual_test on the bump by itself. Mixing the sha bump with the new feature makes the review and merging work monotonically more difficult, with no upside.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)


tools/workspace/meshcat/repository.bzl line 13 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The commit message should say "Upgrade meshcat to latest commit" in its body somewhere.

Really, the bump should have been a separate pull request, and we need to do meshcat_manual_test on the bump by itself. Mixing the sha bump with the new feature makes the review and merging work monotonically more difficult, with no upside.

=> #20437

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're just lacking focusing about what the design arc here is supposed to be.

I think that's it.

The on obvious missing issue is the ability to have the VR controllers drive bodies in Drake. It takes it from being a purely visual thing to being a fully virtual reality thing.

I agree that a C++ API seems troublesome. Enabling it at the browser seems simple and straightforward. But limiting ourselves to a secret (or even documented) incantation may be dissatisfying. It'd be more user-friendly if there was something on the page that they could use to toggle.

But, yes, I think there's still some decisions to be made about how we want to present this to users. If you think this is enough and everything else simply enriches the feature set, I'm open to that as well.

Reviewable status: 4 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)

a discussion (no related file):
Blocking until #20437 is reviewed and merged (and then this can be rebased on top of that).



-- commits line 3 at r2:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If this is supposed to be "secret", why are we forcing the author to put any details in the commit message? What is the defect, exactly?

Well, in its current state the commit body is wrong.

I appreciate your point about it not necessarily needing a body. For myself, I've been known to troll commit messages when looking for things and I like not having to go dig through the changes or the original PR to find out what's going on. And given the nature of the PR summary, it seems tailor made for the purpose. An easy copy and paste.

But the one thing that does need to happen is for the commit body, as is, to go away.


tools/workspace/meshcat/repository.bzl line 13 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

=> #20437

That's what I suspected.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The on obvious missing issue is the ability to have the VR controllers drive bodies in Drake. It takes it from being a purely visual thing to being a fully virtual reality thing.

If that capability is actually required for this feature to be useful, then we shouldn't even add the URL to turn on a broken feature. I imagine, though, that this feature is useful even without that capability? I haven't tried it out personally, but that's my impression. In which case "cannot drive bodies" is irrelevant for whether this should stay "secret" or not.

It'd be more user-friendly if there was something on the page that they could use to toggle.

So you're the feature reviewer who has volunteered to shepard this along. You get to decide whether that toggle is required for MVP or not. If yes, we ditch this PR and wait for the toggle. If no, we document this PR as-is and merge it, with helpful comments release notes.

(BTW I assume we'll want the "vr mode" to remain intact even after a page reload? In which case, even with a toggle, it would need to be reflected in the query string anyway for persistence.)

If you think this is enough ...

I defer the question. I'm not the feature reviewer. As platform reviewer, I just need the author and the feature reviewer to come to an agreement that is self-consistent and clearly explained in the end-user documentation and/or code comments.

Reviewable status: 4 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)


-- commits line 3 at r2:

Well, in its current state the commit body is wrong.

That's fair.

And given the nature of the PR summary, it seems tailor made for the purpose.

If the goal is to document how the URL works, that should go in the actual documentation. I suppose it could also go in the commit message as a nice-to-have, but that's kind of missing the forest for the tree.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is going in circles a bit. Where I was, mentally, was "This is good enough to have some group of people "in the know" exercise it and find out if it's useful without everyone creating forks." It wasn't my intent to broadcast this generally. There's still an open question of how much utility does this have for a drake user and, based on that, how much affordance does it need beyond this hack. This position feels like it lies between the two options I perceive you're advocating: either it's really a feature, or it shouldn't merge.

So, this PR is sufficient for what I had in mind, but apparently not for what you have in mind. So, the real question is: is what I have in mind, well intentioned though it may be, misdirected?

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform), labeled "do not merge" (waiting on @WawasCode)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Blocking until #20437 is reviewed and merged (and then this can be rebased on top of that).

FYI As discovered in #20437, the upgrade in three.js version has changed the lighting. Thus, everything looks different. Before Drake swallows the change to meshcat, we need to decide what we're doing about lighting changes. See the discussion in #20437 for more details.

For now, this PR will simply have to be on hold.

+(status: do not review)
+(status: do not merge)



-- commits line 3 at r2:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Well, in its current state the commit body is wrong.

That's fair.

And given the nature of the PR summary, it seems tailor made for the purpose.

If the goal is to document how the URL works, that should go in the actual documentation. I suppose it could also go in the commit message as a nice-to-have, but that's kind of missing the forest for the tree.

Also going in documentation really comes down to the decision on whether we're rolling with this as a feature. I agree, if we are, then it needs a home. It's just not clear where that home is because the documentation won't be particularly discoverable -- hence my preference for doing something in -GUI if we're going to go with this as a real feature.


tools/workspace/meshcat/repository.bzl line 13 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

That's what I suspected.

Marking this resolved as we'll need to handle meshcat in a separate PR and this PR is sufficiently otherwise blocked now.

@jwnimmer-tri
Copy link
Collaborator

Where I was, mentally, was "This is good enough to have some group of people "in the know" exercise it and find out if it's useful without everyone creating forks."

That's fine.

It wasn't my intent to broadcast this generally.

Sure, but putting the documentation in the wrong place is not good solution to that objective. We should still document the query string argument somewhere proper (e.g., the class Meshcat doxygen), but then mark it "experimental" or somesuch -- to convey that we're not sure whether it will stick around with the current spelling.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can go with that. @experimental seems to satisfy all parties.

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform), labeled "do not merge" (waiting on @WawasCode)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe a more clear way to state my position...

It's OK to land "unstable" features to master. Sometimes we have code that we think might evolve and/or vanish as we learn more about the solution space, so we don't want our default stability guarantees to hold). The way we do that is to land them but opt-out of stability in their documentation.

It's not OK to land features that are defective (e.g., sufficiently undocumented as to confuse our maintainers, or not suitably tested). Thinking that through more carefully now, it means that this PR also needs one or more new "meshcat manual test" cases, where it prints new URL(s) with new the query string(s) and instructions for how to test that they function as-intended. (The test case could be amended to use the on-screen toggle, if we decided to add that later.)

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform), labeled "do not merge" (waiting on @WawasCode)

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems reasonable. The burden for testing the VR is not particularly worse than gamepad. Both require something extrinsic. But the VR can be tested with a browser plug in. Also, we'll have to make sure that the VR phase of the test is last. How one ends the VR session isn't necessarily obvious and life becomes simpler if the user doesn't have to.

@WawasCode If you like, I can submit a PR against this PR with the changes we're discussing above.

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform), labeled "do not merge" (waiting on @WawasCode)

Copy link
Contributor Author

@WawasCode WawasCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you like, I can submit a PR against this PR with the changes we're discussing above.

Yes, sure. Would you like me to include documentation as comments in the code? Or do you prefer to handle this yourself when adding Doxygen documentation?

Disclaimer: Just a heads up, I am currently back in Germany and will only be able to dedicate around 10 hours a week to this project. I apologize in advance if I'm unable to respond on the same day. :)

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform), labeled "do not merge"

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

FYI As discovered in #20437, the upgrade in three.js version has changed the lighting. Thus, everything looks different. Before Drake swallows the change to meshcat, we need to decide what we're doing about lighting changes. See the discussion in #20437 for more details.

For now, this PR will simply have to be on hold.

+(status: do not review)
+(status: do not merge)

Is there something I can help with there?

Should I remove the meshcat version bump?


Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go ahead and submit a PR to your PR just so we can continue to shepherd this through. We appreciate your contribution and with your return to Germany and your reduced number of cycles, it makes sense to accommodate where we can.

Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform), labeled "do not merge" (waiting on @WawasCode)

a discussion (no related file):

Previously, WawasCode (Watsche Khalatyan) wrote…

Is there something I can help with there?

Should I remove the meshcat version bump?

I submitted PR 155 to meshcat to deal with the changes in three.js. And we've got #20442 that is pulling the upgraded meshcat into Drake. So, this can rebase on top of that, relying on the fact that meshcat has already been upgraded.


@jwnimmer-tri
Copy link
Collaborator

Feel free to re-assign a platform reviewer once feature review is complete. Removing myself for now: -@jwnimmer-tri.

@jwnimmer-tri jwnimmer-tri removed their assignment Oct 30, 2023
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WawasCode I've submitted a PR against this. Please take a look.

After merging that PR (or its edited form), we'll need to rebase this PR on top of master without the meshcat/repository.bzl changes. Master is now up to date on meshcat.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @WawasCode)

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WawasCode Let me know if you'd like to pass this PR over to me. I won't unilaterally take it, but if you want to get it off your hands, I'll catch it.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @WawasCode)

@WawasCode WawasCode force-pushed the webxr-html branch 2 times, most recently from 70e7023 to 2d30356 Compare October 31, 2023 15:44
Copy link
Contributor Author

@WawasCode WawasCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be good to go. Depending on the upcoming workload I might hand it over.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @WawasCode)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I submitted PR 155 to meshcat to deal with the changes in three.js. And we've got #20442 that is pulling the upgraded meshcat into Drake. So, this can rebase on top of that, relying on the fact that meshcat has already been upgraded.

Ok I rebased everything.



-- commits line 2 at r2:

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: You'll want to fix this typo (I've edited the PR title, you can steal it from there).

Done.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a 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) -(status: do not review)

:LGTM:

One erroneous whitespace added. But, otherwise, ready for platform review. +@jwnimmer-tri for on-call platform review, please.

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), labeled "do not merge" (waiting on @WawasCode)


tools/workspace/meshcat/repository.bzl line 15 at r3 (raw file):

        commit = "fa29aecebd5f1712844c3b98967a7d21707df5a0",
        sha256 = "bdd505e90ab4e46958714a909f0a45d42d905eee0fa87637b97092035a4df7a3",  # noqa

nit: Extraneous white space introduced. This file should not be touched.

@WawasCode WawasCode force-pushed the webxr-html branch 2 times, most recently from 4edf8b4 to 7ef71af Compare October 31, 2023 17:00
Copy link
Contributor Author

@WawasCode WawasCode left a 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)


tools/workspace/meshcat/repository.bzl line 15 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Extraneous white space introduced. This file should not be touched.

Done.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform)

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

a discussion (no related file):
Working

A bit later today, I'll circle back and run the manual test to see if it works for me.



geometry/meshcat.html line 203 at r4 (raw file):

        viewer.handle_command({
          type: "enable_webxr",
          mode: "ar"

Do we need to hard-code the list of supported modes in this file?

It seems to me like we could just unconditionally pass query argument into the command, with no ahead-of-time validation. Presumably the message received already needs to validate the input anyway, so this seems like twice the work for no gain.


geometry/meshcat.html line 208 at r4 (raw file):

    }

    const controllerMode = urlParams.get('controller');

BTW This this a good name? I'm not sure that "show the VR controller in the VR view" will be the only "controller" that this program knows about.

If the plan is to revisit this name later, prior to removing the "experimental" disclaimer, that's fine too.


geometry/test/meshcat_manual_test.cc line 517 at r4 (raw file):

  std::cout << "If you don't have VR hardware installed on your machine, "
               "you'll have to install the WebXR API emulator appropriate to "
               "your browser. E.g., for Google Chrome "

A manual test should not use "e.g.". The actions expected of the person running the test must not be left up for interpretation.

If we want the user to test in their everyday browser (whichever one it is), then we need to provide the emulator links for both chrome and firefox (or link to the Doxygen page that has the add-on links).

If we want the user to test specifically in chrome, then we need to say that clearly.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 5 unresolved discussions

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

A bit later today, I'll circle back and run the manual test to see if it works for me.

Done



geometry/test/meshcat_manual_test.cc line 522 at r4 (raw file):

Open the window showing the WebXR emulator

It was not obvious that this meant to push F12 click over to the WebXR tab. (Needs more explanatory text.)


geometry/test/meshcat_manual_test.cc line 529 at r4 (raw file):

You should be able to manipulate the view in the WebXR emulator.

It was not obvious that this meant to click on(?) the headset mesh in the emulator pane to uncover the colored arrows and panels and then click-drag those to move the headset. (Needs more explanatory text.)

Copy link
Contributor Author

@WawasCode WawasCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 unresolved discussions


geometry/meshcat.html line 203 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Do we need to hard-code the list of supported modes in this file?

It seems to me like we could just unconditionally pass query argument into the command, with no ahead-of-time validation. Presumably the message received already needs to validate the input anyway, so this seems like twice the work for no gain.

Done.


geometry/meshcat.html line 208 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW This this a good name? I'm not sure that "show the VR controller in the VR view" will be the only "controller" that this program knows about.

If the plan is to revisit this name later, prior to removing the "experimental" disclaimer, that's fine too.

I'm not even sure if we are going to keep the visualizer after we leave "experimental". They aren't really useful. The only purpose it currently serves is aesthetics. I can change the name of it to something that better reflects what we are enabling if needed for this PR.


geometry/test/meshcat_manual_test.cc line 517 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

A manual test should not use "e.g.". The actions expected of the person running the test must not be left up for interpretation.

If we want the user to test in their everyday browser (whichever one it is), then we need to provide the emulator links for both chrome and firefox (or link to the Doxygen page that has the add-on links).

If we want the user to test specifically in chrome, then we need to say that clearly.

Added the link for Firefox and changed the wording.


geometry/test/meshcat_manual_test.cc line 522 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Open the window showing the WebXR emulator

It was not obvious that this meant to push F12 click over to the WebXR tab. (Needs more explanatory text.)

Added a more detailed explanation.


geometry/test/meshcat_manual_test.cc line 529 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

You should be able to manipulate the view in the WebXR emulator.

It was not obvious that this meant to click on(?) the headset mesh in the emulator pane to uncover the colored arrows and panels and then click-drag those to move the headset. (Needs more explanatory text.)

Added a more detailed explanation.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion


geometry/meshcat.html line 208 at r4 (raw file):

Previously, WawasCode (Watsche Khalatyan) wrote…

I'm not even sure if we are going to keep the visualizer after we leave "experimental". They aren't really useful. The only purpose it currently serves is aesthetics. I can change the name of it to something that better reflects what we are enabling if needed for this PR.

Mostly I just wanted to bring it up for consideration. I'll let you and Sean decide what's best.

@jwnimmer-tri
Copy link
Collaborator

@drake-jenkins-bot ok to test

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your CI failure comes down to having a couple of lines are code that are two long. I've flagged them below. You can confirm things are good by running:

bazel test --config=lint //geometry/...

before merging.

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: 3 unresolved discussions


geometry/meshcat.html line 208 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Mostly I just wanted to bring it up for consideration. I'll let you and Sean decide what's best.

At this point I'm still on the fence. It's tagged as part of the experimental VR feature. I'm not going to sweat its name at this point if it's not clear it has value (or that it'll end up conflicting with anything in the future). So, I'd vote for leave it as is with the (non-trivial) possibility that it evaporates in the future.


geometry/test/meshcat_manual_test.cc line 521 at r5 (raw file):

               "webxr-api-emulator/mjddjgeghkdijejnciaefnkjmkafnnje\n"
               "If you are using Firefox see:\n "
               "https://addons.mozilla.org/de/firefox/addon/webxr-api-emulator/";

nit: This line is too long.


geometry/test/meshcat_manual_test.cc line 525 at r5 (raw file):

               "button at the bottom that says \"Enter VR\".\n";
  std::cout << "Open the developer tools of your browser (F12). At the top "
               "of the developer tools windows click on the dooble arrows icon "

nit: typo

Suggestion:

double 

geometry/test/meshcat_manual_test.cc line 535 at r5 (raw file):

            << "  - You should be able to manipulate the view in the WebXR "
               "emulator to affect what you see."
            << "  - Clicking on the headset/controller mesh for the first time in "

nit: This line is too long.

Copy link
Contributor Author

@WawasCode WawasCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. Do you have a guideline I can refer to when coding to prevent this from happening in the future?

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)


geometry/test/meshcat_manual_test.cc line 521 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This line is too long.

Done.


geometry/test/meshcat_manual_test.cc line 525 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: typo

Done.


geometry/test/meshcat_manual_test.cc line 535 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This line is too long.

Done.

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spelling of bazel test --config=lint //path/to/changes/... is what I use to find any lint before I push to the PR. If a problem is found, it typically comes with a command you can copy and paste into the console to correct the linting error.

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),SeanCurtis-TRI(platform)

@SeanCurtis-TRI SeanCurtis-TRI merged commit e719d47 into RobotLocomotion:master Nov 1, 2023
@jwnimmer-tri
Copy link
Collaborator

Do you have a guideline I can refer to when coding to prevent this from happening in the future?

The big picture is https://drake.mit.edu/developers.html which links to https://drake.mit.edu/styleguide/cppguide.html, but for minutiae like "line too long" the easiest answer is just to run the tests.

That would be something like bazel test //geometry:all to run all of the tests, or as Sean says something like bazel test --config=lint //... to run all linter tests (across the whole codebase, or just bazel test //geometry/... --config=lint for just one subtree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: none This pull request should not be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants