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.
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.