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

[3.x] Take resolution into account when setting the max shadow cubemap size (omni light shadow jaggies fix) #47160

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

puchik
Copy link
Contributor

@puchik puchik commented Mar 19, 2021

#37678 fixed shadow issues for 4.0, but #24587 still occurs in 3.x. Specifically, omni lights used a hardcoded value for max shadow cubemap size.

Screenshots are from the reproduction project in #24587
2048x2048
image
4096x4096
image
8192x8192
image
16384x16384
image
image

@puchik puchik requested a review from a team as a code owner March 19, 2021 06:06
@akien-mga akien-mga added this to the 3.3 milestone Mar 19, 2021
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on. Fixing this should result in much better shadowmapping for lots of users.

I support the change in theory, but I think your logic is off.

Also, the same change needs to be done for GLES2.

drivers/gles3/rasterizer_scene_gles3.cpp Outdated Show resolved Hide resolved
@puchik
Copy link
Contributor Author

puchik commented Mar 21, 2021

Thanks for looking over this. You make good points and I pushed changes that should hopefully make more sense and added the same to GLES2.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

I think we need to think through this in combination with the logic for selecting a cubemap here:

for (int i = shadow_cubemaps.size() - 1; i >= 0; i--) {
//find appropriate cubemap to render to
if (shadow_cubemaps[i].size > shadow_size * 2)
break;
cubemap_index = i;
}

The largest value that shadow_size can take is 1/2 atlas_size. So this chunk of code can be though of as "give me the largest cubemap you have that is equal to or less than the size of the shadow atlas. This is way too large. This means if we are rendering to a 2048x2048 quadrant (the default first quadrant), this logic would allow us to render to a cubemap where each face is 4096x4096. Previously, the largest cubemap was 512x512, so it looked okay. But we are still being wasteful with resources for smaller quadrants.

For now, I suggest we:

  1. Change the bitshift to 3
  2. clamp to max cubemap size (or 1024)
  3. merge this PR
  4. investigate better selection logic so we arent rendering to a larger cubemap than we need to.

drivers/gles2/rasterizer_scene_gles2.cpp Outdated Show resolved Hide resolved
@clayjohn
Copy link
Member

I made a PR to your local branch last night with some additional changes I made while testing out your PR. Please take a look and let me know what you think.

Co-authored-by: Clay John <clayjohn@gmail.com>
@puchik
Copy link
Contributor Author

puchik commented Mar 23, 2021

Oh, somehow I didn't get an email notification for that. Anyway, that's a great addition and I merged it and squashed the commits 👍

@akien-mga akien-mga merged commit 1ed0280 into godotengine:3.x Mar 23, 2021
@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.

None yet

3 participants