Fixed lingering Sky Shader bug (tentative, needs confirmation)

[erichlof]

Description of the problem

I was playing around with syntopia's GLSL implementation of the Preetham sky model and using the original code on glslsandbox, came across the Android bugs and Windows ANGLE bugs involving the TotalRayleigh() function which uses pow() with very small values. I then realized three.js uses almost the same code with the same issues that have been discussed and resolved or worked around in the past.

I replaced the pow(Lambda, vec3(4.0)) calculation with a pre-calculated constant vec3 Kray4 . Lambda is constant anyway so there's no need to calculate it every pixel. It is stored away in Kray4 for reuse.

Now, using the original sky model Rayleigh calculations, the shader works both on my Windows/ANGLE device as well as my Android smartphone.

Please visit: http://glslsandbox.com/e#36146.2 update with new const vec3 totalRayleigh: http://glslsandbox.com/e#36146.5 and confirm if it is working on your devices as well.

Once we have confirmation, I can make a PR if you like. Glancing at the calculations, there are some optimizations to be made involving storing constants instead of calculating them over and over again for each pixel. For example line 70 in the example above, the following calc could be made a constant: vec3 inverse = 1.0 / ( 3.0 * N * Kral4 * ( 6.0 - 7.0 * pn ) );

One more minor issue is that we could remove the changing from exp(n) to pow(e,n) in #8614 and the simplified calculation in #5602

ping @zz85 @brianchirls @mrdoob @arturitu

Three.js version

• [x] Dev
• [x] r81
• [ ] ...

  Browser

• [x] All of them
• [ ] Chrome
• [ ] Firefox
• [ ] Internet Explorer

  OS

• [x] All of them
• [ ] Windows
• [ ] Linux
• [ ] Android
• [ ] IOS

  Hardware Requirements (graphics card, VR Device, ...)

[brianchirls]

A couple of things here:

First, the shader you sent doesn't work on my S6.

Second, I think you're looking at the master version of SkyShader, which is not the latest. I've already made a significant optimization refactor in #9684, which is on dev and should be included in the next release. Please see the new shaders and if you find further optimizations, I'd be happy to take a look.

Thanks.

[erichlof]

@brianchirls Ok thanks for taking a look. I have an S7 and it works as expected so that's why I needed confirmation before I went any further with this. There must be some discrepancies in the Android GPU drivers between the S6 and the latest S7.

mrdoob changed the main three.js page to target dev so I'm pretty sure I'm looking at the latest SkyShader.js file with your #9684 refactors.

Come to think of it, I have an older S5 lying around from my mobile contract. I'll try to get the original equations working on that and report back.

[brianchirls]

I'm actually gonna be at Samsung today so I'll try and ask around about these issues.

I don't see any reference to pow on Lambda so I guess I'm not following exactly what you're proposing. But now that I've taken another look, I do see at least a couple places where some redundant calculation is happening. If I have time today, I'll try and iron those out.

[erichlof]

Ok great! (about the Samsung issue investigations).
About the pow(Lambda,vec3(4.0)), that was the original issue that was breaking the TotalRayleigh() function, which in turn prompted this: #5602 This is a simplification of the original equations in the Preetham model. For the first implemntation, have a look at zz85's original PR: https://github.com/mrdoob/three.js/commit/55dd8add1e4c11af67d0c7b53d7efa540a2dd41a which in turn comes from: https://github.com/SimonWallner/kocmoc-demo/blob/RTVIS/media/shaders/scattering.glsl and https://github.com/SimonWallner/kocmoc-demo/blob/RTVIS/media/shaders/base.frag

