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

OutlinePass: Support meshes with grouped BufferGeometry #24152

Closed
wants to merge 2 commits into from
Closed

OutlinePass: Support meshes with grouped BufferGeometry #24152

wants to merge 2 commits into from

Conversation

ingun37
Copy link
Contributor

@ingun37 ingun37 commented May 28, 2022

Description

OutlinePass takes extra parameter selectedMaterialGroups: Map<Mesh, Material[]> which enables outlining each BufferGeometry Group. Each (Mesh, Material[]) pair represents which sub-geometries (identified by Materials) should be outlined.

outlinepass

I have tested various combinations of (Material.visible * material selection) and they all worked correctly so far.

Screen Shot 2022-05-28 at 3 39 40 PM

@ingun37
Copy link
Contributor Author

ingun37 commented May 28, 2022

Test failed at DeepScan so I checked the logs but they all seem to come from the unrelate files. What should I do? For example, I didn't change VOXLoader.js but I still get this error
3f3f38f4d0e60c18e481e1046133284e676ba744_2_690x226

@Mugen87
Copy link
Collaborator

Mugen87 commented May 28, 2022

Sorry, but I think this change introduces an inappropriate complexity to OutlinePass. The class is already hard to maintain. And with this change, it becomes even worse.

@mrdoob This is another good example why I think multi-material support should be removed.

@ingun37 If possible, please avoid using BufferGeometry.groups in your scenario and represent each group as a single mesh. You can use SceneUtils.createMeshesFromMultiMaterialMesh() for this purpose.

@ingun37
Copy link
Contributor Author

ingun37 commented May 28, 2022

@Mugen87 I have a question. If I use separate Mesh then how can I share index buffer between different Meshes? I thought the point of using BufferGeometry.Groups was in there.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 28, 2022

I don't think this was the primary intention. It's more a side effect that occurs when using a multi-material mesh.

However, you still end up with multiple draw calls. Splitting up the index buffer into multiple ones or just using non-indexed geometries right from the beginning should have no noticeable impact on performance. It should also be easier to handle and process geometry data (although this is somewhat subjective).

@Mugen87
Copy link
Collaborator

Mugen87 commented May 28, 2022

Since we have stated to not add multi-material specific logic to example modules like exporters (see #22555), it's only consistent to apply the same principle to post processing passes.

@Mugen87 Mugen87 closed this May 28, 2022
@ingun37
Copy link
Contributor Author

ingun37 commented May 28, 2022

@Mugen87 In my case there are many Meshes associated to the same geometry but different index ranges and materials. Interestingly enough I recently compared two versions of my library, one avoid dividing geometry by utilizing BufferGeometry.groups, and the other divide geometry just like what SceneUtils.creameMeshesFromMultiMaterialMesh() does. The GPU memory usage difference was dramatic. Former (BufferGeometry.groups) took 700mb while the other took 1200mb. (V8 heap memory had about the same benefit as well). I think the performance advantage comes from less fragmentation and buffer binding.

Not only performance but code also had gotten much tidy being somewhat more natural data structure for my case.

It's sad to hear you are planning to remove BufferGeometry.groups.🥲 I wish there will be some sort of equivalent. Can I know when will it deprecated?

@Mugen87
Copy link
Collaborator

Mugen87 commented May 28, 2022

Can I know when will it deprecated?

There are no concrete plans yet (e.g. a fixed release).

Do you mind demonstrating the performance differences with two live examples?

@ingun37
Copy link
Contributor Author

ingun37 commented May 28, 2022

@Mugen87 sure, ill try and let you know

@Mugen87
Copy link
Collaborator

Mugen87 commented May 28, 2022

Sidenote: SceneUtils.creameMeshesFromMultiMaterialMesh() produce geometries with an own set of buffer attributes. This was done since it is the most straightforward approach and appropriate for most use cases.

In certain use cases however, it might be more resource efficient to reuse buffer attributes across geometries and use BufferGeoemtry.drawRange in order to decide what part of the buffer data should be rendered. However, this is the more complicated setup.

@ingun37 ingun37 deleted the grouped-geometry-outlinepass branch May 28, 2022 11:52
@ingun37
Copy link
Contributor Author

ingun37 commented May 28, 2022

@Mugen87 Here's the live examples.

  • Environment
    • EC2 g4dn instance with NVIDA GPU
    • Headless Chromium with GPU Acceleration
    • I used nvidia-smi to profile GPU
  • Use BufferGeometry.groups.
  • Use SceneUtils.createMeshesFromMultiMaterialMesh

They try to load the same dummy data with the same code except the second one converts using SceneUtils.createMeshesFromMultiMaterialMesh at the end then dispose the original geometry. 382MB to 1144MB is significant. I think BufferGeometry.groups definitely has advantages in performance.

In certain use cases however, it might be more resource efficient to reuse buffer attributes across geometries and use BufferGeoemtry.drawRange in order to decide what part of the buffer data should be rendered.

I have question to this comment. Does sharing BufferAttribute with different drawRange work in the same way as BufferGeometry.groups internally? Will it have the same performance?

a

b

@Mugen87
Copy link
Collaborator

Mugen87 commented May 28, 2022

Does sharing BufferAttribute with different drawRange work in the same way as BufferGeometry.groups internally?

Basically yes. The draw range is just defined differently (not via groups but drawRange).

@ingun37
Copy link
Contributor Author

ingun37 commented May 29, 2022

@Mugen87 Just letting you know to thank you that I changed from using BufferGeometry.groups to sharing BufferAttributes and it works perfectly, retains the same performance, and code got even tidier! As I was working on it I felt too that BufferGeometry.groups might be bringing unnecessary complexity. Also it didn't feel like "the more complex setup" at all. 😄

@Mugen87
Copy link
Collaborator

Mugen87 commented May 30, 2022

Good to hear that!

I'm also glad you have made the performance analysis since it demonstrates the limitations of SceneUtils.creameMeshesFromMultiMaterialMesh(). If somebody else runs into the same performance issue, we know what to do now.

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

2 participants