Skip to content

Commit

Permalink
Use consistent names for marker generics (#7788)
Browse files Browse the repository at this point in the history
# Objective

Several places in the ECS use marker generics to avoid overlapping trait implementations, but different places alternately refer to it as `Params` and `Marker`. This is potentially confusing, since it might not be clear that the same pattern is being used. Additionally, users might be misled into thinking that the `Params` type corresponds to the `SystemParam`s of a system.

## Solution

Rename `Params` to `Marker`.
  • Loading branch information
JoJoJet committed Feb 23, 2023
1 parent 12aadfd commit ee4c8c5
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 46 deletions.
16 changes: 8 additions & 8 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ impl App {
/// #
/// app.add_system(my_system);
/// ```
pub fn add_system<P>(&mut self, system: impl IntoSystemConfig<P>) -> &mut Self {
pub fn add_system<M>(&mut self, system: impl IntoSystemConfig<M>) -> &mut Self {
let mut schedules = self.world.resource_mut::<Schedules>();

if let Some(default_schedule) = schedules.get_mut(&*self.default_schedule_label) {
Expand Down Expand Up @@ -406,7 +406,7 @@ impl App {
/// #
/// app.add_systems((system_a, system_b, system_c));
/// ```
pub fn add_systems<P>(&mut self, systems: impl IntoSystemConfigs<P>) -> &mut Self {
pub fn add_systems<M>(&mut self, systems: impl IntoSystemConfigs<M>) -> &mut Self {
let mut schedules = self.world.resource_mut::<Schedules>();

if let Some(default_schedule) = schedules.get_mut(&*self.default_schedule_label) {
Expand All @@ -420,10 +420,10 @@ impl App {
}

/// Adds a system to the provided [`Schedule`].
pub fn add_system_to_schedule<P>(
pub fn add_system_to_schedule<M>(
&mut self,
schedule_label: impl ScheduleLabel,
system: impl IntoSystemConfig<P>,
system: impl IntoSystemConfig<M>,
) -> &mut Self {
let mut schedules = self.world.resource_mut::<Schedules>();

Expand All @@ -437,10 +437,10 @@ impl App {
}

/// Adds a collection of system to the provided [`Schedule`].
pub fn add_systems_to_schedule<P>(
pub fn add_systems_to_schedule<M>(
&mut self,
schedule_label: impl ScheduleLabel,
systems: impl IntoSystemConfigs<P>,
systems: impl IntoSystemConfigs<M>,
) -> &mut Self {
let mut schedules = self.world.resource_mut::<Schedules>();

Expand Down Expand Up @@ -471,7 +471,7 @@ impl App {
/// App::new()
/// .add_startup_system(my_startup_system);
/// ```
pub fn add_startup_system<P>(&mut self, system: impl IntoSystemConfig<P>) -> &mut Self {
pub fn add_startup_system<M>(&mut self, system: impl IntoSystemConfig<M>) -> &mut Self {
self.add_system_to_schedule(CoreSchedule::Startup, system)
}

Expand All @@ -496,7 +496,7 @@ impl App {
/// )
/// );
/// ```
pub fn add_startup_systems<P>(&mut self, systems: impl IntoSystemConfigs<P>) -> &mut Self {
pub fn add_startup_systems<M>(&mut self, systems: impl IntoSystemConfigs<M>) -> &mut Self {
self.add_systems_to_schedule(CoreSchedule::Startup, systems)
}

Expand Down
20 changes: 10 additions & 10 deletions crates/bevy_ecs/src/schedule/condition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ pub type BoxedCondition = Box<dyn ReadOnlySystem<In = (), Out = bool>>;
///
/// Implemented for functions and closures that convert into [`System<In=(), Out=bool>`](crate::system::System)
/// with [read-only](crate::system::ReadOnlySystemParam) parameters.
pub trait Condition<Params>: sealed::Condition<Params> {
pub trait Condition<Marker>: sealed::Condition<Marker> {
/// Returns a new run condition that only returns `true`
/// if both this one and the passed `and_then` return `true`.
///
Expand Down Expand Up @@ -53,7 +53,7 @@ pub trait Condition<Params>: sealed::Condition<Params> {
/// Note that in this case, it's better to just use the run condition [`resource_exists_and_equals`].
///
/// [`resource_exists_and_equals`]: common_conditions::resource_exists_and_equals
fn and_then<P, C: Condition<P>>(self, and_then: C) -> AndThen<Self::System, C::System> {
fn and_then<M, C: Condition<M>>(self, and_then: C) -> AndThen<Self::System, C::System> {
let a = IntoSystem::into_system(self);
let b = IntoSystem::into_system(and_then);
let name = format!("{} && {}", a.name(), b.name());
Expand Down Expand Up @@ -100,30 +100,30 @@ pub trait Condition<Params>: sealed::Condition<Params> {
/// # app.run(&mut world);
/// # assert!(world.resource::<C>().0);
/// ```
fn or_else<P, C: Condition<P>>(self, or_else: C) -> OrElse<Self::System, C::System> {
fn or_else<M, C: Condition<M>>(self, or_else: C) -> OrElse<Self::System, C::System> {
let a = IntoSystem::into_system(self);
let b = IntoSystem::into_system(or_else);
let name = format!("{} || {}", a.name(), b.name());
CombinatorSystem::new(a, b, Cow::Owned(name))
}
}

impl<Params, F> Condition<Params> for F where F: sealed::Condition<Params> {}
impl<Marker, F> Condition<Marker> for F where F: sealed::Condition<Marker> {}

mod sealed {
use crate::system::{IntoSystem, ReadOnlySystem};

pub trait Condition<Params>:
IntoSystem<(), bool, Params, System = Self::ReadOnlySystem>
pub trait Condition<Marker>:
IntoSystem<(), bool, Marker, System = Self::ReadOnlySystem>
{
// This associated type is necessary to let the compiler
// know that `Self::System` is `ReadOnlySystem`.
type ReadOnlySystem: ReadOnlySystem<In = (), Out = bool>;
}

impl<Params, F> Condition<Params> for F
impl<Marker, F> Condition<Marker> for F
where
F: IntoSystem<(), bool, Params>,
F: IntoSystem<(), bool, Marker>,
F::System: ReadOnlySystem,
{
type ReadOnlySystem = F::System;
Expand Down Expand Up @@ -373,8 +373,8 @@ pub mod common_conditions {
/// #
/// # fn my_system() { unreachable!() }
/// ```
pub fn not<Params>(
condition: impl Condition<Params>,
pub fn not<Marker>(
condition: impl Condition<Marker>,
) -> impl ReadOnlySystem<In = (), Out = bool> {
condition.pipe(|In(val): In<bool>| !val)
}
Expand Down
34 changes: 17 additions & 17 deletions crates/bevy_ecs/src/schedule/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ impl SystemConfig {
}
}

fn new_condition<P>(condition: impl Condition<P>) -> BoxedCondition {
fn new_condition<M>(condition: impl Condition<M>) -> BoxedCondition {
let condition_system = IntoSystem::into_system(condition);
assert!(
condition_system.is_send(),
Expand Down Expand Up @@ -100,7 +100,7 @@ pub trait IntoSystemSetConfig: sealed::IntoSystemSetConfig {
///
/// The `Condition` will be evaluated at most once (per schedule run),
/// the first time a system in this set prepares to run.
fn run_if<P>(self, condition: impl Condition<P>) -> SystemSetConfig;
fn run_if<M>(self, condition: impl Condition<M>) -> SystemSetConfig;
/// Suppress warnings and errors that would result from systems in this set having ambiguities
/// (conflicting access but indeterminate order) with systems in `set`.
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfig;
Expand Down Expand Up @@ -139,7 +139,7 @@ where
self.into_config().after(set)
}

fn run_if<P>(self, condition: impl Condition<P>) -> SystemSetConfig {
fn run_if<M>(self, condition: impl Condition<M>) -> SystemSetConfig {
self.into_config().run_if(condition)
}

Expand Down Expand Up @@ -179,7 +179,7 @@ impl IntoSystemSetConfig for BoxedSystemSet {
self.into_config().after(set)
}

fn run_if<P>(self, condition: impl Condition<P>) -> SystemSetConfig {
fn run_if<M>(self, condition: impl Condition<M>) -> SystemSetConfig {
self.into_config().run_if(condition)
}

Expand Down Expand Up @@ -254,7 +254,7 @@ impl IntoSystemSetConfig for SystemSetConfig {
self
}

fn run_if<P>(mut self, condition: impl Condition<P>) -> Self {
fn run_if<M>(mut self, condition: impl Condition<M>) -> Self {
self.conditions.push(new_condition(condition));
self
}
Expand All @@ -274,7 +274,7 @@ impl IntoSystemSetConfig for SystemSetConfig {
///
/// This has been implemented for boxed [`System<In=(), Out=()>`](crate::system::System)
/// trait objects and all functions that turn into such.
pub trait IntoSystemConfig<Params>: sealed::IntoSystemConfig<Params> {
pub trait IntoSystemConfig<Marker>: sealed::IntoSystemConfig<Marker> {
/// Convert into a [`SystemConfig`].
#[doc(hidden)]
fn into_config(self) -> SystemConfig;
Expand All @@ -294,7 +294,7 @@ pub trait IntoSystemConfig<Params>: sealed::IntoSystemConfig<Params> {
///
/// The `Condition` will be evaluated at most once (per schedule run),
/// when the system prepares to run.
fn run_if<P>(self, condition: impl Condition<P>) -> SystemConfig;
fn run_if<M>(self, condition: impl Condition<M>) -> SystemConfig;
/// Suppress warnings and errors that would result from this system having ambiguities
/// (conflicting access but indeterminate order) with systems in `set`.
fn ambiguous_with<M>(self, set: impl IntoSystemSet<M>) -> SystemConfig;
Expand All @@ -303,9 +303,9 @@ pub trait IntoSystemConfig<Params>: sealed::IntoSystemConfig<Params> {
fn ambiguous_with_all(self) -> SystemConfig;
}

impl<Params, F> IntoSystemConfig<Params> for F
impl<Marker, F> IntoSystemConfig<Marker> for F
where
F: IntoSystem<(), (), Params> + sealed::IntoSystemConfig<Params>,
F: IntoSystem<(), (), Marker> + sealed::IntoSystemConfig<Marker>,
{
fn into_config(self) -> SystemConfig {
SystemConfig::new(Box::new(IntoSystem::into_system(self)))
Expand Down Expand Up @@ -333,7 +333,7 @@ where
self.into_config().after(set)
}

fn run_if<P>(self, condition: impl Condition<P>) -> SystemConfig {
fn run_if<M>(self, condition: impl Condition<M>) -> SystemConfig {
self.into_config().run_if(condition)
}

Expand Down Expand Up @@ -373,7 +373,7 @@ impl IntoSystemConfig<()> for BoxedSystem<(), ()> {
self.into_config().after(set)
}

fn run_if<P>(self, condition: impl Condition<P>) -> SystemConfig {
fn run_if<M>(self, condition: impl Condition<M>) -> SystemConfig {
self.into_config().run_if(condition)
}

Expand Down Expand Up @@ -440,7 +440,7 @@ impl IntoSystemConfig<()> for SystemConfig {
self
}

fn run_if<P>(mut self, condition: impl Condition<P>) -> Self {
fn run_if<M>(mut self, condition: impl Condition<M>) -> Self {
self.conditions.push(new_condition(condition));
self
}
Expand All @@ -465,9 +465,9 @@ mod sealed {

use super::{SystemConfig, SystemSetConfig};

pub trait IntoSystemConfig<Params> {}
pub trait IntoSystemConfig<Marker> {}

impl<Params, F: IntoSystem<(), (), Params>> IntoSystemConfig<Params> for F {}
impl<Marker, F: IntoSystem<(), (), Marker>> IntoSystemConfig<Marker> for F {}

impl IntoSystemConfig<()> for BoxedSystem<(), ()> {}

Expand All @@ -490,7 +490,7 @@ pub struct SystemConfigs {
}

/// Types that can convert into a [`SystemConfigs`].
pub trait IntoSystemConfigs<Params>
pub trait IntoSystemConfigs<Marker>
where
Self: Sized,
{
Expand Down Expand Up @@ -550,7 +550,7 @@ where
/// Use [`run_if`](IntoSystemSetConfig::run_if) on a [`SystemSet`] if you want to make sure
/// that either all or none of the systems are run, or you don't want to evaluate the run
/// condition for each contained system separately.
fn distributive_run_if<P>(self, condition: impl Condition<P> + Clone) -> SystemConfigs {
fn distributive_run_if<M>(self, condition: impl Condition<M> + Clone) -> SystemConfigs {
self.into_configs().distributive_run_if(condition)
}

Expand Down Expand Up @@ -637,7 +637,7 @@ impl IntoSystemConfigs<()> for SystemConfigs {
self
}

fn distributive_run_if<P>(mut self, condition: impl Condition<P> + Clone) -> SystemConfigs {
fn distributive_run_if<M>(mut self, condition: impl Condition<M> + Clone) -> SystemConfigs {
for config in &mut self.systems {
config.conditions.push(new_condition(condition.clone()));
}
Expand Down
12 changes: 6 additions & 6 deletions crates/bevy_ecs/src/schedule/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,13 +190,13 @@ impl Schedule {
}

/// Add a system to the schedule.
pub fn add_system<P>(&mut self, system: impl IntoSystemConfig<P>) -> &mut Self {
pub fn add_system<M>(&mut self, system: impl IntoSystemConfig<M>) -> &mut Self {
self.graph.add_system(system);
self
}

/// Add a collection of systems to the schedule.
pub fn add_systems<P>(&mut self, systems: impl IntoSystemConfigs<P>) -> &mut Self {
pub fn add_systems<M>(&mut self, systems: impl IntoSystemConfigs<M>) -> &mut Self {
self.graph.add_systems(systems);
self
}
Expand Down Expand Up @@ -556,7 +556,7 @@ impl ScheduleGraph {
&self.conflicting_systems
}

fn add_systems<P>(&mut self, systems: impl IntoSystemConfigs<P>) {
fn add_systems<M>(&mut self, systems: impl IntoSystemConfigs<M>) {
let SystemConfigs { systems, chained } = systems.into_configs();
let mut system_iter = systems.into_iter();
if chained {
Expand All @@ -574,13 +574,13 @@ impl ScheduleGraph {
}
}

fn add_system<P>(&mut self, system: impl IntoSystemConfig<P>) {
fn add_system<M>(&mut self, system: impl IntoSystemConfig<M>) {
self.add_system_inner(system).unwrap();
}

fn add_system_inner<P>(
fn add_system_inner<M>(
&mut self,
system: impl IntoSystemConfig<P>,
system: impl IntoSystemConfig<M>,
) -> Result<NodeId, ScheduleBuildError> {
let SystemConfig {
system,
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ impl<Param: SystemParam> FromWorld for SystemState<Param> {
// This trait has to be generic because we have potentially overlapping impls, in particular
// because Rust thinks a type could impl multiple different `FnMut` combinations
// even though none can currently
pub trait IntoSystem<In, Out, Params>: Sized {
pub trait IntoSystem<In, Out, Marker>: Sized {
type System: System<In = In, Out = Out>;
/// Turns this value into its corresponding [`System`].
fn into_system(this: Self) -> Self::System;
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_ecs/src/system/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ pub use system_piping::*;
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
pub fn assert_is_system<In, Out, Params, S: IntoSystem<In, Out, Params>>(sys: S) {
pub fn assert_is_system<In, Out, Marker, S: IntoSystem<In, Out, Marker>>(sys: S) {
if false {
// Check it can be converted into a system
// TODO: This should ensure that the system has no conflicting system params
Expand All @@ -137,7 +137,7 @@ pub fn assert_is_system<In, Out, Params, S: IntoSystem<In, Out, Params>>(sys: S)
/// This should be used when writing doc examples,
/// to confirm that systems used in an example are
/// valid systems.
pub fn assert_is_read_only_system<In, Out, Params, S: IntoSystem<In, Out, Params>>(sys: S)
pub fn assert_is_read_only_system<In, Out, Marker, S: IntoSystem<In, Out, Marker>>(sys: S)
where
S::System: ReadOnlySystem,
{
Expand Down Expand Up @@ -208,7 +208,7 @@ mod tests {
system.run((), &mut world);
}

fn run_system<Param, S: IntoSystem<(), (), Param>>(world: &mut World, system: S) {
fn run_system<Marker, S: IntoSystem<(), (), Marker>>(world: &mut World, system: S) {
let mut schedule = Schedule::default();
schedule.add_system(system);
schedule.run(world);
Expand Down Expand Up @@ -475,7 +475,7 @@ mod tests {
_buffer: Vec<u8>,
}

fn test_for_conflicting_resources<Param, S: IntoSystem<(), (), Param>>(sys: S) {
fn test_for_conflicting_resources<Marker, S: IntoSystem<(), (), Marker>>(sys: S) {
let mut world = World::default();
world.insert_resource(BufferRes::default());
world.insert_resource(A);
Expand Down

0 comments on commit ee4c8c5

Please sign in to comment.