FOG_EXP2: Avoid possible over-/underflow when squaring on mediump

[EliasHasle]

No description provided.

[EliasHasle]

Context: 69a8d3b#diff-70001492be345858ece4ba692249de2c

Before that commit, there was independent potential for overflow and underflow.

Using the/a square function would improve readability and likely not affect performance. Should I modify to square( fogDensity * fogDepth )? Edit: No, apparently square is not defined anywhere. (and pow is implemented differently and not defined for non-positive x, according to this source: https://community.khronos.org/t/pow-x-2-different-then-x-x/70839)

Hm, I believe reintroducing square will also be useful, but always make sure to chain multiplications before squareing.

[WestLangley]

Why not write it like so?

float fogFactor = 1.0 - exp( - pow2( fogDensity * fogDepth ) );

[EliasHasle]

@WestLangley Quoting https://community.khronos.org/t/pow-x-2-different-then-x-x/70839 (which refers to the docs)

it is implemented as:
pow(x,y) = exp2 (y * log2 (x))

Would you want to replace all squares with this? log2(x) will fail for non-positive x, and exp and log are more demanding to calculate than simple multiplications, right? pow is made to be general, but for squaring, only multiplication is needed.

[EliasHasle]

More support for my PR:
https://docs.nvidia.com/drive/nvvib_docs/NVIDIA%20DRIVE%20Linux%20SDK%20Development%20Guide/baggage/GLSL_ES_Specification_3.00.4.pdf states that the operators are, as expected, left-associative.

Here are descriptions (non-official source) of float precisions and a mention of the problem that @WestLangley tackles with #17323 : http://asawicki.info/news_1596_watch_out_for_reduced_precision_normalizelength_in_opengl_es.html

Now consider the task of multiplying small,small,big,big in some order. If any part of the calculation fails, the whole calculation fails. small*small can cause underflow when the scale of the product is too small to be represented by the exponent bits. big*big can cause overflow if the scale of the product is too large to be represented by the exponent bits. small*big (or big*small) gives the best chance of avoiding extremes on the exponent bits. And the other two multiplicands can be grouped the same way without introducing more risks of overflow. Eventually (small*big)*(small*big) must be evaluated, and this will eventually work in many cases where (small*small)*(big*big) would fail.

But it is not exactly elegant to write out all squares as products where all factors are repeated. This would be more elegant:

float xy = x*y;
float xysq = xy*xy;

And this would be more elegant, and be about as easy for the compiler to optimize:

//In common lib:
float square(float x) {
    return x*x;
}

//In shader:
float xysq = square(x*y);

Apparently, square was defined as a macro earlier, and removed by the same commit that made the underflow-unsafe expression for the fog: 69a8d3b

I believe a function is much better than a macro for this, because a macro will expand the argument and evaluate it two times, whereas a function will evaluate the argument and then square only the result.

I am beginning to feel that I should include a new square function and use it in this PR. Is that OK @mrdoob ?

[WestLangley]

#17324 (comment).

[EliasHasle]

I answered that above.pow(x,2) is less readable than square(x), and the pow function is more general (less optimizable at compile time), yet does not support non-positive x.

[WestLangley]

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/common.glsl.js#L13

[WestLangley]

float fogFactor = 1.0 - exp( - pow2( fogDensity * fogDepth ) );

For clarity, can you please fix the formatting and squash the commits?

[EliasHasle]

I have fixed the formatting, but don't know how to squash the commits. I made the edit in the online editor.

[EliasHasle]

I failed to squash the commits, but achieved the intended outcome by using the sledgehammer of force-push. 😨 Looks right, although the "Thank you, @WestLangley!" disappeared. Thanks, anyway. 😄

[EliasHasle]

Abandoning in favor of #17355. EDIT: Or not.

[EliasHasle]

I would like to reopen this, since it has merit on its own, as described in the discussion above. The large, neglected and currently outdated #17592 should not be holding back a bug fix.

I should add that this is now more properly called a Draft PR, since the newest version is completely untested. It theoretically prevents some overflow/underflow situations that may arise with mediump.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize	Gzipped	Diff from dev
617.5 kB	154.5 kB	-17 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize	Gzipped	Diff from dev
409.2 kB	100.6 kB	-17 B