Skip to content
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

Merged
merged 6 commits into from Oct 29, 2019

Conversation

lilleyse
Copy link
Contributor

Fixes obj's like this where normals and uvs don't exist but they are referenced by faces.

v -1.000000 -1.000000 1.000000
v -1.000000 1.000000 1.000000
v -1.000000 -1.000000 -1.000000
v -1.000000 1.000000 -1.000000
v 1.000000 -1.000000 1.000000
v 1.000000 1.000000 1.000000
v 1.000000 -1.000000 -1.000000
v 1.000000 1.000000 -1.000000
f 1/1/1 2/2/1 4/3/1 3/4/1
f 3/5/2 4/6/2 8/7/2 7/8/2
f 7/9/3 8/10/3 6/11/3 5/12/3
f 5/13/4 6/14/4 2/15/4 1/16/4
f 3/5/5 7/17/5 5/18/5 1/16/5
f 8/19/6 4/6/6 2/15/6 6/20/6

Previously the glTF would have a uv and normal accessor full of NaNs.

@@ -496,6 +497,14 @@ describe('loadObj', () => {
}
});

it('does not add missing normals and uvs', async() => {

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)) {

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?

Copy link
Contributor Author

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.

@OmarShehata
Copy link

OmarShehata commented Oct 29, 2019

Looks good!

@lilleyse I don't have write access, but feel free to merge.

@lilleyse lilleyse merged commit dcaba34 into master Oct 29, 2019
@lilleyse lilleyse deleted the missing-attributes branch October 29, 2019 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants