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

torsteingrindvik · 2022-11-20T12:31:11Z

Objective

Allow more use cases where the user may benefit from both ExtractComponentPlugin and UniformComponentPlugin.

Solution

Add an associated type to ExtractComponent in order to allow specifying the output component (or bundle).

Make extract_component return an Option<_> such that components can be extracted only when needed.

What problem does this solve?

ExtractComponentPlugin allows extracting components, but currently the output type is the same as the input.
This means that use cases such as having a settings struct which turns into a uniform is awkward.

For example we might have:

struct MyStruct {
    enabled: bool,
    val: f32
}

struct MyStructUniform {
    val: f32
}

With the new approach, we can extract MyStruct only when it is enabled, and turn it into its related uniform.

This chains well with UniformComponentPlugin.

The user may then:

app.add_plugin(ExtractComponentPlugin::<MyStruct>::default());
app.add_plugin(UniformComponentPlugin::<MyStructUniform>::default());

This then saves the user a fair amount of boilerplate.

Changelog

Changed

ExtractComponent can specify output type, and outputting is optional.

Add an associated type to `ExtractComponent` in order to allow specifying the output component. Make `extract_component` return an `Option<_>` such that components can be extracted only when needed. What problem does this solve? `ExtractComponentPlugin` allows extracting components, but currently the output type is the same as the input. This means that use cases such as having a settings struct which turns into a uniform is awkward. For example we might have: ```rust struct MyStruct { enabled: bool, val: f32 } struct MyStructUniform { val: f32 } ``` With the new approach, we can extract `MyStruct` only when it is enabled, and turn it into its related uniform. This chains well with `UniformComponentPlugin`. The user may then: ```rust app.add_plugin(ExtractComponentPlugin::<MyStruct>::default()); app.add_plugin(UniformComponentPlugin::<MyStructUniform>::default()); ``` This then saves the user a fair amount of boilerplate. Signed-off-by: Torstein Grindvik <[email protected]>

Signed-off-by: Torstein Grindvik <[email protected]>

torsteingrindvik · 2022-11-20T13:03:19Z

Added a second commit which uses the changes on the bloom plugin.

The results are:

Bloom reuses ExtractComponentPlugin
Bloom reuses UniformComponentPlugin
Eliminated a struct: BloomUniforms
Eliminated a struct: BloomUniformIndex
Eliminated a system: extract_bloom_settings
Eliminated a system: prepare_bloom_uniforms

Signed-off-by: Torstein Grindvik <[email protected]>

alice-i-cecile · 2022-11-20T14:33:20Z

crates/bevy_core_pipeline/src/bloom/mod.rs

