Make render phases render world resources instead of components. #13277
Conversation
This commit makes us stop using the render world ECS for `BinnedRenderPhase` and `SortedRenderPhase` and instead use resources with `EntityHashMap`s inside. There are three reasons to do this: 1. We can use `clear()` to clear out the render phase collections instead of recreating the components from scratch, allowing us to reuse allocations. 2. This is a prerequisite for retained bins, because components can't be retained from frame to frame in the render world, but resources can. 3. We want to move away from storing anything in components in the render world ECS, and this is a step in that direction. This patch results in a small performance benefit, due to point (1) above.
pcwalton
added
A-Rendering
There was a problem hiding this comment.
seems relatively straightforward as far as changes go. code flow changes (like !camera.active { continue }) make sense to me. I was able to run the 3d_shapes example using this PR and add a system that accessed and logged out the resources, seeing the relevant camera entity.
consider my review partial since I haven't done many rendering reviews yet.
There was a problem hiding this comment.
Haven't looked at the code yet. I profiled on the following scenes:
- San Miguel
- Bistro
I found no significant performance degradation or improvement on my (extremely GPU bound) laptop (11th gen i7 w/ integrated graphics).
I tested the following examples and found no visual artifacts or other issues compared to current main:
2d_shapeatomospheric_fogcolor_animationpbrmany_foxesgltf_skinned_meshmotion_blur
Approving based on tests. Will take a look at the code when I can.
|
Addressed review comments. |
|
Marking as draft not only because of the review comment but also because this triggered #13402, which it should not have, so I want to figure out why that is. |
This fixes the problem that led to bevyengine#13402. I'm not sure if this had any negative consequences, but it didn't replicate the prior behavior with the `Or` query filter, so I changed it to match.
|
I found the problem. I don't think they were actually rendered, but this was inefficient, so I added the check back. |
pcwalton
added
the
S-Ready-For-Final-Review
Commit 3f5a090 added a reference to `STANDARD_MATERIAL_FLAGS_BASE_COLOR_UV_BIT`, a nonexistent identifier, in the alpha discard portion of the prepass shader. Moreover, the logic didn't make sense to me. I think the code was trying to choose between the two UV sets depending on which is present, so I made it do that. I noticed this when trying Bistro with #13277. I'm not sure why this issue didn't manifest itself before, but it's clearly a bug, so here's a fix. We should probably merge this before 0.14.
| let view_entity = graph.view_entity(); | ||
| let Some(transparent_phase) = transparent_phases.get(&view_entity) else { | ||
| return Ok(()); |
There was a problem hiding this comment.
nit: Since this is now a resource and inserted during extract, is this case even possible? It strikes me that even if it can happen, that it would be surprising/erroneous. Might merit a warning and/or .expect?
(It looks like most of the new resource accesses follow this pattern.)
There was a problem hiding this comment.
From what I can tell, Bevy style is to use bailouts like this even when the problem should never happen to reduce unwinding overhead. This may or may not be a good thing, but I try to follow it. We could use error! there though.
There was a problem hiding this comment.
Actually, this will happen normally for the view entities that are created for cascaded shadow maps. Since shadow maps don't render any transparent meshes, their view entities won't have entries in ViewSortedRenderPhases<Transparent3d>. This is fine and skipping them is the correct thing to do. So this logic is correct as is, I believe.
We invoked the `extract_default_ui_camera_view` system twice: once for 2D cameras and once for 3D cameras. This was fine before moving to resources for render phases, but, after the move to resources, the first thing such systems do is to clear out all the entities-to-be-rendered from the previous frame. So, if the scheduler happened to run `extract_default_ui_camera_view::<Camera2d>` first, then all the UI elements that it queued would be overwritten by the `extract_default_ui_camera_view::<Camera3d>` system, or vice versa. The ordering dependence is the reason why this problem was intermittent. This commit fixes the problem by merging the two systems into one systems, using an `Or` query filter.
We invoked the `extract_default_ui_camera_view` system twice: once for 2D cameras and once for 3D cameras. This was fine before moving to resources for render phases, but, after the move to resources, the first thing such systems do is to clear out all the entities-to-be-rendered from the previous frame. So, if the scheduler happened to run `extract_default_ui_camera_view::<Camera2d>` first, then all the UI elements that it queued would be overwritten by the `extract_default_ui_camera_view::<Camera3d>` system, or vice versa. The ordering dependence is the reason why this problem was intermittent. This commit fixes the problem by merging the two systems into one systems, using an `Or` query filter. ## Migration Guide * The `bevy_ui::render::extract_default_ui_camera_view` system is no longer parameterized over the specific type of camera and is hard-wired to either `Camera2d` or `Camera3d` components.
This commit makes us stop using the render world ECS for
BinnedRenderPhaseandSortedRenderPhaseand instead use resources withEntityHashMaps inside. There are three reasons to do this:We can use
clear()to clear out the render phase collections instead of recreating the components from scratch, allowing us to reuse allocations.This is a prerequisite for retained bins, because components can't be retained from frame to frame in the render world, but resources can.
We want to move away from storing anything in components in the render world ECS, and this is a step in that direction.
This patch results in a small performance benefit, due to point (1) above.
Changelog
Changed
BinnedRenderPhaseandSortedRenderPhaserender world components have been replaced withViewBinnedRenderPhasesandViewSortedRenderPhasesresources.Migration Guide
BinnedRenderPhaseandSortedRenderPhaserender world components have been replaced withViewBinnedRenderPhasesandViewSortedRenderPhasesresources. Instead of querying for the components, look the camera entity up in theViewBinnedRenderPhases/ViewSortedRenderPhasestables.