Skip to content

[BREAKING] Feature: KHR_materials_anisotropy #7668

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

Merged
merged 46 commits into from
May 21, 2025

Conversation

emadurandal
Copy link
Contributor

@emadurandal emadurandal commented May 14, 2025

This PR is an implementation of KHR_materials_anisotropy.

Notice

  • The valid value range for the anisotropy property of StandardMaterial is changed from [-1,1] to [0,1].
  • The visual behavior differs from the existing anisotropy implementation.

image
image
image
image

I also noticed during implementation that there seems to be a discrepancy in the results in the implementation variation of the getTBN() shader function. The code for the “object space TBN” case seems to compute a different (possibly incorrect) result than the codes for the other branches. When going through the “object space TBN” branch, the result of the anisotropic direction is the opposite of what is expected.

https://github.com/playcanvas/engine/blob/48a2276427d7860354655fdcc59f10572459e5ff/src/scene/shader-lib/chunks-glsl/lit/frag/TBN.js

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a draft implementation for KHR_materials_anisotropy in the WebGL(GLSL) pipeline, updating material properties, shaders, parsers, and examples to support anisotropy and its rotation.

  • Added new material properties and texture maps for anisotropy and anisotropy rotation.
  • Updated shader chunks and programs to utilize anisotropy parameters and integrate with the GGX specular branch.
  • Extended the GLB material parser to handle the new anisotropy extension and updated relevant tests and examples.

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
utils/plugins/rollup-types-fixup.mjs Added new anisotropy-related material property definitions.
test/scene/materials/standard-material.test.mjs Extended tests to cover anisotropy and its associated properties.
src/scene/shader-lib/programs/standard.js Integrated anisotropy defines into the shader generation for GGX specular.
src/scene/shader-lib/chunks-glsl/standard/frag/stdFrontEnd.js Included anisotropy shader chunks in the fragment front-end.
src/scene/shader-lib/chunks-glsl/standard/frag/stdDeclaration.js Declared anisotropy uniforms and texture allocations.
src/scene/shader-lib/chunks-glsl/standard/frag/anisotropyRotation.js & anisotropy.js Added new shader functions for sampling anisotropy and its rotation.
src/scene/shader-lib/chunks-glsl/lit/frag/reflDirAniso.js Updated reflection direction calculation to incorporate anisotropic effects.
src/scene/shader-lib/chunks-glsl/lit/frag/lightSpecularAnisoGGX.js Modified specular computation logic to account for anisotropy blending.
src/scene/materials/standard-material.js Updated material property definitions and parameter setting for anisotropy.
src/framework/parsers/glb-parser.js Extended GLB parsing to support anisotropy material extension.
examples/* Added new examples to demonstrate anisotropy strength, rotation, disc and lamp features.

@Maksims
Copy link
Collaborator

Maksims commented May 15, 2025

Why this PR is marked as "breaking"? If there are breaking changes to APIs, please document them in first post. We always try to avoid breaking anything public, and if that is not possible, then provide a deprecation path, so old ways still work and devs have time to migrate without forcing them with broken state.

@emadurandal
Copy link
Contributor Author

@Maksims I may have used the wrong meaning of “breaking”. There are no API-disruptive changes in this PR. I just wanted to communicate that the appearance of anisotropic reflections could be different than before. If it is inappropriate to add “breaking” in such a case, I will remove it.
(I am new to PlayCanvas development and am still getting used to the conventions.)

@mvaligursky
Copy link
Contributor

This looks pretty great! Some feedback:

  1. I would only add a single texture, instead of anisotropyMap and anisotropyRotationMap. Data is always stored in a single texture, so no need for it to consume two samplers.
  2. Similarly then for the new chunks - we should just have one, and add defines as needed to control its behavior.

@mvaligursky
Copy link
Contributor

I agree this should be marked as BREAKING, as the visuals of this feature enabled will be different then before. We use this flag to bring it up to attention of our users.

@emadurandal
Copy link
Contributor Author

Some feedback:

Thank you for the feedback.
Please wait a moment while I modify the code.
If we were to use a single Sampler, we would not be able to specify the channel in MapChannel.
It will be fixed in the rg channel for Rotation and in the b channel for Strength.

#endif

#ifdef STD_ANISOTROPY_TEXTURE
dAnisotropy *= texture2DBias({STD_ANISOTROPY_TEXTURE_NAME}, {STD_ANISOTROPY_TEXTURE_UV}, textureBias).b;
Copy link
Contributor

Choose a reason for hiding this comment

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

only sample the texture one time to a temporary and get the data out, instead of twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at d1504aa

@@ -84,6 +84,15 @@ export default /* glsl */`
#include "clearCoatNormalPS"
#endif

// anisotropy
#ifdef LIT_SPECULAR
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd be clener to write #if defined(LIT_SPECULAR) && defined() ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at d9bbc21

@@ -197,6 +206,14 @@ export default /* glsl */`
litArgs_clearcoat_worldNormal = ccNormalW;
#endif

