Skip to content

Conversation

wrangelvid
Copy link
Contributor

@wrangelvid wrangelvid commented Apr 24, 2025

This change is Reviewable

@jwnimmer-tri jwnimmer-tri self-assigned this Apr 24, 2025
Copy link
Contributor

@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: 0 of 5 files reviewed, 1 unresolved discussion


dist/main.min.js.THIRD_PARTY_LICENSES.json line 40 at r1 (raw file):

  {
    "name": "three",
    "version": "0.163.0",

Here we can see that the lock file has two different versions of three added to the bundle (176 and 163). We have an invariant that there must be exactly one version for Meshcat.

This is usually because one of the other dependencies like wwobjloader2 specifies a specific version of three, so usually we also need to upgrade those other dependencies in lockstep so that all bundled code agrees on a uniform version of three.

Copy link
Contributor

@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: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @wrangelvid)


dist/main.min.js.THIRD_PARTY_LICENSES.json line 40 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Here we can see that the lock file has two different versions of three added to the bundle (176 and 163). We have an invariant that there must be exactly one version for Meshcat.

This is usually because one of the other dependencies like wwobjloader2 specifies a specific version of three, so usually we also need to upgrade those other dependencies in lockstep so that all bundled code agrees on a uniform version of three.

(See #180 for past discussion.)

Copy link
Contributor Author

@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.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)


dist/main.min.js.THIRD_PARTY_LICENSES.json line 40 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(See #180 for past discussion.)

Good catch! I should've looked into the console. I wasn't quite sure why we use wwobjloader2 in the first place ... so I removed it as the test files provided the same visual output. Alternatively, I would propose using overrides in yarn to force using a single version of three.

Copy link
Contributor Author

@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.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)


dist/main.min.js.THIRD_PARTY_LICENSES.json line 40 at r1 (raw file):

Previously, wrangelvid (David von Wrangel) wrote…

Good catch! I should've looked into the console. I wasn't quite sure why we use wwobjloader2 in the first place ... so I removed it as the test files provided the same visual output. Alternatively, I would propose using overrides in yarn to force using a single version of three.

Ahh. It looks like ./cube.mtl has been missing!

Copy link
Contributor

@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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @wrangelvid)


dist/main.min.js.THIRD_PARTY_LICENSES.json line 40 at r1 (raw file):

Previously, wrangelvid (David von Wrangel) wrote…

Ahh. It looks like ./cube.mtl has been missing!

The https://www.npmjs.com/package/wwobjloader2#feature-overview lists some features that might be relevant but not covered by the current test cases (for more arcane OBJ features). Switching out which OBJ loader we use will require some non-trivial testing latency by several stakeholders.

If there is a version of wwobjloader2 that is compatible with the three we want, that might be a safer path. It looks like as of version 5 they no longer pin three, so anything 5 or newer might work?

Copy link
Contributor Author

@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.

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)


dist/main.min.js.THIRD_PARTY_LICENSES.json line 40 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The https://www.npmjs.com/package/wwobjloader2#feature-overview lists some features that might be relevant but not covered by the current test cases (for more arcane OBJ features). Switching out which OBJ loader we use will require some non-trivial testing latency by several stakeholders.

If there is a version of wwobjloader2 that is compatible with the three we want, that might be a safer path. It looks like as of version 5 they no longer pin three, so anything 5 or newer might work?

Fair. I addressed the multiple three import in the resolutions.

Copy link
Contributor

@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 confirmed that I could rebuild the minified dist file and it came out identical to what was pushed here.

Reviewed 5 of 5 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @wrangelvid)


a discussion (no related file):
Working

All of the code and tests here look good. Next step is to open a Drake PR with the git sha bump and do all of the Drake tests. I'll open that PR momentarily.

Copy link
Contributor Author

@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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

All of the code and tests here look good. Next step is to open a Drake PR with the git sha bump and do all of the Drake tests. I'll open that PR momentarily.

Does it make sense to use the sha from #190, do the tests in drake s.t. we can land both in drake at once?

Copy link
Contributor

@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: all files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)


a discussion (no related file):

Previously, wrangelvid (David von Wrangel) wrote…

Does it make sense to use the sha from #190, do the tests in drake s.t. we can land both in drake at once?

Before any PR lands, all tests including Drake must pass, so the only different tactic would be to combine both PRs into one, but I think it's better to keep them separate.

Copy link
Contributor Author

@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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Before any PR lands, all tests including Drake must pass, so the only different tactic would be to combine both PRs into one, but I think it's better to keep them separate.

Sounds good.

Copy link
Contributor

@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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @wrangelvid)

@jwnimmer-tri jwnimmer-tri merged commit 109a4e2 into meshcat-dev:master Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants