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

Valid meshes do not require len(points) == len(obj:vn) #1397

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

GenevieveBuckley
Copy link

@GenevieveBuckley GenevieveBuckley commented Mar 6, 2023

Closes #1261
Closes #1377

This PR removes a check for point data consistency, since that restriction is not actually fulfilled by many valid mesh files.

meshio/src/meshio/_mesh.py

Lines 165 to 169 in 0138cc8

if len(self.point_data[key]) != len(self.points):
raise ValueError(
f"len(points) = {len(self.points)}, "
f'but len(point_data["{key}"]) = {len(self.point_data[key])}'
)

This PR also adds another test for this case.
I have used the example .obj file from #1261 (comment)

v  -0.50 -0.50 -0.50
v  -0.50 0.50 -0.50
v  0.50 0.50 -0.50
v  0.50 -0.50 -0.50
v  -0.50 -0.50 0.50
v  0.50 -0.50 0.50
v  0.50 0.50 0.50
v  -0.50 0.50 0.50

vn 0.00 0.00 -1.00
vn 0.00 0.00 1.00
vn 0.00 -1.00 0.00
vn 1.00 0.00 0.00
vn 0.00 1.00 0.00
vn -1.00 0.00 0.00

vt 1.00 0.00 0.00
vt 1.00 1.00 0.00
vt 0.00 1.00 0.00
vt 0.00 0.00 0.00

g Box

f 1/1/1 2/2/1 3/3/1
f 3/3/1 4/4/1 1/1/1
f 5/4/2 6/1/2 7/2/2
f 7/2/2 8/3/2 5/4/2
f 1/4/3 4/1/3 6/2/3
f 6/2/3 5/3/3 1/4/3
f 4/4/4 3/1/4 7/2/4
f 7/2/4 6/3/4 4/4/4
f 3/4/5 2/1/5 8/2/5
f 8/2/5 7/3/5 3/4/5
f 2/4/6 1/1/6 5/2/6
f 5/2/6 8/3/6 2/4/6

@GenevieveBuckley
Copy link
Author

More history/context: this point data consistency check was added in PR #1067.

Also in that PR 8e87d45, a pytest skip line was added to an existing test of the elephav.obj mesh.

@pytest.mark.skip("Fails point data consistency check.")

There's no description in the text of the PR, so I'm a little unclear about what problems this check for point data consistency was intended to prevent, or why the test was skipped.

So, while the test suite is passing for me, it may also be possible that I'll need to add another test (to check we really are preventing whatever bad thing we need to avoid). Please let me know if that's the case.

@GenevieveBuckley GenevieveBuckley changed the title Valid meshes do not require len(points) == len(obj:vt) Valid meshes do not require len(points) == len(obj:vn) Mar 6, 2023
@GenevieveBuckley GenevieveBuckley mentioned this pull request Mar 6, 2023
@jaunkst
Copy link

jaunkst commented Jul 26, 2024

getting this myself just loading an obj from blender
ValueError: len(points) = 2496, but len(point_data["obj:vn"]) = 2494

@TimeTravelerFromNow
Copy link

Hello, just posted this comment on issue #1487 which is related to this fix

How we could validate a different way

The problem with this assertion for .obj loading is that the vertex texture attribute "obj:vt" and other keys in valid .obj files commonly are not the same count as the number of vertices.
I have a suggestion how this could be changed to validate the "face" data which appears as

f 1/1 2/2 3/3 4/4
f 5/1 8/2 7/3 6/4
f 1/1 5/2 6/3 2/4
in an obj file.
For example, f 5/1 8/2 7/3 6/4 is saying face 2 is made up of vertex 5 8 7 6. With corresponding vertex textures index 1 2 3 4 defined in the file as vt 0.000000 0.000000 etc.

Reading this documentation about the obj format https://learnwebgl.brown37.net/, what we should validate is that the indices in each face data doesn't exceed the length of the available attributes.
So if there are 4 "obj:vt" we should read each face, so

f 1/1 2/2 3/3 4/4 would pass the validation since for each vertex_index/texture_index, texture_index <= len(point_data["obj:vt"])

f 1/1 2/2 3/3 4/5 would fail since 4/5 has a texture_index = 5 > len(point_data["obj:vt"])

...still just a thought, however I would be happy to open a PR to try this approach

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.

[BUG] [BUG] len(points) != len(obj:vt)
3 participants