Fix: Send CurrentView/ PointAndSpotLightShadowPrimaryCamera ’s world position via Immediates for gpu vis range culling

[kfc35]

Objective

• Also fixes Shadows disappear abruptly in visibility range example #23991
• (Better?) Alternate to Fix: Use the auxiliary entity (associated camera view)’s position via ViewUniform for gpu visibility range culling  #24133 . Decisions made can be mixed and matched between the two, just let me know
• @JMS55 took a look and suggested using Immediates to avoid bloating ViewUniforms. There was also a concern about using RetainedViewEntity to pass information about what camera to use along. This is that alternate implementation path.

Solution

• Rely on the CurrentView mechanism to get the current view’s world position, used when preprocessing directional light shadow views. This will be used for gpu vis range culling dir light shadows.
• For Point/Spot shadow cameras (aka RootNonCameraViews), a marker component has been added to denote the camera to use: PointAndSpotLightShadowPrimaryCamera. There must only be one camera with this component. This camera’s position will be used for gpu vis range culling point/spotlight shadows.
  • This component could probably be expanded later in functionality if desired, but I just went with what’s simplest.
  • If we don’t want to have the PointAndSpotLightShadowPrimaryCamera component, we can revert
    ac87f8e — the previous logic just queried for extracted cameras and took the world position of the first one returned by the query.

Note

Copying this comment from @JMS55:
With this PR, we lose out on PreprocessingOnly support for WebGPU, until immediates are added to the spec and wgpu supports it.

Testing

• cargo run --example visibility_range -- --no-cpu-culling works with a directional light as desired
• I updated the light in the visibility_range example to be a Spot or PointLight like so:

SpotLight {
            intensity: 1_000_000_000.0, // lumens
            range: 110.0,
            color: Color::WHITE,
            shadow_maps_enabled: true,
            radius: 10.0,
            ..default()
        },
        Transform::from_rotation(Quat::from_array([
            0.6539259,
            -0.34646285,
            0.36505926,
            -0.5648683,
        ]))
        .with_translation(vec3(57.693, 34.334, -6.422)),

And added the PointAndSpotLightShadowPrimaryCamera to the Camera3d in the example, and then ran the example. The shadows of spot and point lights also look normal.

[JMS55]

Note: With this PR, we lose out on PreprocessingOnly support for WebGPU, until immediates are added to the spec and wgpu supports it.

[kfc35]

I will add a marker component for the camera to use for gpu vis culling spot and point lights shadows, that should be ready to review in a few hours

[github-actions]

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-24197

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

[beicause]

I'm not sure if PointAndSpotLightShadowPrimaryCamera is correct. The original implementation #12916 says:

Cascaded shadow maps show the HLOD level of the view they're associated with. Point light and spot light shadow maps, which have no CSMs, display all HLOD levels that are visible in any view.

[kfc35]

Thanks for finding that @beicause

My thoughts given that information:

• I agree that the new component I made is probably not correct then with what is originally intended. I’ll revert it after finalizing what to do.
• It sounds like I should be passing in a list of camera world positions to the preprocessor shader code for the point and spot light shadows case then, to check if the shadow is within range of any extracted camera view. If it’s not in range of any camera, it should be culled.
• Does this information change whether we prefer to put this information in Immediates vs a Uniform? Immediates are 64 bytes guaranteed at minimum according to the spec. That’s 4 camera positions max (4 x vec3, one u32 is already taken for occlusion culling). I assume I need to set a max number on the list anyway (Since you can’t have variable length arrays in wgsl? I’m not familiar but that’s the impression I’ve been getting). In this case, for this Immediates PR specifically, I’m thinking that I’ll send max 4 camera view world positions to be tested for gpu visibility range culling for point and spot light shadow views.

I’ll get to implementing this logic in about 18 hours unless there are any objections (and may work on it sooner if I get some timely affirmations)

[beicause]

IMO we should consider reverting #23115. This may introduce extra overhead on GPU and regression on WebGPU before immediates is supported.

And I saw lights are still checking visibility range on CPU: Edit: I misread it

bevy/crates/bevy_light/src/lib.rs

Line 429 in 2002a18

!visible_entity_ranges.entity_is_in_range_of_view(entity, *view)

[JMS55]

It sounds like I should be passing in a list of camera world positions to the preprocessor shader code for the point and spot light shadows case then, to check if the shadow is within range of any extracted camera view. If it’s not in range of any camera, it should be culled.

Seems seems quite expensive 😬 . Also I don't know what that would even look like, I feel like the shadows would be all weird.

[kfc35]

Seems seems quite expensive 😬 . Also I don't know what that would even look like, I feel like the shadows would be all weird.

I agree that it would look weird 😢 I’m not sure how to interpret the original intent for visibility ranges in this context then...

IMO we should consider reverting #23115.

Reverting is definitely an option at this point… NoCpuCulling could use some more time to develop for 0.20

I’ll just keep this PR as is for now. Thanks for the quick input!

[stuartparmenter]

Doesn't this continue skip the entire preprocess dispatch for the view?

[kfc35]

For the PointLight/SpotLight shadow view, yes

But I would consider that a user configuration error at this point, so the shadows just wouldn’t be rendered.

(However we do not need to continue this conversation because I will close this PR! It seems the consensus is converging on reverting the offending PR)