Skip to content

Revert "Move visibility range checking from the CPU to the GPU if NoCpuCulling is specified. (#23115)#24252

Merged
alice-i-cecile merged 3 commits into
bevyengine:mainfrom
kfc35:revert_gpu_vis_range_culling
May 13, 2026
Merged

Revert "Move visibility range checking from the CPU to the GPU if NoCpuCulling is specified. (#23115)#24252
alice-i-cecile merged 3 commits into
bevyengine:mainfrom
kfc35:revert_gpu_vis_range_culling

Conversation

@kfc35
Copy link
Copy Markdown
Contributor

@kfc35 kfc35 commented May 12, 2026 •
edited
Loading

This reverts commit ebfbc3f.

Objective

Solution

Testing

  • cargo run --example visibility_range works normal now.
  • Tested some other 3d examples make sure the pipeline is ok — atmosphere, pccm

@kfc35 kfc35 added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 12, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in Rendering May 12, 2026
@Zeophlite Zeophlite added this to the 0.19 milestone May 12, 2026
@Zeophlite
Copy link
Copy Markdown
Contributor

Zeophlite commented May 12, 2026

I think this is the pragmatic choice

JMS55
JMS55 approved these changes May 12, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Yeah I think this is the right choice for 0.19.

Can we have a partial revert though? I think it makes sense to keep the code, but only enable it for non-shadow views. We can figure out how to pass directional light camera entities later, and what to do about point/spotlight shadows.

@kfc35
Copy link
Copy Markdown
Contributor Author

kfc35 commented May 13, 2026 •
edited
Loading

We chatted on Discord and will proceed with a full revert

@kfc35 kfc35 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 13, 2026
@alice-i-cecile
Copy link
Copy Markdown
Member

alice-i-cecile commented May 13, 2026

@pcwalton I'm going to merge this now in the interest of shipping.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 13, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 13, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 13, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 13, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 13, 2026
Merged via the queue into bevyengine:main with commit dc3a6cb May 13, 2026
47 checks passed
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in Rendering May 13, 2026
pcwalton added a commit to pcwalton/bevy that referenced this pull request May 14, 2026
@pcwalton
Correctly handle visibility ranges in shadow maps.
74955d2 
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 *primary
camera*. The primary camera is the one 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, `PrimaryCamera`, has been added, which allows the
developer to customize which camera is primary. In the absence of this
component, this PR implements a simple heuristic to determine the
primary camera: to prefer 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 `PrimaryCamera` component.

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

This commit is the proper fix for bevyengine#23991. PR bevyengine#24252 attempted to fix
this problem by reverting bevyengine#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 bevyengine#23115 on top of this patch and modifying it to use the new
`lod_view_world_position`, that the issue reported in bevyengine#23991 disappears.
@pcwalton pcwalton mentioned this pull request May 14, 2026
Merged
mgi388 pushed a commit to mgi388/bevy that referenced this pull request May 18, 2026
@pcwalton
Correctly handle visibility ranges in shadow maps. (bevyengine#24289)
74d39c6 
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 bevyengine#24197
tried to do.

This commit is the proper fix for bevyengine#23991. PR bevyengine#24252 attempted to fix
this problem by reverting bevyengine#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 bevyengine#23115 on top of this patch and modifying it to use the new
`lod_view_world_position`, that the issue reported in bevyengine#23991 disappears.
@pcwalton pcwalton mentioned this pull request May 18, 2026
Merged
pull Bot pushed a commit to VitalyAnkh/bevy that referenced this pull request May 19, 2026
@pcwalton
Reapply GPU-driven HLOD evaluation, using the new HLOD view origin. (b…
a0baf1a 
…evyengine#24343)

This PR undoes the revert of bevyengine#23115 that was done in bevyengine#24252. As part of
doing so, it makes GPU-driven visibility range evaluation use the same
semantics, introduced in bevyengine#24289, as CPU-driven visibility range
evaluation.

The first commit in this PR is the un-revert, and the second commit is
the change to use the new machinery bevyengine#24289. This means that you can
simply review the second commit (which is very short).

To test, run the `visibility_range` example with `--no-cpu-culling`, and
use the WASD keys to move behind the flight helmet and zoom out, while
closely watching the shadow. Note that the shadow now properly reflects
the LOD of the model.
mockersf pushed a commit to mockersf/bevy that referenced this pull request May 21, 2026
@pcwalton @mockersf
Correctly handle visibility ranges in shadow maps. (bevyengine#24289)
b40dead 
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 bevyengine#24197
tried to do.

This commit is the proper fix for bevyengine#23991. PR bevyengine#24252 attempted to fix
this problem by reverting bevyengine#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 bevyengine#23115 on top of this patch and modifying it to use the new
`lod_view_world_position`, that the issue reported in bevyengine#23991 disappears.
mockersf pushed a commit to mockersf/bevy that referenced this pull request May 21, 2026
@pcwalton @mockersf
Reapply GPU-driven HLOD evaluation, using the new HLOD view origin. (b…
43995c2 
…evyengine#24343)

This PR undoes the revert of bevyengine#23115 that was done in bevyengine#24252. As part of
doing so, it makes GPU-driven visibility range evaluation use the same
semantics, introduced in bevyengine#24289, as CPU-driven visibility range
evaluation.

The first commit in this PR is the un-revert, and the second commit is
the change to use the new machinery bevyengine#24289. This means that you can
simply review the second commit (which is very short).

To test, run the `visibility_range` example with `--no-cpu-culling`, and
use the WASD keys to move behind the flight helmet and zoom out, while
closely watching the shadow. Note that the shadow now properly reflects
the LOD of the model.
mockersf pushed a commit that referenced this pull request May 21, 2026
@pcwalton @mockersf
Correctly handle visibility ranges in shadow maps. (#24289)
5e7db95 
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.
mockersf pushed a commit that referenced this pull request May 21, 2026
@pcwalton @mockersf
Reapply GPU-driven HLOD evaluation, using the new HLOD view origin. (#…
a3a359a 
…24343)

This PR undoes the revert of #23115 that was done in #24252. As part of
doing so, it makes GPU-driven visibility range evaluation use the same
semantics, introduced in #24289, as CPU-driven visibility range
evaluation.

The first commit in this PR is the un-revert, and the second commit is
the change to use the new machinery #24289. This means that you can
simply review the second commit (which is very short).

To test, run the `visibility_range` example with `--no-cpu-culling`, and
use the WASD keys to move behind the flight helmet and zoom out, while
closely watching the shadow. Note that the shadow now properly reflects
the LOD of the model.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Zeophlite Zeophlite Zeophlite approved these changes

@JMS55 JMS55 JMS55 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

Rendering
Status: Done

Milestone

0.19

Development

Successfully merging this pull request may close these issues.

Shadows disappear abruptly in visibility range example

4 participants

@kfc35 @Zeophlite @alice-i-cecile @JMS55