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.

Sign up for GitHub

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

Jump to bottom

Lazy initialization of the upAdjustment matrix (Three.js) #601

Closed
wants to merge 5 commits into from
Closed

Lazy initialization of the upAdjustment matrix (Three.js) #601

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f3b5d8a
Reuse upAdjustment matrix between tiles
Nmzik Jul 5, 2024
9428136
pass upMatrix to the loader's constructor
Nmzik Jul 5, 2024
4f1bbfd
Update I3DMLoader
Nmzik Jul 5, 2024
ec10318
Revert constructor signatures
Nmzik Jul 6, 2024
c63026c
Move upAdjustment to the global scope
Nmzik Jul 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions src/three/TilesRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const X_AXIS = new Vector3( 1, 0, 0 );
const Y_AXIS = new Vector3( 0, 1, 0 );
Expand Down Expand Up @@ -551,23 +552,27 @@ export class TilesRenderer extends TilesRendererBase {
const loadIndex = tile._loadIndex;
let promise = null;

const upAxis = this.rootTileSet.asset && this.rootTileSet.asset.gltfUpAxis || 'y';
const cached = tile.cached;
const cachedTransform = cached.transform;
if ( ! upAdjustment ) {

const upAdjustment = new Matrix4();
switch ( upAxis.toLowerCase() ) {
const upAxis = this.rootTileSet.asset && this.rootTileSet.asset.gltfUpAxis || 'y';
upAdjustment = new Matrix4();
switch ( upAxis.toLowerCase() ) {

case 'x':
upAdjustment.makeRotationAxis( Y_AXIS, - Math.PI / 2 );
break;
case 'x':
upAdjustment.makeRotationAxis( Y_AXIS, - Math.PI / 2 );
break;

case 'y':
upAdjustment.makeRotationAxis( X_AXIS, Math.PI / 2 );
break;
case 'y':
upAdjustment.makeRotationAxis( X_AXIS, Math.PI / 2 );
break;

}

}

const cached = tile.cached;
const cachedTransform = cached.transform;

Comment on lines +573 to +575
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const fileType = ( readMagicBytes( buffer ) || extension ).toLowerCase();
switch ( fileType ) {

Expand All @@ -577,7 +582,7 @@ export class TilesRenderer extends TilesRendererBase {
loader.workingPath = workingPath;
loader.fetchOptions = fetchOptions;

loader.adjustmentTransform.copy( upAdjustment );
loader.adjustmentTransform = upAdjustment;
Comment on lines -580 to +585
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's still use the "copy" function instead of assigning the variable.


promise = loader.parse( buffer );
break;
Expand All @@ -600,7 +605,7 @@ export class TilesRenderer extends TilesRendererBase {
loader.workingPath = workingPath;
loader.fetchOptions = fetchOptions;

loader.adjustmentTransform.copy( upAdjustment );
loader.adjustmentTransform = upAdjustment;

promise = loader.parse( buffer );
break;
Expand All @@ -613,7 +618,7 @@ export class TilesRenderer extends TilesRendererBase {
loader.workingPath = workingPath;
loader.fetchOptions = fetchOptions;

loader.adjustmentTransform.copy( upAdjustment );
loader.adjustmentTransform = upAdjustment;

promise = loader
.parse( buffer )
Expand Down
Loading