Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Filter material handles on extraction #4178

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Filter material handles on extraction #4178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd34813
Only extract SpecializedMaterial2d handles if they are visible
superdump May 2, 2022
a75fa80
Filter material handles on extraction
superdump Mar 8, 2022
1794083
bevy_pbr: Use clone_weak for material handles
superdump Mar 13, 2022
9b3d816
ExtractComponentPlugin: Support extracting the component only for vis…
superdump May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bevy_pbr/src/material.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ 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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why didn't you add the visibility filter in the general plugin instead of doing a custom version for just material?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@mockersf mockersf Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a query that includes Option<ComputedVisibility> and extracts if maybe_computed_visibility.map_or(true, |computed_visibility| computed_visibility.is_visible)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative that I see is to have two separate queries, one with ComputedVisibility in it, one with a Without filter.

Copy link
Member

@mockersf mockersf May 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.add_plugin(ExtractComponentPlugin::<Handle<M>>::extract_visible())
.add_plugin(RenderAssetPlugin::<M>::default());
if let Ok(render_app) = app.get_sub_app_mut(RenderApp) {
render_app
Expand Down
43 changes: 40 additions & 3 deletions crates/bevy_render/src/render_component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{
render_resource::{std140::AsStd140, DynamicUniformVec},
renderer::{RenderDevice, RenderQueue},
view::ComputedVisibility,
RenderApp, RenderStage,
};
use bevy_app::{App, Plugin};
Expand Down Expand Up @@ -131,18 +132,38 @@ fn prepare_uniform_components<C: Component>(
///
/// Therefore it sets up the [`RenderStage::Extract`](crate::RenderStage::Extract) step
/// for the specified [`ExtractComponent`].
pub struct ExtractComponentPlugin<C, F = ()>(PhantomData<fn() -> (C, F)>);
pub struct ExtractComponentPlugin<C, F = ()> {
only_extract_visible: bool,
marker: PhantomData<fn() -> (C, F)>,
}

impl<C, F> Default for ExtractComponentPlugin<C, F> {
fn default() -> Self {
Self(PhantomData)
Self {
only_extract_visible: false,
marker: PhantomData,
}
}
}

impl<C, F> ExtractComponentPlugin<C, F> {
pub fn extract_visible() -> Self {
Self {
only_extract_visible: true,
marker: PhantomData,
}
}
}

impl<C: ExtractComponent> Plugin for ExtractComponentPlugin<C> {
fn build(&self, app: &mut App) {
if let Ok(render_app) = app.get_sub_app_mut(RenderApp) {
render_app.add_system_to_stage(RenderStage::Extract, extract_components::<C>);
if self.only_extract_visible {
render_app
.add_system_to_stage(RenderStage::Extract, extract_visible_components::<C>);
} else {
render_app.add_system_to_stage(RenderStage::Extract, extract_components::<C>);
}
}
}
}
Expand Down Expand Up @@ -170,3 +191,19 @@ fn extract_components<C: ExtractComponent>(
*previous_len = values.len();
commands.insert_or_spawn_batch(values);
}

/// This system extracts all visible components of the corresponding [`ExtractComponent`] type.
fn extract_visible_components<C: ExtractComponent>(
mut commands: Commands,
mut previous_len: Local<usize>,
mut query: StaticSystemParam<Query<(Entity, Read<ComputedVisibility>, C::Query), C::Filter>>,
) {
let mut values = Vec::with_capacity(*previous_len);
for (entity, computed_visibility, query_item) in query.iter_mut() {
if computed_visibility.is_visible {
values.push((entity, (C::extract_component(query_item),)));
}
}
*previous_len = values.len();
commands.insert_or_spawn_batch(values);
}
2 changes: 1 addition & 1 deletion crates/bevy_sprite/src/mesh2d/material.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ impl<M: SpecializedMaterial2d> Default for Material2dPlugin<M> {
impl<M: SpecializedMaterial2d> Plugin for Material2dPlugin<M> {
fn build(&self, app: &mut App) {
app.add_asset::<M>()
.add_plugin(ExtractComponentPlugin::<Handle<M>>::default())
.add_plugin(ExtractComponentPlugin::<Handle<M>>::extract_visible())
.add_plugin(RenderAssetPlugin::<M>::default());
if let Ok(render_app) = app.get_sub_app_mut(RenderApp) {
render_app
Expand Down