[Merged by Bors] - ExtractComponent output optional associated type by torsteingrindvik · Pull Request #6699 · bevyengine/bevy

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 <torstein.grindvik@nordicsemi.no>

Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>

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 <torstein.grindvik@nordicsemi.no>

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

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


Doc strings :)

Added a docstring.

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

+
    /// 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 <torstein.grindvik@nordicsemi.no>

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

alice-i-cecile

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

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

+    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

-    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

+/// 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 <torstein.grindvik@nordicsemi.no>

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 <52322338+torsteingrindvik@users.noreply.github.com>

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 <52322338+torsteingrindvik@users.noreply.github.com>

torsteingrindvik added 2 commits November 20, 2022 13:57

Use ExtractComponentPlugin and UniformComponentPlugin for Bloom

d52ac8d

Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>

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 <torstein.grindvik@nordicsemi.no>

alice-i-cecile reviewed Nov 20, 2022

View reviewed changes

Comment thread crates/bevy_render/src/extract_component.rs Outdated

alice-i-cecile reviewed Nov 20, 2022

View reviewed changes

Comment thread crates/bevy_render/src/extract_component.rs Outdated

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 <torstein.grindvik@nordicsemi.no>

Update crates/bevy_render/src/extract_component.rs

a0c02d7

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Update crates/bevy_render/src/extract_component.rs

1316e25

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

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 <torstein.grindvik@nordicsemi.no>

Hide BloomUniform in docs

603c12f

Signed-off-by: Torstein Grindvik <torstein.grindvik@nordicsemi.no>

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

Uh oh!

Uh oh!

Conversation

torsteingrindvik commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Changelog

Changed

Uh oh!

torsteingrindvik commented Nov 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JMS55 Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JMS55 left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Nov 21, 2022

Uh oh!

bors Bot commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

torsteingrindvik commented Nov 20, 2022 •

edited

Loading

alice-i-cecile Nov 20, 2022 •

edited

Loading

JMS55 Nov 20, 2022 •

edited

Loading