CSM: Avoid circular dependency in camera computations.

[OndrejSpanel]

Fixed #25246.

Description

CSM computations in CSM.update used light.shadow.camera position and orientation, but those were set during the rendering based on light.position (see LightShadow.updateMatrices), which itself was set later in the CSM.update. This led to a kind of chicken and egg problem, as a result the shadows in the first frame rendered ever or after significant camera orientation change had some shadows missing because of improper bounding boxes computed for the shadow frustums.

The PR avoids the circular dependency, using lightDirection to compute the transformation matrix directly.

[OndrejSpanel]

@Mugen87 @WestLangley Can someone please check the PR? The fix is short and simple, but finding the exact cause was quite time consuming and I would hate to lose that work.

[OndrejSpanel]

There is some screenshot difference reported after the last commit:

ERROR! Diff wrong in 0.938 of pixels in file: webgl_shadowmap_csm

Does this mean means most pixels are wrong, or that one pixel is wrong?

When running locally I do not see any noticeable difference.

[Mugen87]

https://threejs.org/examples/webgl_shadowmap_csm
https://raw.githack.com/OndrejSpanel/three.js/fix_csm_computations/examples/webgl_shadowmap_csm.html

The boxes have a random height but it seems the shadow quality itself has changed when comparing dev and prod.

[OndrejSpanel]

On my computer I do not see any difference in the shadow quality, only in the random box size (which is not new, therefore I do not think it should be a problem). If you see a difference, can you please post some screenshots?

[Mugen87]

Granted, the difference is not easy to spot. But check the shadows of the second and third box.

Prod:

Dev:

[OndrejSpanel]

I have run make-screenshot locally. One difference I see compared is previous version contained some artifact line, which is not present in my version:

I commit the screenshot from the new version, though I am not convinced it will pass, as this cannot account for 93% of pixels.

When you open 9024c70, you should see the artifact (using Swipe works best for me).

[OndrejSpanel]

Granted, the difference is not easy to spot.

If there is any difference in the shadow quality, I am willing to investigate where does it come from, as I did not expect any. Yet, does it explain why the test is failing, esp. now, when it is failing against the screenshot captured on my computer with the fix implemented?

[Mugen87]

Not sure why the E2E test fails under these conditions. I'll try a re-run.

[OndrejSpanel]

It has already failed three times in a row, therefore I guess it sees something significant. I have no idea what could it be, though. 😟

[OndrejSpanel]

I have reverted the DefaultUp commit - let us see if it has any influence on the test on the runner (I see no difference on my computer). Based on the result, I will try a few more commits to pinpoint the issue.

[OndrejSpanel]

I think it is obvious now something is wrong with Object3D.DefaultUp during the e2e test. The most compelling evidence is fea82c0. I guess the variable is either not initialized, or has a different value.

I do not see any issue when running npm run make-screenshot webgl_shadowmap_csm on my computer even when it is using the DefaultUp,

I have to say I am confused and unsure how to proceed.

[Mugen87]

TBH, as long as #25246 is fixed, I'm okay with the PR in its current state^^. When merged, I have a look at the E2E issue on my local computer.

[OndrejSpanel]

I see you approved it - should I use the version with camera.up, or rather define my own _up? Each of them has limitations: camera.up is changed when rendering cubemaps, my own _up does not follow any changes in Object3D.DefaultUp.

I have no idea how would I debug the e2e runner issues - even console.log does not seem to work in npm run make-screenshot, therefore all I can see is the task has failed or succeeded, which does not tell much.

[OndrejSpanel]

When merged, I have a look at the E2E issue on my local computer.

I hope you will see something, as I did not see any issues not only with the example, but with E2E task as well - the screenshot was produced fine even with DefaultUp.

[Mugen87]

Wait....Object3D.DefaultUp is deprecated (see #25188). Try it with Object3D.DEFAULT_UP.

[OndrejSpanel]

My PR was based on r.145dev previously. I have rebased it to current dev and added DEFAULT_UP. It seems it is still failing.

[OndrejSpanel]

I am dropping the DEFAULT_UP commit again. For anyone interested it is stored in https://github.com/OndrejSpanel/three.js/tree/failing_e2e_csm_defaultup

[Mugen87]

@gkjohnson What do you think about this PR (and the original issue)?

[OndrejSpanel]

What can be done about the PR now?

[Mugen87]

I suppose the issue is also present in the original repository: https://github.com/StrandedKitty/three-csm

@StrandedKitty Do you mind having a look at #25246 and this PR?

[Mugen87]

@mrdoob When CSM was added to the repo with #18481, the sources from the original repository were copied over and now the implementations have diverged.

I think it would be better to refactor the code and organize it similar to the external examples (bvh, pathtracer, ifc etc.). Meaning include a three-csm build from a CDN. The code is then maintained in https://github.com/StrandedKitty/three-csm and not here.

[StrandedKitty]

@OndrejSpanel This is great that you've figured it out! Pretty clever solution. I was aware of this bug for quite some time but it seemed too minor to spend time looking into. Actually I think I have the same issue in my another CSM implementation built on top of a custom engine...

[mrdoob]

@Mugen87 I guess it's up to @StrandedKitty...

@StrandedKitty What would you like us to do?

[StrandedKitty]

@StrandedKitty What would you like us to do?

I agree that it's better to maintain the CSM code in one place (in the original repository) and use it in the example through import. I think when I originally made a PR to add CSM example here none of the existing examples used imports so we didn't even consider this option at that point.

[OndrejSpanel]

If the original repo should be "the single source of truth", I would like to remind of another PR of mine, which went into this repo: #23631. You might find others fixes in https://github.com/mrdoob/three.js/commits/master/examples/webgl_shadowmap_csm.html

[LeviPesin]

#25119 also slightly modified CSM's code. Here is the complete list.

[StrandedKitty]

@OndrejSpanel @LeviPesin Thanks, of course I will have to move all the recent changes that are only present here to my repository. And after this I will make a new PR to modify the example. But for now we should merge this PR.

[Mugen87]

@StrandedKitty As an orientation, check out how webgl_raycaster_bvh is organized and how it includes three-mesh-bvh. Ideally, the CSM example is structured similar 👍 .