Correctly handle visibility ranges in shadow maps.

[pcwalton]

Right now, visibility ranges are always resolved relative to the view. This is incorrect for shadow maps in two ways:

1. Visibility ranges for meshes in directional light shadow maps should be resolved relative to the camera that the cascades are associated with.

2. Visibility ranges for meshes in point and spot light shadow maps should be resolved relative to some camera.

To properly solve (2), this commit introduces the notion of a shadow LOD origin. The shadow LOD origin is the point that visibility ranges are relative to, when rendering views not associated with any camera. Point and spot light shadow maps are currently not associated with a camera, and therefore we need this extra notion in order to properly evaluate visibility ranges.

(As a follow-up, we should introduce the notion of own shadow maps, which will allow each camera to have separate shadow maps for point and spot lights. That feature is however out of scope for this patch, which simply seeks to make the existing semantics consistent.)

A new component, ShadowLodOrigin, has been added, which allows the developer to customize the shadow LOD origin. In the absence of this component, this PR implements a simple heuristic to determine the shadow LOD origin: to prefer an origin that coincides with cameras that render to a window. This heuristic should suffice in the vast majority of cases, so developers will rarely have to manually use the ShadowLodOrigin component.

A new field, lod_view_world_position, has been added to View to supply the position of the shadow LOD origin to the GPU. This is much simpler than introducing a new uniform or using immediates, as #24197 tried to do.

This commit is the proper fix for #23991. PR #24252 attempted to fix this problem by reverting #23115. However, this didn't actually fix the issue, because the semantics were still inconsistent. This commit constitutes the correct fix for the issue. I verified that, after un-reverting #23115 on top of this patch and modifying it to use the new lod_view_world_position, that the issue reported in #23991 disappears.

[pcwalton]

You may be wondering why reverting #23115 seemed to work. The answer is that, when CPU culling is in use, all meshes are submitted to the GPU if they're visible in any camera, as determined by the visibility ranges. This means that the visibility ranges mostly work for the shadows in the visibility_ranges example, but they will break if multiple cameras are present, or if point or spot light shadow maps are used. This PR constitutes the correct fix.

After this PR lands, I intend to submit a follow-up PR to unrevert #23115, updated to use the new lod_view_world_position field.

[beicause]

Should CPU culling for point light shadow maps be changed to be relative to PrimaryCamera, too?

[pcwalton]

It should effectively be already, because the "CPU culling" is really just culling shadow maps that aren't in view of any camera (see VisibilityRange::is_visible_at_all), and that is compatible with the semantics here.

[beicause]

It should effectively be already, because the "CPU culling" is really just culling shadow maps that aren't in view of any camera (see VisibilityRange::is_visible_at_all), and that is compatible with the semantics here.

Maybe I'm wrong, but shouldn't we cull meshes that aren't in view of PrimaryCamera here instead of any camera?

bevy/crates/bevy_light/src/lib.rs

Line 592 in a399db0

!visible_entity_ranges.entity_is_in_range_of_any_view(entity)

[alice-i-cecile]

I'm nervous about the introduction of this. My understanding is that per-camera shadow maps is likely to always be too expensive.

This design feels fundamentally wrong for split-screen games, even though it works for many others.

Could we change the semantics here to allow for tagging multiple cameras, and then taking the union of shadows that should be seen by any of these cameras? We'd need to rename this component obviously. We can even bound the max number of cameras used for this comfortably: 4 or 8 is plenty.

[pcwalton]

That won't work for GPU driven because you would have a sequential loop over an array for every mesh in the shader. The right solution is has_own_point_and_spot_light_shadow_maps, which is something I'll implement after 0.19.

[pcwalton]

In other words, per-camera shadow maps aren't necessarily too expensive. It really depends on the scene. There's a general feeling that they're too expensive by default, but we should support them at least on an opt-in basis. cart and Griffin disagree as to whether they should be enabled by default; I'm neutral on that question and simply think we should support both.

[alice-i-cecile]

I really like this fallback strategy: good defaults with manual control.

[kfc35]

It should effectively be already, because the "CPU culling" is really just culling shadow maps that aren't in view of any camera (see VisibilityRange::is_visible_at_all), and that is compatible with the semantics here.

Maybe I'm wrong, but shouldn't we cull meshes that aren't in view of PrimaryCamera here instead of any camera?

bevy/crates/bevy_light/src/lib.rs

Line 592 in a399db0

!visible_entity_ranges.entity_is_in_range_of_any_view(entity)

IIUC this isn't absolutely necessary because that filter isnt excessively filtering out meshes, and the logic inside this PR will cover any meshes / shadows that absolutely should be culled. It would be redundant to put the logic there too (but i could understand wanting the logic to be consistent for consistency's sake). This is just my understanding of the code though, may be flawed!

[pcwalton]

That check is redundant (or would be if we had proper CPU culling for shadow maps--but I'll do that in a followup), but I would still like to keep it because it avoids overhead of render world extraction for meshes that aren't going to be visible anyway. Extraction from the main world to the render world can be expensive and it's nice to be able to avoid it when we can.

[pcwalton]

Maybe I'm wrong, but shouldn't we cull meshes that aren't in view of PrimaryCamera here instead of any camera?

bevy/crates/bevy_light/src/lib.rs

Line 592 in a399db0

!visible_entity_ranges.entity_is_in_range_of_any_view(entity)

Yes, I changed this.

[pcwalton]

1. I renamed PrimaryCamera to ShadowLodOrigin per discussion in #rendering-dev on Discord.

2. ShadowLodOrigin no longer needs to be a camera.

3. We now sort the query by entity ID and choose the lowest one in the case of a tie. This ensures that the chosen camera remains consistent.

4. A warning is now emitted if point/spot lights are present and neither the ShadowLodOrigin nor any camera that renders to the window are present.

[urben1680]

We now sort the query by entity ID and choose the lowest one in the case of a tie. This ensures that the chosen camera remains consistent.

I may miss some context here but consistent in which way? PartialOrd of Entity seems to work on the entity index value as the higher bits and the generation as the lower bits. If I spawn a new camera that may have a lower or higher entity index than existing cameras.

[kfc35]

Looks great! I tested out the example with a spotlight and it works as expected

[pcwalton]

@urben1680 I mean consistent if you never spawn or despawn cameras, even if you do all sorts of other things to change archetypes and so forth.

[laundmo]

This causes a panic on startup when using bevy_render but not bevy_pbr, for example when using the 2d feature, due to the new RenderShadowLodOrigin only being registered in PbrPlugin but used by ViewPlugin

• RenderShadowLodOrigin causes crash when not using PBR feature #24352