[Merged by Bors] - Filter material handles on extraction #4178
Conversation
superdump
added
A-Rendering
crates/bevy_pbr/src/material.rs
Outdated
| query: Query<(Entity, &ComputedVisibility, &Handle<M>)>, | ||
| mut prev_len: Local<usize>, | ||
| ) { | ||
| let mut materials = Vec::with_capacity(*prev_len); |
There was a problem hiding this comment.
This seems overly elaborate: I would prefer to rebase off of #4244 and use the size hint.
|
General idea makes a ton of sense, and those perf wins are great. IMO we should prioritize #4244 and build off of it for this to have a faster and less bespoke approach to get the query's size. |
Do we have to make PRs wait on other PRs when we don't know how long other PRs may take to merge? I think it's better to address the things that need changing due to something like #4244 across the codebase in #4244 or a follow-up to it. And then try to make sure that those patterns / APIs are used in PRs that are merged after. |
|
Totally fair. Can you please make a TODO then @superdump? |
| @@ -201,7 +201,6 @@ impl<M: SpecializedMaterial> Default for MaterialPlugin<M> { | |||
| impl<M: SpecializedMaterial> Plugin for MaterialPlugin<M> { | |||
| fn build(&self, app: &mut App) { | |||
| app.add_asset::<M>() | |||
| .add_plugin(ExtractComponentPlugin::<Handle<M>>::default()) | |||
There was a problem hiding this comment.
why didn't you add the visibility filter in the general plugin instead of doing a custom version for just material?
There was a problem hiding this comment.
You can only filter on component presence? ComputedVisibility is a bool. When I was benchmarking using ComputedVisibility as a marker component, I would then use it as a filter. But the adding/removing of a ComputedVisibility marker component had significant cost as I recall
There was a problem hiding this comment.
I mean doing
--- a/crates/bevy_render/src/render_component.rs
+++ b/crates/bevy_render/src/render_component.rs
@@ -164,13 +164,15 @@ impl<T: Asset> ExtractComponent for Handle<T> {
fn extract_components<C: ExtractComponent>(
mut commands: Commands,
mut previous_len: Local<usize>,
- mut query: StaticSystemParam<Query<(Entity, C::Query), C::Filter>>,
+ mut query: StaticSystemParam<Query<(Entity, ComputedVisibility, C::Query), C::Filter>>,
) where
<C::Filter as WorldQuery>::Fetch: FilterFetch,
{
let mut values = Vec::with_capacity(*previous_len);
- for (entity, query_item) in query.iter_mut() {
- values.push((entity, (C::extract_component(query_item),)));
+ for (entity, computed_visiblity, query_item) in query.iter_mut() {
+ if computed_visiblity.is_visible {
+ values.push((entity, (C::extract_component(query_item),)));
+ }
}
*previous_len = values.len();
commands.insert_or_spawn_batch(values);so that all ExtractComponentPlugin would check for visibility
There was a problem hiding this comment.
Then all components being extracted would have to have a ComputedVisibility component. I suppose you could use an Option<> in the query. And if maybe_computed_visibility.is_none() or maybe_computed_visibility.unwrap().is_visible but it also feels a bit unexpected perhaps?
There was a problem hiding this comment.
Do we have the guarantee that all SpecializedMaterial will have the component ComputedVisibility?
It could make sense to use ComputedVisibility as a global filter on everything that will be extracted?
There was a problem hiding this comment.
I agree that hard-coding here feels odd given how general this is. Visibility is pretty generalized, but I imagine it isn't "exactly" as general as the concept of "component extraction". Maybe we just need an ExtractVisibleComponentPlugin variant? Or an ExtractComponentPlugin::<T>::extract_visible() constructor?
There was a problem hiding this comment.
How about a query that includes Option<ComputedVisibility> and extracts if maybe_computed_visibility.map_or(true, |computed_visibility| computed_visibility.is_visible)?
There was a problem hiding this comment.
that feels like it would make all extract a little slower when we should be able to say at build time wether there is the ComputedVisibility component or not
There was a problem hiding this comment.
The alternative that I see is to have two separate queries, one with ComputedVisibility in it, one with a Without filter.
There was a problem hiding this comment.
yup, following Cart suggestion of using ExtractComponentPlugin::<T>::extract_visible() instead of ExtractComponentPlugin::<T>::default() to trigger using the filtered query sounds nice to me
|
this wouldn't really be improved by #4244: the |
Will do when I’m at my computer. EDIT: Or not due to the filter comment above. |
Ah yep, you're right there. Sorry for misleading; and skip the TODO. |
There was a problem hiding this comment.
I think this is a good idea, but I do think this should be generalized and applied where relevant rather than hard coding it.
|
I've retested after the Not sure why extraction of StandardMaterial handles is now about 0.1ms slower than I measured before, but it's still faster and way faster system commands as most of the entities are not visible. |
superdump
force-pushed
the
filter-material-handles
branch
from
3b4d0a5 to
9b3d816
Compare
Done. I looked over the places using ExtractComponentPlugin and it's only used for 2D/3D materials currently, outside of examples. I chose to leave the examples using ::default() to not add noise to them as that argument has been used previously. 2D doesn't use culling... though I just realised it does have ComputedVisibility if only for propagated Visibility and RenderLayers. I'll switch it to extract_visible() there too. Beyond that we maybe 'need to' port extraction of other components to ExtractComponentPlugin, but that could be a separate PR in my opinion. |
|
bors r+ |
# Objective - While optimising many_cubes, I noticed that all material handles are extracted regardless of whether the entity to which the handle belongs is visible or not. As such >100k handles are extracted when only <20k are visible. ## Solution - Only extract material handles of visible entities. - This improves `many_cubes -- sphere` from ~42fps to ~48fps. It reduces not only the extraction time but also system commands time. `Handle<StandardMaterial>` extraction and its system commands went from 0.522ms + 3.710ms respectively, to 0.267ms + 0.227ms an 88% reduction for this system for this case. It's very view dependent but...
bors
bot
changed the title
# Objective - While optimising many_cubes, I noticed that all material handles are extracted regardless of whether the entity to which the handle belongs is visible or not. As such >100k handles are extracted when only <20k are visible. ## Solution - Only extract material handles of visible entities. - This improves `many_cubes -- sphere` from ~42fps to ~48fps. It reduces not only the extraction time but also system commands time. `Handle<StandardMaterial>` extraction and its system commands went from 0.522ms + 3.710ms respectively, to 0.267ms + 0.227ms an 88% reduction for this system for this case. It's very view dependent but...
# Objective - While optimising many_cubes, I noticed that all material handles are extracted regardless of whether the entity to which the handle belongs is visible or not. As such >100k handles are extracted when only <20k are visible. ## Solution - Only extract material handles of visible entities. - This improves `many_cubes -- sphere` from ~42fps to ~48fps. It reduces not only the extraction time but also system commands time. `Handle<StandardMaterial>` extraction and its system commands went from 0.522ms + 3.710ms respectively, to 0.267ms + 0.227ms an 88% reduction for this system for this case. It's very view dependent but...
Objective
Solution
many_cubes -- spherefrom ~42fps to ~48fps. It reduces not only the extraction time but also system commands time.Handle<StandardMaterial>extraction and its system commands went from 0.522ms + 3.710ms respectively, to 0.267ms + 0.227ms an 88% reduction for this system for this case. It's very view dependent but...