Skip to content

Conversation

@jwnimmer-tri
Copy link
Contributor

@jwnimmer-tri jwnimmer-tri commented Feb 6, 2024

These were accidentally lost in the switch to msgpack-javascript (#132).

Closes #138.

+@SeanCurtis-TRI for both reviews, please.


It's not really practical to test this with an HTML file in this repository. The problem is with the msgpack decoding, which we usually don't test here (we author JSON in tests, not msgpack).

Instead, I tested it manually with this Python program that uses meshcat-python:

import meshcat
import meshcat.geometry as g

vis = meshcat.Visualizer()
vis["path"].set_object(
    g.StlMeshGeometry.from_file("/home/jwnimmer/jwnimmer-tri/drake/geometry/test/quad_cube.stl"),
    g.MeshPhongMaterial())

import time
time.sleep(60)

With the master branch meshcat, it errors out with no cube (the browser console has an error message). With this branch, the browser shows the cube.

I didn't test the int32 decoder. (I can't find anywhere its used.) Hopefully the visual inspection and copy-paste from Uint32 is sufficient.


This change is Reviewable

These were accidentally lost in the switch to msgpack-javascript.
@jwnimmer-tri jwnimmer-tri force-pushed the msgpack-ext-types-fix branch from 3e4da12 to 4f3f184 Compare February 6, 2024 03:52
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.

:LGTM: X2

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jwnimmer-tri)


src/index.js line 38 at r1 (raw file):

    const to_return = new Uint8Array(data.byteLength);
    let dataview = new DataView(data.buffer, data.byteOffset, data.byteLength);
    for (let i = 0; i < to_return.length; i++) {

BTW Is this a javascript thing instead of ++i?

Code quote:

i++

Copy link
Contributor Author

@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 @jwnimmer-tri)


src/index.js line 38 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Is this a javascript thing instead of ++i?

TBH, I have no idea. I quick peek at stack overflow suggests that in JS it doesn't matter. The index.js is currently divided with about half of the loops doing it each way.

@jwnimmer-tri jwnimmer-tri merged commit d9a2319 into meshcat-dev:master Feb 6, 2024
@jwnimmer-tri jwnimmer-tri deleted the msgpack-ext-types-fix branch February 6, 2024 23:20
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