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

Conversation

TheRawMeatball
Copy link
Member

Objective

Make it easier to have stateful exclusive systems

Solution

Add Local support to exclusive systems.

Note: this PR is a follow up to #3945, and while it's not strictly required, it's highly recommended, and this PR is branched off of it.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 14, 2022
@TheRawMeatball TheRawMeatball added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Feb 14, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I like this functionality a great deal: it opens up some very useful patterns (particularly around storing system state and schedules). Code quality looks good, but this needs a test or two to verify that it works.

I would also like a doc test to demonstrate this, probably on the ExclusiveSystem trait where it's most discoverable.

P.S. This should play nicely with the plans to make exclusive systems less special in Stageless; although I suspect we'll be able to cut a lot of the code needed here.

/// 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
/// 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.

/// 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.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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.

mut part_one_state: Local<SystemState<(SRes<u32>, SCommands)>>,
mut part_two_state: Local<SystemState<SQuery<Read<MyComponent>>>>,
) {
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 🤔

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Took a bit to follow the macro, but otherwise a rather succinct and useful change here.
Running into this when implementing animation exclusive systems that needs query caching.

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 4, 2022
}

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

};
}

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

@TheRawMeatball
Copy link
Member Author

Closing in favor of #4166

bors bot pushed a commit that referenced this pull request Sep 26, 2022
…arams (#6083)

# Objective

The [Stageless RFC](bevyengine/rfcs#45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move.

This is an alternative to #4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns).

## Solution

This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). 

This means you can remove all cases of `exclusive_system()`:

```rust
// before
commands.add_system(some_system.exclusive_system());
// after
commands.add_system(some_system);
```

I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems:

```rust
fn some_exclusive_system(
    world: &mut World,
    transforms: &mut QueryState<&Transform>,
    state: &mut SystemState<(Res<Time>, Query<&Player>)>,
) {
    for transform in transforms.iter(world) {
        println!("{transform:?}");
    }
    let (time, players) = state.get(world);
    for player in players.iter() {
        println!("{player:?}");
    }
}
```

Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system.

I added some targeted SystemParam `static` constraints, which removed the need for this:
``` rust
fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {}
```

## Related

- #2923
- #3001
- #3946

## Changelog

- `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait.
- `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems
- `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam`
- Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api.

## Migration Guide

Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems:

```rust
// Old (0.8)
app.add_system(some_exclusive_system.exclusive_system());
// New (0.9)
app.add_system(some_exclusive_system);
```

Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis:

```rust
// Old (0.8)
app.add_system(some_system.exclusive_system().at_end());
// New (0.9)
app.add_system(some_system.at_end());
```

Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons:
```rust
// Old (0.8)
fn some_system(world: &mut World) {
  let mut transforms = world.query::<&Transform>();
  for transform in transforms.iter(world) {
  }
}
// New (0.9)
fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) {
  for transform in transforms.iter(world) {
  }
}
```
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
…arams (bevyengine#6083)

# Objective

The [Stageless RFC](bevyengine/rfcs#45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move.

This is an alternative to bevyengine#4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns).

## Solution

This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). 

This means you can remove all cases of `exclusive_system()`:

```rust
// before
commands.add_system(some_system.exclusive_system());
// after
commands.add_system(some_system);
```

I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems:

```rust
fn some_exclusive_system(
    world: &mut World,
    transforms: &mut QueryState<&Transform>,
    state: &mut SystemState<(Res<Time>, Query<&Player>)>,
) {
    for transform in transforms.iter(world) {
        println!("{transform:?}");
    }
    let (time, players) = state.get(world);
    for player in players.iter() {
        println!("{player:?}");
    }
}
```

Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system.

I added some targeted SystemParam `static` constraints, which removed the need for this:
``` rust
fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {}
```

## Related

- bevyengine#2923
- bevyengine#3001
- bevyengine#3946

## Changelog

- `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait.
- `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems
- `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam`
- Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api.

## Migration Guide

Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems:

```rust
// Old (0.8)
app.add_system(some_exclusive_system.exclusive_system());
// New (0.9)
app.add_system(some_exclusive_system);
```

Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis:

```rust
// Old (0.8)
app.add_system(some_system.exclusive_system().at_end());
// New (0.9)
app.add_system(some_system.at_end());
```

Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons:
```rust
// Old (0.8)
fn some_system(world: &mut World) {
  let mut transforms = world.query::<&Transform>();
  for transform in transforms.iter(world) {
  }
}
// New (0.9)
fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) {
  for transform in transforms.iter(world) {
  }
}
```
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…arams (bevyengine#6083)

# Objective

The [Stageless RFC](bevyengine/rfcs#45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move.

This is an alternative to bevyengine#4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns).

## Solution

This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). 

This means you can remove all cases of `exclusive_system()`:

```rust
// before
commands.add_system(some_system.exclusive_system());
// after
commands.add_system(some_system);
```

I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems:

```rust
fn some_exclusive_system(
    world: &mut World,
    transforms: &mut QueryState<&Transform>,
    state: &mut SystemState<(Res<Time>, Query<&Player>)>,
) {
    for transform in transforms.iter(world) {
        println!("{transform:?}");
    }
    let (time, players) = state.get(world);
    for player in players.iter() {
        println!("{player:?}");
    }
}
```

Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system.

I added some targeted SystemParam `static` constraints, which removed the need for this:
``` rust
fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {}
```

## Related

- bevyengine#2923
- bevyengine#3001
- bevyengine#3946

## Changelog

- `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait.
- `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems
- `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam`
- Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api.

## Migration Guide

Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems:

```rust
// Old (0.8)
app.add_system(some_exclusive_system.exclusive_system());
// New (0.9)
app.add_system(some_exclusive_system);
```

Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis:

```rust
// Old (0.8)
app.add_system(some_system.exclusive_system().at_end());
// New (0.9)
app.add_system(some_system.at_end());
```

Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons:
```rust
// Old (0.8)
fn some_system(world: &mut World) {
  let mut transforms = world.query::<&Transform>();
  for transform in transforms.iter(world) {
  }
}
// New (0.9)
fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) {
  for transform in transforms.iter(world) {
  }
}
```
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…arams (bevyengine#6083)

# Objective

The [Stageless RFC](bevyengine/rfcs#45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move.

This is an alternative to bevyengine#4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns).

## Solution

This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). 

This means you can remove all cases of `exclusive_system()`:

```rust
// before
commands.add_system(some_system.exclusive_system());
// after
commands.add_system(some_system);
```

I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems:

```rust
fn some_exclusive_system(
    world: &mut World,
    transforms: &mut QueryState<&Transform>,
    state: &mut SystemState<(Res<Time>, Query<&Player>)>,
) {
    for transform in transforms.iter(world) {
        println!("{transform:?}");
    }
    let (time, players) = state.get(world);
    for player in players.iter() {
        println!("{player:?}");
    }
}
```

Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system.

I added some targeted SystemParam `static` constraints, which removed the need for this:
``` rust
fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {}
```

## Related

- bevyengine#2923
- bevyengine#3001
- bevyengine#3946

## Changelog

- `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait.
- `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems
- `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam`
- Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api.

## Migration Guide

Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems:

```rust
// Old (0.8)
app.add_system(some_exclusive_system.exclusive_system());
// New (0.9)
app.add_system(some_exclusive_system);
```

Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis:

```rust
// Old (0.8)
app.add_system(some_system.exclusive_system().at_end());
// New (0.9)
app.add_system(some_system.at_end());
```

Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons:
```rust
// Old (0.8)
fn some_system(world: &mut World) {
  let mut transforms = world.query::<&Transform>();
  for transform in transforms.iter(world) {
  }
}
// New (0.9)
fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) {
  for transform in transforms.iter(world) {
  }
}
```
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants