Duplicate materials with mismatching attributes #162
Conversation
lilleyse
force-pushed
the
material-duplication
branch
from
0a4f4f1 to
1f0278a
Compare
lib/createGltf.js
Outdated
| @@ -194,6 +198,31 @@ function getTexture(gltf, texture) { | |||
| }; | |||
| } | |||
|
|
|||
| function cloneMaterial(material, removeTextures) { | |||
| if (material === null || typeof material !== 'object') { | |||
There was a problem hiding this comment.
should this be !defined(material)?
There was a problem hiding this comment.
This is to handle typeof null being "object".
I might remove it though since a material will never have a null property, and this function is already somewhat specialized for cloning materials.
lib/createGltf.js
Outdated
| } | ||
| } | ||
| } | ||
|
|
||
| function getMaterial(gltf, materials, materialName) { |
There was a problem hiding this comment.
Not a huge deal, but maybe this is named too similarly now to getMaterialIndex considering that's supposed to do something kind of different. Maybe something like getOrCreateGltfMaterial would be more descriptive.
lib/createGltf.js
Outdated
| break; | ||
| } | ||
| materialName = originalMaterialName + '-' + suffix++; | ||
| } |
There was a problem hiding this comment.
This looks like it's basically doing a getUniqueId, but I think having it as an inlined loop here kind of hurts readability. Consider a getUniqueId helper.
lib/createGltf.js
Outdated
| primitive.material = materialName; | ||
| primitiveInfoByMaterial[materialName] = primitiveInfo; | ||
|
|
||
| var material = getMaterialByName(splitMaterials, materialName); |
There was a problem hiding this comment.
In general it seems like there's a lot of looping in here, which was also true for master. I wonder if this poses a performance risk for objs that have a large number of materials like the San Miguel model at https://casual-effects.com/data/. We can address that in a followup PR if it becomes a problem, it might also be cleaner to pass around material dictionaries instead within the conversion pipeline.
There was a problem hiding this comment.
I timed this just in case. It doesn't look like it's much of a performance concern. Obj parsing just completely trumps it.
Split materials: 5.306ms
Total: 42602.737ms
lilleyse
force-pushed
the
material-duplication
branch
from
6ed450c to
a79a953
Compare
|
@likangning93 should be ready now. |
lilleyse
force-pushed
the
material-duplication
branch
from
a79a953 to
eaca567
Compare
|
👍 Just waiting on #161 now |
|
@likangning93 - ready now. |
|
👍 |
Fixes #160
Duplicates materials whose referring primitives don't have the same attribute types. E.g. if a textured material is referenced by a primitive with both positions and uvs and a primitive with just positions, the material will be duplicated such that the first copy is unchanged and the second copy has textures removed.
#161 needs to be merged first in order for the tests to pass.
To do:
1.0branch.