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.

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

[Merged by Bors] - ExtractComponent output optional associated type #6699

Closed
124 changes: 52 additions & 72 deletions crates/bevy_core_pipeline/src/bloom/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,26 @@ use bevy_app::{App, Plugin};
use bevy_asset::{load_internal_asset, HandleUntyped};
use bevy_ecs::{
prelude::{Component, Entity},
query::{QueryState, With},
query::{QueryItem, QueryState, With},
system::{Commands, Query, Res, ResMut, Resource},
world::{FromWorld, World},
};
use bevy_math::UVec2;
use bevy_reflect::{Reflect, TypeUuid};
use bevy_render::{
camera::ExtractedCamera,
extract_component::{
ComponentUniforms, DynamicUniformIndex, ExtractComponent, ExtractComponentPlugin,
UniformComponentPlugin,
},
prelude::Camera,
render_graph::{Node, NodeRunError, RenderGraph, RenderGraphContext, SlotInfo, SlotType},
render_phase::TrackedRenderPass,
render_resource::*,
renderer::{RenderContext, RenderDevice, RenderQueue},
renderer::{RenderContext, RenderDevice},
texture::{CachedTexture, TextureCache},
view::ViewTarget,
Extract, RenderApp, RenderStage,
RenderApp, RenderStage,
};
#[cfg(feature = "trace")]
use bevy_utils::tracing::info_span;
Expand All @@ -35,6 +39,8 @@ impl Plugin for BloomPlugin {
load_internal_asset!(app, BLOOM_SHADER_HANDLE, "bloom.wgsl", Shader::from_wgsl);

app.register_type::<BloomSettings>();
app.add_plugin(ExtractComponentPlugin::<BloomSettings>::default());
app.add_plugin(UniformComponentPlugin::<BloomUniform>::default());

let render_app = match app.get_sub_app_mut(RenderApp) {
Ok(render_app) => render_app,
Expand All @@ -43,10 +49,7 @@ impl Plugin for BloomPlugin {

render_app
.init_resource::<BloomPipelines>()
.init_resource::<BloomUniforms>()
.add_system_to_stage(RenderStage::Extract, extract_bloom_settings)
.add_system_to_stage(RenderStage::Prepare, prepare_bloom_textures)
.add_system_to_stage(RenderStage::Prepare, prepare_bloom_uniforms)
.add_system_to_stage(RenderStage::Queue, queue_bloom_bind_groups);

{
Expand Down Expand Up @@ -151,13 +154,39 @@ impl Default for BloomSettings {
}
}

impl ExtractComponent for BloomSettings {
type Query = (&'static Self, &'static Camera);

type Filter = ();
type Out = BloomUniform;

fn extract_component((settings, camera): QueryItem<'_, Self::Query>) -> Option<Self::Out> {
if !(camera.is_active && camera.hdr) {
return None;
}

camera.physical_viewport_size().map(|size| {
let min_view = size.x.min(size.y) / 2;
let mip_count = calculate_mip_count(min_view);
let scale = (min_view / 2u32.pow(mip_count)) as f32 / 8.0;

BloomUniform {
threshold: settings.threshold,
knee: settings.knee,
scale: settings.scale * scale,
intensity: settings.intensity,
}
})
}
}

pub struct BloomNode {
view_query: QueryState<(
&'static ExtractedCamera,
&'static ViewTarget,
&'static BloomTextures,
&'static BloomBindGroups,
&'static BloomUniformIndex,
&'static DynamicUniformIndex<BloomUniform>,
)>,
}

Expand Down Expand Up @@ -227,7 +256,11 @@ impl Node for BloomNode {
},
));
prefilter_pass.set_render_pipeline(downsampling_prefilter_pipeline);
prefilter_pass.set_bind_group(0, &bind_groups.prefilter_bind_group, &[uniform_index.0]);
prefilter_pass.set_bind_group(
0,
&bind_groups.prefilter_bind_group,
&[uniform_index.index()],
);
if let Some(viewport) = camera.viewport.as_ref() {
prefilter_pass.set_camera_viewport(viewport);
}
Expand All @@ -252,7 +285,7 @@ impl Node for BloomNode {
downsampling_pass.set_bind_group(
0,
&bind_groups.downsampling_bind_groups[mip as usize - 1],
&[uniform_index.0],
&[uniform_index.index()],
);
if let Some(viewport) = camera.viewport.as_ref() {
downsampling_pass.set_camera_viewport(viewport);
Expand All @@ -278,7 +311,7 @@ impl Node for BloomNode {
upsampling_pass.set_bind_group(
0,
&bind_groups.upsampling_bind_groups[mip as usize - 1],
&[uniform_index.0],
&[uniform_index.index()],
);
if let Some(viewport) = camera.viewport.as_ref() {
upsampling_pass.set_camera_viewport(viewport);
Expand All @@ -304,7 +337,7 @@ impl Node for BloomNode {
upsampling_final_pass.set_bind_group(
0,
&bind_groups.upsampling_final_bind_group,
&[uniform_index.0],
&[uniform_index.index()],
);
if let Some(viewport) = camera.viewport.as_ref() {
upsampling_final_pass.set_camera_viewport(viewport);
Expand Down Expand Up @@ -522,17 +555,6 @@ impl FromWorld for BloomPipelines {
}
}

fn extract_bloom_settings(
mut commands: Commands,
cameras: Extract<Query<(Entity, &Camera, &BloomSettings), With<Camera>>>,
) {
for (entity, camera, bloom_settings) in &cameras {
if camera.is_active && camera.hdr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you removed this, where are you checking for these conditions? Did I miss this somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, that was my bad.
Added that check in the extract component impl for BloomSettings

commands.get_or_spawn(entity).insert(bloom_settings.clone());
}
}
}

#[derive(Component)]
struct BloomTextures {
texture_a: CachedTexture,
Expand All @@ -554,7 +576,7 @@ fn prepare_bloom_textures(
mut commands: Commands,
mut texture_cache: ResMut<TextureCache>,
render_device: Res<RenderDevice>,
views: Query<(Entity, &ExtractedCamera), With<BloomSettings>>,
views: Query<(Entity, &ExtractedCamera), With<BloomUniform>>,
) {
let mut texture_as = HashMap::default();
let mut texture_bs = HashMap::default();
Expand Down Expand Up @@ -602,59 +624,17 @@ fn prepare_bloom_textures(
}
}

#[derive(ShaderType)]
struct BloomUniform {
/// The uniform struct extracted from [`BloomSettings`] attached to a [`Camera`].
/// Will be available for use in the Bloom shader.
#[doc(hidden)]
#[derive(Component, ShaderType, Clone)]
pub struct BloomUniform {
Copy link
Member

Choose a reason for hiding this comment

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

Doc strings :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a docstring.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep this struct private? I don't like exposing bloom implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like that too, but by doing impl ExtractComponent on something public like BloomSettings and having the Out type be BloomUniform means that BloomUniform too must be public (else compile error)

Copy link
Member

@alice-i-cecile alice-i-cecile Nov 20, 2022

Choose a reason for hiding this comment

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

You can use #[doc(hidden)] to reduce public visibility.

Copy link
Contributor

@JMS55 JMS55 Nov 20, 2022

Choose a reason for hiding this comment

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

I don't love this, but I think the reduction in boilerplate is worth it, given that we can somewhat mitigate it with doc hidden (does anyone know if rust analyzer ignores hidden items when it comes to autocomplete?). I'm good with these changes once doc hidden is added :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the hidden attr now.
I kept the docstring anyway since I saw that was done in other instances of hiding pub items in Bevy.

threshold: f32,
knee: f32,
scale: f32,
intensity: f32,
}

#[derive(Resource, Default)]
struct BloomUniforms {
uniforms: DynamicUniformBuffer<BloomUniform>,
}

#[derive(Component)]
struct BloomUniformIndex(u32);

fn prepare_bloom_uniforms(
mut commands: Commands,
render_device: Res<RenderDevice>,
render_queue: Res<RenderQueue>,
mut bloom_uniforms: ResMut<BloomUniforms>,
bloom_query: Query<(Entity, &ExtractedCamera, &BloomSettings)>,
) {
bloom_uniforms.uniforms.clear();

let entities = bloom_query
.iter()
.filter_map(|(entity, camera, settings)| {
let size = match camera.physical_viewport_size {
Some(size) => size,
None => return None,
};
let min_view = size.x.min(size.y) / 2;
let mip_count = calculate_mip_count(min_view);
let scale = (min_view / 2u32.pow(mip_count)) as f32 / 8.0;

let uniform = BloomUniform {
threshold: settings.threshold,
knee: settings.knee,
scale: settings.scale * scale,
intensity: settings.intensity,
};
let index = bloom_uniforms.uniforms.push(uniform);
Some((entity, (BloomUniformIndex(index))))
})
.collect::<Vec<_>>();
commands.insert_or_spawn_batch(entities);

bloom_uniforms
.uniforms
.write_buffer(&render_device, &render_queue);
}

#[derive(Component)]
struct BloomBindGroups {
prefilter_bind_group: BindGroup,
Expand All @@ -667,10 +647,10 @@ fn queue_bloom_bind_groups(
mut commands: Commands,
render_device: Res<RenderDevice>,
pipelines: Res<BloomPipelines>,
uniforms: Res<BloomUniforms>,
uniforms: Res<ComponentUniforms<BloomUniform>>,
views: Query<(Entity, &ViewTarget, &BloomTextures)>,
) {
if let Some(uniforms) = uniforms.uniforms.binding() {
if let Some(uniforms) = uniforms.binding() {
for (entity, view_target, textures) in &views {
let prefilter_bind_group = render_device.create_bind_group(&BindGroupDescriptor {
label: Some("bloom_prefilter_bind_group"),
Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_core_pipeline/src/core_2d/camera_2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ pub struct Camera2d {
impl ExtractComponent for Camera2d {
type Query = &'static Self;
type Filter = With<Camera>;
type Out = Self;

fn extract_component(item: QueryItem<'_, Self::Query>) -> Self {
item.clone()
fn extract_component(item: QueryItem<'_, Self::Query>) -> Option<Self> {
Some(item.clone())
}
}

Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_core_pipeline/src/core_3d/camera_3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,10 @@ impl From<Camera3dDepthLoadOp> for LoadOp<f32> {
impl ExtractComponent for Camera3d {
type Query = &'static Self;
type Filter = With<Camera>;
type Out = Self;

fn extract_component(item: QueryItem<'_, Self::Query>) -> Self {
item.clone()
fn extract_component(item: QueryItem<'_, Self::Query>) -> Option<Self> {
Some(item.clone())
}
}

Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_core_pipeline/src/fxaa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,10 @@ impl Default for Fxaa {
impl ExtractComponent for Fxaa {
type Query = &'static Self;
type Filter = With<Camera>;
type Out = Self;

fn extract_component(item: QueryItem<Self::Query>) -> Self {
item.clone()
fn extract_component(item: QueryItem<Self::Query>) -> Option<Self> {
Some(item.clone())
}
}

Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_core_pipeline/src/tonemapping/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,9 @@ impl Tonemapping {
impl ExtractComponent for Tonemapping {
type Query = &'static Self;
type Filter = With<Camera>;
type Out = Self;

fn extract_component(item: QueryItem<Self::Query>) -> Self {
item.clone()
fn extract_component(item: QueryItem<Self::Query>) -> Option<Self::Out> {
Some(item.clone())
}
}
33 changes: 28 additions & 5 deletions crates/bevy_render/src/extract_component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,26 @@ pub trait ExtractComponent: Component {
type Query: WorldQuery + ReadOnlyWorldQuery;
/// Filters the entities with additional constraints.
type Filter: WorldQuery + ReadOnlyWorldQuery;

/// The output from extraction.
///
/// Returning `None` based on the queried item can allow early optimization,
/// for example if there is an `enabled: bool` field on `Self`, or by only accepting
/// values within certain thresholds.
///
/// The output may be different from the queried component.
/// This can be useful for example if only a subset of the fields are useful
/// in the render world.
///
/// `Out` has a [`Bundle`] trait bound instead of a [`Component`] trait bound in order to allow use cases
/// such as tuples of components as output.
type Out: Bundle;

// TODO: https://github.com/rust-lang/rust/issues/29661
// type Out: Component = Self;

/// Defines how the component is transferred into the "render world".
fn extract_component(item: QueryItem<'_, Self::Query>) -> Self;
fn extract_component(item: QueryItem<'_, Self::Query>) -> Option<Self::Out>;
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add a Clone trait bound and add a default impl here? Some(item.clone()) seems to be the common pattern.

Copy link
Member

Choose a reason for hiding this comment

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

Extracting uncloneable components generally seems impossible or at least poorly thought out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm not too sure.

After these changes, the output type can be something entirely different than what is queried.
So a Clone bound on Self in that case is not strictly needed.

}

/// This plugin prepares the components of the corresponding type for the GPU
Expand Down Expand Up @@ -172,10 +190,11 @@ impl<C: ExtractComponent> Plugin for ExtractComponentPlugin<C> {
impl<T: Asset> ExtractComponent for Handle<T> {
type Query = Read<Handle<T>>;
type Filter = ();
type Out = Handle<T>;

#[inline]
fn extract_component(handle: QueryItem<'_, Self::Query>) -> Self {
handle.clone_weak()
fn extract_component(handle: QueryItem<'_, Self::Query>) -> Option<Self::Out> {
Some(handle.clone_weak())
}
}

Expand All @@ -187,7 +206,9 @@ fn extract_components<C: ExtractComponent>(
) {
let mut values = Vec::with_capacity(*previous_len);
for (entity, query_item) in &query {
values.push((entity, C::extract_component(query_item)));
if let Some(component) = C::extract_component(query_item) {
values.push((entity, component));
}
}
*previous_len = values.len();
commands.insert_or_spawn_batch(values);
Expand All @@ -202,7 +223,9 @@ fn extract_visible_components<C: ExtractComponent>(
let mut values = Vec::with_capacity(*previous_len);
for (entity, computed_visibility, query_item) in &query {
if computed_visibility.is_visible() {
values.push((entity, C::extract_component(query_item)));
if let Some(component) = C::extract_component(query_item) {
values.push((entity, component));
}
}
}
*previous_len = values.len();
Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_ui/src/camera_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ impl Default for UiCameraConfig {
impl ExtractComponent for UiCameraConfig {
type Query = &'static Self;
type Filter = With<Camera>;
type Out = Self;

fn extract_component(item: QueryItem<'_, Self::Query>) -> Self {
item.clone()
fn extract_component(item: QueryItem<'_, Self::Query>) -> Option<Self> {
Some(item.clone())
}
}
5 changes: 3 additions & 2 deletions examples/shader/shader_instancing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,10 @@ struct InstanceMaterialData(Vec<InstanceData>);
impl ExtractComponent for InstanceMaterialData {
type Query = &'static InstanceMaterialData;
type Filter = ();
type Out = Self;

fn extract_component(item: QueryItem<'_, Self::Query>) -> Self {
InstanceMaterialData(item.0.clone())
fn extract_component(item: QueryItem<'_, Self::Query>) -> Option<Self> {
Some(InstanceMaterialData(item.0.clone()))
}
}

Expand Down