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

[Merged by Bors] - Add new SystemState and rename old SystemState to SystemMeta #2283

Closed
wants to merge 6 commits into from

Conversation

cart
Copy link
Member

@cart cart commented May 31, 2021

This enables SystemParams to be used outside of function systems. Anything can create and store SystemState, which enables efficient "param state cached" access to SystemParams.

It adds a ReadOnlySystemParamFetch trait, which enables safe SystemState::get calls without unique world access.

I renamed the old SystemState to SystemMeta to enable us to mirror the QueryState naming convention (but I'm happy to discuss alternative names if people have other ideas). I initially pitched this as ParamState, but given that it needs to include full system metadata, that doesn't feel like a particularly accurate name.

#[derive(Eq, PartialEq, Debug)]
struct A(usize);

#[derive(Eq, PartialEq, Debug)]
struct B(usize);

let mut world = World::default();
world.insert_resource(A(42));
world.spawn().insert(B(7));

// we get nice lifetime elision when declaring the type on the left hand side
let mut system_state: SystemState<(Res<A>, Query<&B>)> = SystemState::new(&mut world);
let (a, query) = system_state.get(&world);
assert_eq!(*a, A(42), "returned resource matches initial value");
assert_eq!(
    *query.single().unwrap(),
    B(7),
    "returned component matches initial value"
);

// mutable system params require unique world access
let mut system_state: SystemState<(ResMut<A>, Query<&mut B>)> = SystemState::new(&mut world);
let (a, query) = system_state.get_mut(&mut world);

// static lifetimes are required when declaring inside of structs
struct SomeContainer {
  state: SystemState<(Res<'static, A>, Res<'static, B>)>
}

// this can be shortened using type aliases, which will be useful for complex param tuples
type MyParams<'a> = (Res<'a, A>, Res<'a, B>);
struct SomeContainer {
  state: SystemState<MyParams<'static>>
}

// It is the user's responsibility to call SystemState::apply(world) for parameters that queue up work   
let mut system_state: SystemState<(Commands, Query<&B>)> = SystemState::new(&mut world);
{
  let (mut commands, query) = system_state.get(&world);
  commands.insert_resource(3.14);
}
system_state.apply(&mut world);

Future Work

  • Actually use SystemState inside FunctionSystem. This would be trivial, but it requires FunctionSystem to wrap SystemState in Option in its current form (which complicates system metadata lookup). I'd prefer to hold off until we adopt something like the later designs linked in [Slightly less WIP] Design SystemParams to hold state #1364, which enable us to contruct Systems using a World reference (and also remove the need for .system).
  • Consider a "scoped" approach to automatically call SystemState::apply when systems params are no longer being used (either a container type with a Drop impl, or a function that takes a closure for user logic operating on params).

@cart cart added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels May 31, 2021
@cart
Copy link
Member Author

cart commented Jun 1, 2021

Alrighty this should be good to go now.

@tiagolam
Copy link
Contributor

tiagolam commented Jun 1, 2021

Trying to understand this change with regards to what you've proposed in #2089 (I was working out similar changes as part of that), and this seems to largely address that. This makes it trivial now to address that issue, simply by adding a new World system_params method that constructs and returns this new SystemState. Is this what you had in mind?

Your point in "Future Work" around reusing this in the FunctionSystem would also go towards the same comment on #2089.

@cart
Copy link
Member Author

cart commented Jun 2, 2021

Hmm yeah I see no harm in adding that now. In fact: we'll want to do that now to ensure "world id safety" like we do in QueryState. Otherwise people can pass multiple worlds into the same SystemState, which is unlikely, but still very unsafe.

@cart
Copy link
Member Author

cart commented Jun 2, 2021

Arg maybe it shouldn't live on world. Systems are an abstraction built "on top" of World. It feels a bit dirty to include it there (and we can have world id safety without it).

Its not really much of an ergonomics win anyway:

let mut system_state = SystemState::<Query<&A>>::new(&mut world);
let mut system_state = world.system_state::<Query<&A>>();

@cart
Copy link
Member Author

cart commented Jun 2, 2021

Ooh missed archetype updates too. Adding a test + fix now :)

@tiagolam
Copy link
Contributor

tiagolam commented Jun 2, 2021

Arg maybe it shouldn't live on world. Systems are an abstraction built "on top" of World. It feels a bit dirty to include it there (and we can have world id safety without it).

Its not really much of an ergonomics win anyway:

let mut system_state = SystemState::<Query<&A>>::new(&mut world);
let mut system_state = world.system_state::<Query<&A>>();

Yeah, seems sensible. This (breaking vs keeping this abstraction) was confusing me as I was delving into System and FunctionSystem, as it didn't fit in like the QueryState case.

@cart
Copy link
Member Author

cart commented Jun 2, 2021

Alrighty now I think this is good to go.

@NathanSWard
Copy link
Contributor

Just as a heads up, I don't feel too comfortable with the ECS implementation (yet) to comfortably approve an ECS PR that updates this much internals.
I requested a few other people who (I think) are more familiar with the area to approve :)

@cart
Copy link
Member Author

cart commented Jun 2, 2021

bors r+

bors bot pushed a commit that referenced this pull request Jun 2, 2021
This enables `SystemParams` to be used outside of function systems. Anything can create and store `SystemState`, which enables efficient "param state cached" access to `SystemParams`.

