-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Undefined behaviour in KHR_materials_volume #2390
Comments
We should test this with a sample model using various colors, some of which have zeros in certain color channels, to confirm if the epsilon solution is working correctly. |
Test model created. This is based on AttenuationTest, but the blocks here lose all red, then all red and green, then half blue, then finally attenuation color becomes completely black. The upper row is transmission (baseColor), the lower row is attenuationColor at 1.0 thickness/distance, for comparison. The screenshot looks like it rendered correctly in the current incarnation of Sample Viewer. The presence of reflections on the black cube suggest that there's not any shader NaN problems going on there. |
The issue is only visible with a large attentuation distance. In this model the left most material has an epsilon in the red channel while the second cube has red set to 0. Both have an attentuation distance of 30. |
Here the epsilon is 1e-7 if I use an epsilon of 1e-64 the colors match roughly. But such values hardly make sense for e.g. 8 bit colors and I don't think the color output is the expected result. |
We want to avoid If the 1e-7 epsilon is used, perhaps I'll contribute a warning to the validator to tell users to stay away. |
There's nothing physically wrong with primary RGB colors to make them invalid (or even warn about them) on the spec level. |
Remember this is color per distance here. "After 1 meter, red loses half its strength, and after another meter, red loses half its remaining strength" is a valid absorption per distance rate. But, "After one meter, green is completely absorbed" is a different statement, no? How fast was green absorbed? Red approaches zero but mathematically never perfectly arrives there. If green gets there after a meter, then green must be using a different equation or must be approaching zero infinitely fast. |
I would assume that e.g. a zero in the red channel means immediate absorbtion at the surface and therefore it ignores distance. This is also currently happening (by chance) in sample viewer. I think it also has applications, for example a scene 30 meters underwater where no red light should be present. On the other hand this could also be achieved by using only lights without red. I don't know if there is a good fix for near zero values. This is a floating point precicsion issue and therefore the color could also appear different based on the precision supported by shaders or hardware. But maybe this is a fringe case which can be ignored? |
The current code is vec3 attenuationCoefficient = -log(attenuationColor) / attenuationDistance;
vec3 transmittance = exp(-attenuationCoefficient * transmissionDistance); Let's inline the vec3 transmittance = exp((log(attenuationColor) / attenuationDistance) * transmissionDistance); Let's regroup a bit: vec3 transmittance = exp(log(attenuationColor) * (transmissionDistance / attenuationDistance)); Both vec3 transmittance = pow(attenuationColor, transmissionDistance / attenuationDistance); |
That simplification looks great! We should test it with the original AttenuationTest sample asset as well as the ones provided in this thread. Perhaps we should include the simplified math in the spec too? |
As described in this sample viewer issue: KhronosGroup/glTF-Sample-Viewer#489
The KHR_materials_volume spec defines the following equation:
where
c
isattenuationColor
andd
isattenuationDistance
.If any of the color components are zero this leads to undefined behaviour.
This can be tested with e.g. Sample Viewer or three.js.
In the sample viewer issue the introduction of an epsilon value is suggested.
Clarification for this issue should be added to the specification.
The text was updated successfully, but these errors were encountered: