Skip to content

Conversation

@sunag
Copy link
Collaborator

@sunag sunag commented Sep 26, 2019

@sunag sunag mentioned this pull request Sep 26, 2019
12 tasks
@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 26, 2019

When opening your live link, the normal map still looks flipped. However, if I switched to BackSided and then back to FrontSided, it looks good. Is this the intended behavior?

@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2019

@Mugen87 I forgot to define the default side, the default side is DoubleSided and not FrontSided.

@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2019

My issue is that apparently in non NodeMaterial too DoubleSided and FrontSided have different results rendering the front face only?

@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2019

I think that it should be < 0.0?

@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2019

@sunag sunag changed the title NodeMaterial example - add material.side option perturbNormal2Arb - invert S, T when the UV direction is backwards (from mirrored faces) Sep 26, 2019
@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2019

@arobertson0 @Mugen87 Seems correct now?

@WestLangley
Copy link
Collaborator

@sunag

To test, remove the lid and bottom like so:

teapot = new TeapotBufferGeometry( 15, 18, false, false );

so you can see what is happening.

FrontSide should be extruded; BackSide should be indented.

DoubleSide should be extruded on the front and indented on the back.

/ping @donmccurdy

@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2019

Hmm @WestLangley seems that BackSide issue in NodeMaterial is related with this:

if ( material.side === BackSide ) uniforms.normalScale.value.negate();

@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2019

Maybe should be we move this to the shader?

@sunag
Copy link
Collaborator Author

sunag commented Sep 26, 2019

Anyway, this seems to fix DoubleSided fliped normal in front-face in core shader?

@WestLangley
Copy link
Collaborator

WestLangley commented Sep 27, 2019

Double-sided materials with a normal map are broken in the core shader. Breakage occurred in #17158 (confirmed).

The breakage is easily-seen in webgl_decals.html. Make the following change in the example, and the decals will become inverted:

var material = decalMaterial.clone();
material.side = THREE.DoubleSide; // add this

/ping @JordyvanDortmont
/ping @donmccurdy (to confirm desired behavior)

@WestLangley
Copy link
Collaborator

@sunag Whether the triangle is front-facing is a separate issue from whether the geometry and or UVs are mirrored. All we need is a test to replace gl_FrontFacing.

@WestLangley
Copy link
Collaborator

@sunag Oops. I did not realize you proposed a change to the core shader in this PR. I also proposed #17586.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 27, 2019

I also proposed #17586.

I think it's better to have a single PR and use a uniform solution in normalmap_pars_fragment.glsl.js and NormalMapNode.js . I've tested both approaches and both fix the issue in webgl_materials_nodes.

@mrdoob Which one do you prefer? I suggest to update this PR with your desired solution and close #17586.

@sunag
Copy link
Collaborator Author

sunag commented Sep 27, 2019

I liked the PR of @WestLangley, I suggest merge the PR of @WestLangley first and after I fix any conflicts of this PR for NodeMaterial.

@WestLangley
Copy link
Collaborator

@sunag Done!

@sunag sunag changed the title perturbNormal2Arb - invert S, T when the UV direction is backwards (from mirrored faces) NodeMaterial - Fix DoubleSided NormalMap flipped Sep 27, 2019
@sunag
Copy link
Collaborator Author

sunag commented Sep 27, 2019

The approach used currently to BackSided is incompatible with NodeMaterial and a bit more hard to implement. I think in leave this fix to another PR.

if ( material.side === BackSide ) uniforms.normalScale.value.negate();

@JordyvanDortmont
Copy link
Contributor

JordyvanDortmont commented Sep 27, 2019

I'm sorry for breaking the normals for double-sided materials!

I tested both PRs on a GTX 1070 and I see differences between the two PRs suggested. One is related to the DoubleSide issue, and another is related to BackSide.

While I was writing this, this PR has changed, so I'll describe the current behaviour of this PR.

The normals are flipped for DoubleSide when use node material is unchecked, so the teapot is indented on the front instead of extruded. Is this the case for you as well?

The normals flip when webgl_materials_node is set to BackSide and you check/uncheck use node material. I think @sunag mentions this here: #17584 (comment) and here: #17584 (comment)

When the DoubleSide issue is resolved, I'll test the PR on an Adreno GPU.

@sunag
Copy link
Collaborator Author

sunag commented Sep 27, 2019

I was thinking of fix BackSided using shader, but with node seems easier... I will commit this fix.

@sunag sunag changed the title NodeMaterial - Fix DoubleSided NormalMap flipped NodeMaterial - Fix DoubleSided and BackSided NormalMap flipped Sep 27, 2019
@sunag
Copy link
Collaborator Author

sunag commented Sep 27, 2019

The normals are flipped for DoubleSide when use node material is unchecked, so the teapot is indented on the front instead of extruded. Is this the case for you as well?

My live example is only for nodematerial, the core was fixed in WestLangley PR #17586.

@sunag
Copy link
Collaborator Author

sunag commented Sep 27, 2019

I think it is ready to merge?

@arobertson0
Copy link
Contributor

I think it is ready to merge?

Looks great. As you said, this matches #17586. Thanks for the fix.

@mrdoob mrdoob added this to the r109 milestone Sep 30, 2019
@mrdoob mrdoob merged commit 404125a into mrdoob:dev Sep 30, 2019
@mrdoob
Copy link
Owner

mrdoob commented Sep 30, 2019

Thanks!

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.

Node Material normals flipped

6 participants