Skip to content

Conversation

@elalish
Copy link
Contributor

@elalish elalish commented Feb 28, 2023

Related issue: #9250

Description

The PBR Next working group at Khronos is actively working on an anisotropy extension now, so I'm starting work to implement it in order to find any flaws early. The spec has not been updated recently, so don't read it closely just yet. The current idea is to store anisotropy as a two-channel vector texture, with a scalar strength multiplier. This means the shader will have to decompose it into polar coordinates (strength and angle), but it will allow better texture compression and better interpolation.

We want to wait until the extension is settled before merging this, but this will allow our progress to be followed more openly. FYI @emackey.

Results so far for an IBL that represents a directional light (left) vs the actual directional light (right):
imageimage
See for yourself in this example.

@Mugen87 Mugen87 added this to the r??? milestone Feb 28, 2023
@LeviPesin
Copy link
Contributor

Not sure what happened with the job https://github.com/mrdoob/three.js/actions/runs/4288360805/jobs/7470277469 on this PR... Seems like a GitHub issue because the job itself completed just in a few minutes.

// http://www.thetenthplanet.de/archives/1180
vec3 perturbNormal2Arb( vec3 eye_pos, vec3 surf_norm, vec3 mapN, float faceDirection ) {
mat3 getTangentSpace( vec3 eye_pos, vec3 surf_norm ) {
Copy link
Contributor Author

@elalish elalish Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WestLangley I still haven't actually implemented anisotropy here yet, but I realized I'll need the tangent space for it. It wasn't very accessible, so I did this refactor of the normal map shader chunks. There should be no change in behavior here so far, and I think this both simplifies the code and reduces some duplicate calculations in the clearcoat normal map. But, it would be great to get some early feedback here since I want to base my anisotropy calculations on this.

Copy link
Contributor Author

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have a draft implementation of anisotropy for both direct and indirect lighting. This is largely following Filament's implementation (I need to add links), except for the parameterization and mapping of the inputs.

material.anisotropy = length( anisotropyV );
// Roughness along the anisotropy tangent is the material roughness, while the bitangent roughness increases with anisotropy.
material.alphaB = pow2( mix( material.roughness, 1.0, material.anisotropy ) );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key mapping we're trying to figure out for the spec - every renderer seems to use a different mapping, so we're looking to ensure the encoded values are perceptually linear-ish, and that the direct and IBL approximations agree decently well.

// GGX Distribution, Schlick Fresnel, GGX_SmithCorrelated Visibility
vec3 BRDF_GGX( const in vec3 lightDir, const in vec3 viewDir, const in vec3 normal, const in vec3 f0, const in float f90, const in float roughness ) {
vec3 BRDF_GGX_Clearcoat( const in vec3 lightDir, const in vec3 viewDir, const in vec3 normal, const in PhysicalMaterial material) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another refactor here is to make the Clearcoat BRDF the special function instead of the iridescence BRDF. This is nice because clearcoat always uses the simple version, whereas the base BRDF get adjusted by several things, so far iridescence and anisotropy. This feels cleaner to me, hopefully others agree, especially since I removed a lot of the function inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, this did also necessitate moving the PhysicalMaterial definition into its own chunk so it could be included earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see the problem - okay, coming back tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I did a different refactor instead due to troubles with include order. Basically bsdfs.glsl.js had mostly Physical BRDFs that are only used by lights_physical_pars_fragment.glsl.js, so I just moved those functions into that file (a little big now, but self-contained). The only other function used broadly was BRDF_Lambert, so I moved that to common.glsl.js. Now bsdfs.glsl.js is just BlinnPhong functions, and no longer imported by meshphysical.

Sorry this makes the PR a bit hard to read - probably best to look at commits individually.

@elalish elalish marked this pull request as ready for review March 3, 2023 00:47
@elalish
Copy link
Contributor Author

elalish commented Mar 3, 2023

I just switched this from Draft because for some reason the CI stopped running on my PR and I thought that might fix it, but no luck. Is there some quota I exceeded? Oh well, I guess I'll just run it locally for now...

@mrdoob
Copy link
Owner

mrdoob commented Mar 3, 2023

I just switched this from Draft because for some reason the CI stopped running on my PR and I thought that might fix it, but no luck. Is there some quota I exceeded? Oh well, I guess I'll just run it locally for now...

/ping @LeviPesin

@LeviPesin
Copy link
Contributor

No idea 🤷‍♂️ You can enable actions in your fork (i.e. https://github.com/elalish/three.js) to at least run them there, but no idea why they no longer work in this PR...

@LeviPesin
Copy link
Contributor

LeviPesin commented Mar 3, 2023

Can you merge dev into your branch? Maybe it's something related to not updated actions.
But this is also strange -- deprecated versions were planned to be removed only on 1 June and 22 September...

@LeviPesin
Copy link
Contributor

LeviPesin commented Mar 3, 2023

Ah, it seems it was just a GitHub Actions' outage.

@elalish
Copy link
Contributor Author

elalish commented Mar 4, 2023

Okay, shaders are all working, but still need to test the direct lighting path.

@elalish
Copy link
Contributor Author

elalish commented Apr 3, 2023

Thanks, I pulled your changes in. Here's the reason for the test failure:
image
Something to do with the double-sided rendering changes perhaps?

@elalish
Copy link
Contributor Author

elalish commented Apr 3, 2023

Anyway, there's a fair bit of disagreement on this one, even between the path tracers, and ours looks decent to me, so I think it's fine for now: https://modelviewer.dev/fidelity/#khronos-LightsPunctualLamp

@elalish
Copy link
Contributor Author

elalish commented Apr 3, 2023

@mrdoob Okay, so the one problem I've found with your UVs update is that IBLs now fail to compile when we have anisotropy, no map, and no tangents. The reason is that even if no map is using the UVs, anisotropy still needs them to calculate tangents if they're not supplied. I also noticed you duplicated some code in normal_fragment_begin.glsl, which is kind of related. There's no real difference between tbn and tbn2, we're just unsure which UV coordinate name is available (they'll all give the same results). It might be nice to have some "master uv" that just grabs the first set that's defined for cases like this.

@donmccurdy
Copy link
Collaborator

... there's a fair bit of disagreement on this one, even between the path tracers ...

I'm been beginning to wonder whether it's really possible to transport punctual lights effectively between engines, without also controlling more of the image formation chain. At minimum, setting up equivalent exposure in each engine, where exposure = 1 in one is not necessarily the same thing as exposure = 1 in another. Blender's new "Lighting Mode: Standard" export option makes a good case study here.

... we're just unsure which UV coordinate name is available (they'll all give the same results)

Are we sure TEXCOORD_0 and TEXCOORD_1 are going to give the same results? This seems to be an open question under KhronosGroup/glTF#1798 (comment), with the current proposal being that vertex tangents are required if anisotropyTexture is not defined.

@emackey
Copy link

emackey commented Apr 4, 2023

@donmccurdy With yesterday's draft anisotropy update, I've added this proposed wording. What do you think?

All meshes that use nonzero anisotropy strength SHOULD supply TANGENT vectors as a mesh attribute. If TANGENT vectors are not supplied for such a mesh, TEXCOORD_0 MUST be supplied instead, such that tangent vectors can be automatically calculated.

I'm honestly not certain about hard-coding the zero in the spec like that, but the anisotropyTexture is optional where the tangents are required, and it feels perhaps too cumbersome to specify a bunch of logic for looking up the TEXCOORD number based on this map if it happens to exist, and what to do if it does not exist.

Sorry, this is off-topic here though, feel free to post a reply on KhronosGroup/glTF#1798.

To directly answer the question:

Are we sure TEXCOORD_0 and TEXCOORD_1 are going to give the same results?

No, the "U" direction can be completely different for alternate maps, so the tangents can flow different ways based on the TEXCOORD index number. Makes me wonder why glTF has only the one TANGENT attribute, actually, but at least that singularity may be of use to anisotropy.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Apr 4, 2023

Potential conflicts here:

# anisotropyTexture normalTexture TANGENTS outcome
A TEXCOORD_0 TEXCOORD_0 ✅ use existing tangents
B TEXCOORD_0 TEXCOORD_0 ✅ generate tangents with TEXCOORD_0
C TEXCOORD_0 TEXCOORD_1 ⚠️ tangents valid for which UVs?
D TEXCOORD_1 ⚠️ how to generate tangents?
E TEXCOORD_1 TEXCOORD_0 ⚠️ tangents valid for which UVs?
F TEXCOORD_1 TEXCOORD_0 ⚠️ how to generate tangents?

In cases D+F, the implementation would need to calculate TANGENTS for the normal map and anisotropy with competing sources of texture coordinates. This is possible (if unwieldy) for a runtime, but impossible for a processing tool like glTF Transform or gltfpack trying to output valid glTF files.

I'm not sure what it'd mean if TANGENTS are present, but normalTexture and anisotropyTexture use different texture coordinates — one or the other must be incorrect?

I don't love the idea of having multiple sets of tangents in the file — it's really rare that we need to generate MikkTSpace tangents at all, computing tangents in the shader works well for a large majority of normal maps we see in three.js. I generally recommend users bake vertex tangents with MikkTSpace only when the alternatives fail; MikkTSpace is too expensive for us to do by default. But perhaps something like TANGENTS_n should be considered in a hypothetical glTF 3.0.

In the meantime, would it make sense to say anisotropyTexture is restricted to use the same UV layout as normalTexture, falling back to TEXCOORD_0 if neither is defined?

@elalish
Copy link
Contributor Author

elalish commented Apr 4, 2023

In the meantime, would it make sense to say anisotropyTexture is restricted to use the same UV layout as normalTexture, falling back to TEXCOORD_0 if neither is defined?

I like this idea. I think that means in three we could combine normalMapUv and anisotropyMapUv. Looking at the code, it would also be great to combine clearCoatNormalMapUv, but not sure if we can get away with changing the spec at this point. I'd love to understand better the multi-UV channel thing in glTF - individual texture transforms I get (for tiling) but I don't quite understand having whole separate mappings.

@mrdoob mrdoob modified the milestones: r152, r153 Apr 27, 2023
@elalish
Copy link
Contributor Author

elalish commented May 10, 2023

I believe this is ready for final review and merging. I've added an example and the spec has been updated such that our handling of UVs is now compliant, as they are not allowed to mix and match.

@mrdoob mrdoob merged commit 22c5f25 into mrdoob:dev May 11, 2023
@emackey
Copy link

emackey commented May 11, 2023

@elalish Congrats on the merge!

One wrinkle, looks like the AnisotropyBarnLamp.glb sample model merged here contains one incorrect (old) name, anisotropyDirectionTexture instead of anisotropyTexture in the material's extension JSON. I just pushed an updated GLB to KhronosGroup/glTF-Sample-Models#366.

The loader code looks good to me as far as parameter names in the glTF. Awesome work here.

@elalish
Copy link
Contributor Author

elalish commented May 11, 2023

@emackey Ah, that makes sense why it was looking a little odd then. Eric said he's almost done updating the model (from an artistic standpoint) anyway, so I'll probably wait for that merge and then update it.

#endif
#ifdef USE_NORMALMAP_TANGENTSPACE
#if defined( USE_NORMALMAP_TANGENTSPACE ) || defined( USE_CLEARCOAT_NORMALMAP ) || defined( USE_ANISOTROPY )
Copy link
Collaborator

@Mugen87 Mugen87 Jun 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elalish It looks like there is an issue with this particular line of code. Users report about assets that throw a shader error now:

Program Info Log: Fragment shader is not compiled.


FRAGMENT

ERROR: 0:1384: 'vNormalMapUv' : undeclared identifier
ERROR: 0:1384: 'getTangentFrame' : no matching overloaded function found
ERROR: 0:1384: '=' : dimension mismatch
ERROR: 0:1384: '=' : cannot convert from 'const mediump float' to 'highp 3X3 matrix of float'


  1379: #endif
  1380: #if defined( USE_NORMALMAP_TANGENTSPACE ) || defined( USE_CLEARCOAT_NORMALMAP ) || defined( USE_ANISOTROPY )
  1381: 	#ifdef USE_TANGENT
  1382: 		mat3 tbn = mat3( normalize( vTangent ), normalize( vBitangent ), normal );
  1383: 	#else
> 1384: 		mat3 tbn = getTangentFrame( - vViewPosition, normal, vNormalMapUv );
  1385: 	#endif
  1386: 	#if defined( DOUBLE_SIDED ) && ! defined( FLAT_SHADED )
  1387: 		tbn[0] *= faceDirection;
  1388: 		tbn[1] *= faceDirection;
  1389: 	#endif
  1390: #endif

It seems not right to use USE_CLEARCOAT_NORMALMAP and USE_ANISOTROPY at this point since vNormalMapUv is only defined when a normal map is present (meaning when USE_NORMALMAP is defined).

What was the reason for adding both defines to this check?

An asset for reproduction is available here: https://discourse.threejs.org/t/shader-error-0-validate-status-false/53195

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I may have missed that case. I did add this to ensure the tangents were defined, but perhaps a similar thing is necessary for the UVs? Anisotropy requires UV coordinates (and ideally tangents too), even if no normal map is present. I think it's perfectly reasonable to throw an error if anisotropy is requested without any UV channels being present (that's in the glTF extension spec too).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey sorry to jump in, I was debugging this bug and got here.
@elalish It's possible to have UVs without a normal map being present, which won't create the vNormalMapUv in the shader.
I solved it by setting USE_UV in material.defines that will make vUv available and use that when normal map is not available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's the solution - to set USE_UV in the WebGLRenderer when anisotropy is present even if there's no normal map. Want to try a PR? I'll be happy to review.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also necessary to support the clear coat case. Otherwise the car asset will still fail (it does not use anisotropy).

Copy link
Collaborator

@Mugen87 Mugen87 Jun 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, I don't think USE_UV solves the above issue. USE_NORMALMAP defines the vNormalMapUv varying which is required to make the shader code work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would be nice if the UVs were a little less closely-tied to particular maps, but I'm not sure how much surgery you want to do, @Mugen87. Basically we just need a UV coordinate when anisotropy is used, even if maps aren't present. I think there is some tension between the smallest change to make that work, and the most readable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with a hot fix (smallest change) for now. Things can be refactored later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a small PR #26334 With these changes the model from sketchfab loads without throwing an error.

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.

8 participants