KHR_materials_clearcoat_darkening #2518
Conversation
bghgary
left a comment
bghgary
left a comment
There was a problem hiding this comment.
Some minor issues. And again I don't know the math to review that part, but otherwise, LGTM.
extensions/2.0/Vendor/EXT_materials_clearcoat_darkening/README.md
Outdated
Show resolved
Hide resolved
| "clearcoatDarkeningTexture": { | ||
| "allOf": [ { "$ref": "textureInfo.schema.json" } ], | ||
| "description": "The clearcoat layer darkening texture.", | ||
| "gltf_detailedDescription": "The clearcoat layer darkening texture. The values are stored in sRGB. Assume white colour if no texture is supplied." |
There was a problem hiding this comment.
Is this correct? Should it be linear instead? We should also indicate the channel being used. Similar to this?
There was a problem hiding this comment.
It should definitely be linear. Copy-paste error, I think.
Thanks.
extensions/2.0/Vendor/EXT_materials_clearcoat_darkening/README.md
Outdated
Show resolved
Hide resolved
extensions/2.0/Vendor/EXT_materials_clearcoat_darkening/README.md
Outdated
Show resolved
Hide resolved
|
|
||
| \(T = (1-R) / (1 + R + R² + R³ + ...)\) | ||
| \(T = (1-R) / (1/(1-R))\) | ||
| \(T = (1-R)²\) |
There was a problem hiding this comment.
I don't think this formatted well. It's supposed to be 3 lines?
There was a problem hiding this comment.
Yeah. I'm using a markdown preview in VSCode and it's quite different from what GitHub seems to display.
| \(T = (1-R)²\) | ||
|
|
||
| The $(1-R)$ in the numerator represents the initial transmission of light through the coat and the denominator accounts for the infinite reflections within the coating. This converges to $1/(1-R)$. | ||
| Note that $(1-R)/(1+R)$ is mathematically equivalent to $(1-R)²$ but is more numerically stable as $R$ approaches 1.0. |
There was a problem hiding this comment.
I don't follow. How is
There was a problem hiding this comment.
Oops. Yeah, they aren't mathematically equivalent at all. This should have been mathematically "similar".
Also, the comment about the approximation being more numerically stable seems suspect too... I might remove this entirely.
There was a problem hiding this comment.
I've just removed this. I've tested (1-R)^2 and I don't see any issues with it. It matches our raytracer a bit better too.
| "allOf": [ { "$ref": "glTFProperty.schema.json" } ], | ||
| "properties": { | ||
| "clearcoatDarkeningFactor": { | ||
| "type": "array", |
There was a problem hiding this comment.
If it's just one value, consider making it a number instead of an array.
MiiBond
force-pushed
the
mbond/EXT_materials_clearcoat_darkening
branch
from
a001fbe to
5c5e4ae
Compare
MiiBond
changed the title
4 participants
This extension adds the ability to adjust the amount of physically correct darkening caused by interreflections within the coat.
It's designed to provide the same functionality as the
coat_darkeningparameter in OpenPBR.