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

Visual instance layers are regarded during shadow culling #61841

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

markusneg
Copy link
Contributor

This PR deals with #43827 and #54613 by simply passing the visible layers to update_shadow of the VisualServer. This seems to work for both directional and non-directional lights. After changing the layers of a GeometryInstance, affected shadows are updated.

What is not yet done are updated shadows after changing the Camera cull_mask. I am not sure how to get the affected lights.

todo: setting Camera cull_mask should mark affected shadows dirty somehow
@markusneg markusneg requested a review from a team as a code owner June 9, 2022 07:16
@akien-mga akien-mga added this to the 3.5 milestone Jun 9, 2022
@lawnjelly
Copy link
Member

lawnjelly commented Jun 9, 2022

This looks on the surface good. There is something I need to refresh my memory on though, it came up for me when looking at some shadow optimizations in 2019. The problem is that for non-directional lights (spot and omnis) Godot doesn't render the shadow map continuously, it only renders when it is dirty, i.e. when the light moves, or one of the instances move.

I was kind of surprised by this, as I didn't expect this situation to happen that often in real games, as opposed to tech demos. But this prevented me using a tighter cull on the shadow casters to the view frustum intersection, because this optimization assumed that they were re-rendered each frame.

So the potential problem I can see is that this may not play well with camera cull masks. If you have two cameras with different cull masks, you essentially have to keep rendering the shadow map for each camera for each frame, and this dirty must be disabled? Unless you have a shadow map per camera (which I don't think is the case currently).

UPDATE: Turns out it looks like there is a shadow atlas per viewport, turns out my assumption was wrong, see below. 🙂

So although this works for a single camera, have you tried it with multiple cameras with multiple cull masks?

@markusneg
Copy link
Contributor Author

@lawnjelly, thanks for the background info. As far as I understood, shadows for non-directionals are rendered into the viewports shadow atlases which are usually unique to different cameras. I created a demo project which shows a scene from two cameras. The position and visual layer of the shadow caster are animated. It seems to show correct behavior.

@lawnjelly
Copy link
Member

lawnjelly commented Jun 9, 2022

@lawnjelly, thanks for the background info. As far as I understood, shadows for non-directionals are rendered into the viewports shadow atlases which are usually unique to different cameras. I created a demo project which shows a scene from two cameras. The position and visual layer of the shadow caster are animated. It seems to show correct behavior.

Ah that's good, that may not be a problem then if that is the case. 😁 (sorry I'm not super familiar with this area, but would rather try to point out anything that could be affected and be wildly wrong, as there aren't many that work in this area at the moment.)

As an aside, that is slightly scary to find that it might be rendering shadow maps per viewport. This could have a lot of performance implications, we should check this.

@mrjustaguy
Copy link
Contributor

Does this PR have a 4.0 counter part? Asking as it is relevant there too.

@markusneg
Copy link
Contributor Author

Does this PR have a 4.0 counter part? Asking as it is relevant there too.

No, this PR doesn't.

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.

Looks good to me. I tested carefully with the test project in #43827 including changing the light cull layers on the light source and everything appeared to work fine.

If you are interested, could you take a stab at making the same changes in 4.0?

Finally, this is a riskier than usual PR, so let's merge for 3.6 but not cherrypick until after it has had more testing.

@markusneg
Copy link
Contributor Author

If you are interested, could you take a stab at making the same changes in 4.0?

Yes, I will look into it soon.

@akien-mga akien-mga modified the milestones: 3.x, 3.6 Sep 23, 2022
@akien-mga akien-mga merged commit 24ebc32 into godotengine:3.x Sep 23, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

5 participants