LatheGeometry fix + optimization (dependent on PR#2867 'Geometry.mergeVertices fix') #2868

Merged
merged 6 commits into from Jan 7, 2013

3 participants

@bhouston
Contributor

I noticed that LatheGeometry has a bug/contradiction and is slower than necessary:

This PR removes addresses the above via:

  • I removed the assumption that the lathe geometry is always closed -- I no longer wrap the vertices. To make a closed geometry, called Geometry.mergeVertices after generation and it will close the main seam as well as the top and bottom correctly if they can be merged. Thus this is a general solution.
  • I optimized rotation around the axis via Math.cos and Math.sin.

I updated webgl_geometry_normals.html (here: bhouston@f454d09 ) so that you can see the LatheGeometry examples with partial lathes and unclosed lathes and see that they all work correctly (and with correct normals after running Geometry.mergeVertices.)

@mrdoob mrdoob merged commit f454d09 into mrdoob:dev Jan 7, 2013
Owner
mrdoob commented Jan 7, 2013

Thanks!

Collaborator

@bhouston

There are problems here with the UVs. I am not sure this was tested -- it looks like the problems existed before your recent changes.

  1. The texture is reversed.
  2. The entire texture is not mapped to the surface.
  3. Also, there are multiple vertex normals at shared vertices on the seam.

Would you mind having a look at my dev-lathe branch, and providing a second opinion on my (very simple) attempt at addressing these issues. There is more than one way to solve this problem, but if you concur, I'll submit a PR.

One point -- the points array, I guess, is assumed to be increasing in z. Actually, it would be better to pass in a 2D THREE.Path in x and y, and rotate that around the x-axis, but I didn't change any of that.

Contributor
bhouston commented Jan 8, 2013

@WestLangley, to resolve the shared vertices just call Geometry.mergeVertices(), I fixed it so that it will work correctly on LatheGeometry results. Remember that LatheGeometries are not necessary closed at the top and along the main seam so many simple approaches are not general solutions.

The reversal of the texture should be fairly easy to resolve, but it seems like this was done on purpose in the code so I left it in.

The entire texture should be mapped, I'll have a look at this.

Contributor
bhouston commented Jan 8, 2013

@WestLangley, I just looked at your changes, I like them. You are using this.mergeVertices() which is by far the best method to fix the duplicate vertices. I am okay with the test of your changes. I would suggest doing a PR with your changes as I will support it.

Collaborator

@bhouston Done. Thank you for your code review. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment