Lazy initialization of the upAdjustment matrix (Three.js) #601
Conversation
Nmzik
commented
Jul 5, 2024
•
Nmzik
commented
- Instead of creating a new Matrix4 per tile, reuse it.
- Pass the upAdjustment matrix to each loader constructor, keeping a reference to it without instantiating and copying the matrix itself.
There was a problem hiding this comment.
Thanks! I'd prefer not to change the Loader constructor signatures, though, and think we should keep the new Matrix4 on the class. There's already a ton of memory allocation that's going to necessarily happen during during loading and parsing of model geometry and one more Matrix4 is going to be really minor.
src/three/TilesRenderer.js
Outdated
| this.upAdjustment = null; | ||
|
|
There was a problem hiding this comment.
Can we move this to a global value, instead? See tempMat etc above.
| loader.adjustmentTransform.copy( upAdjustment ); | ||
| loader.adjustmentTransform = upAdjustment; |
There was a problem hiding this comment.
Let's still use the "copy" function instead of assigning the variable.
| const cached = tile.cached; | ||
| const cachedTransform = cached.transform; | ||
|
|
There was a problem hiding this comment.
Is there a reason these were moved? It would be best to only perform a minimal amount of changes in a PR, I think, and avoid unneeded code organization updates. IT makes it easier to understand what's changing and why.
| @@ -24,6 +24,7 @@ const INITIAL_FRUSTUM_CULLED = Symbol( 'INITIAL_FRUSTUM_CULLED' ); | |||
| const tempMat = new Matrix4(); | |||
| const tempMat2 = new Matrix4(); | |||
| const tempVector = new Vector3(); | |||
| let upAdjustment = null; | |||
There was a problem hiding this comment.
Let's just create the matrix4 here. Only creating something like a matrix4 when needed is a bit of an over optimization, I think, especially when we're creating other variables. In fact we can probably just use of the existing tempMat objects as long as it's not being used in the parseTiles function, already.
