Skip to content

Cleanup new Color/Matrix() #26024

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

Closed
wants to merge 9 commits into from
Closed

Cleanup new Color/Matrix() #26024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2fad0c0
Update NRRDLoader.js
LeviPesin May 9, 2023
6521d42
Update LDrawLoader.js
LeviPesin May 9, 2023
275ffce
Update AmbientLightProbe.js
LeviPesin May 9, 2023
e65b293
Update HemisphereLightProbe.js
LeviPesin May 9, 2023
836bbdb
Rework `ConvertType` a bit
LeviPesin May 9, 2023
4588c95
Update ShaderNode.js
LeviPesin May 9, 2023
13bef6c
Fix
LeviPesin May 9, 2023
f7b35bd
Revert ShaderNode changes
LeviPesin May 9, 2023
2b33b34
Update NRRDLoader.js
LeviPesin May 11, 2023
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
26 changes: 8 additions & 18 deletions examples/jsm/loaders/LDrawLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,7 +896,7 @@ class LDrawParsedCache {

materials[ material.userData.code ] = material;

} else {
} else {

console.warn( 'LDrawLoader: Error parsing material' + lp.getLineNumberString() );

Expand Down Expand Up @@ -1023,23 +1023,13 @@ class LDrawParsedCache {
colorCode = lp.getToken();
material = getLocalMaterial( colorCode );

const posX = parseFloat( lp.getToken() );
const posY = parseFloat( lp.getToken() );
const posZ = parseFloat( lp.getToken() );
const m0 = parseFloat( lp.getToken() );
const m1 = parseFloat( lp.getToken() );
const m2 = parseFloat( lp.getToken() );
const m3 = parseFloat( lp.getToken() );
const m4 = parseFloat( lp.getToken() );
const m5 = parseFloat( lp.getToken() );
const m6 = parseFloat( lp.getToken() );
const m7 = parseFloat( lp.getToken() );
const m8 = parseFloat( lp.getToken() );

const matrix = new Matrix4().set(
m0, m1, m2, posX,
m3, m4, m5, posY,
m6, m7, m8, posZ,
const m = [];
for ( let i = 0; i < 12; i++ ) m.push( parseFloat( lp.getToken() ) );

const matrix = new Matrix4(
m[ 3 ], m[ 4 ], m[ 5 ], m[ 0 ],
m[ 6 ], m[ 7 ], m[ 8 ], m[ 1 ],
m[ 9 ], m[ 10 ], m[ 11 ], m[ 2 ],
0, 0, 0, 1
);

Expand Down
20 changes: 5 additions & 15 deletions examples/jsm/loaders/NRRDLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,9 +419,7 @@ class NRRDLoader extends Loader {
const spacingZ = new Vector3().fromArray( headerObject.vectors[ 2 ] ).length();
volume.spacing = [ spacingX, spacingY, spacingZ ];


// Create IJKtoRAS matrix
Copy link
Owner

@mrdoob mrdoob May 12, 2023

Choose a reason for hiding this comment

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

Shouldn't this comment be moved too?

Copy link
Contributor Author

@LeviPesin LeviPesin May 13, 2023

Choose a reason for hiding this comment

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

I think no, because transitionMatrix is a part of building IJKtoRAS matrix.

volume.matrix = new Matrix4();

const transitionMatrix = new Matrix4();

Expand All @@ -445,34 +443,26 @@ class NRRDLoader extends Loader {

}


if ( ! headerObject.vectors ) {

volume.matrix.set(
1, 0, 0, 0,
0, 1, 0, 0,
0, 0, 1, 0,
0, 0, 0, 1 );
volume.matrix = new Matrix4();
volume.inverseMatrix = new Matrix4();

} else {

const v = headerObject.vectors;

const ijk_to_transition = new Matrix4().set(
const ijk_to_transition = new Matrix4(
v[ 0 ][ 0 ], v[ 1 ][ 0 ], v[ 2 ][ 0 ], 0,
v[ 0 ][ 1 ], v[ 1 ][ 1 ], v[ 2 ][ 1 ], 0,
v[ 0 ][ 2 ], v[ 1 ][ 2 ], v[ 2 ][ 2 ], 0,
0, 0, 0, 1
);

const transition_to_ras = new Matrix4().multiplyMatrices( ijk_to_transition, transitionMatrix );

volume.matrix = transition_to_ras;
volume.matrix = ijk_to_transition.multiply( transitionMatrix );
volume.inverseMatrix = volume.matrix.clone().invert();

}

volume.inverseMatrix = new Matrix4();
volume.inverseMatrix.copy( volume.matrix ).invert();

volume.RASDimensions = [
Math.floor( volume.xLength * spacingX ),
Expand Down
4 changes: 1 addition & 3 deletions src/lights/AmbientLightProbe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@ class AmbientLightProbe extends LightProbe {

this.isAmbientLightProbe = true;

const color1 = new Color().set( color );

// without extra factor of PI in the shader, would be 2 / Math.sqrt( Math.PI );
this.sh.coefficients[ 0 ].set( color1.r, color1.g, color1.b ).multiplyScalar( 2 * Math.sqrt( Math.PI ) );
this.sh.coefficients[ 0 ].setFromColor( new Color( color ) ).multiplyScalar( 2 * Math.sqrt( Math.PI ) );

}

Expand Down
7 changes: 2 additions & 5 deletions src/lights/HemisphereLightProbe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,8 @@ class HemisphereLightProbe extends LightProbe {

this.isHemisphereLightProbe = true;

const color1 = new Color().set( skyColor );
const color2 = new Color().set( groundColor );

const sky = new Vector3( color1.r, color1.g, color1.b );
const ground = new Vector3( color2.r, color2.g, color2.b );
const sky = new Vector3().setFromColor( new Color( skyColor ) );
const ground = new Vector3().setFromColor( new Color( groundColor ) );

// without extra factor of PI in the shader, should = 1 / Math.sqrt( Math.PI );
const c0 = Math.sqrt( Math.PI );
Expand Down