-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add InstancedMesh example #187
base: master
Are you sure you want to change the base?
Add InstancedMesh example #187
Conversation
Also add itemSize: 16 for instanceMatrix
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: 0 of 1 files reviewed, all discussions resolved
a discussion (no related file):
+@joemasterjohn for review.
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 all commit messages.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions
a discussion (no related file):
Hi @siddancha, thanks for the contribution. Please consider some of the comments I left below.
test/instanced_mesh.html
line 21 at r1 (raw file):
type: "set_object", path: "/meshcat/instanced_cubes", object: {
I don't find much value in writing out JSON by hand for examples, especially JavaScript examples.
- It has to be manually updated to keep up with newer THREE.js object format versioning.
- It doesn't provide the user a method for generating their own JSON for their own purposes. It is still mysterious which properties must exist or how they should be formatted. Especially things like uuids etc.
I think an example written in JS that gives the user a way to spit out the JSON representation of whatever THREE.js object they're dealing with would be good. That would give them the reference to implement the object format correctly in whatever client code they are writing. Consider my update to your example here that does exactly that: https://github.com/joemasterjohn/meshcat/tree/instanced-mesh-example
If you agree, add me as a collaborator on your fork. I can push that commit (with co-author credits), and we can merge this very nice example.
test/instanced_mesh.html
line 100 at r1 (raw file):
// Set InstancedMesh properties. // Here, we are updating the colors of the cubes. viewer.handle_command({
This happens to work because the command executes before the first render call. Indeed if you put this call to set_property
into a setTimeout(..., 5000)
you won't see the color change. This is because any change to a buffer needs to be indicated by setting the needsUpdate
attribute to true (see https://threejs.org/docs/#manual/en/introduction/How-to-update-things).
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 1 of 1 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @siddancha)
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.
Thanks for building on this example -- your interactive demo is awesome! I'm not convinced that the example should be written in native Javascript/THREE.js code vs. handwritten JSONs; more details in my reply to your comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @joemasterjohn)
test/instanced_mesh.html
line 21 at r1 (raw file):
Previously, joemasterjohn (Joe Masterjohn) wrote…
I don't find much value in writing out JSON by hand for examples, especially JavaScript examples.
- It has to be manually updated to keep up with newer THREE.js object format versioning.
- It doesn't provide the user a method for generating their own JSON for their own purposes. It is still mysterious which properties must exist or how they should be formatted. Especially things like uuids etc.
I think an example written in JS that gives the user a way to spit out the JSON representation of whatever THREE.js object they're dealing with would be good. That would give them the reference to implement the object format correctly in whatever client code they are writing. Consider my update to your example here that does exactly that: https://github.com/joemasterjohn/meshcat/tree/instanced-mesh-example
If you agree, add me as a collaborator on your fork. I can push that commit (with co-author credits), and we can merge this very nice example.
Hi @joemasterjohn! Thanks for building on this example -- your interactive demo is awesome! Correct me if I'm wrong, but I still think that writing out the JSON is what we need to demo this in the Meshcat API. Here's my reasoning:
- I'm just following the other examples in the
test
folder, which also write out JSONs by hand. Instead, you created anInstancedMesh
object in native Javascript/THREE.js, and are passing its JSON to theset_object
command via the.toJSON()
call. None of the other test examples use the.toJSON()
call. - I think this makes sense because the way a non-JavaScript user would interact with the Meshcat API (e.g. via meshcat-python) is precisely by hand-writing JSONs and passing them to the
set_object
command. For example, I'm working on a PR to create InstancedMeshes in Meshcat from Drake: SEE CODE HERE. To implement this, I need to know what InstancedMesh's JSON looks like. Your native Javascript/THREE.js code unfortunately doesn't tell me much about what its JSON looks like and what fields I should set (THREE.js is to blame for that).
With this example (along the same lines as other examples) I'm hoping to expose what the nominal format of Meshcat's API would look like for InstancedMeshes. Does that make sense?
test/instanced_mesh.html
line 100 at r1 (raw file):
Previously, joemasterjohn (Joe Masterjohn) wrote…
This happens to work because the command executes before the first render call. Indeed if you put this call to
set_property
into asetTimeout(..., 5000)
you won't see the color change. This is because any change to a buffer needs to be indicated by setting theneedsUpdate
attribute to true (see https://threejs.org/docs/#manual/en/introduction/How-to-update-things).
Agreed! I agree that we need to set instanceMatrix.needsUpdate
after executing set_property
commands, like you did in your branch. I will copy that once the previous comment is resolved.
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.
Maybe we could include both my hand-written JSON example and your THREE.js based example?
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @joemasterjohn)
Resolves #141
The MeshCat API already implicitly supports threejs' InstancedMesh. Since InstancedMesh is a subclass of Object3D, the
set_object
API can be used to create an InstancedMesh of a given geometry and material.This PR adds an example (in the
test
folder) that creates an InstancedMesh of BoxGeometries arranged in a 3x3x3 grid. Furthermore, the example updates the color buffer of the InstancedMesh using theset_property
API. This showcases how one might update the colors (and similarly, poses) of instances in an InstancedMesh without deleting and creating a new InstancedMesh object.After

set_object
:After

set_property
:This change is