Fix render layer change propagation for GPU-culled meshes - #1
Conversation
beicause
left a comment
beicause
left a comment
There was a problem hiding this comment.
AI-generated follow-up to bevyengine#25690 — authored entirely by AI, no human review; not tested at runtime (static checks only). Rationale per hunk in the inline comments; overall description in the PR description.
| /// | ||
| /// You must call this after adding entities to the list via | ||
| /// [`Self::add_entity`]. | ||
| /// You must call this after pushing entities onto the list, as the |
There was a problem hiding this comment.
Removed add_entity() as well: its only callers on main were exactly the lines rewritten in collect_gpu_culled_meshes_for_subview, so it would be dead code after this PR. (During the bevyengine#25670 review its removal was suggested to be deferred to a follow-up PR — happy to drop this hunk if maintainers prefer that. AI-authored; untested.)
There was a problem hiding this comment.
This is good, i was worried people are using this public api, dont know if this requires a migration guide?
Pure render-layer changes on a mesh never reached `RenderGpuCulledEntities::update()` because `extract_meshes_for_gpu_building` only iterates entities it considers changed, and `RenderLayers` wasn't part of its change filter. A static mesh whose only change is its render layers would therefore keep a stale layer relevance in all views forever, and the `changed_layers` list would only be populated when a mesh happened to be re-extracted for some other reason. Add `Changed<RenderLayers>` and `RemovedComponents<RenderLayers>` to the change detection.
d46880d to
3f15240
Compare
Its only callers were already converted to direct `added_entities` pushes, leaving it as dead code; update `sort_added_entities`' doc accordingly. (Its removal was deferred to a follow-up PR during the bevyengine#25670 review.)
3f15240 to
672fa54
Compare
beicause
changed the title
|
"New shadow-map subviews" I would like to fix this in another PR to not increase the complexity here. The reason this still works right now is due to how view can trigger change detection for each mesh via cpu culling |
|
Ive ran a live test against this, and changing layers is working, but found another issue and will commit that fix as well |
2 participants
Objective
Follow-up to #25690, based on an AI code review against
main.Changes
Propagate render-layer changes of GPU-culled meshes (bug fix).
RenderGpuCulledEntities::update()now records layer changes, but it only runs for meshes that extraction picked up that frame, and the extraction filter had noChanged<RenderLayers>/RemovedComponents<RenderLayers>. A static mesh whose only change is its render layers would never propagate. Adds both toextract_meshes_for_gpu_building.API cleanup. Removed
RenderVisibleEntitiesClass::add_entity()— its only two callers onmainare exactly the lines rewritten here, so it would be dead code;sort_added_entities' doc updated. (Its removal was deferred to a follow-up in the #25670 review — happy to drop this hunk.) Plus!=over.ne()and a doc note onupdate().Remaining known limitations
RenderShadowMapVisibleEntities::is_added()), but shadow subviews live in a per-RetainedViewEntitymap. A fresh subview under an existing light component (new camera → new directional-light cascades, or a cascade count change) starts empty, misses all previously registered GPU-culled meshes, and never catches up. Pre-existing onmain.ExtractedViewis removed and the view's GPU-culling table stops receiving the per-frame diffs; meshes registered/deregistered meanwhile are missing/stale after restore. Pre-existing onmain; this is also whyRenderVisibleEntitiesdeliberately persists in the zero-size path per #25670.Testing
cargo check -p bevy_pbr,cargo clippy -p bevy_render -p bevy_pbr,cargo fmt --check,bevy_rendervisibility unit tests.Camera::is_activeand delayed camera spawn withNoCpuCullingmeshes (repro from #25690); changeRenderLayerson a staticNoCpuCullingmesh with two cameras on disjoint layers.