Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tighter shadow culling - fix light colinear to frustum edge #89714

Merged

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Mar 20, 2024

In rare situations if a light is placed near colinear to a frustum edge, the extra culling plane derived can have an inaccurate normal due to floating point error. This PR detects colinear triangles, and prevents adding a culling plane in this situation.

Fixes #89702

Notes

  • Epsilons were measured with typical scales (assuming 32 bit real, there's little to gain by making them smaller for 64 bit). These should hopefully be fine but if there are any more reports they can be adjusted.
  • There are other alternative ways of checking for co-linearity (some slightly faster, e.g. dot product of two normalized sides), but this seems pretty foolproof and is called only once per edge per light per frame, so accuracy is more important than speed.

@lawnjelly lawnjelly added this to the 4.x milestone Mar 20, 2024
@lawnjelly lawnjelly force-pushed the tighter_light_cull_colinear_fix branch from 2dc1791 to 5598a57 Compare March 20, 2024 15:13
@lawnjelly lawnjelly marked this pull request as ready for review March 20, 2024 15:14
@lawnjelly lawnjelly requested a review from a team as a code owner March 20, 2024 15:14
@lawnjelly lawnjelly modified the milestones: 4.x, 4.3 Mar 20, 2024
@lawnjelly lawnjelly force-pushed the tighter_light_cull_colinear_fix branch from 5598a57 to d64abff Compare March 20, 2024 16:28
In rare situations if a light is placed near colinear to a frustum edge, the extra culling plane derived can have an inaccurate normal due to floating point error.
This PR detects colinear triangles, and prevents adding a culling plane in this situation.
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased on top of master 7a42afb), this PR doesn't resolve the issue from the original MRP (#89702 (comment)):

simplescreenrecorder-2024-04-04_15.53.14.mp4

Even if the above video was recorded at 60 FPS, the Max FPS project setting was set to 20 for demonstration purposes.

Disabling the Tighter Shadow Caster Culling project setting resolves the issue. I'm using a 16:9 aspect ratio which is identical to the base window size, so I assume it's what mrjustaguy tested this with.

@lawnjelly
Copy link
Member Author

lawnjelly commented Apr 4, 2024

Ah I'll try again, it could be the epsilon needs adjusting. 👍

I was using a modified version of the scene as it didn't require all the models to show the bug.

UPDATE:
Hmm, I can't seem to replicate it here now, even with the original project. Also both my modified versions work fine.

But given the MRP runs in fullscreen, there could be some small difference given that the bug is to do with float error and epsilons. I'll keep trying to replicate it.

It's possible that I could expose the epsilon in a test build to confirm it fixes for you / tweak the value, as it's pretty empirical if I remember right.

Or you could just bump it up in the source file, it should be the second epsilon currently set to 0.00001f. If you bump it up to say 0.001f and it eliminates the problem then we know it just needs a slightly higher setting.

(Also just to eliminate the obvious, it might be worth double checking you were running the PR build. It's pretty easy to e.g. run a release build after compiling dev build, etc, I've done this many times. 😁 )

Anything special you could mention about your build is useful, compiler, screen resolution, 32 or 64 bit build etc.

UPDATE:
Been trying for an hour now, various resolutions, can't get it to replicate. The frustum is determined by the aspect ratio, so that could affect things, but I tried to give a fairly conservative epsilon.

So I guess @Calinou it would need you to try a few different epsilons and see at what point it is getting fixed on your system. But definitely double check it is running the correct build,

@Calinou
Copy link
Member

Calinou commented Apr 4, 2024

But definitely double check it is running the correct build,

This was indeed an outdated build issue, for the same reason as in #70766 (comment). Apologies.

I've tested the PR locally and it works now.

@akien-mga akien-mga merged commit b5064a6 into godotengine:master Apr 5, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tighter Shadow Caster Culling causes some object shadows to not render for a Frame
4 participants