-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix parsing faces that reference non-existing attributes #218
Conversation
specs/lib/loadObjSpec.js
Outdated
@@ -496,6 +497,14 @@ describe('loadObj', () => { | |||
} | |||
}); | |||
|
|||
it('does not add missing normals and uvs', async() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space after async
@@ -215,7 +215,7 @@ function loadObj(objPath, options) { | |||
} | |||
|
|||
// UVs | |||
if (defined(u)) { | |||
if (defined(u) && (globalUvs.length > 0)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So is an OBJ with normals that are referenced by not defined a valid OBJ? I can see it loads fine in Blender. I'm just wondering how far does this need to go to validate the file.
For example, if you add just one normal definition, this check passes, but most of the normals are still undefined, and you get the same problem again. We could check the length of globalNormals compared to the index it's asking for, but I'm not sure how common that is vs an OBJ with just 0 defined normals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It falls under the case of "probably not valid but easy to work around", which I think is an ok mentality for an obj reader (not for more well defined formats).
If some of the normals are defined, but not all, that's a harder issue to work around. I added some runtime errors when that happens.
03610bf
to
ff26d5d
Compare
Looks good! @lilleyse I don't have write access, but feel free to merge. |
Fixes obj's like this where normals and uvs don't exist but they are referenced by faces.
Previously the glTF would have a uv and normal accessor full of NaNs.