Fix perturbNormal2Arb() + DoubleSide

[donmccurdy]

Solution from #13716 handles front faces, but must be inverted with THREE.DoubleSide. Issue can be seen on the backs of glTF NormalTangentTest and NormalTangentMirrorTest models.

method	screenshot
before
after

[WestLangley]

What about back-sided materials? Do they behave as you think they should?

Sorry, I am not understanding what you expect to see... Perhaps if you could link to a live example it would be helpful. :)

In the current three.js implementation, an "extrusion" when viewed from the front appears as an extrusion (not an indentation) when viewed from the back -- only mirrored. This is true for normal maps and bump maps. Do you believe it should be otherwise?

[donmccurdy]

What about back-sided materials?

I suppose for consistency if we go this direction, it should be:

float scale = sign( st1.t * st0.s - st0.t * st1.s ); // we do not care about the magnitude
scale *= float( gl_FrontFacing ) * 2.0 - 1.0;

In the current three.js implementation, an "extrusion" when viewed from the front appears as an extrusion (not an indentation) when viewed from the back -- only mirrored.

Yes, that's what I see without this PR. With this PR an "extrusion" becomes an indentation from the back, which is what I'd originally expected. I had assumed current behavior was introduced with #13716, but if the behavior predates that, I don't feel strongly about changing it.

[WestLangley]

I am open to what the behavior should be.

The current behavior in three.js was established years ago: an extrusion when viewed from the front appears as an extrusion when viewed from the back.

Any idea on how a normalMap-textured plane appears when viewed from the back in other engines?

/ping @emackey Do you know?

[donmccurdy]

Links to live demos in various engines here: https://github.com/cx20/gltf-test#agi-sample-models

Of those marked with "✅", all are rendering front extrusions as back indentations. The others are broken in more fundamental ways, so I can't tell what convention they use.

[WestLangley]

Of those marked with "✅", all are rendering front extrusions as back indentations.

That is a fundamental difference from the behavior of three.js, where extrusions from the front also appear as extrusions from the back.

If we want to change that behavior in three.js, then we need to address it first -- for normal maps and bump maps.

Afterwards, we will need to revisit normalScale flipping, mirrored wrapping, and tangent-less normal maps.

[emackey]

The back side of this sample model has a screenshot and description in the README.

The glTF spec has this to say on the topic:

The back-face must have its normals reversed before the lighting equation is evaluated

The typical implementation of this is in the fragment shader:

    if (!gl_FrontFacing)
    {
        n = -n;
    }

[mrdoob]

Thanks!

[donmccurdy]

@mrdoob — Just make sure it's clear, this PR changes an older behavior of three.js: extrusions on the front will now be indentations on the back for normal maps. Is that OK? It does seem more like other engines this way. But we should also do the same for bump maps as @WestLangley mentions, to be consistent.

[mrdoob]

Yes, I think it makes sense that the back side has the normal inverted.

[WestLangley]

Yes, I think it makes sense that the back side has the normal inverted.

@donmccurdy Is that what you are seeing with this PR -- for THREE.BackSide, in particular?

[WestLangley]

Guys, as I said, I do not think this PR is correct. The shader does not know it is rendering a back-sided material.

There are a lot of use cases: mirrored geometries, negatively-scaled meshes, back-sided materials, double-sided materials, repeat wrapping, mirrored repeat wrapping, flipped textures, flipped uv's etc., and all the 2^N possible combinations.

This feature must be implemented in a comprehensive manner -- not piecemeal, or we will end up with a whack-a-mole problem. I am not sure it is worth pursuing, quite frankly.

I don't feel strongly about changing it.

Me either. In fact, one could make a compelling argument that the back-side should render the same as the front-side.

[emackey]

Is there a way to limit this change to just glTF models? It is specified correct behavior for glTF.

[donmccurdy]

Yes, I think it makes sense that the back side has the normal inverted.

@donmccurdy Is that what you are seeing with this PR -- for THREE.BackSide, in particular?