#ifdef LIT_SPECULAR
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at d9bbc21

#endif

#ifdef STD_ANISOTROPY_TEXTURE
dAnisotropy *= textureSampleBias({STD_ANISOTROPY_TEXTURE_NAME}, {STD_ANISOTROPY_TEXTURE_NAME}Sampler, {STD_ANISOTROPY_TEXTURE_UV}, uniform.textureBias).b;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, sample the texture once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at d1504aa

@@ -85,6 +85,15 @@ export default /* wgsl */`
#include "clearCoatNormalPS"
#endif

// anisotropy
#ifdef LIT_SPECULAR
Copy link
Contributor

Choose a reason for hiding this comment

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

one liner define pls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at d9bbc21

@@ -13,6 +13,7 @@ import blurVSMPS from '../chunks-glsl/lit/frag/blurVSM.js';
import clearCoatPS from '../chunks-glsl/standard/frag/clearCoat.js';
import clearCoatGlossPS from '../chunks-glsl/standard/frag/clearCoatGloss.js';
import clearCoatNormalPS from '../chunks-glsl/standard/frag/clearCoatNormal.js';
import anisotropyPS from '../chunks-glsl/standard/frag/anisotropy.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

these are sorted alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at 6bdd3b2

@@ -187,6 +188,7 @@ const shaderChunksGLSL = {
clearCoatPS,
clearCoatGlossPS,
clearCoatNormalPS,
anisotropyPS,
Copy link
Contributor

Choose a reason for hiding this comment

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

these are sorted alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at 6bdd3b2

@@ -182,6 +183,7 @@ const shaderChunksWGSL = {
clearCoatPS,
clearCoatGlossPS,
clearCoatNormalPS,
anisotropyPS,
Copy link
Contributor

Choose a reason for hiding this comment

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

these are sorted alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at 6bdd3b2

@@ -13,6 +13,7 @@ import blurVSMPS from '../chunks-wgsl/lit/frag/blurVSM.js';
import clearCoatPS from '../chunks-wgsl/standard/frag/clearCoat.js';
import clearCoatGlossPS from '../chunks-wgsl/standard/frag/clearCoatGloss.js';
import clearCoatNormalPS from '../chunks-wgsl/standard/frag/clearCoatNormal.js';
import anisotropyPS from '../chunks-wgsl/standard/frag/anisotropy.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

these are sorted alphabetically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed at 6bdd3b2

@mvaligursky
Copy link
Contributor

This looks great! I added a bunch or small comments, should be easy to wrap it up now.

@emadurandal
Copy link
Contributor Author

Thank you for the comments. All fixed.

@mvaligursky
Copy link
Contributor

by the way, you can run npm run lint -- --fix to fix most of the lint issues automatically

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@mvaligursky mvaligursky merged commit b0b7a92 into playcanvas:main May 21, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants