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

Properly set size of shadow atlas quadrant when subdivision is 8 or higher. #91545

Merged
merged 1 commit into from
May 4, 2024

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented May 4, 2024

Fixes: #89312

I still can't figure out why this seemingly regressed between dev1 and dev2. Both of these problems have been present for a long time.

Problem 1: Shadow atlas quadrant allocation

Each atlas stores shadows for up to subdivision * subdivision lights (omnilights count for 2). When we set the quadrant subdivisions we reserve enough space in the shadows array for that many lights. We use the length of this shadows array to validate how many shadows can go in each quadrant.

When we set the size of the atlas, we have to clear the quadrants and then resize them. But there we were reserving enough space for 1 << subdivision lights. This is not always the same as subdivision * subdivision! For the last quadrant, this led the system to think that it could store 256 shadows, when it really only had space for 64. So the 65th shadow would try to render outside the framebuffer and cause the error.

Oddly enough I caught this bug in the OpenGL renderer when implementing shadows. I remember at the time noticing it, but not doing anything in the RD renderers because it wasn't causing any issues and I didn't have time to investigate why.

Problem 2: No validation for render pass rect size

draw_list_begin is supposed to validate the size of the render pass rect. But it was missing a pair of parentheses, so instead of checking if the rect was fully inside the framebuffer. It checked if the rect began outside the framebuffer on the X-axis, but otherwise fit within the framebuffer. This bug has been present for a long time.

Not every device crashed/froze despite us allowing values that should not have been allowed as different drivers handle breaking spec differently.

@akien-mga
Copy link
Member

If those two issues pre-exist the apparent regression, should we consider cherry-picking this to 4.2?

@akien-mga akien-mga merged commit 7ebc866 into godotengine:master May 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@clayjohn clayjohn deleted the RD-shadow-atlas branch May 5, 2024 20:39
@clayjohn
Copy link
Member Author

clayjohn commented May 5, 2024

If those two issues pre-exist the apparent regression, should we consider cherry-picking this to 4.2?

I would need to make a separate PR as this one won't cherrypick cleanly. However, the crash/freeze doesn't happen in versions before 4.3 dev2. In earlier versions these issues are present, but the symptom is not

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.

Many lights on screen renders editor and runtime unresponsive until restart [4.3 dev]
2 participants