Attempt to load missing resources by looking in the same directory as the obj #117
Conversation
lilleyse
force-pushed
the
resources-in-root
branch
from
ce1ca89
to
2c21105
Compare
lib/loadMtl.js
Outdated
| @@ -222,6 +222,11 @@ function loadMaterialTexture(material, name, texturePath, textureOptions, mtlDir | |||
| texturePromise = Promise.resolve(); | |||
| } else { | |||
| texturePromise = loadTexture(texturePath, textureOptions) | |||
| .catch(function() { | |||
There was a problem hiding this comment.
Should this also happen if options.secure && outsideDirectory?
Also not sure this should happen as a generic catch, since loadTexture can also reject due to a pngjs or jpegjs error. So technically this could cause the wrong texture to be loaded if the intended texture fails due to decode-related problems, although that'd probably be a pretty contrived case.
There was a problem hiding this comment.
Should this also happen if options.secure && outsideDirectory?
Yes definitely. I pushed a fix for that.
Also not sure this should happen as a generic catch, since loadTexture can also reject due to a pngjs or jpegjs error. So technically this could cause the wrong texture to be loaded if the intended texture fails due to decode-related problems, although that'd probably be a pretty contrived case.
I also considered this but didn't think it would ever really happen.
lib/loadObj.js
Outdated
| return loadMtl(mtlPath, options) | ||
| .catch(function() { |
There was a problem hiding this comment.
Same concern about using a generic catch here.
|
Updated. |
lib/loadMtl.js
Outdated
| // Try looking for the texture in the same directory as the obj | ||
| texturePromise = loadTexture(shallowPath, textureOptions) | ||
| .catch(function() { | ||
| options.logger('Could not read texture file at ' + texturePath + ' because it is outside of the mtl directory and the secure flag is true. This texture will be ignored.'); |
There was a problem hiding this comment.
Also not sure this should happen as a generic catch, since loadTexture can also reject due to a pngjs or jpegjs error. So technically this could cause the wrong texture to be loaded if the intended texture fails due to decode-related problems, although that'd probably be a pretty contrived case.
I also considered this but didn't think it would ever really happen.
Agreed that the case described above is unlikely, but, using generic catches like this right now can also make the logged output message inaccurate, which makes the input harder for the user to fix.
For example, this message could log either because the texture couldn't be found at all (described), because it was corrupted, or because it was found in the root but was corrupted.
There was a problem hiding this comment.
Maybe sufficient to log the error in addition to a more general warning for basename(texturePath)?
lilleyse
force-pushed
the
resources-in-root
branch
from
fb1f5a0
to
138c3f6
Compare
lilleyse
force-pushed
the
resources-in-root
branch
from
138c3f6
to
e2db0ac
Compare
|
@likangning93 updated. I went with your suggestion here: #117 (comment). |
Fixes #54