With THREE.BackSide I am still seeing the normals inverted. There are some other effects I wasn't expecting (more metallic, less color?), but I haven't looked into that yet.

DoubleSide:

BackSide:

glTF does not provide an equivalent to THREE.BackSide, so I can't easily compare this across other engines.

Is there a way to limit this change to just glTF models?

Not easily, no... I'd rather not fork MeshStandardMaterial for metal/rough the way we've done for spec/gloss. Could be a material option but seems like a very specific case. Without this, threejs will still handle DamagedHelmet correctly and the front sides of NormalTangentTest and NormalTangentMirrorTest, but fails on the backs of the NormalTangent* tests.

[emackey]

I guess the front is probably the more important side to get correct for consistency between engines. Not sure how strongly the glTF users should rely on back-side normal map compatibility...

[bhouston]

If you use the fix I had originally it worked in all cases properly. The reason my fixed worked is that I checked if compared to the derived normal if the UV space was inverted and then fixed it.

WestLangley's fix just tried to make the UV space always facing the camera. My fix flipped the UV space it if didn't aligned with the normal. Those are different results.

If you adopt my solution it just works.

[WestLangley]

OK @bhouston I will have a look. Thanks.

[WestLangley]

@bhouston I have tested your approach and I have been unable to achieve what @donmccurdy is requesting: front extrusions render as back indentations for both back-sided and double-sided materials. I have only tested the simple cases to start; not the mirrored cases. Perhaps I am missing something...

Maybe you can file a PR to clarify?

I should point out again that this PR is not correct and needs to be reverted. At least what we had previously was consistent in behavior: front extrusions always render as extrusions, even from the back.

[mrdoob]

I should point out again that this PR is not correct and needs to be reverted.

As far as I understand, this PR is not incorrect, it's just not complete. It adds support for DoubleSide. We can add support for BackSide in another PR.

[WestLangley]

As far as I understand, this PR is not incorrect, it's just not complete.

OK, that is a valid way to think about it.

[pailhead]

@emackey

Is there a way to limit this change to just glTF models? It is specified correct behavior for glTF.

Yes, three.js can change arbitrary material's shaders on the fly.

Please take alook at these proof of concept demos:

http://dusanbosnjak.com/test/webGL/three-material-includes/webgl_loader_gltf_extensions.html
http://dusanbosnjak.com/test/webGL/three-material-includes/

1. GLTFLoader takes 100% of some material (MeshStandardMaterial in this case) and changes what it needs. If the GLTF contains a regular material it does nothing, if it contains a spec/gloss extension, it extends the standard material.
2. The app/demo, not caring where the mesh/material came from (GLTFLoader or another loader, or created dynamically) applies another material change in order to allow instancing.

This being said, if three.js has a need to handle FOO like BAR, but some format/spec/standard has a need to treat FOO like BAZ, i see no reason for this not to happen.

GLTFLoader should be able to do whatever it wants, without compelling the entire library to do the same. The simple reasoning here is that there are many many other loaders that work with three.js too. However, if GLTFLoader were a first class citizen, it would make sense to keep it inline with the rest of the library.

[donmccurdy]

I very strongly prefer to avoid modifying builtin three.js materials within GLTFLoader any more than necessary. Because other common formats like COLLADA, OBJ, and FBX do not have a well-defined PBR model, glTF provides a particular opportunity for us to make our PBR implementation consistent with authoring workflows and artist expectations.

This is not compelling the entire library to do anything. If the changes do not make sense for three.js as a whole, we won't make them.

[pailhead]

It seems extremely taxing to decide which changes make sense for three.js and which do not. It's a lot of posts, some for, some against.

While that is happening, is there a reason for three.js to make experimenting with something like this complicated or impossible?

As is i can address this for myself, in my sandbox, in my app, without affecting any of you. However the tool that i'm supposed to use is VERY verbose, and it's features (not bugs) make it somewhat fragile.