Skip to content

Docs: Clarify BufferGeometry.groups. #23740

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

Merged
merged 3 commits into from
Mar 22, 2022
Merged

Docs: Clarify BufferGeometry.groups. #23740

merged 3 commits into from
Mar 22, 2022

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 17, 2022

Fixed #22555.

Description

I'd like to clarify the policy of using BufferGeometry.groups:

When groups are defined, it is invalid when parts of the vertex data are in groups but others are not. Such partial definitions are problematic for logic operating on group data. It is also not valid when groups overlap.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 17, 2022

Such partial definitions are problematic for logic operating on group data.

That's a surprise to me - I'd assumed groups could be resized smaller (like .drawRange) safely. Are you referring to renderer logic, exporter logic, or runtime operations like bounding boxes and raycasting?

For another example, is it OK if all indices are used but they only point to only a subset of the vertices? This may need to be explained in a bit more detail.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 17, 2022

I've planned to add a helper for converting a multi-material mesh into several meshes. This feature requires merging of groups for a proper implementation. Not having the above policy makes an implementation noticeably more complex. Hence, I'd like to have a more strict usage of groups and avoid unnecessary complexity.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 17, 2022

For another example, is it OK if all indices are used but they only point to only a subset of the vertices?

What is the use case of such a setup? TBH, I do not want to honor use cases which are theoretically possible just because the API currently allows it but are not relevant in production. There should always be a compelling reasons why edges cases should be supported.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Mar 17, 2022

What is the use case of such a setup? TBH, I do not want to honor use cases which are theoretically possible ...

This is a very real use case, at least for geometry that does not use groups. For example, I might define a large vertex buffer, and then have several meshes drawn with different materials, each using indices that point to a subset of the vertex buffer. Is that a problem for the helper you want to add? Or only a problem if the mesh has ≥2 groups? GLTFLoader can create meshes like this, but never creates groups.

Personally I'm lukewarm on multi-material meshes and geometry.groups. I feel like they might be more maintenance burden than they're worth, and don't pair well with APIs like InstancedMesh or BatchedMesh. I certainly don't feel like grouped geometry needs to be supported in every API.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 17, 2022

Is that a problem for the helper you want to add?

No it isn't. As long as the groups cover the entire index (so there are no gaps).

WebGPURenderer does not support groups/multi materials and there is no plan so far to add this feature. The helper should make it possible to convert multi-material meshes to a representation that can be handled by both renderers. It is also useful for exporters, see #22555.

Eventually, I hope we can stop multi-material support in WebGLRenderer at some point. The helper is then required in any case.

@donmccurdy
Copy link
Collaborator

I wonder if it would be worthwhile to log some kind of warning when the renderer detects a geometry that doesn't meet this requirement. That might help us to discover any use cases we've missed before enforcing this rule. I can't think of use cases myself but it would surprise me if there weren't a few...

In any case, I'm comfortable with this PR. 👍

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 20, 2022

I wonder if it would be worthwhile to log some kind of warning when the renderer detects a geometry that doesn't meet this requirement.

I'm afraid it will be a bit problematic to detect such geometries since a reliable check needs to be done per frame. And the check itself is not that trivial. Right now, I recommend to not enhance the renderer with validation checks and wait how many users actually complain about the upcoming BufferGeometryUtils.mergeGroups() and SceneUtils.createMeshesFromMultiMaterialMesh().

@donmccurdy
Copy link
Collaborator

Sounds good to me!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 22, 2022

Since #23756 has been merged, this PR can be merged, too.

@Mugen87 Mugen87 merged commit 5b5d64f into mrdoob:dev Mar 22, 2022
@Mugen87 Mugen87 added this to the r139 milestone Mar 22, 2022
@iuriiiurevich
Copy link
Contributor

iuriiiurevich commented Apr 5, 2022

@Mugen87
I am a bit confused... Isn't it ok to intersect geometry groups?
So this pattern is now considered bad?
Why? As far as I can see it works well.

By the way, thank you for all your work!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 5, 2022

Yes, please don't do that anymore. The reason for this is mentioned above. I would recommend to not use groups at all but model meshes independently. You can still share geometry or buffer attribute data between them.

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Docs: Clarify BufferGeometry.groups.

* Docs: More updates to BufferGeometry.groups.

* Docs: Improve BufferGeometry.groups description.
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.

Provide multi-material support in exporters.
3 participants