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

Make 2D shadows respect z_index #93881

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanielKauss
Copy link

@DanielKauss DanielKauss commented Jul 3, 2024

This PR adds the ability to sort 2D light occluders using their z_index. Helps with #64939. Now, light occluders only cast shadows on objects that have a lower or equal z_index. I had written another PR that did the same as this one a while ago (#86689), but I didn't really like the approach I took since it was on the GPU side. It also now works on directional lights and on both forward+ and mobile renderers.

Old shadows on the left, new ones on the right:
LightComp

This approach doesn't modify any shaders or low-level graphics code, and instead uses only the CPU. This is achieved by calculating the shadow map inside RendererCanvasRenderRD instead of RendererViewport. The shadow map gets recalculated if the lowest z_index of all active occluders is smaller than the current object's z_index, since the occluder then has to be removed from the shadow map.

Performance-wise, in the best-case scenario where only one z_layer is used for all objects, the performance is the same since the shadow map is only calculated once. In the worst-case scenario, the shadow map gets calculated as many times as there are z_layers occupied by occluders. This shouldn't be a problem because usually you only need a few z_layers for occluders.

I hope that this PR can get merged since I feel like 2D shadows are currently unusable. This is still my first time contributing, so please let me know if I need to change or add anything!

@DanielKauss DanielKauss requested review from a team as code owners July 3, 2024 04:04
@Chaosus Chaosus added this to the 4.x milestone Jul 3, 2024
@AThousandShips AThousandShips changed the title 2D shadows respect z_index Make 2D shadows respect z_index Jul 3, 2024
@AThousandShips AThousandShips added bug regression cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release and removed enhancement labels Jul 3, 2024
@AThousandShips AThousandShips modified the milestones: 4.x, 4.4 Jul 3, 2024
@Mister-Link
Copy link

Mister-Link commented Oct 5, 2024

Not sure what proper etiquette is for commenting on PRs but I tested on my end and it fixes z-indexing perfectly as expected (red shadow doesn't overlap foreground). Too bad we'll have to wait some time yet to get it merged, but thank you sincerely for fixing this massive problem.

image

@clayjohn
Copy link
Member

We took a look at this in today's rendering meeting and agreed that this seems like a useful new feature however:

  1. It doesn't close CanvasItem light_mask is ignored by LightOccluder2D occluder_light_mask #64939 since it doesn't address the root of that bug, which is the fact that the occluder_light_mask isn't working. This PR creates a new workflow which appears to allow users to achieve the same visual affect in a different way. CanvasItem light_mask is ignored by LightOccluder2D occluder_light_mask #64939 should still be fixed in addition to doing this PR.
  2. The implementation appears mostly good except for one thing, by moving the shadow update into the render_canvas_items() function, it is now called for each light, for each occluder, for each canvas layer. Previously it was only for each light, for each occluder. The cost of shadows will now be multiplied by the number of canvas layers unnecessarily and that can be a big problem. Either the code needs to be extracted back to being done per viewport, or a system needs to be added to track when a shadow has been updated and then only update it on the first canvas layer that needs it. Shadows should not be re-rendered for each canvas layer

@DanielKauss
Copy link
Author

Thank you for coming back to this PR!

Regarding #64939, if I remember correctly there was no easy fix for it without rewriting a big part of the shader and rendering code. I recall that the light_mask on the occluder decided if it was rendered to the shadow map for that particular light, but the shadow is still visible on all light_mask layers of the light. There is no way to only apply a shadow to a particular light_mask.
However, I might be misremembering something since it's been a while.

As for the canvas layers, I don't actually think it would be that much of a performance difference since you usually have them on separate layers which would need a shadow map update anyway. Either way it should be a somewhat simple fix, probably by maintaining z_end between canvases.

Anyway, I will fix this and the merge conflicts and hopefully get this PR into a somewhat mergeable state. Might take a few days, since I don't have much time at the moment, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release regression topic:rendering topic:2d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants