Skip to content

Implement Label traits for their boxed variants #2505

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

Closed
vladbat00 wants to merge 4 commits into from
Closed

Implement Label traits for their boxed variants #2505

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
5e651d0
Implement StageLabel for Box<dyn StageLabel>
vladbat00 Jul 20, 2021
646a2c9
Implement for all label types
vladbat00 Apr 11, 2022
7db5610
Introduce a boxed label wrapper type
vladbat00 Apr 11, 2022
6150070
Hide the box wrapper constructor from the docs
vladbat00 Apr 11, 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
10 changes: 5 additions & 5 deletions crates/bevy_app/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use std::fmt::Debug;

#[cfg(feature = "trace")]
use bevy_utils::tracing::info_span;
bevy_utils::define_label!(AppLabel);
bevy_utils::define_label!(AppLabel, BoxedAppLabel);

#[allow(clippy::needless_doctest_main)]
/// A container of app logic and data.
Expand Down Expand Up @@ -56,7 +56,7 @@ pub struct App {
pub runner: Box<dyn Fn(App)>,
/// A container of [`Stage`]s set to be run in a linear order.
pub schedule: Schedule,
sub_apps: HashMap<Box<dyn AppLabel>, SubApp>,
sub_apps: HashMap<BoxedAppLabel, SubApp>,
}

