-
-
Notifications
You must be signed in to change notification settings - Fork 107
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 obj file loading #84
base: master
Are you sure you want to change the base?
Conversation
One thing missing from this is how to deal with vertices with multiple normals, for example:
Here vertex 1 and 3 have two normals, and I think only face or averaged normals make sense here? |
src/common/obj_loader.h
Outdated
n(i, 1) = attrib.normals[3*i+1]; | ||
n(i, 2) = attrib.normals[3*i+2]; | ||
} | ||
|
||
if (attrib.texcoords.size() > 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.
Looks like this would fail for uv coords and colors as well -- can we fix this and add a small test with the file you uploaded?
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.
I don't think vertex colors will be a problem, since the OBJ file format forces every vertex to redefine the color, even if it's not unique. Unlike vertex normals and UV, which can be reused.
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.
Good point about colors. We should still make the same change for texcoords at the line below though
Thanks for the pull request and sorry for getting to it so late -- I've had less time to focus on open source lately. I added one small comment above then this should be mergeable. |
Looks great! Thanks for the fix! I'll merge when the CI passes |
Fixes #83.
When loading obj files, the
attrib.normals
vector only contains the unique normals present in the mesh. To associate a normal to a vertex you have to use the face indices structure. In the current code, the following obj file causes an out of bounds array access:I think this issue is present with UV coordinate loading as well.