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

Jump to bottom

[Merged by Bors] - Make RunOnce a non-manual System impl #3922

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Make RunOnce a non-manual System impl #3922

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
74f979b
Systemyeet in run criteria on stages
DJMcNab Feb 12, 2022
cdfb980
Make RunOnce not a manual system implementation
DJMcNab Feb 12, 2022
2f8ae8e
Rename to `ShouldRun::once`
DJMcNab Apr 9, 2022
5548e56
Add docs
DJMcNab Apr 25, 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
4 changes: 2 additions & 2 deletions crates/bevy_app/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use bevy_ecs::{
event::Events,
prelude::{FromWorld, IntoExclusiveSystem},
schedule::{
IntoSystemDescriptor, RunOnce, Schedule, Stage, StageLabel, State, StateData, SystemSet,
IntoSystemDescriptor, Schedule, ShouldRun, Stage, StageLabel, State, StateData, SystemSet,
SystemStage,
},
system::Resource,
Expand Down Expand Up @@ -591,7 +591,7 @@ impl App {
.add_stage(
StartupSchedule,
Schedule::default()
.with_run_criteria(RunOnce::default())
.with_run_criteria(ShouldRun::once)
.with_stage(StartupStage::PreStartup, SystemStage::parallel())
.with_stage(StartupStage::Startup, SystemStage::parallel())
.with_stage(StartupStage::PostStartup, SystemStage::parallel()),
Expand Down
12 changes: 5 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 @@ -27,7 +27,7 @@ pub use system_set::*;

use std::fmt::Debug;

use crate::{system::System, world::World};
use crate::{system::IntoSystem, world::World};
use bevy_utils::HashMap;

/// A container of [`Stage`]s set to be run in a linear order.
Expand Down Expand Up @@ -76,7 +76,7 @@ impl Schedule {
}

#[must_use]
pub fn with_run_criteria<S: System<In = (), Out = ShouldRun>>(mut self, system: S) -> Self {
pub fn with_run_criteria<S: IntoSystem<(), ShouldRun, P>, P>(mut self, system: S) -> Self {
self.set_run_criteria(system);
self
}
Expand All @@ -92,11 +92,9 @@ impl Schedule {
self
}

pub fn set_run_criteria<S: System<In = (), Out = ShouldRun>>(
&mut self,
system: S,
) -> &mut Self {
self.run_criteria.set(Box::new(system));
pub fn set_run_criteria<S: IntoSystem<(), ShouldRun, P>, P>(&mut self, system: S) -> &mut Self {
self.run_criteria
.set(Box::new(IntoSystem::into_system(system)));
self
}

Expand Down
65 changes: 16 additions & 49 deletions crates/bevy_ecs/src/schedule/run_criteria.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
use crate::{
archetype::ArchetypeComponentId,
component::ComponentId,
query::Access,
schedule::{BoxedRunCriteriaLabel, GraphNode, RunCriteriaLabel},
system::{BoxedSystem, IntoSystem, System},
system::{BoxedSystem, IntoSystem, Local},
world::World,
};
use std::borrow::Cow;
Expand Down Expand Up @@ -44,6 +41,21 @@ pub enum ShouldRun {
NoAndCheckAgain,
}

impl ShouldRun {
/// A run criterion which returns [`ShouldRun::Yes`] exactly once.
///
/// This leads to the systems controlled by it only being
/// executed one time only.
pub fn once(mut ran: Local<bool>) -> ShouldRun {
Copy link
Member

@cart cart Apr 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ooh this is interesting. One more small consistency thing to discuss api on:

This uses "normal system" add-using-function-name syntax add_system(foo.with_run_criteria(ShouldRun::once)), which has the benefit of being exactly what you would do for a "non-closure system".

However if were were to add a theoretical ShouldRun::n_times, it would need to return a closure to store the value of n.

So we would have the inconsistent styles:

app.
  .add_system(foo.with_run_criteria(ShouldRun::once))
  .add_system(foo.with_run_criteria(ShouldRun::n_times(42))

So the question is: is this ok? (this is not a loaded question ... i dont have strong opinions here). When using the "functions on structs" pattern for systems, what are our "rules"?
Should they be:

  1. When using struct-functions, always use functions returning impl Fn/FnMut for consistency
  2. When using struct-functions, prefer "normal system" style when possible and return impl Fn/FnMut only when required by the context.
  3. When using struct-functions, prefer "normal system" style when possible, but prefer consistency across functions on the struct if there is a mismatch.
  4. Use normal_system when possible. Only use struct-functions for closure-style systems.

(3) (4) (edit: accidentally referenced the wrong item here ... see my correction below) loses the organizational benefits of struct style (which would be very confusing), so thats the only option I'm actively against.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Use const generics. ShouldRun::n_times::<{42}>.

I'd generally lean towards option 2. I think we need to do the same migration for FixedTimestep, but I hadn't done it yet because of stageless.

Copy link
Member

Choose a reason for hiding this comment

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

My preference is 4: I think they're clearer in general (especially in the docs), and we should have strong reasons to use struct-functions.

3 is definitely bad.

Copy link
Member

Choose a reason for hiding this comment

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

Arg sorry I inserted an extra list item, invalidating my reference to (3). Sorry for the confusion (bad cart!). I meant to say:

(4) loses the organizational benefits of struct style (which would be very confusing), so thats the only option I'm actively against.

Copy link
Member

Choose a reason for hiding this comment

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

I think this would be actively bad UX:

app
  .add_system(foo.with_run_criteria(should_run_once)
  .add_system(foo.with_run_criteria(ShouldRun::n_times(42))

Copy link
Member

Choose a reason for hiding this comment

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

Use const generics. ShouldRun::n_times::<{42}>.

Good point. There are cases where this would significantly increase the amount of internal statefullness / Local hackery required (ex: a timer that ticks down from 42 rather than up from 0 would need to have an init phase enforced with locals). But it would have the benefit of not using function calls, so its worth considering. It feels like there might be cases where closures are still required though (ex: the input is set at runtime).

Copy link
Member Author

@DJMcNab DJMcNab Apr 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This discussion shouldn't block this PR landing, right?

I think in a lot of cases, we can avoid having systems being externally configurable. At some point, we'll probably want something Local-like initialised from a const parameter. Not sure if that's possible at the moment.

My preference is option 2. Option 2 keeps the rules for struct scoped systems the same as module scoped systems.
It's worth noting that as far as I know, the _system suffix is idiomatic for functions which return system functions, which applies both when struct scoped and module scoped.

Edit: Change from option 3 to option 2, since I got it wrong.

Copy link
Member

Choose a reason for hiding this comment

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

This discussion shouldn't block this PR landing, right?

Only in that it would be a breaking change to switch from the current style to a "return FnMut for everything on a struct" style. I do think "only use FnMut when you need it" is pretty consistent with what we do today (there is already precedent for the current pattern in bevy).

So yeah Option 2 seems like a reasonable thing to default to. Lets roll with that and we can revisit later if we feel inclined.

if *ran {
ShouldRun::No
} else {
*ran = true;
ShouldRun::Yes
}
}
}

#[derive(Default)]
pub(crate) struct BoxedRunCriteria {
criteria_system: Option<BoxedSystem<(), ShouldRun>>,
Expand Down Expand Up @@ -324,48 +336,3 @@ impl RunCriteria {
}
}
}

#[derive(Default)]
pub struct RunOnce {
ran: bool,
archetype_component_access: Access<ArchetypeComponentId>,
component_access: Access<ComponentId>,
}

impl System for RunOnce {
type In = ();
type Out = ShouldRun;

fn name(&self) -> Cow<'static, str> {
Cow::Borrowed(std::any::type_name::<RunOnce>())
}

fn component_access(&self) -> &Access<ComponentId> {
&self.component_access
}

fn archetype_component_access(&self) -> &Access<ArchetypeComponentId> {
&self.archetype_component_access
}

fn is_send(&self) -> bool {
true
}

unsafe fn run_unsafe(&mut self, _input: (), _world: &World) -> ShouldRun {
if self.ran {
ShouldRun::No
} else {
self.ran = true;
ShouldRun::Yes
}
}

fn apply_buffers(&mut self, _world: &mut World) {}

fn initialize(&mut self, _world: &mut World) {}

fn update_archetype_component_access(&mut self, _world: &World) {}

fn check_change_tick(&mut self, _change_tick: u32) {}
}