-#[derive(ShaderType)]
-struct BloomUniform {
+#[derive(Component, ShaderType, Clone)]
+pub struct BloomUniform {


Doc strings :)

Added a docstring.

crates/bevy_render/src/extract_component.rs

alice-i-cecile · 2022-11-20T14:39:25Z

crates/bevy_render/src/extract_component.rs

    /// 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>;


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

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

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.

Signed-off-by: Torstein Grindvik <[email protected]>

Co-authored-by: Alice Cecile <[email protected]>

alice-i-cecile

The lack of associated type defaults sucks, but this is still an improvement.

JMS55 · 2022-11-20T15:38:39Z

crates/bevy_core_pipeline/src/bloom/mod.rs

+    type Out = BloomUniform;
+
+    fn extract_component((settings, camera): QueryItem<'_, Self::Query>) -> Option<Self::Out> {
+        if let Some(size) = camera.physical_viewport_size() {


Replace if-let with Option::map.

JMS55 · 2022-11-20T15:39:59Z

crates/bevy_core_pipeline/src/bloom/mod.rs

-    cameras: Extract<Query<(Entity, &Camera, &BloomSettings), With<Camera>>>,
-) {
-    for (entity, camera, bloom_settings) in &cameras {
-        if camera.is_active && camera.hdr {


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

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

JMS55 · 2022-11-20T15:41:16Z

crates/bevy_core_pipeline/src/bloom/mod.rs

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


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

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)

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

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

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.

Signed-off-by: Torstein Grindvik <[email protected]>

JMS55

lgtm!

alice-i-cecile · 2022-11-21T13:10:46Z

bors r+

# Objective Allow more use cases where the user may benefit from both `ExtractComponentPlugin` _and_ `UniformComponentPlugin`. ## Solution Add an associated type to `ExtractComponent` in order to allow specifying the output component (or bundle). Make `extract_component` return an `Option<_>` such that components can be extracted only when needed. What problem does this solve? `ExtractComponentPlugin` allows extracting components, but currently the output type is the same as the input. This means that use cases such as having a settings struct which turns into a uniform is awkward. For example we might have: ```rust struct MyStruct { enabled: bool, val: f32 } struct MyStructUniform { val: f32 } ``` With the new approach, we can extract `MyStruct` only when it is enabled, and turn it into its related uniform. This chains well with `UniformComponentPlugin`. The user may then: ```rust app.add_plugin(ExtractComponentPlugin::<MyStruct>::default()); app.add_plugin(UniformComponentPlugin::<MyStructUniform>::default()); ``` This then saves the user a fair amount of boilerplate. ## Changelog ### Changed - `ExtractComponent` can specify output type, and outputting is optional. Co-authored-by: Torstein Grindvik <[email protected]>

bors · 2022-11-21T13:35:54Z

Pull request successfully merged into main.

Build succeeded:

# Objective Allow more use cases where the user may benefit from both `ExtractComponentPlugin` _and_ `UniformComponentPlugin`. ## Solution Add an associated type to `ExtractComponent` in order to allow specifying the output component (or bundle). Make `extract_component` return an `Option<_>` such that components can be extracted only when needed. What problem does this solve? `ExtractComponentPlugin` allows extracting components, but currently the output type is the same as the input. This means that use cases such as having a settings struct which turns into a uniform is awkward. For example we might have: ```rust struct MyStruct { enabled: bool, val: f32 } struct MyStructUniform { val: f32 } ``` With the new approach, we can extract `MyStruct` only when it is enabled, and turn it into its related uniform. This chains well with `UniformComponentPlugin`. The user may then: ```rust app.add_plugin(ExtractComponentPlugin::<MyStruct>::default()); app.add_plugin(UniformComponentPlugin::<MyStructUniform>::default()); ``` This then saves the user a fair amount of boilerplate. ## Changelog ### Changed - `ExtractComponent` can specify output type, and outputting is optional. Co-authored-by: Torstein Grindvik <[email protected]>

torsteingrindvik added 2 commits November 20, 2022 13:57

Use ExtractComponentPlugin and UniformComponentPlugin for Bloom

d52ac8d

Signed-off-by: Torstein Grindvik <[email protected]>

torsteingrindvik force-pushed the extract-component branch from 7b55bfb to d52ac8d Compare November 20, 2022 12:59

Use Bundle as output instead of Component in ExtractComponent

813ad39

Signed-off-by: Torstein Grindvik <[email protected]>

alice-i-cecile reviewed Nov 20, 2022

View reviewed changes

crates/bevy_render/src/extract_component.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 20, 2022

View reviewed changes

crates/bevy_render/src/extract_component.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Nov 20, 2022

View reviewed changes

torsteingrindvik and others added 3 commits November 20, 2022 15:42

Add docstring to BloomUniform

6767323

Signed-off-by: Torstein Grindvik <[email protected]>

Update crates/bevy_render/src/extract_component.rs

a0c02d7

Co-authored-by: Alice Cecile <[email protected]>

Update crates/bevy_render/src/extract_component.rs

1316e25

Co-authored-by: Alice Cecile <[email protected]>

alice-i-cecile approved these changes Nov 20, 2022

View reviewed changes

alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Nov 20, 2022

JMS55 suggested changes Nov 20, 2022

View reviewed changes

torsteingrindvik added 2 commits November 20, 2022 16:55

Fix check for active+hdr, use Option::map

373f0de

Signed-off-by: Torstein Grindvik <[email protected]>

Hide BloomUniform in docs

603c12f

Signed-off-by: Torstein Grindvik <[email protected]>

JMS55 approved these changes Nov 20, 2022

View reviewed changes

james7132 approved these changes Nov 20, 2022

View reviewed changes

james7132 added this to the 0.10 milestone Nov 20, 2022

james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 20, 2022

bors bot changed the title ~~ExtractComponent output optional associated type~~ [Merged by Bors] - ExtractComponent output optional associated type Nov 21, 2022

bors bot closed this Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

torsteingrindvik commented Nov 20, 2022 •

edited

Loading

torsteingrindvik commented Nov 20, 2022

alice-i-cecile Nov 20, 2022

torsteingrindvik Nov 20, 2022

alice-i-cecile Nov 20, 2022

alice-i-cecile Nov 20, 2022

torsteingrindvik Nov 20, 2022

alice-i-cecile left a comment

JMS55 Nov 20, 2022

torsteingrindvik Nov 20, 2022

JMS55 Nov 20, 2022

torsteingrindvik Nov 20, 2022

JMS55 Nov 20, 2022

torsteingrindvik Nov 20, 2022

alice-i-cecile Nov 20, 2022 •

edited

Loading

JMS55 Nov 20, 2022 •

edited

Loading

torsteingrindvik Nov 20, 2022

JMS55 left a comment

alice-i-cecile commented Nov 21, 2022

bors bot commented Nov 21, 2022

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

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

Conversation

torsteingrindvik commented Nov 20, 2022 • edited Loading

Objective

Solution

Changelog

Changed

torsteingrindvik commented Nov 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile Nov 20, 2022 • edited Loading

Choose a reason for hiding this comment

JMS55 Nov 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JMS55 left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Nov 21, 2022

bors bot commented Nov 21, 2022

torsteingrindvik commented Nov 20, 2022 •

edited

Loading

alice-i-cecile Nov 20, 2022 •

edited

Loading

JMS55 Nov 20, 2022 •

edited

Loading