[geometry] Fix Meshcat browser console error messages

[jwnimmer-tri]

Prior to this, Uncaught ReferenceError: urlParams is not defined was printed from any static HTML page.

Amends #20430.

+@SeanCurtis-TRI for both reviews, please.

---

This change is 

[SeanCurtis-TRI]

Alternative solution suggested. See below.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)

---

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

    }

    const queryString = window.location.search;

I'd suggest the real solution is to move these two lines up above <!-- CONNECTION BLOCK BEGIN --> (line 138).

That way the static html can still be passed the webxr parameters and webxr can be used even on a static page. In fact, the trailing webxr initializing code could move up with it as well so the connection boiler plate would simply be the last chunk of the page's javascript.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'd suggest the real solution is to move these two lines up above <!-- CONNECTION BLOCK BEGIN --> (line 138).

That way the static html can still be passed the webxr parameters and webxr can be used even on a static page. In fact, the trailing webxr initializing code could move up with it as well so the connection boiler plate would simply be the last chunk of the page's javascript.

Maybe you'd like to do that in a follow-up PR?

Promising support for offline VR/XR also means adding more test scripting to meshcat_manual_test to ensure that the feature continues to work, and I'm not sure that's worth it if we think the long-term goal is to offer a UI settings panel in lieu of the query params?

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Maybe you'd like to do that in a follow-up PR?

Promising support for offline VR/XR also means adding more test scripting to meshcat_manual_test to ensure that the feature continues to work, and I'm not sure that's worth it if we think the long-term goal is to offer a UI settings panel in lieu of the query params?

It's only an offline thing.

The error is the implication that it's part of communication with a server. I think that's orthogonal to what we might do with future UX.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

It's only an offline thing.

The error is the implication that it's part of communication with a server. I think that's orthogonal to what we might do with future UX.

Maybe I wasn't clear? The setting panel thing wasn't a load-bearing part of my discussion, maybe I should have omitted that part.

To recap: Offline VR/XR is not currently supported. It doesn't work, and it has never worked. One side-effect of that brokenness is browser console spam. This PR fixes that spam, so is a tiny monotonic improvement.

Adding offline VR support would be a new feature that requires new tests. I don't want to write those tests, because neither I nor my relevant users need that feature. Adding that feature & tests would distract from more important work. (The current use case for VR is teleop, which doesn't make sense when offline.)

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Maybe I wasn't clear? The setting panel thing wasn't a load-bearing part of my discussion, maybe I should have omitted that part.

To recap: Offline VR/XR is not currently supported. It doesn't work, and it has never worked. One side-effect of that brokenness is browser console spam. This PR fixes that spam, so is a tiny monotonic improvement.

Adding offline VR support would be a new feature that requires new tests. I don't want to write those tests, because neither I nor my relevant users need that feature. Adding that feature & tests would distract from more important work. (The current use case for VR is teleop, which doesn't make sense when offline.)

I can appreciate your point that it hasn't been tested. I'd argue it hasn't worked because of an oversight in the original PR and not by design. I can accept the idea that to offer it as a proper feature, it needs to be tested (even as horrible a medium as meshcat_manual_test is).

The PR clearly placed the VR stuff outside of the static block. That is clear intention. The fact that it went untested was a failure in the PR process. No one was thinking about the html mangling applied by the static html process.

If that PR had been thinking about static html, the additional code would've likewise been placed outside the communication block, the feature would've been available for static html, it would have been equally untested, but you wouldn't be proposing this change.

This PR's patch doesn't fix the defect in the original PR. This merely papers over a symptom. In fact, it swaps one defect for another. By placing this code within the communication block, it implies a relationship that wasn't intended and doesn't actually exist.

Your final point is its not worth your time to fix it for real. I can accept that. Lots of defects exist and they require priority. But the defect transformation has moved from one easily observable defect (about which no one else has ever complained) for a different defect (very difficult to detect). The only documentation is this discussion. At the very least, I feel you're obliged to put in a TODO or, better yet, an issue.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

---

geometry/meshcat.html line 177 at r1 (raw file):
Ah, I see your perspective now.

To me, this was never intended to be a working feature.
To you, it was supposed to be working, but had a bug.
The docs aren't clear one way or another.

I've squared that away by clarifying the docs to say it's not supported, along with a hint in the HTML.

The PR clearly placed the VR stuff outside of the static block. That is clear intention... By placing this code within the communication block, it implies a relationship that wasn't intended and doesn't actually exist.

I didn't read it that way. Those PRs were a big pile of confusion. Some of it leaked into the code and never got wiped up. Since nobody ever tried using offline xr, I think it's fair to decide that it wasn't supposed to be working.

The code is self-consistent again now -- offline mode is future work.

(about which no one else has ever complained)

That indicates a hole in our testing. I've amended the manual test now to monitor browser console output.

[SeanCurtis-TRI]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ah, I see your perspective now.

To me, this was never intended to be a working feature.
To you, it was supposed to be working, but had a bug.
The docs aren't clear one way or another.

I've squared that away by clarifying the docs to say it's not supported, along with a hint in the HTML.

The PR clearly placed the VR stuff outside of the static block. That is clear intention... By placing this code within the communication block, it implies a relationship that wasn't intended and doesn't actually exist.

I didn't read it that way. Those PRs were a big pile of confusion. Some of it leaked into the code and never got wiped up. Since nobody ever tried using offline xr, I think it's fair to decide that it wasn't supposed to be working.

The code is self-consistent again now -- offline mode is future work.

(about which no one else has ever complained)

That indicates a hole in our testing. I've amended the manual test now to monitor browser console output.

The changes you've made make it clear that we understand one another. I'm perfectly content with this resolution.

---

geometry/test/meshcat_manual_test.cc line 207 at r2 (raw file):

  std::cout << R"""(
Open the developer tools of your browser (F12) and within that panel switch to

BTW I like this addition the most.

[jwnimmer-tri]

+@sammy-tri for platform review per schedule, please.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignees sammy-tri(platform),SeanCurtis-TRI(platform)

---

geometry/test/meshcat_manual_test.cc line 207 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I like this addition the most.

Yup, I was going to do this in a future PR but the discussions helped me realize it was important to front-load it, as the most important piece.

[sammy-tri]

(confirmed the error is gone)

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)

[jwnimmer-tri]

@SeanCurtis-TRI ping for LGTM emoji?

[SeanCurtis-TRI]

Sorry about that. I got distracted.

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