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 for duplicate vertices in .obj importing #29

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

Fix for duplicate vertices in .obj importing #29

wants to merge 4 commits into from

Conversation

mmcauliffe
Copy link
Contributor

Resolves #28. Based off of http://www.vtk.org/gitweb?p=VTK.git;a=blob;f=Examples/Modelling/Cxx/Delaunay3D.cxx, uses vtkCleanPolyData class to remove identical vertices prior to menpo processing.

@menpobot
Copy link

Can one of the admins verify this patch?

@jabooth
Copy link
Member

jabooth commented Aug 29, 2016

@mmcauliffe thanks so much for this!

My only slight concern here is the ordering of the vertices and triangulation. Great that after the dedupe we are left with the correct total number of vertices/tris, tcoords, but do we know if the semantic meaning is retained? (i.e. vertex listed 2014'th in the .obj file is still the 2014'th vertex in self.points, the trilist is identical to the way it is ordered in the .obj file). This is important for some applications.

My gut tells me that it probably does, which is great, just feels a little grim as VTK is generating a bunch of extra vertices and then cleaning them all back up, and we are counting on the cleanup to be an exact reversal of the original duplication.

(To be clear this was obviously wrong in the current shipping version, and your fix is definitely an improvement, just want to confirm if it fully addresses the issue so that our loader returns something identical to what would be expected from the file. 👍)

@mmcauliffe
Copy link
Contributor Author

Yeah, so my visual inspection of clustering the Wave Kernel Signature from meshes using the old importer and the new looked identical (and that relies on ordering of the vertices and triangles). I've added a couple more asserts just to explicitly test it, so the first, tenth, last and ninth from last points and triangles are the same when imported as in the .obj file.

@patricksnape
Copy link
Contributor

Unfortunately this fix causes the following artefacts for the texture:

image

The issue with the VTK vertex duplication is that the james.obj mesh has per-face texture coordinates and VTK only supports per-vertex texture coordinates. This is actually a good question - @jabooth we don't support per-face texture coordinates either - do we? I believe we were seeing the correct behaviour from assimp previously because assimp was performing some magic to ensure that mesh was correctly formed.

@patricksnape
Copy link
Contributor

@jabooth and I discussed this at length and we came to the following conclusions:

  • We need to be able to support per-face texture coordinates
    • In order to achieve this, we should add a new property on meshes that is something like is_tcoords_per_vertex which will be True if the tcoords has the same length as points.
    • For visualisation purposes, this requires duplicating vertices in order to have a tcoord per-vertex.
    • Provide a convenience method for getting the mesh that includes the duplicated per-vertex tcoords, in exactly the same manner that VTK currently is.
  • Fix the VTK importer to not duplicate vertices in the per-face case.

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.

4 participants