Skip to content
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

BatchedMesh: add per-instance setOpacity #28151

Closed
wants to merge 2 commits into from

Conversation

Kikedao
Copy link

@Kikedao Kikedao commented Apr 17, 2024

Description

This PR adds per-instance setOpacity and getOpacity to BatchedMesh.

I created this PR inspired by these two threads from the forum:
https://discourse.threejs.org/t/batchedmesh-per-geometry-opacity/61284
https://discourse.threejs.org/t/batchedmesh-set-per-instance-opacity/64170

I tested it in the project I'm working on and it seems to work wonders with absolute perfect performance.

If you are fine with this implementing setColorAt shouldn't be too difficult, and with that the InstancedMesh and BatchedMesh APIs would finally converge :).

Here is a video capture of it animating individually the opacity of all instances in the BatchedMesh with the help of gsap (the links you see in the graph are tube geometries all used in the same BatchedMesh):

2024-04-17.23-47-32_1_4.mp4

Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
674.7 kB (167.2 kB) 675.9 kB (167.3 kB) +1.2 kB

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
454.1 kB (109.6 kB) 454.6 kB (109.7 kB) +472 B

@Kikedao Kikedao changed the title Batchedmesh: add per-instance setOpacity BatchedMesh: add per-instance setOpacity Apr 18, 2024
@Kikedao
Copy link
Author

Kikedao commented Apr 18, 2024

Worth mentioning that the material used in the BatchedMesh of the video is as simple as this:

this.three.material = new THREE.MeshLambertMaterial({ 
    color: 0xffffff,
    transparent: true,
    vertexColors: true
});

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 29, 2024

TBH, I don't like the idea of moving material properties to BatchedMesh as suggested. This will only blow up the API of BatchedMesh and transform the implementation to a hard to maintain code base. As implied in this comment by @gkjohnson, it's seems viable to investigate a more generalized approach.

@Kikedao
Copy link
Author

Kikedao commented Apr 29, 2024

I get your point, yet here is how I see it:
The problem with BatchedMesh is that the only way to be able to 'set' any kind of per-instance property is to use textures. BatchedMesh implementation is already doing that for its setMatrixAt method to be able to position the instances, encoding the per-instance matrices in a texture.
The let down for me was discovering that BatchedMesh doesn't have the same API as InstancedMesh for setting things per-instance:

  • InstancedMesh has setMatrixAt and setColorAt
  • BatchedMesh has setMatrixAt and setVisibleAt (yet setVisibleAt is a bit special...)

So my take is to just converge both classes APIs so they support at least setMatrixAt, setColorAt, and as I suggest in this PR setOpacityAt, which I think should be added to InstancedMesh too for completeness.
I find being able to set these three properties per-instance in both classes is the minimum you would expect to be able to manipulate them a little bit and control how the instances are displayed for a lot of usages.

At least I think we should add setColorAt, it makes no sense that InstancedMesh has it and not BatchedMesh.
If you agree BatchedMesh should have a setColorAt method just as InstancedMesh does I can create a new PR to add it, just want to know if you agree so I don't do it for nothing (then we can discuss if setOpacityAt would be also useful to be added to both classes).

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 29, 2024

It feels consistent to align both APIs and add setColorAt() to BatchedMesh. setColorAt() should be RGB only though (so no alpha).

@gkjohnson Are you okay with that?

@gkjohnson
Copy link
Collaborator

It feels consistent to align both APIs and add setColorAt() to BatchedMesh
...
@gkjohnson Are you okay with that?

I'll defer to you but my feeling is that this is all already possible with shader overrides, etc as my prior demo shows. I'd rather see some thought put into what a more extensible API would look like to support per-object material properties. Even with InstancedMesh users were quickly asking for other material setting support if I recall correctly.

If you'd just like to just add setColorAt to align with InstancedMesh I won't block it. I don't think I support adding more property support like opacity, though, without considering some more robust solutions to this issue.

@Kikedao
Copy link
Author

Kikedao commented May 1, 2024

Thx for your feedback @Mugen87 and @gkjohnson, I will then close this PR and add just setColorAt for BatchedMesh in a new one.

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.

None yet

3 participants