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

[Merged by Bors] - Auto-label function systems with SystemTypeIdLabel #4224

Closed
wants to merge 6 commits into from
Closed

[Merged by Bors] - Auto-label function systems with SystemTypeIdLabel #4224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e8a5c6a
Auto-label function systems
cart Mar 16, 2022
649987b
a.after(b)
cart Mar 16, 2022
76ca7f6
fix ambiguity test
cart Mar 16, 2022
a964e8e
Update crates/bevy_ecs/src/system/function_system.rs
cart Mar 16, 2022
ca7c674
Make SystemTypeIdLabel ZST, remove boxing from AsSystemLabel
cart Mar 23, 2022
c39276e
clippy
cart Mar 23, 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
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ pub mod prelude {
system::{
Commands, In, IntoChainSystem, IntoExclusiveSystem, IntoSystem, Local, NonSend,
NonSendMut, Query, QuerySet, RemovedComponents, Res, ResMut, System,
SystemParamFunction,
},
world::{FromWorld, Mut, World},
};
Expand Down
46 changes: 28 additions & 18 deletions crates/bevy_ecs/src/schedule/stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1583,15 +1583,25 @@ mod tests {
fn ambiguity_detection() {
use super::{find_ambiguities, SystemContainer};

fn find_ambiguities_first_labels(
fn find_ambiguities_first_str_labels(
cart marked this conversation as resolved.
Show resolved Hide resolved
systems: &[impl SystemContainer],
) -> Vec<(BoxedSystemLabel, BoxedSystemLabel)> {
find_ambiguities(systems)
.drain(..)
.map(|(index_a, index_b, _conflicts)| {
(
systems[index_a].labels()[0].clone(),
systems[index_b].labels()[0].clone(),
systems[index_a]
.labels()
.iter()
.find(|a| (&***a).type_id() == std::any::TypeId::of::<&str>())
cart marked this conversation as resolved.
Show resolved Hide resolved
.unwrap()
.clone(),
systems[index_b]
.labels()
.iter()
.find(|a| (&***a).type_id() == std::any::TypeId::of::<&str>())
.unwrap()
.clone(),
)
})
.collect()
Expand Down Expand Up @@ -1621,7 +1631,7 @@ mod tests {
.with_system(component.label("4"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("1"), Box::new("4")))
|| ambiguities.contains(&(Box::new("4"), Box::new("1")))
Expand All @@ -1636,7 +1646,7 @@ mod tests {
.with_system(resource.label("4"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("1"), Box::new("4")))
|| ambiguities.contains(&(Box::new("4"), Box::new("1")))
Expand All @@ -1661,7 +1671,7 @@ mod tests {
.with_system(resource.label("4"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("0"), Box::new("3")))
|| ambiguities.contains(&(Box::new("3"), Box::new("0")))
Expand All @@ -1680,7 +1690,7 @@ mod tests {
.with_system(resource.label("4").in_ambiguity_set("a"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("0"), Box::new("3")))
|| ambiguities.contains(&(Box::new("3"), Box::new("0")))
Expand All @@ -1693,7 +1703,7 @@ mod tests {
.with_system(component.label("2"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("0"), Box::new("1")))
|| ambiguities.contains(&(Box::new("1"), Box::new("0")))
Expand All @@ -1706,7 +1716,7 @@ mod tests {
.with_system(component.label("2").after("0"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("1"), Box::new("2")))
|| ambiguities.contains(&(Box::new("2"), Box::new("1")))
Expand All @@ -1720,7 +1730,7 @@ mod tests {
.with_system(component.label("3").after("1").after("2"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("1"), Box::new("2")))
|| ambiguities.contains(&(Box::new("2"), Box::new("1")))
Expand All @@ -1734,7 +1744,7 @@ mod tests {
.with_system(component.label("3").after("1").after("2"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert_eq!(ambiguities.len(), 0);

let mut stage = SystemStage::parallel()
Expand All @@ -1744,7 +1754,7 @@ mod tests {
.with_system(component.label("3").after("1").after("2"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("1"), Box::new("2")))
|| ambiguities.contains(&(Box::new("2"), Box::new("1")))
Expand Down Expand Up @@ -1774,7 +1784,7 @@ mod tests {
);
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("1"), Box::new("2")))
|| ambiguities.contains(&(Box::new("2"), Box::new("1")))
Expand Down Expand Up @@ -1824,7 +1834,7 @@ mod tests {
);
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert_eq!(ambiguities.len(), 0);

let mut stage = SystemStage::parallel()
Expand Down Expand Up @@ -1855,7 +1865,7 @@ mod tests {
);
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.parallel);
let ambiguities = find_ambiguities_first_str_labels(&stage.parallel);
assert!(
ambiguities.contains(&(Box::new("1"), Box::new("4")))
|| ambiguities.contains(&(Box::new("4"), Box::new("1")))
Expand Down Expand Up @@ -1889,7 +1899,7 @@ mod tests {
.with_system(empty.exclusive_system().label("6").after("2").after("5"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.exclusive_at_start);
let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start);
assert!(
ambiguities.contains(&(Box::new("1"), Box::new("3")))
|| ambiguities.contains(&(Box::new("3"), Box::new("1")))
Expand Down Expand Up @@ -1926,7 +1936,7 @@ mod tests {
.with_system(empty.exclusive_system().label("6").after("2").after("5"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.exclusive_at_start);
let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start);
assert!(
ambiguities.contains(&(Box::new("2"), Box::new("3")))
|| ambiguities.contains(&(Box::new("3"), Box::new("2")))
Expand All @@ -1952,7 +1962,7 @@ mod tests {
.with_system(empty.exclusive_system().label("3").in_ambiguity_set("a"));
stage.initialize_systems(&mut world);
stage.rebuild_orders_and_dependencies();
let ambiguities = find_ambiguities_first_labels(&stage.exclusive_at_start);
let ambiguities = find_ambiguities_first_str_labels(&stage.exclusive_at_start);
assert_eq!(ambiguities.len(), 0);
}

Expand Down
27 changes: 15 additions & 12 deletions crates/bevy_ecs/src/schedule/system_descriptor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ use crate::{
AmbiguitySetLabel, BoxedAmbiguitySetLabel, BoxedSystemLabel, IntoRunCriteria,
RunCriteriaDescriptorOrLabel, SystemLabel,
},
system::{BoxedSystem, ExclusiveSystem, ExclusiveSystemCoerced, ExclusiveSystemFn, IntoSystem},
system::{
AsSystemLabel, BoxedSystem, ExclusiveSystem, ExclusiveSystemCoerced, ExclusiveSystemFn,
IntoSystem,
},
};

/// Encapsulates a system and information on when it run in a `SystemStage`.
Expand Down Expand Up @@ -105,9 +108,9 @@ pub struct ParallelSystemDescriptor {

fn new_parallel_descriptor(system: BoxedSystem<(), ()>) -> ParallelSystemDescriptor {
ParallelSystemDescriptor {
labels: system.default_labels(),
system,
run_criteria: None,
labels: Vec::new(),
before: Vec::new(),
after: Vec::new(),
ambiguity_sets: Vec::new(),
Expand All @@ -126,10 +129,10 @@ pub trait ParallelSystemDescriptorCoercion<Params> {
fn label(self, label: impl SystemLabel) -> ParallelSystemDescriptor;

/// Specifies that the system should run before systems with the given label.
fn before(self, label: impl SystemLabel) -> ParallelSystemDescriptor;
fn before<Marker>(self, label: impl AsSystemLabel<Marker>) -> ParallelSystemDescriptor;
cart marked this conversation as resolved.
Show resolved Hide resolved

/// Specifies that the system should run after systems with the given label.
fn after(self, label: impl SystemLabel) -> ParallelSystemDescriptor;
fn after<Marker>(self, label: impl AsSystemLabel<Marker>) -> ParallelSystemDescriptor;

/// Specifies that the system is exempt from execution order ambiguity detection
/// with other systems in this set.
Expand All @@ -150,13 +153,13 @@ impl ParallelSystemDescriptorCoercion<()> for ParallelSystemDescriptor {
self
}

fn before(mut self, label: impl SystemLabel) -> ParallelSystemDescriptor {
self.before.push(Box::new(label));
fn before<Marker>(mut self, label: impl AsSystemLabel<Marker>) -> ParallelSystemDescriptor {
self.before.push(Box::new(label.as_system_label()));
self
}

fn after(mut self, label: impl SystemLabel) -> ParallelSystemDescriptor {
self.after.push(Box::new(label));
fn after<Marker>(mut self, label: impl AsSystemLabel<Marker>) -> ParallelSystemDescriptor {
self.after.push(Box::new(label.as_system_label()));
self
}

Expand All @@ -182,11 +185,11 @@ where
new_parallel_descriptor(Box::new(IntoSystem::into_system(self))).label(label)
}

fn before(self, label: impl SystemLabel) -> ParallelSystemDescriptor {
fn before<Marker>(self, label: impl AsSystemLabel<Marker>) -> ParallelSystemDescriptor {
new_parallel_descriptor(Box::new(IntoSystem::into_system(self))).before(label)
}

fn after(self, label: impl SystemLabel) -> ParallelSystemDescriptor {
fn after<Marker>(self, label: impl AsSystemLabel<Marker>) -> ParallelSystemDescriptor {
new_parallel_descriptor(Box::new(IntoSystem::into_system(self))).after(label)
}

Expand All @@ -207,11 +210,11 @@ impl ParallelSystemDescriptorCoercion<()> for BoxedSystem<(), ()> {
new_parallel_descriptor(self).label(label)
}

fn before(self, label: impl SystemLabel) -> ParallelSystemDescriptor {
fn before<Marker>(self, label: impl AsSystemLabel<Marker>) -> ParallelSystemDescriptor {
new_parallel_descriptor(self).before(label)
}

fn after(self, label: impl SystemLabel) -> ParallelSystemDescriptor {
fn after<Marker>(self, label: impl AsSystemLabel<Marker>) -> ParallelSystemDescriptor {
new_parallel_descriptor(self).after(label)
}

Expand Down
69 changes: 68 additions & 1 deletion crates/bevy_ecs/src/system/function_system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@ use crate::{
archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId},
component::ComponentId,
query::{Access, FilteredAccessSet},
schedule::SystemLabel,
system::{
check_system_change_tick, ReadOnlySystemParamFetch, System, SystemParam, SystemParamFetch,
SystemParamState,
},
world::{World, WorldId},
};
use bevy_ecs_macros::all_tuples;
use std::{borrow::Cow, marker::PhantomData};
use std::{borrow::Cow, fmt::Debug, hash::Hash, marker::PhantomData};

/// The metadata of a [`System`].
pub struct SystemMeta {
Expand Down Expand Up @@ -421,6 +422,47 @@ where
self.system_meta.name.as_ref(),
);
}
fn default_labels(&self) -> Vec<Box<dyn SystemLabel>> {
vec![Box::new(self.func.as_system_label())]
}
}

/// A [`SystemLabel`] that was automatically generated for a system on the basis of its `TypeId`.
pub struct SystemTypeIdLabel<T: 'static>(PhantomData<fn() -> T>);
Copy link
Member

Choose a reason for hiding this comment

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

This couldn't be derive(Default) because of the type parameter bounds, but should it still be Default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. This would enable people to construct these labels without an instance to a system type. Maybe thats good. Is there a use case?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. This would enable people to construct these labels without an instance to a system type. Maybe thats good. Is there a use case?

This could be useful if you wanted to initialize an array of these things, especially if we also impl'd Copy. I'm not immediately sure why you'd want to do that though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha I'm impatient, so I'm just going to merge this now. I'm happy to add this if we decide its needed.


impl<T> Debug for SystemTypeIdLabel<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_tuple("SystemTypeIdLabel")
.field(&std::any::type_name::<T>())
.finish()
}
}
impl<T> Hash for SystemTypeIdLabel<T> {
fn hash<H: std::hash::Hasher>(&self, _state: &mut H) {
// All SystemTypeIds of a given type are the same.
}
}
impl<T> Clone for SystemTypeIdLabel<T> {
fn clone(&self) -> Self {
Self(PhantomData)
}
}

impl<T> Copy for SystemTypeIdLabel<T> {}

impl<T> PartialEq for SystemTypeIdLabel<T> {
#[inline]
fn eq(&self, _other: &Self) -> bool {
// All labels of a given type are equal, as they will all have the same type id
true
}
}
impl<T> Eq for SystemTypeIdLabel<T> {}

impl<T> SystemLabel for SystemTypeIdLabel<T> {
fn dyn_clone(&self) -> Box<dyn SystemLabel> {
Box::new(*self)
}
}

/// A trait implemented for all functions that can be used as [`System`]s.
Expand Down Expand Up @@ -489,3 +531,28 @@ macro_rules! impl_system_function {
}

all_tuples!(impl_system_function, 0, 16, F);

/// Used to implicitly convert systems to their default labels. For example, it will convert
/// "system functions" to their [`SystemTypeIdLabel`].
pub trait AsSystemLabel<Marker> {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a short doc comment stating this is used for system -> SystemLabel coercion plus a pointer to SystemLabel which illustrates the ergonomics.

It'd also stave off the perpetually missing #[forbid(missing_docs)] for the crate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call!

type SystemLabel: SystemLabel;
fn as_system_label(&self) -> Self::SystemLabel;
Copy link
Member

Choose a reason for hiding this comment

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

I'll record that I'm still not super happy with this being a method, but I don't think that's changing.

Copy link
Member Author

@cart cart Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I'm not (yet) convinced. I think there are good reasons to make "manually retrieving the label from a system instance" a part of our api. We don't abstract out "other" labels and we let those instances float around. Why abstract out these?
I'm down to continue discussing, but I dont want to block on this.

}

impl<In, Out, Param: SystemParam, Marker, T: SystemParamFunction<In, Out, Param, Marker>>
AsSystemLabel<(In, Out, Param, Marker)> for T
{
type SystemLabel = SystemTypeIdLabel<Self>;

fn as_system_label(&self) -> Self::SystemLabel {
SystemTypeIdLabel(PhantomData::<fn() -> Self>)
}
}

impl<T: SystemLabel + Clone> AsSystemLabel<()> for T {
type SystemLabel = T;

fn as_system_label(&self) -> Self::SystemLabel {
self.clone()
}
}
5 changes: 5 additions & 0 deletions crates/bevy_ecs/src/system/system.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::{
archetype::{Archetype, ArchetypeComponentId},
component::ComponentId,
query::Access,
schedule::SystemLabel,
world::World,
};
use std::borrow::Cow;
Expand Down Expand Up @@ -56,6 +57,10 @@ pub trait System: Send + Sync + 'static {
/// Initialize the system.
fn initialize(&mut self, _world: &mut World);
fn check_change_tick(&mut self, change_tick: u32);
/// The default labels for the system
fn default_labels(&self) -> Vec<Box<dyn SystemLabel>> {
cart marked this conversation as resolved.
Show resolved Hide resolved
Vec::new()
}
}

/// A convenience type alias for a boxed [`System`] trait object.
Expand Down
Loading