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: Make the transform matrix / texture optional #29084

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

nkallen
Copy link
Contributor

@nkallen nkallen commented Aug 8, 2024

Related issue: #29036

Description

This is a proposal to make the batching matrix texture optional. This diff has no other changes.

Copy link

github-actions bot commented Aug 8, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
685.1 kB (169.6 kB) 685.4 kB (169.7 kB) +233 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
462 kB (111.5 kB) 462.2 kB (111.5 kB) +233 B

@gkjohnson
Copy link
Collaborator

There should be some changes needed to BatchedMesh in order to prevent the class from creating a matrix texture until it's needed, right?

@nkallen
Copy link
Contributor Author

nkallen commented Aug 9, 2024

If you want me to add that, I'm happy to.

I currently access this functionality by doing the following:

export class MyBatchedMesh extends THREE.Mesh {
    isBatchedMesh = true;

    constructor(private readonly _maxInstanceCount: number, geometry?: THREE.BufferGeometry) {
        super(geometry);
    }

    readonly _multiDrawStarts = new Int32Array(this._maxInstanceCount);
    readonly _multiDrawCounts = new Int32Array(this._maxInstanceCount);
    _multiDrawCount = 0;

    _multiDrawInstances: Int32Array | null = null;
    _colorsTexture = null;
    _matricesTexture = null;
}

@nkallen
Copy link
Contributor Author

nkallen commented Aug 9, 2024

... I suppose this relates to the previous discussion about points and lines. I'm trying to be surgical/conservative in the changes to three.js. If you guys want a more specific design/direction, please let me know.

@gkjohnson
Copy link
Collaborator

If this change is going to be made it should be made in a way that enables it be used by end users and not just applications that know about the undocumented functionality.

Before more work is done I'll let @Mugen87 weigh in but I support the changes to BatchedMesh considering the benefits outlined in #29036 (comment).

@nkallen
Copy link
Contributor Author

nkallen commented Aug 9, 2024

Hi,

I will mirror this behavior in BatchedMesh:

	setColorAt( instanceId, color ) {

		if ( this._colorsTexture === null ) {

			this._initColorsTexture();

		}

But yes please confirm that you all are happy with this direction before I do it

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 9, 2024

TBH, I always have a hard time to approve changes which are so specialized that only a handful of developers will find the feature useful. However the component gets more complex for everyone and I'm not sure the benefits of #29036 (comment) outweighs the above fundamental issue.

That said, I'm somewhat sick of being the spoiler when others want features get merged so I want to defer to @mrdoob here. I can accept if these changes land in BatchedMesh but I'm unsure about the general direction of this component. I have the feeling its getting too complex and over-engineered.

@nkallen
Copy link
Contributor Author

nkallen commented Aug 9, 2024

I understand -- I am pushing what seems like a lot of unusual usecases here. But -- and this is just my opinion but -- multidraw is not some weird exotic thing. It's a classic feature of opengl and it is the classic way to render large scenes. It is recommended in lots of nvidia published best practices literature, in siggraph presentations, etc. -- and I would say generally any scene with 1k or more objects should strongly consider using it. And it doesn't need to be hard to use or weird at all.

Three.js currently has a very rich BatchedMesh implementation. It is a bit overspecialized from my perspective, but it really doesn't need much work to expose multidraw to all threejs users, from meshes, points, lines, etc.

@gkjohnson
Copy link
Collaborator

I'm unsure about the general direction of this component. I have the feeling its getting too complex and over-engineered.

Can you explain what you'd prefer for an API? Batching is a fairly crucial and expected part of modern graphics pipelines and it's a complex topic. Most engines will do this batching for you (for both static and dynamic objects) by finding objects with common materials and automatically merging them into an underlying buffer to render. Beyond being a bit "too smart" for my taste, adding functionality into three.js' renderer would only balloon the maintenance complexity significantly. In my opinion it took far too long to become available. It's a rendering technique that enables projects that would otherwise be performance prohibitive on a lot of platforms.

Compressing and "defragging" or remaking these buffers, as #28638 does, is something engines will typically do for you, as well, and important for larger game worlds that may want to cycle assets in and out of the buffer as new areas are travelled between. I would like to figure out how to overcome this issue of code being too complex when the underlying issue is inherently complicated. Should it be moved to examples again? Would separating the class into several components help somehow? I'm also interested to hear @mrdoob's opinion here, as well.

It is a bit overspecialized from my perspective

I'm curious as to what you'd change, as well. The BatchedMesh interface is designed to be fairly flexible while still exposing an interface that can be used by users who don't know how or don't care to manage buffers manually or write shaders to get things working, as much of the three.js API is designed.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 12, 2024

Batching is of course fine and I'm more than happy to finally have BatchedMesh in three.js. I just fear if we add a too complex API with too many configuration options, the maintenance could be negatively affected.

@nkallen
Copy link
Contributor Author

nkallen commented Aug 12, 2024

For our needs I would prefer threejs expose the building blocks in addition to an end-to-end solution. We would really like an interface like this

interface Object3D { isBatch: boolean; multiDrawStarts?: Int32Array; ... }

. then ensure the various shaders are compatible with multidraw,

. then provide the tools to use textures for uniforms like matrices and colors.

A bit more like BufferGeomtryUtils

BatchedMesh could then be a wrapper tying all this together. If you choose to have an allocator built in, I think it might also make sense to implement it as part of BufferGeometryUtils. For our part we have a custom allocator on top of a GLBufferAttribute; our allocator is a port of something written by Sebastian Aaltonen and allows us to have a dynamic scene (objects created+destroyed) without the need to defrag.

So that said -- how much needs to change to make this work? Probably very little. A few dozen lines of code, maybe move a couple functions into Utils instead of where they are now. Or it could be a gradual refactoring process, motivated by concrete use-cases that arise in practice...

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.

3 participants