-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make render phases render world resources instead of components. #13277
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
i guess iterating all views and filtering for those with matching phases could be slower than using query filters (as we were before), but views are very low count so i don't think it'll ever be significant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_shape
atomospheric_fog
color_animation
pbr
many_foxes
gltf_skinned_mesh
motion_blur
Approving based on tests. Will take a look at the code when I can.
Addressed review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: all those EntityHashSet
s could be Local
s to avoid allocs each frame
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. |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems pretty straight-forward. Some of the check-and-bail-outs added irk me a little because they might wind up as silent failures, but it's not a regression compared to the query filter before.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Migration guide is invalid; |
Ah found it with git blame : #12453 |
…sparent3d> Addresses changes from Bevy [12453](bevyengine/bevy#12453), Bevy [13277](bevyengine/bevy#13277). Inspired by similar fixes by bevy_ecs_tilemap (StarArawn/bevy_ecs_tilemap@d234b18).
This commit makes us stop using the render world ECS for
BinnedRenderPhase
andSortedRenderPhase
and instead use resources withEntityHashMap
s 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
BinnedRenderPhase
andSortedRenderPhase
render world components have been replaced withViewBinnedRenderPhases
andViewSortedRenderPhases
resources.Migration Guide
BinnedRenderPhase
andSortedRenderPhase
render world components have been replaced withViewBinnedRenderPhases
andViewSortedRenderPhases
resources. Instead of querying for the components, look the camera entity up in theViewBinnedRenderPhases
/ViewSortedRenderPhases
tables.