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

Compute ParametricGeometry normals from tangent vectors #11056

Merged
merged 3 commits into from Mar 26, 2017

Conversation

@jaxry
Copy link
Contributor

jaxry commented Mar 25, 2017

The ParametricGeometry class generates vertex normals as usual from the geometry's faces. However, this method leads to incorrectly computed normals for closed parametric surfaces. Where two ends of a surface meet, a seam is created since the positions along the edge are shared, but the indices of the geometry aren't.

For example, here you can see two seams on this torus created using ParametricGeometry:
normals from faces

This PR attempts to solve the problem. These seams are removed if you calculate the normals from the parametric equation itself.
normals from derivatives
By approximating tangent vectors for each vertex via the finite difference method and taking the cross product of the vectors, a correct surface normal is retrieved.

I have a few concerns about my code.

  • The computeVertexNormals method remains untouched, so if this method is called, the normals will be computed from the geometry faces again. The method could be overridden in ParametricGeometry and made to compute normals from the parametric equation, but the overridden method would contain a lot of duplicated code, since the method would still function very similarly. I'm not sure how to proceed.
// approximate tangent plane vectors via central difference

pu.subVectors( func( u + EPS, v ), func( u - EPS, v ) );
pv.subVectors( func( u, v + EPS ), func( u, v - EPS ) );

This comment has been minimized.

Copy link
@jaxry

jaxry Mar 25, 2017

Author Contributor

Should the Vector3s returned by func be .cloneed here in case the Vector3s actually reference the same object? Or is there a better way to do this?

This comment has been minimized.

Copy link
@Mugen87

Mugen87 Mar 25, 2017

Collaborator

func normally returns a new instance of Vector3, see https://github.com/mrdoob/three.js/blob/dev/examples/js/ParametricGeometries.js

This comment has been minimized.

Copy link
@WestLangley

WestLangley Mar 25, 2017

Collaborator

@Mugen87 Actually, there should be no reason to instantiate 4 * slices * stacks Vector3's when using central differences. We could add an optionalTarget to func() and reuse just two instances.

var result = optionalTarget || new THREE.Vector3();
...
return result.set( x, y, z );

This comment has been minimized.

Copy link
@Mugen87

Mugen87 Mar 25, 2017

Collaborator

Sounds reasonable. We can also adjust some entities in THREE.ParametricGeometries so they use the optionalTarget. But let's do this in a separate PR, okay?

@jaxry Are you interested in doing this 😉 ?

This comment has been minimized.

Copy link
@WestLangley

WestLangley Mar 25, 2017

Collaborator

Yes. Separate PR.

@jaxry jaxry changed the title Compute ParametricGeometry normals from derivative Compute ParametricGeometry normals from tangent vectors Mar 25, 2017
@Mugen87

This comment has been minimized.

Copy link
Collaborator

Mugen87 commented Mar 25, 2017

The computeVertexNormals method remains untouched, so if this method is called, the normals will be computed from the geometry faces again. The method could be overridden in ParametricGeometry and made to compute normals from the parametric equation, but the overridden method would contain a lot of duplicated code, since the method would still function very similarly. I'm not sure how to proceed.

There is no need to override computeVertexNormals. It's not intended that users call computeVertexNormals after the geometry was generated. That applies to all geometric primitives of three.js.

@@ -72,6 +77,16 @@ function ParametricBufferGeometry( func, slices, stacks ) {
var p = func( u, v );
vertices.push( p.x, p.y, p.z );

// approximate tangent plane vectors via central difference

pu.subVectors( func( u + EPS, v ), func( u - EPS, v ) );

This comment has been minimized.

Copy link
@WestLangley

WestLangley Mar 25, 2017

Collaborator

Is func( u, v ) guaranteed to be well-defined for values of u, v outside [ 0, 1 ] ?

This comment has been minimized.

Copy link
@Mugen87

Mugen87 Mar 25, 2017

Collaborator

That's a good point! According to the implementation of func(), values outside [ 0, 1 ] can be problematic.

This comment has been minimized.

Copy link
@WestLangley

WestLangley Mar 25, 2017

Collaborator

Then, use non-centered differences with a special case for the edge. By reusing p, only two additional calls are needed.


// cross product of tangent plane vectors returns surface normal

normal.crossVectors( pu, pv );

This comment has been minimized.

Copy link
@WestLangley

WestLangley Mar 25, 2017

Collaborator
normal.crossVectors( pu, pv ).normalize();

This comment has been minimized.

Copy link
@Mugen87

Mugen87 Mar 25, 2017

Collaborator

👍

// generate normals

this.computeVertexNormals();
this.normalizeNormals();

This comment has been minimized.

Copy link
@WestLangley

WestLangley Mar 25, 2017

Collaborator

not needed

@WestLangley

This comment has been minimized.

Copy link
Collaborator

WestLangley commented Mar 25, 2017

Yes, computeVertexNormals() is a heuristic and should not be called by constructors when the normals can be computed analytically or by finite differences.

This is a good change as long as func() is not called with values that are out-of-range.

@jaxry

This comment has been minimized.

Copy link
Contributor Author

jaxry commented Mar 25, 2017

Thanks for the feedback. I implemented your suggestions.

@mrdoob mrdoob merged commit ed8bf99 into mrdoob:dev Mar 26, 2017
@mrdoob

This comment has been minimized.

Copy link
Owner

mrdoob commented Mar 26, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.