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 obj file loading #84

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

NarinderS
Copy link

@NarinderS NarinderS commented Feb 13, 2024

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:

o Plane
v 0.000000 0.000000 0.000000
v 0.353553 0.353553 0.000000
v 0.353553 -0.353553 0.000000
vn -0.0000 -0.0000 1.0000
s 0
f 2//1 1//1 3//1

I think this issue is present with UV coordinate loading as well.

@NarinderS
Copy link
Author

NarinderS commented Feb 13, 2024

One thing missing from this is how to deal with vertices with multiple normals, for example:

o Plane
v 0.000000 0.000000 0.000000
v 0.353553 0.353553 0.000000
v 0.353553 -0.353553 0.000000
v 0.044583 -0.475301 0.307226
vn -0.0000 -0.0000 1.0000
vn 0.5022 0.5022 0.7040
s 0
f 2//1 1//1 3//1
f 1//2 4//2 3//2

Here vertex 1 and 3 have two normals, and I think only face or averaged normals make sense here?

n(i, 1) = attrib.normals[3*i+1];
n(i, 2) = attrib.normals[3*i+2];
}

if (attrib.texcoords.size() > 0) {
Copy link
Owner

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?

Copy link
Author

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.

Copy link
Owner

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

@fwilliams
Copy link
Owner

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.

@NarinderS NarinderS requested a review from fwilliams October 17, 2024 00:11
@fwilliams
Copy link
Owner

Looks great! Thanks for the fix! I'll merge when the CI passes

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.

Incorrect vertex normals when loading obj file
2 participants