Backport changes from master to the 1.0 branch #163
Conversation
There was a problem hiding this comment.
@lilleyse couple comments, but otherwise looks like it will work as-described when integrated with CesiumGS/gltf-pipeline#430.
lib/createGltf.js
Outdated
| break; | ||
| } | ||
| materialName = originalMaterialName + '-' + suffix++; | ||
| } |
There was a problem hiding this comment.
Same comment as in 163, splitting this out to a separate getUniqueId function seems more readable to me.
lib/loadObj.js
Outdated
| nodes = cleanNodes(nodes); | ||
| if (nodes.length === 0) { | ||
| return Promise.reject(new RuntimeError(objPath + ' does not have any geometry data')); | ||
| } | ||
| return loadMaterials(mtlPaths, objPath, options) | ||
| .then(function(materials) { | ||
| if (materials.length > 0 && !usesMaterials) { |
There was a problem hiding this comment.
materials here looks to be a dictionary instead of an array.
lib/loadObj.js
Outdated
| @@ -494,6 +514,23 @@ function getImagePaths(materials) { | |||
| return Object.keys(imagePaths); | |||
| } | |||
|
|
|||
| function assignDefaultMaterial(nodes, materials) { | |||
| var defaultMaterial = materials[0].name; | |||
There was a problem hiding this comment.
Same as above, looks like materials is an object. Maybe this pathway needs a separate spec? I only managed to get here with an obj that didn't usemtl but had a material library.
There was a problem hiding this comment.
Maybe there's already a spec for this case but it's currently too permissive to catch the bug above + here.
There was a problem hiding this comment.
Thanks for catching this. I had brought over the functionality without bringing over the tests.
As I was looking at which tests needed to be brought over I noticed that a lot of PRs that made it into master never made it into 1.0. This PR kind of blew up because of that and I renamed the title accordingly...
The only PR I didn't bring over is #124 which is a lot more work than the others and isn't super common to see. The alpha texture would need to be written to the diffuse texture alpha channel which requires all the raw pixel reading/packing in master.
There was a problem hiding this comment.
I will also test all the obj models I have tomorrow to make sure nothing broke.
There was a problem hiding this comment.
Also some PRs were not needed to back port:
lilleyse
force-pushed
the
fixes-from-master
branch
from
2387fe0 to
ade20d8
Compare
lilleyse
force-pushed
the
fixes-from-master
branch
2 times, most recently
from
971f1d7 to
d721d4f
Compare
lilleyse
force-pushed
the
fixes-from-master
branch
from
d721d4f to
376d228
Compare
lilleyse
changed the title
Includes fixes ported from #161 and #162. Also includes some fixes in
loadObjthat I noticed were inmasterbut not the1.0branch.