BatchedMesh: Add "toJSON" and "copy" support #27131
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
src/core/Object3D.js
Outdated
| object._geometryInitialized = this._geometryInitialized; | ||
| object._geometryCount = this._geometryCount; | ||
|
|
||
| object._matricesTexture = this._matricesTexture.toJSON(); |
There was a problem hiding this comment.
Can we avoid serializing the internal texture? Ideally, it is just recreated in BatchedMesh if it is missing.
Besides, can we avoid the underscore syntax in the JSON object?
Also, at some point this logic should be moved to BatchedMesh.toJSON(). Object3D should not access private variables of subclasses.
There was a problem hiding this comment.
Can we avoid serializing the internal texture? Ideally, it is just recreated in BatchedMesh if it is missing.
This texture is how the matrices are canonically stored. I can generate a list of serialized matrices from the texture instead, though, if that's acceptable.
Besides, can we avoid the underscore syntax in the JSON object?
Yup! I'll fix it.
Also, at some point this logic should be moved to BatchedMesh.toJSON(). Object3D should not access private variables of subclasses.
I thought so, too, but this looked like how all the other classes like InstancedMesh were working. Though it's unclear to me why this is needed.
There was a problem hiding this comment.
This texture is how the matrices are canonically stored. I can generate a list of serialized matrices from the texture instead, though, if that's acceptable.
I forgot after #27130 there is only the texture that holds the matrices. In this case, I'm fine with serializing directly the texture.
I thought so, too, but this looked like how all the other classes like InstancedMesh were working. Though it's unclear to me why this is needed.
We can try to find a different solution in the future. Just wanted to note it in this PR.
There was a problem hiding this comment.
BTW: ObjectLoader should also be able to handle BatchedMesh. Maybe we can add this to the todo list.
|
Sorry, approved a bit too early. The removal of the underscore in the JSON would still be nice^^! |
Fixed!
Added to the list! |
| @@ -563,17 +563,29 @@ class BatchedMesh extends Mesh { | |||
|
|
|||
| } | |||
|
|
|||
| copy() { | |||
| copy( source ) { | |||
There was a problem hiding this comment.
Can we have copy return this to make it consistent with parent classes Object3D and Mesh?
Related issue: #22376, #27111
Description
Add support for "toJSON" and "copy" to "BatchedMesh".
Upcoming PRs