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.

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

Locals in exclusive systems #3946

Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,10 @@ path = "examples/ecs/component_change_detection.rs"
name = "event"
path = "examples/ecs/event.rs"

[[example]]
name = "exclusive_system_tricks"
path = "examples/ecs/exclusive_system_tricks.rs"

[[example]]
name = "fixed_timestep"
path = "examples/ecs/fixed_timestep.rs"
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/schedule/system_descriptor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ impl IntoSystemDescriptor<()> for ExclusiveSystemDescriptor {
}
}

impl<F> IntoSystemDescriptor<()> for ExclusiveSystemFn<F>
impl<F, P> IntoSystemDescriptor<()> for ExclusiveSystemFn<F, P>
where
F: FnMut(&mut crate::prelude::World) + Send + Sync + 'static,
ExclusiveSystemFn<F, P>: ExclusiveSystem,
{
fn into_descriptor(self) -> SystemDescriptor {
new_exclusive_descriptor(Box::new(self)).into_descriptor()
Expand Down
95 changes: 58 additions & 37 deletions crates/bevy_ecs/src/system/exclusive_system.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use bevy_ecs_macros::all_tuples;

use crate::{
archetype::ArchetypeGeneration,
system::{check_system_change_tick, BoxedSystem, IntoSystem},
prelude::{FromWorld, Local},
system::{check_system_change_tick, BoxedSystem, IntoSystem, Resource},
world::World,
};
use std::borrow::Cow;
Expand All @@ -15,59 +18,77 @@ pub trait ExclusiveSystem: Send + Sync + 'static {
fn check_change_tick(&mut self, change_tick: u32);
}

pub struct ExclusiveSystemFn<F> {
pub struct ExclusiveSystemFn<F, L> {
func: F,
name: Cow<'static, str>,
last_change_tick: u32,
locals: Option<L>,
}

impl<F> ExclusiveSystem for ExclusiveSystemFn<F>
where
F: FnMut(&mut World) + Send + Sync + 'static,
{
fn name(&self) -> Cow<'static, str> {
self.name.clone()
}
macro_rules! impl_exclusive_system {
($($t:ident),*) => {
#[allow(non_snake_case)]
impl<F, $($t: FromWorld + Resource,)*> ExclusiveSystem for ExclusiveSystemFn<F, ($($t,)*)>
where
F: FnMut(&mut World, $(Local<$t>,)*) + Send + Sync + 'static,
{
fn name(&self) -> Cow<'static, str> {
self.name.clone()
}

fn run(&mut self, world: &mut World) {
// The previous value is saved in case this exclusive system is run by another exclusive
// system
let saved_last_tick = world.last_change_tick;
world.last_change_tick = self.last_change_tick;
fn run(&mut self, world: &mut World) {
// The previous value is saved in case this exclusive system is run by another exclusive
// system
let saved_last_tick = world.last_change_tick;
world.last_change_tick = self.last_change_tick;

(self.func)(world);
let ($($t,)*) = self.locals.as_mut().unwrap();
(self.func)(world, $(Local::wrap($t),)*);

let change_tick = world.change_tick.get_mut();
self.last_change_tick = *change_tick;
*change_tick += 1;
let change_tick = world.change_tick.get_mut();
self.last_change_tick = *change_tick;
*change_tick += 1;

world.last_change_tick = saved_last_tick;
}
world.last_change_tick = saved_last_tick;
}

fn initialize(&mut self, _: &mut World) {}
fn initialize(&mut self, _world: &mut World) {
self.locals = Some(($($t::from_world(_world),)*));
}

fn check_change_tick(&mut self, change_tick: u32) {
check_system_change_tick(&mut self.last_change_tick, change_tick, self.name.as_ref());
}
fn check_change_tick(&mut self, change_tick: u32) {
check_system_change_tick(
&mut self.last_change_tick,
change_tick,
self.name.as_ref(),
);
}
}


impl<F, $($t: FromWorld + Resource,)*> IntoExclusiveSystem<&mut World, ExclusiveSystemFn<F, ($($t,)*)>> for F
where
F: FnMut(&mut World, $(Local<$t>,)*) + Send + Sync + 'static,
{
fn exclusive_system(self) -> ExclusiveSystemFn<F, ($($t,)*)> {
ExclusiveSystemFn {
func: self,
name: core::any::type_name::<F>().into(),
last_change_tick: 0,
locals: None,
}
}
}

};
}

all_tuples!(impl_exclusive_system, 0, 12, L);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
all_tuples!(impl_exclusive_system, 0, 12, L);
all_tuples!(impl_exclusive_system, 0, 16, L);

I would rather have all all_tuples work on the same maximum number of items in a tuple, so 16. It would be less surprising for users


pub trait IntoExclusiveSystem<Params, SystemType> {
fn exclusive_system(self) -> SystemType;
}

impl<F> IntoExclusiveSystem<&mut World, ExclusiveSystemFn<F>> for F
where
F: FnMut(&mut World) + Send + Sync + 'static,
{
fn exclusive_system(self) -> ExclusiveSystemFn<F> {
ExclusiveSystemFn {
func: self,
name: core::any::type_name::<F>().into(),
last_change_tick: 0,
}
}
}

pub struct ExclusiveSystemCoerced {
system: BoxedSystem<(), ()>,
archetype_generation: ArchetypeGeneration,
Expand Down
7 changes: 7 additions & 0 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::{
archetype::{Archetype, ArchetypeComponentId, ArchetypeGeneration, ArchetypeId},
component::ComponentId,
prelude::FromWorld,
query::{Access, FilteredAccessSet},
system::{
check_system_change_tick, ReadOnlySystemParamFetch, System, SystemParam, SystemParamFetch,
Expand Down Expand Up @@ -168,6 +169,12 @@ impl<Param: SystemParam> SystemState<Param> {
}
}

impl<Param: SystemParam> FromWorld for SystemState<Param> {
fn from_world(world: &mut World) -> Self {
Self::new(world)
}
}

/// A trait for defining systems with a [`SystemParam`] associated type.
///
/// This facilitates the creation of systems that are generic over some trait
Expand Down
6 changes: 6 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,12 @@ impl<'w, 's> SystemParamFetch<'w, 's> for WorldState {
/// ```
pub struct Local<'a, T: Resource>(&'a mut T);

impl<'a, T: Resource> Local<'a, T> {
pub(crate) fn wrap(x: &'a mut T) -> Self {
Self(x)
}
}

// SAFE: Local only accesses internal state
unsafe impl<T: Resource> ReadOnlySystemParamFetch for LocalState<T> {}

Expand Down
20 changes: 20 additions & 0 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ mod spawn_batch;
mod world_cell;

pub use crate::change_detection::Mut;
use bevy_ecs_macros::all_tuples;
pub use entity_ref::*;
pub use spawn_batch::*;
pub use world_cell::*;
Expand Down Expand Up @@ -1224,6 +1225,25 @@ impl<T: Default> FromWorld for T {
}
}

/// Wrapper type to enable getting multiple [`FromWorld`] types in one type.
///
/// This wrapper is necessary because a [`FromWorld`] implementation for a tuple of
/// [`FromWorld`] implementing types would conflict with the [`FromWorld`] impl over all
/// [`Default`] type.
pub struct FromWorldWrap<T>(pub T);

macro_rules! impl_from_world {
($($t:ident),*) => {
impl<$($t: FromWorld,)*> FromWorld for FromWorldWrap<($($t,)*)> {
fn from_world(_world: &mut World) -> Self {
FromWorldWrap(($($t::from_world(_world),)*))
}
}
};
}

all_tuples!(impl_from_world, 0, 12, T);
Copy link
Member

Choose a reason for hiding this comment

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

same


struct MainThreadValidator {
main_thread: std::thread::ThreadId,
}
Expand Down
1 change: 1 addition & 0 deletions examples/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ Example | File | Description
`ecs_guide` | [`ecs/ecs_guide.rs`](./ecs/ecs_guide.rs) | Full guide to Bevy's ECS
`component_change_detection` | [`ecs/component_change_detection.rs`](./ecs/component_change_detection.rs) | Change detection on components
`event` | [`ecs/event.rs`](./ecs/event.rs) | Illustrates event creation, activation, and reception
`exclusive_system_tricks` | [`ecs/exclusive_system_tricks.rs`](./ecs/exclusive_system_tricks.rs) | A basic guide to using exclusive systems and exclusive world access.
`fixed_timestep` | [`ecs/fixed_timestep.rs`](./ecs/fixed_timestep.rs) | Shows how to create systems that run every fixed timestep, rather than every tick
`generic_system` | [`ecs/generic_system.rs`](./ecs/generic_system.rs) | Shows how to create systems that can be reused with different types
`hierarchy` | [`ecs/hierarchy.rs`](./ecs/hierarchy.rs) | Creates a hierarchy of parents and children entities
Expand Down
57 changes: 57 additions & 0 deletions examples/ecs/exclusive_system_tricks.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
use bevy::{
ecs::system::{lifetimeless::*, SystemState},
prelude::*,
};

fn main() {
App::new()
.init_resource::<MyCustomSchedule>()
.insert_resource(5u32)
Copy link
Member

Choose a reason for hiding this comment

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

Use wrapper types, even in examples.

.add_system(simple_exclusive_system.exclusive_system())
.add_system(stateful_exclusive_system.exclusive_system())
.run();
}

#[derive(Default)]
struct MyCustomSchedule(Schedule);

#[derive(Component)]
struct MyComponent;

/// Just a simple exclusive system - this function will run with mutable access to
/// the main app world. This lets it call into other schedules, or modify and query the
TheRawMeatball marked this conversation as resolved.
Show resolved Hide resolved
/// world in hard-to-predict ways, which makes it a powerful primitive. However, because
/// this is usually not needed, and because such wide access makes parallelism impossible,
Copy link
Member

Choose a reason for hiding this comment

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

Explain why not -> should be avoided when not needed.

/// it should generally be avoided.
fn simple_exclusive_system(world: &mut World) {
world.resource_scope(|world, mut my_schedule: Mut<MyCustomSchedule>| {
// The resource_scope method is one of the main tools for working with &mut World.
// This method will temporarily remove the resource from the ECS world and let you
// access it while still keeping &mut World. A particularly popular pattern is storing
// schedules, stages, and other similar "runnables" in the world, taking them out
// using resource_scope, and running them with the world:
my_schedule.0.run(world);
// This is fairly simple, but you can implement rather complex custom executors in this manner.
});
}

/// While it's usually not recommended due to parallelism concerns, you can also use exclusive systems
/// as mostly-normal systems but with the ability to change parameter sets and flush commands midway through.
fn stateful_exclusive_system(
world: &mut World,
mut part_one_state: Local<SystemState<(SRes<u32>, SCommands)>>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mut part_one_state: Local<SystemState<(SRes<u32>, SCommands)>>,
mut part_one_state: Local<SystemState<(Res<u32>, Commands)>>,

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the SystemState API needs the lifetimeless versions.

Copy link
Member

Choose a reason for hiding this comment

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

Oh no. Please leave a comment explaining this then.

That surprises me a lot though; I request standard Res through the SystemState API in exclusive systems all the time.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the problem is the systemstate api; it's that the Locals value must (correctly) be 'static, but a type with non-'static type paramaters cannot itself be `'static' (even though the lifetime cannot be used. I run into a similar issue in #3817.

That is, the issue is Local not SystemState; and the issue in Local is 'due to' our SystemParam design. I don't think this is avoidable.

mut part_two_state: Local<SystemState<SQuery<Read<MyComponent>>>>,
TheRawMeatball marked this conversation as resolved.
Show resolved Hide resolved
) {
let (resource, mut commands) = part_one_state.get(world);
Copy link
Member

Choose a reason for hiding this comment

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

Oh fascinating: this "just works" without initial values because of the FromWorld impl 🤔

let res = *resource as usize;
commands.spawn_batch((0..res).map(|_| (MyComponent,)));

// Don't forget to apply your state, or commands won't take effect!
part_one_state.apply(world);
let query = part_two_state.get(world);
let entity_count = query.iter().len();
// note how the entities spawned in this system are observed,
// and how resources fetched in earlier stages can still be
// used if they're cloned out, or small enough to copy out.
assert_eq!(entity_count, res);
}