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

Flip faces (cw/ccw) and/or normals from material #4910

Closed
wants to merge 4 commits into from

Conversation

Nimanf
Copy link
Contributor

@Nimanf Nimanf commented Jun 10, 2014

As discussed in #4904, the following flags determines our desired flip method (flipSide parameter in Material):

THREE.NoFlip = 0;
THREE.FlipFaces = 1;
THREE.FlipNormals = 2;
THREE.FlipFacesNormals = 3;

By default, THREE.NoFlip, considers material.side === THREE.BackSide, which is the original behavior of three.js. So it won't change the core and only allows developers to flip faces and/or normals in real-time efficiently, regardless of the culling face mode (double/front/back)

one example scene is also included.

@WestLangley
Copy link
Collaborator

// material.side
THREE.FrontSide = 0;
THREE.BackSide = 1;
THREE.DoubleSide = 2;

// material.flipSide
THREE.NoFlip = 0;
THREE.FlipFaces = 1;
THREE.FlipNormals = 2;
THREE.FlipFacesNormals = 3;

There appears to be 3 x 4 = 12 possible ways to set these two property values. Are there really 12 cases?

Does this PR have an impact on the raycasting logic?

Does flipSide need to be explicitly copied when a material is cloned?

@Nimanf
Copy link
Contributor Author

Nimanf commented Jun 10, 2014

  • Yes, 12. These are the features that OpenGL/WebGL provides, but we can't set them in three.js.
    (3 x 2 (cw/ccw) + our flipping normals in shaders)
    Note: The sample file allows you to test all 12 cases.
  • flipSide is for efficient GPU rendering purposes, and has no impact on picking/ray casting.
  • Yes, It should be cloned. I forgot to add it to clone() function. I will add it.

@WestLangley
Copy link
Collaborator

Sorry, I tried, I really tried, but this PR and your answers do not make any sense to me. Maybe I am missing something and @mrdoob can clarify.

I think this PR would lead to total user confusion.

BTW, you also have to accommodate the other renderers -- CanvasRenderer, at a minimum.

@bhouston
Copy link
Contributor

@Nimanf Maybe instead of the four enum values: NoFlip, FlipFaces, FlipNormals and FlipFacesNormals, we just have two boolean flags: FlipFaces, FlipNormals. Two flags is a bit easier to manage that four enums.

@bhouston
Copy link
Contributor

@Nimanf FlipFaces controls CW/CCW + inverting the normals (so we are backwards compatible), while FlipNormals just inverts the normal vector. You can set both and in that case it flips the faces and leaves the normals alone / double inverts the normals (because FlipFaces inverts the normals and FlipNormal also inverts the normals.)

I think this is a more straightforward approach and more "orthogonal' to the existing code.

@mrdoob
Copy link
Owner

mrdoob commented Jun 14, 2014

@Nimanf Maybe instead of the four enum values: NoFlip, FlipFaces, FlipNormals and FlipFacesNormals, we just have two boolean flags: FlipFaces, FlipNormals. Two flags is a bit easier to manage that four enums.

Yeah, that sounds better.

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.

None yet

5 participants