KhronosGroup / glTF

glTF – Runtime 3D Asset Delivery
Other
7.16k stars 1.14k forks source link

Undefined behaviour in KHR_materials_volume #2390

Closed UX3D-haertl closed 3 months ago

UX3D-haertl commented 5 months ago

As described in this sample viewer issue: https://github.com/KhronosGroup/glTF-Sample-Viewer/issues/489

The KHR_materials_volume spec defines the following equation:

σt = -log(c) / d

where c is attenuationColor and d is attenuationDistance. 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.

emackey commented 4 months ago

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.

emackey commented 4 months ago

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.

AttenuationToZero.zip

screenshot

UX3D-haertl commented 4 months ago

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.

grafik AttenuationToZero_Dist.zip

UX3D-haertl commented 4 months ago

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. Also since log(0) is undefined behaviour in GLSL the result can vary based on the driver implementation of the specific graphic card. https://registry.khronos.org/OpenGL-Refpages/es3.0/html/log.xhtml

emackey commented 4 months ago

We want to avoid log(0) of course, and I don't think having zeros in the color makes any physical sense unless the material is intended to be completely opaque in that color channel. Should we warn users not to place zeros there? Or should there be special case code to avoid calling log(0) and instead set that component to 100% absorption? Seems like the latter would hurt performance and add complexity.

If the 1e-7 epsilon is used, perhaps I'll contribute a warning to the validator to tell users to stay away.

lexaknyazev commented 4 months ago

There's nothing physically wrong with primary RGB colors to make them invalid (or even warn about them) on the spec level.

emackey commented 4 months ago

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.

UX3D-haertl commented 4 months ago

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?

lexaknyazev commented 4 months ago

The current code is

vec3 attenuationCoefficient = -log(attenuationColor) / attenuationDistance;
vec3 transmittance = exp(-attenuationCoefficient * transmissionDistance);

Let's inline the attenuationCoefficient removing both minus signs:

vec3 transmittance = exp((log(attenuationColor) / attenuationDistance) * transmissionDistance);

Let's regroup a bit:

vec3 transmittance = exp(log(attenuationColor) * (transmissionDistance / attenuationDistance));

Both exp and log could be removed now:

vec3 transmittance = pow(attenuationColor, transmissionDistance / attenuationDistance);
emackey commented 4 months ago

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?