gkjohnson / three-gpu-pathtracer

Path tracing renderer and utilities for three.js built on top of three-mesh-bvh.
https://gkjohnson.github.io/three-gpu-pathtracer/example/bundle/index.html
MIT License
1.28k stars 125 forks source link

Enabling MIS on macOS / iOS #472

Closed robertoranon closed 6 months ago

robertoranon commented 7 months ago

I've narrowed down the crash (WebGL context lost) on macOS (both Chrome and Safari) / iOS when MIS is enabled to two lines of code, which I have commented out in this PR. They are both in directLightContribution.glsl.js, lines 26 and 38. If you uncomment either of them, the crash occurs again.

robertoranon commented 7 months ago

I am updating the PR as I progress into finding the issue, which now I believe is related to the control flow. In directLightContribution.js, if I substitute all early returns with assigning some result variable, so that control flow keeps going until a last return statement, then the webgl context lost crash disappears (last commit on this PR). So maybe the ANGLE translation to metal has some bugs with big shaders with flow control jumps? I had seen this also when modifying other code parts, adding break or continue statements could trigger the crash, but I couldn't see why. I have still one crash in the attenuateHit function, focusing on it right now

gkjohnson commented 7 months ago

Thanks for tracking these things down! I know it's a tedious process so I appreciate having some other eyes on it.

So maybe the ANGLE translation to metal has some bugs with big shaders with flow control jumps?

I'm hesitant to blame ANGLE, yet, since this all worked before the 14.0 MacOS update. It seems something regressed or change behavior in Apples drivers. Though maybe ANGLE was relying on some kind of functionality it shouldn't have.

Either way if the use of attenuateHit in the direct light contribution function seems to be the issue then maybe it's worth simplifying some of the behavior for area lights first, as in https://github.com/gkjohnson/three-gpu-pathtracer/pull/470#issuecomment-1826775263, may help. Currently "attenuateHit" is checking for fog, lights, and triangle intersections. Given that we don't want lights to be "blockers" anymore there's no reason to include them in the attenuateHit function, which should help simplify it a bit. Basically completing #470 may help this a bit.

robertoranon commented 7 months ago

Ok, I've made some changes in the direction you suggested, i.e. remove the ability to hit lights from traceScene, so if I am understanding correctly, that should give a path tracer that only works with the env map. However, I cannot get rid of the crash, even with the simplified code. In the current modification of this PR, as before, if you comment out the call to attenuateHit at line 28 of directLightContribution.js:

if (
   lightRec.pdf > 0.0 
   && isDirectionValid( lightRec.direction, surf.normal, surf.faceNormal ) 
   && ! attenuateHit( bvh, state, lightRay, lightRec.dist, attenuatedColor )
) {

then it works. Inside the attenutateHit function, what apparently causes the crash is the call to ... traceScene. Even if I transform the entire function to just a call to traceScene, it crashes, which is a bit confusing, because at this point traceScene has been called ... over and over again ?

gkjohnson commented 6 months ago

Hmm - okay thanks for investigating more. Let's get #470 merged first (which would include removing the code for checking lights in attenuate hit) and then look into the MIS issue. I can try a few things to see what might be causing the MIS problem then, too.