It adds a `ReadOnlySystemParamFetch` trait, which enables safe `SystemState::get` calls without unique world access.

I renamed the old `SystemState` to `SystemMeta` to enable us to mirror the `QueryState` naming convention (but I'm happy to discuss alternative names if people have other ideas). I initially pitched this as `ParamState`, but given that it needs to include full system metadata, that doesn't feel like a particularly accurate name.

```rust
#[derive(Eq, PartialEq, Debug)]
struct A(usize);

#[derive(Eq, PartialEq, Debug)]
struct B(usize);

let mut world = World::default();
world.insert_resource(A(42));
world.spawn().insert(B(7));

// we get nice lifetime elision when declaring the type on the left hand side
let mut system_state: SystemState<(Res<A>, Query<&B>)> = SystemState::new(&mut world);
let (a, query) = system_state.get(&world);
assert_eq!(*a, A(42), "returned resource matches initial value");
assert_eq!(
    *query.single().unwrap(),
    B(7),
    "returned component matches initial value"
);

// mutable system params require unique world access
let mut system_state: SystemState<(ResMut<A>, Query<&mut B>)> = SystemState::new(&mut world);
let (a, query) = system_state.get_mut(&mut world);

// static lifetimes are required when declaring inside of structs
struct SomeContainer {
  state: SystemState<(Res<'static, A>, Res<'static, B>)>
}

// this can be shortened using type aliases, which will be useful for complex param tuples
type MyParams<'a> = (Res<'a, A>, Res<'a, B>);
struct SomeContainer {
  state: SystemState<MyParams<'static>>
}

// It is the user's responsibility to call SystemState::apply(world) for parameters that queue up work   
let mut system_state: SystemState<(Commands, Query<&B>)> = SystemState::new(&mut world);
{
  let (mut commands, query) = system_state.get(&world);
  commands.insert_resource(3.14);
}
system_state.apply(&mut world);
```

## Future Work

* Actually use SystemState inside FunctionSystem. This would be trivial, but it requires FunctionSystem to wrap SystemState in Option in its current form (which complicates system metadata lookup). I'd prefer to hold off until we adopt something like the later designs linked in #1364, which enable us to contruct Systems using a World reference (and also remove the need for `.system`).
* Consider a "scoped" approach to automatically call SystemState::apply when systems params are no longer being used (either a container type with a Drop impl, or a function that takes a closure for user logic operating on params).
@bors bors bot changed the title Add new SystemState and rename old SystemState to SystemMeta [Merged by Bors] - Add new SystemState and rename old SystemState to SystemMeta Jun 2, 2021
@bors bors bot closed this Jun 2, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
…ine#2283)

This enables `SystemParams` to be used outside of function systems. Anything can create and store `SystemState`, which enables efficient "param state cached" access to `SystemParams`.

It adds a `ReadOnlySystemParamFetch` trait, which enables safe `SystemState::get` calls without unique world access.

I renamed the old `SystemState` to `SystemMeta` to enable us to mirror the `QueryState` naming convention (but I'm happy to discuss alternative names if people have other ideas). I initially pitched this as `ParamState`, but given that it needs to include full system metadata, that doesn't feel like a particularly accurate name.

```rust
#[derive(Eq, PartialEq, Debug)]
struct A(usize);

#[derive(Eq, PartialEq, Debug)]
struct B(usize);

let mut world = World::default();
world.insert_resource(A(42));
world.spawn().insert(B(7));

// we get nice lifetime elision when declaring the type on the left hand side
let mut system_state: SystemState<(Res<A>, Query<&B>)> = SystemState::new(&mut world);
let (a, query) = system_state.get(&world);
assert_eq!(*a, A(42), "returned resource matches initial value");
assert_eq!(
    *query.single().unwrap(),
    B(7),
    "returned component matches initial value"
);

// mutable system params require unique world access
let mut system_state: SystemState<(ResMut<A>, Query<&mut B>)> = SystemState::new(&mut world);
let (a, query) = system_state.get_mut(&mut world);

// static lifetimes are required when declaring inside of structs
struct SomeContainer {
  state: SystemState<(Res<'static, A>, Res<'static, B>)>
}

// this can be shortened using type aliases, which will be useful for complex param tuples
type MyParams<'a> = (Res<'a, A>, Res<'a, B>);
struct SomeContainer {
  state: SystemState<MyParams<'static>>
}

// It is the user's responsibility to call SystemState::apply(world) for parameters that queue up work   
let mut system_state: SystemState<(Commands, Query<&B>)> = SystemState::new(&mut world);
{
  let (mut commands, query) = system_state.get(&world);
  commands.insert_resource(3.14);
}
system_state.apply(&mut world);
```

## Future Work

* Actually use SystemState inside FunctionSystem. This would be trivial, but it requires FunctionSystem to wrap SystemState in Option in its current form (which complicates system metadata lookup). I'd prefer to hold off until we adopt something like the later designs linked in bevyengine#1364, which enable us to contruct Systems using a World reference (and also remove the need for `.system`).
* Consider a "scoped" approach to automatically call SystemState::apply when systems params are no longer being used (either a container type with a Drop impl, or a function that takes a closure for user logic operating on params).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants