-
-
Notifications
You must be signed in to change notification settings - Fork 35.7k
WEBGPU: make batchMesh._matricesTexture optional #30990
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
base: dev
Are you sure you want to change the base?
WEBGPU: make batchMesh._matricesTexture optional #30990
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
); | ||
let batchingMatrix = mat4(); | ||
|
||
if ( matricesTexture !== null ) { |
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.
@gkjohnson Is BatchedMesh._matricesTexture
supposed to be optional?
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.
more info why I need it to be optional
Im using a custom batched mesh and set it to be .isBatchedMesh = true;
and just need to control _multiDrawCounts
/ _multiDrawStarts
/ _multiDrawCount
mye self.
export class CustomBatchedMesh extends Mesh {
constructor(
geometry: BufferGeometry,
material: MeshStandardMaterialExt,
multiDrawCounts: Int32Array,
multiDrawStarts: Int32Array,
multiDrawCount: number,
isSelected: boolean,
isTransformed: boolean,
opacity_override: number
) {
super(geometry, material as any);
this.material = material;
this.isBatchedMesh = true;
this._multiDrawCounts = multiDrawCounts;
this._multiDrawStarts = multiDrawStarts;
this._multiDrawCount = multiDrawCount;
//just to override defaults
this._matricesTexture = null;
this._indirectTexture = null;
this._colorsTexture = null;
Been working very nice to keep drawcalls down on merged mesh, and would be nice to start trying out webgpu in my application without needing to modify threejs on each release
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.
Sorry, I'm not sure we should support this custom design. Instead, I wonder if we could modify BatchedMesh
to accommodate your use case and then provide proper support in the renderers.
Do you mind explaining in more detail why you can't use BatchedMesh
and what parts had to be changed?
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.
I really need to be able to use webgl multidraw. No need for different world transformations/different colors on merged mesh. So basicly dont need any of these:
_matricesTexture
_indirectTexture
_colorsTexture
Batchmesh already do same check for colorsTexture here:
three.js/src/nodes/accessors/BatchNode.js
Line 115 in d4c1585
if ( colorsTexture !== null ) { |
and here:
three.js/src/renderers/common/RenderObject.js
Lines 713 to 717 in d4c1585
if ( object._colorsTexture !== null ) { | |
cacheKey += object._colorsTexture.uuid + ','; | |
} |
So Ive only added a additional check, so Batchmesh can be used without the textures without any modifications to the core. This wont really break anything from what I can tell.
Ofc, @gkjohnson would be the best guy to ask to be sure :-)
Really just want to start trying out webgpu more with minimal changes to my application. Its a lot more stable and faster now 😄
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.
Have you tried creating an empty texture and using other means for indirection or batched values (e.g., a UBO)?
Note that although the texture will still be created on the GPU, only active uniforms count towards shader resource limits.
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.
From what I can tell it expects some indirect id, so I would then need to also populate more things I dont need to use multidraw part.
three.js/src/nodes/accessors/BatchNode.js
Line 99 in d4c1585
const j = float( indirectId ).mul( 4 ).toInt().toVar(); |
Would it not just be best to have 2 simple checks like colortexture have, and if not used just skip logic around it ?
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.
@gkjohnson Is BatchedMesh._matricesTexture supposed to be optional?
It's not currently but I'm not against it. It was also requested in #29084.
Sorry, I'm not sure we should support this custom design. Instead, I wonder if we could modify BatchedMesh to accommodate your use case and then provide proper support in the renderers.
I agree that understanding the use cases here would be great if we can fold some of these improvements into the core batched mesh.
@vegarringdal Can you explain what kinds of problems the core BatchedMesh implementation is currently causing you? I understand having textures that are unused seems wasteful but what issues is it causing you in practice?
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.
@gkjohnson
Dealing with large bim models.
Mosty do everything to keep memory low, and still keep models loaded on gpu. So have custom way to deal with colors/transformation, use controller "group" per geometry that generates custom meshes.
Only use flatshading, models only on gpu/local postion/indecies are removed. And all drawcall/state in created wasm worker and it updates meshes a lot when they do threeselections/coloring.
For me its just great if batchmesh helps us draw the multidraw part. 😄
Im sure I want to look more into using textures more with tsl, but guess I would end up with more textures, own for colorID, one for transformID, and own textures for transformationMatrix, ColorData(color, opacity...etc). Since those would be shared with many meshes based on user selection/coloring.
Really dont know enough yet, so I migth be very wrong on my ideas here..
Maybe this part should be more dynamic, so its easier to add more textures without core changes ?
String array on names of textures to add ?
three.js/src/renderers/common/RenderObject.js
Lines 705 to 719 in d4c1585
if ( object.isBatchedMesh ) { | |
if ( object._matricesTexture !== null ) { | |
cacheKey += object._matricesTexture.uuid + ','; | |
} | |
if ( object._colorsTexture !== null ) { | |
cacheKey += object._colorsTexture.uuid + ','; | |
} | |
} |
Love threejs and batchedmesh have been very great.
Would just be even greater if we had a check _matricesTexture
was null or not for now, so I can use it for my needs without making a branch/build.
I dont use
batchMesh._matricesTexture
and cant switch to webgpu without modificationsWith these changes it works