updateMaterialIndexAttribute when it needs #666
Conversation
| let needsMaterialIndexUpdate = result.changeType !== NO_CHANGE || materialUuids === null || materialUuids.length !== length; | ||
| for ( let index = 0, length = materials.length; ( index < length ) && ! needsMaterialIndexUpdate; index ++ ) { | ||
|
|
||
| if ( materialUuids === null || materialUuids.length !== length ) { |
There was a problem hiding this comment.
These conditions can be pulled out of the for loop and used in the above needsMaterialIndexUpdate initialization since it's not using the index value at all.
| @@ -176,9 +178,29 @@ export class PathTracingSceneGenerator { | |||
| // generate the geometry | |||
| const result = staticGeometryGenerator.generate( geometry ); | |||
| const materials = result.materials; | |||
| let needsMaterialIndexUpdate = result.changeType !== NO_CHANGE || materialUuids === null || materialUuids.length !== length; | |||
| for ( let index = 0, length = materials.length; ( index < length ) && ! needsMaterialIndexUpdate; index ++ ) { | |||
There was a problem hiding this comment.
nit: Lets rename index to i for consistency with other loops.
And for readability if we could move the needsMaterialIndexUpdate check to a condition block that would best, I think:
if ( ! needsMaterialIndexUpdate ) {
for ( ... ) {
// ...
}
}| } | ||
|
|
||
| const material = materials[ index ]; | ||
| if ( material.uuid !== materialUuids[ index ] ) needsMaterialIndexUpdate = true; |
There was a problem hiding this comment.
We can add a break statement once this is set to true.
|
|
||
| } | ||
|
|
||
| materialUuids = materials.map( material => material.uuid ); |
There was a problem hiding this comment.
This can move inside the below condition since the uuids won't have changed otherwise.
if ( needsMaterialIndexUpdate ) {
updateMaterialIndexAttribute( geometry, materials, materials );
materialUuids = materials.map( material => material.uuid );
}| let materialUuids = null; | ||
|
|
There was a problem hiding this comment.
This should be assigned on the PathTracingSceneGenerator instance itself. Otherwise if we have two different instances of the generator created they will overwrite each others results.
|
fix styles as you recommended(also duplicated logic removed) |
|
Looks excellent! Thank you |
related : #665
This needs to be tested to make sure it's okay
but don't worry, I'll test it, if it's okay, I'll commentthe reason for the PR is that I wonder this solution is right way.