If you calculate those original Lambda values (pow(lambda, vec3(4.0)) according to the Preetham model, the .r (red) first value of the vec3 ends up coming out to 2.1381376e-25 . I don't think Android GPU has this amount of pow() precision, hence my constant pre-calculated value suggestion above.

Thanks again!

[erichlof]

update: return (8.0 * pow(pi, 3.0) * pow(pow(n, 2.0) - 1.0, 2.0) * (6.0 + 3.0 * pn)) / (3.0 * N * pow(lambda, vec3(4.0)) * (6.0 - 7.0 * pn)); doing the calculations by hand I get 9.98324224e-8 for the first .r value of the returned vec3

where as the simplified PR gives 5.31914e-6 for the .r value of the vec3

It might not make a big visual impact to the average viewer, but it's just the principle of the thing, knowingly using an incorrect calculation

Ultimately we could just save the correctly calculated values as a constant vec3. The only caveat is that this only works for Earth skies. If you want a Mars sky or something, all the values are no longer valid because the values of N, pn, and the vec3 lambda have to be updated for that particular atmosphere.

[erichlof]

Compatibility Update: I just booted up my old Samsung Galaxy S5 and the shader link above works perfectly. The only minor visual artifact is that the sun disk is a little too small for my taste and parts of it come and go as it rises and sets. But other than that, the sky part is working as expected.

I'm confused as to why it's not running on @brianchirls 's later Galaxy S6 . Brian, are you trying the shader link in the latest Chrome for Android on your S6? Just curious. Thanks for checking all this stuff out. :)

[erichlof]

Well, it's been 25 years since Algebra-1 so I apologize for miscalculating the Preetham equation for Rayleigh scattering. I did not group the terms correctly. Here is the original paper, scroll down to the middle of page 23: https://www.cs.utah.edu/~shirley/papers/sunsky/sunsky.pdf

8.0 * pow(pi, 3.0) * pow(pow(n, 2.0) - 1.0, 2.0)......................6.0 + 3.0 * pn --------------------------------------------------.. X ....... ( ---------------- ) 3.0 * N * pow(lambda, vec3(4.0)).................................................6.0 - 7.0 * pn

The above is in GLSL-friendly format (sorry for the formatting spacing periods, don't know how to copy equations from pdf's to github yet). Plugging in the aforementioned tiny lambda.r value, I get 0.00000580453 Now this is very close to @arturitu 's simplified value of 0.00000531914 I'm not sure why there are discrepancies.

In any case, I propose making a new constant vec3 called totalRayleigh, using the above calculations from the Preetham paper. Here's how it would like in SkyShader.js: const vec3 totalRayleigh = vec3( 0.00000580453, 0.00001278534, 0.00002853075 );

Then on https://github.com/mrdoob/three.js/blob/bc48aed587c0833c44f92f83dc1b0109cc2244ab/examples/js/SkyShader.js#L94-L96 We change line 96 to read: "vBetaR = totalRayleigh * rayleighCoefficient;",

If everyone's ok with this, I'll make a new PR with all the changes. Thanks!

edit: jury's still out on the exp(n) vs. pow(e,n) issue on Samsung Galaxy S6. The shader works perfectly on my S5 and S7, so I'm confused by the S6 issue. Did @zz85 and kenrussell draw any conclusions back in April when this issue was reported by Brian in #8614? p.s. sorry for all the editing of this comment :)

[brianchirls]

But simplifiedRayleigh is already a constant, so how does your change make this any more efficient? Is there an actual bug here that needs to be fixed?

I talked to my contacts at Samsung about the exp bug and got a vague "we'll look into it." Seems like it's one of a few problems with the driver for that particular GPU. So I wouldn't count on a fix for that any time soon.

[erichlof]

simplifiedRayleigh is off by a little bit. My calculations come from the cited paper. There's no speedup regarding that proposed. However, some of the other calculations can be squashed into a const also. For example, I'm pretty sure some of this can be more efficient: https://github.com/mrdoob/three.js/blob/bc48aed587c0833c44f92f83dc1b0109cc2244ab/examples/js/SkyShader.js#L73-L77 and this: https://github.com/mrdoob/three.js/blob/bc48aed587c0833c44f92f83dc1b0109cc2244ab/examples/js/SkyShader.js#L134-L142 I'll look into it before drafting a PR.

Thank you for asking about the S6 bug. My mom has an S6, so I'll see if I can borrow it for testing purposes! :-D

[brianchirls]

Okay thanks for clarifying. I'm looking forward to your PR. I'll hold off on my optimization until I see yours just so we don't step on each other's toes.

[erichlof]

Hi, sounds good! I haven't had a chance to test on the S6 yet, but I'll go ahead and put forth the const optimizations so that maybe can get merged. From what you've told me about your S6 and what Samsung said, I'm not expecting any breakthroughs on that front. It's unfortunate that seemingly just that model is affected by the pow() issue.

[brianchirls]

Agreed. Send your PR and I'll test on my S6.

On Oct 24, 2016 11:57 AM, "Erich Loftis" notifications@github.com wrote:

Hi, sounds good! I haven't had a chance to test on the S6 yet, but I'll go ahead and put forth the const optimizations so that maybe can get merged. From what you've told me about your S6 and what Samsung said, I'm not expecting any breakthroughs on that front. It's unfortunate that seemingly just that model is affected by the pow() issue.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mrdoob/three.js/issues/9894#issuecomment-255782542, or mute the thread https://github.com/notifications/unsubscribe-auth/AAeDA5ldhb4umU3LR0i5cpRmJg1gxER3ks5q3NV7gaJpZM4KbOio .

[brianchirls]

@mrdoob looks like this was resolved in #9928. This can probably be closed.