/// Each `SubApp` has its own [`Schedule`] and [`World`], enabling a separation of concerns.
Expand Down Expand Up @@ -864,7 +864,7 @@ impl App {
sub_app_runner: impl Fn(&mut World, &mut App) + 'static,
) -> &mut Self {
self.sub_apps.insert(
Box::new(label),
label.dyn_clone(),
SubApp {
app,
runner: Box::new(sub_app_runner),
Expand All @@ -889,7 +889,7 @@ impl App {
/// an [`Err`] containing the given label.
pub fn get_sub_app_mut(&mut self, label: impl AppLabel) -> Result<&mut App, impl AppLabel> {
self.sub_apps
.get_mut((&label) as &dyn AppLabel)
.get_mut(&label.dyn_clone())
.map(|sub_app| &mut sub_app.app)
.ok_or(label)
}
Expand All @@ -910,7 +910,7 @@ impl App {
/// an [`Err`] containing the given label.
pub fn get_sub_app(&self, label: impl AppLabel) -> Result<&App, impl AppLabel> {
self.sub_apps
.get((&label) as &dyn AppLabel)
.get(&label.dyn_clone())
.map(|sub_app| &sub_app.app)
.ok_or(label)
}
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_derive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ pub fn derive_enum_variant_meta(input: TokenStream) -> TokenStream {
pub fn derive_app_label(input: TokenStream) -> TokenStream {
let input = syn::parse_macro_input!(input as syn::DeriveInput);
let mut trait_path = BevyManifest::default().get_path("bevy_app");
let mut boxed_type_path = trait_path.clone();
trait_path.segments.push(format_ident!("AppLabel").into());
derive_label(input, &trait_path)
boxed_type_path
.segments
.push(format_ident!("BoxedAppLabel").into());
derive_label(input, &trait_path, &boxed_type_path)
}
55 changes: 37 additions & 18 deletions crates/bevy_ecs/macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,43 +424,62 @@ pub fn derive_world_query(input: TokenStream) -> TokenStream {
#[proc_macro_derive(SystemLabel)]
pub fn derive_system_label(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DeriveInput);
let mut trait_path = bevy_ecs_path();
trait_path.segments.push(format_ident!("schedule").into());
trait_path
.segments
.push(format_ident!("SystemLabel").into());
derive_label(input, &trait_path)
let LabelPaths {
trait_path,
boxed_type_path,
} = label_paths("SystemLabel");
derive_label(input, &trait_path, &boxed_type_path)
}

#[proc_macro_derive(StageLabel)]
pub fn derive_stage_label(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DeriveInput);
let mut trait_path = bevy_ecs_path();
trait_path.segments.push(format_ident!("schedule").into());
trait_path.segments.push(format_ident!("StageLabel").into());
derive_label(input, &trait_path)
let LabelPaths {
trait_path,
boxed_type_path,
} = label_paths("StageLabel");
derive_label(input, &trait_path, &boxed_type_path)
}

#[proc_macro_derive(AmbiguitySetLabel)]
pub fn derive_ambiguity_set_label(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DeriveInput);
let mut trait_path = bevy_ecs_path();
trait_path.segments.push(format_ident!("schedule").into());
trait_path
.segments
.push(format_ident!("AmbiguitySetLabel").into());
derive_label(input, &trait_path)
let LabelPaths {
trait_path,
boxed_type_path,
} = label_paths("AmbiguitySetLabel");
derive_label(input, &trait_path, &boxed_type_path)
}

#[proc_macro_derive(RunCriteriaLabel)]
pub fn derive_run_criteria_label(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DeriveInput);
let LabelPaths {
trait_path,
boxed_type_path,
} = label_paths("RunCriteriaLabel");
derive_label(input, &trait_path, &boxed_type_path)
}

struct LabelPaths {
trait_path: syn::Path,
boxed_type_path: syn::Path,
}

fn label_paths(label_trait_name: &'static str) -> LabelPaths {
let mut trait_path = bevy_ecs_path();
trait_path.segments.push(format_ident!("schedule").into());
let mut boxed_type_path = trait_path.clone();
trait_path
.segments
.push(format_ident!("RunCriteriaLabel").into());
derive_label(input, &trait_path)
.push(format_ident!("{}", label_trait_name).into());
boxed_type_path
.segments
.push(format_ident!("Boxed{}", label_trait_name).into());
LabelPaths {
trait_path,
boxed_type_path,
}
}

pub(crate) fn bevy_ecs_path() -> syn::Path {
Expand Down
13 changes: 4 additions & 9 deletions crates/bevy_ecs/src/schedule/label.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
pub use bevy_ecs_macros::{AmbiguitySetLabel, RunCriteriaLabel, StageLabel, SystemLabel};
use bevy_utils::define_label;

define_label!(StageLabel);
define_label!(SystemLabel);
define_label!(AmbiguitySetLabel);
define_label!(RunCriteriaLabel);

pub(crate) type BoxedStageLabel = Box<dyn StageLabel>;
pub(crate) type BoxedSystemLabel = Box<dyn SystemLabel>;
pub(crate) type BoxedAmbiguitySetLabel = Box<dyn AmbiguitySetLabel>;
pub(crate) type BoxedRunCriteriaLabel = Box<dyn RunCriteriaLabel>;
define_label!(StageLabel, BoxedStageLabel);
define_label!(SystemLabel, BoxedSystemLabel);
define_label!(AmbiguitySetLabel, BoxedAmbiguitySetLabel);
define_label!(RunCriteriaLabel, BoxedRunCriteriaLabel);
37 changes: 30 additions & 7 deletions crates/bevy_ecs/src/schedule/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ impl Schedule {
/// schedule.add_stage("my_stage", SystemStage::parallel());
/// ```
pub fn add_stage<S: Stage>(&mut self, label: impl StageLabel, stage: S) -> &mut Self {
let label: Box<dyn StageLabel> = Box::new(label);
let label: BoxedStageLabel = label.dyn_clone();
self.stage_order.push(label.clone());
let prev = self.stages.insert(label.clone(), Box::new(stage));
assert!(prev.is_none(), "Stage already exists: {:?}.", label);
Expand All @@ -135,13 +135,13 @@ impl Schedule {
label: impl StageLabel,
stage: S,
) -> &mut Self {
let label: Box<dyn StageLabel> = Box::new(label);
let label: BoxedStageLabel = label.dyn_clone();
let target = &target as &dyn StageLabel;
let target_index = self
.stage_order
.iter()
.enumerate()
.find(|(_i, stage_label)| &***stage_label == target)
.find(|(_i, stage_label)| stage_label.dyn_clone() == target.dyn_clone())
Copy link
Contributor Author

@vladbat00 vladbat00 Jul 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

DynEq won't work when comparing Box<dyn StageLabel> and &dyn StageLabel otherwise. I wasn't able to find any better way of fixing this.

Copy link
Member

@mockersf mockersf Jul 20, 2021

Choose a reason for hiding this comment

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

the dyn_clone may add some overhead, but it shouldn't be much as labels probably won't be big, and it's only during stage setup

Copy link
Member

@mockersf mockersf Jul 24, 2021

Choose a reason for hiding this comment

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

Could you add a test somewhere that would fail with a Box<dyn StageLabel> without this change? As it doesn't fail on compile, it would make sure we don't change back later on

Copy link
Contributor

@blaind blaind Aug 20, 2021

Choose a reason for hiding this comment

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

This could work for the comparison:
.find(|(_i, stage_label)| stage_label.as_ref() == target)

Copy link
Contributor

@blaind blaind Aug 20, 2021

Choose a reason for hiding this comment

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

Suggested change
.find(|(_i, stage_label)| stage_label.dyn_clone() == target.dyn_clone())
.find(|(_i, stage_label)| stage_label.as_ref() == target)

Copy link
Contributor Author

@vladbat00 vladbat00 Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've added the tests. Apologies for taking so long to do it. :)

@blaind, unfortunately, your suggestion didn't work :( It definitely looks cleaner, but my tests fail if I try it this way.

.map(|(i, _)| i)
.unwrap_or_else(|| panic!("Target stage does not exist: {:?}.", target));

Expand Down Expand Up @@ -169,13 +169,13 @@ impl Schedule {
label: impl StageLabel,
stage: S,
) -> &mut Self {
let label: Box<dyn StageLabel> = Box::new(label);
let label: BoxedStageLabel = label.dyn_clone();
let target = &target as &dyn StageLabel;
let target_index = self
.stage_order
.iter()
.enumerate()
.find(|(_i, stage_label)| &***stage_label == target)
.find(|(_i, stage_label)| stage_label.dyn_clone() == target.dyn_clone())
.map(|(i, _)| i)
.unwrap_or_else(|| panic!("Target stage does not exist: {:?}.", target));

Expand Down Expand Up @@ -307,7 +307,7 @@ impl Schedule {
/// ```
pub fn get_stage<T: Stage>(&self, label: &dyn StageLabel) -> Option<&T> {
self.stages
.get(label)
.get(&label.dyn_clone())
.and_then(|stage| stage.downcast_ref::<T>())
}

Expand All @@ -328,7 +328,7 @@ impl Schedule {
/// ```
pub fn get_stage_mut<T: Stage>(&mut self, label: &dyn StageLabel) -> Option<&mut T> {
self.stages
.get_mut(label)
.get_mut(&label.dyn_clone())
.and_then(|stage| stage.downcast_mut::<T>())
}

Expand Down Expand Up @@ -369,3 +369,26 @@ impl Stage for Schedule {
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_adding_after_boxed_stage() {
let mut schedule = Schedule::default();
schedule.add_stage("first", SystemStage::single_threaded());
let stage = schedule.iter_stages().next().unwrap().0.dyn_clone();
// shouldn't panic
schedule.add_stage_after(stage, "second", SystemStage::single_threaded());
}

#[test]
fn test_adding_before_boxed_stage() {
let mut schedule = Schedule::default();
schedule.add_stage("first", SystemStage::single_threaded());
let stage = schedule.iter_stages().next().unwrap().0.dyn_clone();
// shouldn't panic
schedule.add_stage_before(stage, "second", SystemStage::single_threaded());
}
}
12 changes: 6 additions & 6 deletions crates/bevy_ecs/src/schedule/run_criteria.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ where
L: RunCriteriaLabel,
{
fn into(self) -> RunCriteriaDescriptorOrLabel {
RunCriteriaDescriptorOrLabel::Label(Box::new(self))
RunCriteriaDescriptorOrLabel::Label(self.dyn_clone())
}
}

Expand All @@ -232,24 +232,24 @@ pub trait RunCriteriaDescriptorCoercion<Param> {

impl RunCriteriaDescriptorCoercion<()> for RunCriteriaDescriptor {
fn label(mut self, label: impl RunCriteriaLabel) -> RunCriteriaDescriptor {
self.label = Some(Box::new(label));
self.label = Some(label.dyn_clone());
self.duplicate_label_strategy = DuplicateLabelStrategy::Panic;
self
}

fn label_discard_if_duplicate(mut self, label: impl RunCriteriaLabel) -> RunCriteriaDescriptor {
self.label = Some(Box::new(label));
self.label = Some(label.dyn_clone());
self.duplicate_label_strategy = DuplicateLabelStrategy::Discard;
self
}

fn before(mut self, label: impl RunCriteriaLabel) -> RunCriteriaDescriptor {
self.before.push(Box::new(label));
self.before.push(label.dyn_clone());
self
}

fn after(mut self, label: impl RunCriteriaLabel) -> RunCriteriaDescriptor {
self.after.push(Box::new(label));
self.after.push(label.dyn_clone());
self
}
}
Expand Down Expand Up @@ -320,7 +320,7 @@ impl RunCriteria {
label: None,
duplicate_label_strategy: DuplicateLabelStrategy::Panic,
before: vec![],
after: vec![Box::new(label)],
after: vec![label.dyn_clone()],
}
}
}
Expand Down
Loading