src/lights: move to es6 classes #20018
Conversation
|
I tell a lie.
...
// Compare keys.
var keys1 = Object.keys( val1 );
var keys2 = Object.keys( val2 );
for ( var i = 0, l = keys1.length; i < l; i ++ ) {
if ( keys2.indexOf( keys1[ i ] ) < 0 ) {
return makeFail( 'Property "' + keys1[ i ] + '" is unexpected.' );
}
}
for ( var i = 0, l = keys2.length; i < l; i ++ ) {
if ( keys1.indexOf( keys2[ i ] ) < 0 ) {
return makeFail( 'Property "' + keys2[ i ] + '" is missing.' );
}
}
...It looks like this PR is failing because of the way objects are compared. Having moved properties into the constructors has broken that logic. |
|
Whoop, whoop! ready for review here 👍 wait, what do we make of one e-2-e test round passing, one failing and one cancelled? |
DefinitelyMaybe
changed the title
|
conflicts resolved |
src/lights/Light.js
Outdated
|
|
||
| constructor: Light, | ||
| this.receiveShadow = undefined; |
There was a problem hiding this comment.
These are from old code that was there when I originally did the change.
This commit was the change.
I can take then out.
|
I'm noticing that we're moving a bunch of objects from the prototype to the constructor. Is not possible to keep them in the prototype somehow? We'll use more memory otherwise. |
|
Anyone got any figures regarding this? |
|
@mrdoob @DefinitelyMaybe for private variables that need to be shared across al instances, this is the pattern to use: three.js/src/audio/PositionalAudio.js Lines 5 to 8 in 66b9fa3 |
|
@DefinitelyMaybe Do you mind updating this PR with that pattern? |
…DefinitelyMaybe/three.js into src/lights--move-to-es6-classes
| this._viewportCount = 1; | ||
|
|
||
| ]; | ||
| this._viewports = [ | ||
|
|
||
| } | ||
| new Vector4( 0, 0, 1, 1 ) | ||
|
|
||
| ]; | ||
|
|
||
| Object.assign( LightShadow.prototype, { | ||
| this._projScreenMatrix = new Matrix4(); | ||
|
|
||
| _projScreenMatrix: new Matrix4(), | ||
| this._lightPositionWorld = new Vector3(); | ||
|
|
||
| _lightPositionWorld: new Vector3(), | ||
| this._lookTarget = new Vector3(); | ||
|
|
||
| _lookTarget: new Vector3(), | ||
| } |
There was a problem hiding this comment.
Which should be updated? is it these ones?
There was a problem hiding this comment.
Hmm, this is hard to work with / review.
Do you mind splitting this PR in multiple ones? (One per file)
There was a problem hiding this comment.
I think if we would have made single PR per file from the start we would have been done with this transition.
Right now a single file is blocking many others.
| this._viewportCount = 6; | ||
| this._viewports = [ | ||
| // These viewports map a cube-map onto a 2D texture with the | ||
| // following orientation: | ||
| // | ||
| // xzXZ | ||
| // y Y | ||
| // | ||
| // X - Positive x direction | ||
| // x - Negative x direction | ||
| // Y - Positive y direction | ||
| // y - Negative y direction | ||
| // Z - Positive z direction | ||
| // z - Negative z direction | ||
|
|
||
| // positive X | ||
| new Vector4( 2, 1, 1, 1 ), | ||
| // negative X | ||
| new Vector4( 0, 1, 1, 1 ), | ||
| // positive Z | ||
| new Vector4( 3, 1, 1, 1 ), | ||
| // negative Z | ||
| new Vector4( 1, 1, 1, 1 ), | ||
| // positive Y | ||
| new Vector4( 3, 0, 1, 1 ), | ||
| // negative Y | ||
| new Vector4( 1, 0, 1, 1 ) | ||
| ]; | ||
| this._cubeUps = [ | ||
| new Vector3( 0, 1, 0 ), new Vector3( 0, 1, 0 ), new Vector3( 0, 1, 0 ), | ||
| new Vector3( 0, 1, 0 ), new Vector3( 0, 0, 1 ), new Vector3( 0, 0, - 1 ) | ||
| ]; | ||
| this._frameExtents = new Vector2( 4, 2 ); | ||
|
|
||
| } | ||
|
|
||
| PointLightShadow.prototype = Object.assign( Object.create( LightShadow.prototype ), { | ||
|
|
||
| constructor: PointLightShadow, | ||
|
|
||
| isPointLightShadow: true, | ||
| } |
There was a problem hiding this comment.
and these ones?
the unit tests are currently failing because there are properties that havent made it to the json outputs. Will investigate.