Conversation
|
|
|
Thanks for this contribution, @r800360, and welcome to the project! This is a clean implementation of the proposal in #2859, following the same pattern as the existing One substantive concern is the white point of the thin-film reflectance. The CIE 1931 RGB matrix from the Belcour paper has an equal-energy white point, and since My current thinking is that the simplest robust fix is to normalize in the shader, so that equal-energy white maps to neutral for any matrix the integrator supplies: // Convert back to RGB reflectance, normalizing so that equal-energy white maps to neutral
I = mx_matrix_mul($xyzToWorkingSpace, I) / mx_matrix_mul($xyzToWorkingSpace, vec3(1.0));
I = clamp(I, 0.0, 1.0);@doug-walker, I'd value your perspective before we commit to an approach, so that we're not relying solely on our own reading of the color science. Is a per-channel normalization of this kind a reasonable adaptation for reflectance data, or would you recommend a proper chromatic adaptation transform such as Bradford? And do you agree that blackbody emission should remain unadapted while thin-film reflectance is adapted? Either way, the A few smaller notes:
Finally, since this changelist leaves the option as a fixed |
doug-walker
left a comment
There was a problem hiding this comment.
A few minor suggestions.
For the record, the original Slack thread was here:
https://academysoftwarefdn.slack.com/archives/C0230LWBE2X/p1775741600062129
| { | ||
| // XYZ to CIE 1931 RGB color space (using neutral E illuminant) | ||
| const mat3 XYZ_TO_RGB = mat3(2.3706743, -0.5138850, 0.0052982, -0.9000405, 1.4253036, -0.0146949, -0.4706338, 0.0885814, 1.0093968); | ||
|
|
There was a problem hiding this comment.
Replace with:
// Use Bradford chromatic adaptation to convert the white point from illuminant E,
// used by the Belcour paper, to illuminant D65, expected by the xyzToWorkingSpace
// matrix. The matrix converts {1., 1., 1.} to {0.95046, 1., 1.08906}. (Note that the mat3
// constructor expects a column-major vector.)
const mat3 E_TO_D65 = mat3(0.95314737, -0.03827266, 0.00261539, -0.02661088, 1.02885095, -0.00304736, 0.02391944, 0.00942171, 1.08948971);
Then keep the matrix multiply below, replacing XYZ_TO_RGB with E_TO_D65.
| The LightData struct is built dynamically depending on requirements for | ||
| bound light shaders. | ||
| $xyzToWorkingSpace u_xyzToWorkingSpace mat3 Transform from CIE XYZ to the renderer's linear working color space, | ||
| initialized from GenOptions.xyzToWorkingSpace. |
There was a problem hiding this comment.
Replace "CIE XYZ" with "CIE XYZ D65".
| xyzToWorkingSpace( | ||
| 3.2406f, -0.9689f, 0.0557f, | ||
| -1.5372f, 1.8758f, -0.2040f, | ||
| -0.4986f, 0.0415f, 1.0570f), |
There was a problem hiding this comment.
Five digits is not enough to fully initialize a float (needs 6-7). Here are some more digits:
3.240969941905, -0.969243636281, 0.055630079697, -1.53738317757, 1.875967501508, -0.203976958889, -0.498610760293, 0.041555057407, 1.056971514243
|
|
||
| /// Transform from CIE XYZ to the renderer's linear working color space. | ||
| /// Defaults to the CIE XYZ to lin_rec709 transform. | ||
| Matrix33 xyzToWorkingSpace; |
There was a problem hiding this comment.
Suggest:
// Transform CIE XYZ values, with a D65 white point, to the renderer's linear working color space.
CIE XYZ does not have a fixed white point. In order for this matrix substitution technique to work, the white point on the XYZ side must be specified.
Fixes #2859
Summary
GenOptions::xyzToWorkingSpace, defaulting to the CIE XYZ tolin_rec709transform.u_xyzToWorkingSpace.Testing
MaterialXTest "GenShader: GLSL XYZ to Working Space"MaterialXTestsuite