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

Add &mut World as a system param and make .exclusive_system() optional #4166

Closed
wants to merge 3 commits into from

Conversation

maniwani
Copy link
Contributor

@maniwani maniwani commented Mar 9, 2022

Objective

Solution

  • add SemiSafeCell<'w, World> MaybeUnsafeCell<'w, World> and have it replace &'w World in internal access methods
  • add new required method on SystemParamState so we can (mostly) statically ensure soundness and detect exclusive systems
  • add &mut World as a system param
  • remove ExclusiveSystem* types and traits
  • mark .exclusive_system() method as deprecated (we might need it later to coerce systems into being exclusive)

Related

@maniwani maniwani changed the title yeet .exclusive_system() yeet .exclusive_system() and ExclusiveSystem* Mar 9, 2022
@maniwani maniwani changed the title yeet .exclusive_system() and ExclusiveSystem* yeet .exclusive_system() and ExclusiveSystem* types Mar 9, 2022
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Mar 9, 2022
@alice-i-cecile alice-i-cecile self-requested a review March 9, 2022 23:57
@cart
Copy link
Member

cart commented Mar 10, 2022

Pulling in @Ratysz as this split was an intentional design decision.

@cart
Copy link
Member

cart commented Mar 10, 2022

Some historical context: #1144 (comment)

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 10, 2022

For context, this change is part of the groundwork for stageless: it seemed nicely separable, so I encouraged @maniwani to submit it seperately for ease of review.

It would introduce other, far more convoluted kind of complexity: if exclusive systems are systems, they can have dependencies and can be dependencies.

As far as I can tell, this was the primary argument against it. Managing that complexity is where a large amount of the effort behind bevyengine/rfcs#45 was focused: we take on more complexity with respect to managing the ordering of exclusive systems (and command flushing) in order to dramatically reduce complexity elsewhere in the scheduling internals / API / mental model.

Having experimented with (and helped users with) the exclusive system API since that thread, I pretty firmly agree with your initial positions from that thread:

  • From an api perspective, I see no meaningful difference between an "exclusive system" and a system that manually claims every resource.

  • I think preventing people from adding fn system(world: &mut World) {} to a stage "normally" because it happens to also meet the criteria for a stage seems overly restrictive

  • Imo a "function that operates on World and/or Resources" is a "system"

  • I'd still prefer it if systems were "functions that operate on ECS state". In my ideal world parallelism would be the responsibility of executors and would be a "best effort" sort of thing.

I would also add:

  • the added complexity dramatically increases pain when working with scheduler related code
  • multiple insertion points for exclusive systems is largely unused
  • the inability to order exclusive and non-exclusive systems is often frustrating
  • the concern raised in #1021 is handled by tools to handle execution order ambiguities (and other parts of the stageless design that give users explicit control over command flushing): these are rapidly improving
  • initial feedback on the RFC from general users seems to be strongly in favor of a simpler, non-differentiated model for both system organization and system types

@cart
Copy link
Member

cart commented Mar 10, 2022

I haven't ever stopped believing that exclusive systems should be "normal systems" 😄
Just pulling in relevant context and stakeholders.

@Ratysz
Copy link
Contributor

Ratysz commented Mar 10, 2022

From an api perspective, I see no meaningful difference between an "exclusive system" and a system that manually claims every resource.

As I've said in #1144 (comment):

There is: an exclusive system can add/remove entites without using commands, modifying archetypes then and there, and it can add and remove resources in the same manner; a system that manually claims everything can only change the data contents, not the structure of the data.

This is why had the split conceptually. Every exclusive system is potentially a hard sync point - the "side effects" stageless idea was supposed to codify which exclusive systems exactly are sync points, and which can be scheduled along with parallel systems.

If we can we can do away with the implementation split and the user-facing API split while maintaining this conceptual split (because it's inescepable for actually scheduling things correctly), I'm all for it. The implementation we have is a compromise rather than a deliberate decision - if it were up to me exclusive systems would've been stages instead; ironically, we'll effectively have that with stageless, it'll just be neatly swept under the rug and hidden behind a flatter API.

@maniwani
Copy link
Contributor Author

maniwani commented Mar 10, 2022

maintaining this conceptual split (because it's inescepable for actually scheduling things correctly)

What is "inescapable" and how exactly would we fail to schedule these systems correctly?

The "dependency debacle" you described back in #1021 (comment) could not happen.

far more convoluted kind of complexity: if exclusive systems are systems, they can have dependencies and can be dependencies

This is desirable IMO, and there's nothing convoluted about it, because:

the user adds a [dependency chain of parallel system -> exclusive system -> parallel system], the execution will fail because the executor can't sandwich an exclusive system like that

It can sandwich them and it won't fail. All we had to do was add a mock component and define "total write access" for both the schedule builder and executor to see the conflict properly.

By themselves, all those systems want conflicting access to component A. If their order is indeterminate, the schedule builder would flag a pair as ambiguous, so it is not a heisenbug.

systems 1 and 2 have no defined order but want conflicting access to components: A

We don't even need to say anything about one system being exclusive, because it's irrelevant. There is ambiguous read-write order and that is the "bug".

So from my perspective, there is nothing about exclusive systems worthy of a formal dichotomy and this is purely a clash of aesthetic preferences.

And to that, I don't think that "systems must use commands when they don't have exclusive access to the entire world" is convoluted. Our storage model should just be documented with pretty pictures depicting why we can't instantly spawn entities and insert components without moving data.

@alice-i-cecile
Copy link
Member

For context, the discussion in #1021 occured before we had any formal notion of "system order ambiguities", much less tools to deal with them. I read the linked comment as an initial exploration of the idea (because it's an exteremely obvious case of it).

I agree with @maniwani here: there's nothing fundamentally more challenging about scheduling / ordering exclusive systems or command flushes: they're just guaranteed to be ambiguous with (almost) literally every other system.

@Ratysz
Copy link
Contributor

Ratysz commented Mar 10, 2022

I am talking about the actual implementation, again. When a systems' graph is scheduled, exclusive systems must necessarily be placed at exact spots wrt blocks of specific parallelizable systems, otherwise we massively lose perf because we'd have to effectively rebuild the graph every dispatch. As long as this is understood, I see no issue with the arguments you give.

Again, the API could be whatever we want. This is not about the API.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 10, 2022

When a systems' graph is scheduled, exclusive systems must necessarily be placed at exact spots wrt blocks of specific parallelizable systems, otherwise we massively lose perf because we'd have to effectively rebuild the graph every dispatch.

Right, because otherwise we're spinning our wheels constantly checking a condition (can this exclusive system run) that will never be met.

I see a few options here on the implementation side:

  1. Forbid all ambiguities with exclusive systems.
  2. Randomly decide and lock in the ordering of exclusive systems WRT any ambiguities upon schedule initialization, and then add constraints to enforce this.
  3. Defer all exclusive systems as long as possible, breaking all ambiguities to run parallel systems first.
  4. Run all exclusive systems as soon as possible.

IMO 1 is probably too strict and frustrating, especially before stageless. 2 is, in theory, in keeping with the spirit of deliberate ambiguities, but very chaotic.

Both 3 and 4 have the nice property of clustering exclusive systems together automatically, which should be good for performance.
My intuition is that 3 is a better default than 4: IME exclusive systems tend to be used for taking snapshots of state.

We could formalize this notion with a asap and alap system configuration tool, which would probably be quite useful in general, and allow overriding to the opposite case as desired. This would allow users to resolve all ambiguities for a given system in a single line (except between other ASAP systems).

EDIT: With these tools, I would actually be okay with 1.

@maniwani
Copy link
Contributor Author

maniwani commented Mar 10, 2022

I was talking about the actual implementation. You can see where I've updated the system params to add the proper component access. I just haven't ripped out the special-casing in SystemStage yet.

Also, I didn't understand what you were saying here.

otherwise we massively lose perf because we'd have to effectively rebuild the graph every dispatch

If you meant "now we have to tell users to be careful about where they put exclusive systems or they risk needlessly throwing away parallelism", then yeah, but there's no need for us to rebuild the graph. (edit: I see, you meant registering new archetypes and updating the archetype component access bitsets.)

Systems with &mut World now have component and archetype component access that reflect their exclusivity. They can and will be topsorted like any other system. But yes, when the order between an exclusive system and others is ambiguous, when it runs can vary a lot given the executor is greedy (topsort order still gives priority).

IMO 1 is probably too strict and frustrating, especially before stageless. 2 is, in theory, in keeping with the spirit of deliberate ambiguities, but very chaotic.

IMO, 1 is the only acceptable option. We can reduce the noise and jargon in the ambiguity warnings separately.

@Ratysz
Copy link
Contributor

Ratysz commented Mar 10, 2022

Right, because otherwise we're spinning our wheels constantly checking a condition (can this exclusive system run) that will never be met.

No, that's not it. Exclusive systems can change archetypes, which invalidates parallel systems' cached scheduling data, requiring a rebuild. Moving a parallel system between blocks separated by hard sync points does the same, because said cached data is a bunch of tight and bespoke bitsets build for and around that specific block. It's needed to boil down the absolutely unavoidable "can this run" checks to a single bitset comparison.

@maniwani
Copy link
Contributor Author

maniwani commented Mar 10, 2022

Exclusive systems can change archetypes, which invalidates parallel systems' cached scheduling data, requiring a rebuild.

Right, I guess we can't quite run exclusive systems using the executor yet. The system tasks are spawned before we know if any system can run, and that borrow keeps us from updating their archetype component access. Edit: Instead of updating system archetype component access once at the beginning, we'd want to update a system's access just before signaling it to run.

I guess this PR can just be about removing the internal split everywhere except SystemStage.

@Ratysz
Copy link
Contributor

Ratysz commented Mar 10, 2022

The system tasks are spawned before we know if any system can run, and that borrow keeps us from updating their archetype component access.

That's not how the systems executor works. Tasks are spawned as needed, data is not actually borrowed until needed. The thing that prevents us from whanging in an exclusive system wherever is exactly what I said it is: there is no guarantee that the exclusive system will not be in some way invalidating, so it necessarily has to split the block of parallel systems into two, and parallel systems cannot be freely moved between blocks because the blocks are pre-built for the specific systems in them.

@maniwani
Copy link
Contributor Author

Okay, after discussing on Discord, I will keep this PR limited to what it says on the tin, getting rid of .exclusive_system() and ExclusiveSystem* types/traits. Keeping the InsertionPoint API and not making any change to the ordering rules of SystemStage (no interleaving of exclusive and non-exclusive systems).

@hymm
Copy link
Contributor

hymm commented Mar 10, 2022

would this supercede #3946?

@maniwani
Copy link
Contributor Author

maniwani commented Mar 10, 2022

would this supercede #3946?

Yes.

@maniwani maniwani marked this pull request as ready for review March 11, 2022 03:03
@maniwani
Copy link
Contributor Author

Still need to update tests/imports in the other crates, but bevy_ecs seems to be good.

@maniwani
Copy link
Contributor Author

bors try

Copy link
Member

@TheRawMeatball TheRawMeatball left a comment

Choose a reason for hiding this comment

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

Didn't review the schedule parts very closely, but the rest look good to me.

@maniwani
Copy link
Contributor Author

maniwani commented Aug 1, 2022

This push changed the world_access_level() impl in the SystemParam tuple macro. I tried to do the "convert enums to bitflags and OR reduce" thing at first (like people said not to), but realized it'll miss conflicts between multiple exclusive params. I admit, bad idea.

Now it's just an sequence of match blocks.

@hymm hymm self-requested a review August 1, 2022 19:48
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Code definitely feels better with the recent changes.

crates/bevy_ecs/src/schedule/stage.rs Show resolved Hide resolved
crates/bevy_ecs/src/schedule/system_descriptor.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/function_system.rs Outdated Show resolved Hide resolved
@maniwani maniwani force-pushed the yeet-exclusive-system branch 2 times, most recently from 1f8a627 to 399a4a0 Compare August 3, 2022 23:32
@maniwani maniwani changed the title Add &mut World as a system param and deprecate .exclusive_system() Add &mut World as a system param and make .exclusive_system() optional Aug 3, 2022
@maniwani maniwani 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 Aug 4, 2022
@@ -254,6 +254,30 @@ pub fn impl_param_set(_input: TokenStream) -> TokenStream {
fn apply(&mut self, world: &mut World) {
self.0.apply(world)
}

fn world_access_level() -> WorldAccessLevel {
let mut exclusive = false;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I feel like this could be done more reliably / clearly via an Ord impl and a reduce.

crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/macros/src/lib.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Sep 19, 2022
@cart
Copy link
Member

cart commented Sep 19, 2022

I've already done one pass over this and I'm on board for the general direction / think its a necessary step for stageless. Not fully caught up yet on the changes, but provided the "access modeling" issue we discussed earlier has been resolved, I'm guessing we can get this merged in relatively short order.
I'll get caught up ASAP / do a final review. Then we just need to resolve conflicts and get this merged!
(@maniwani feel free to wait to resolve merge conflicts until after the review / do whenever makes the most sense for you)

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 19, 2022
crates/bevy_ecs/macros/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/system_container.rs Outdated Show resolved Hide resolved
@@ -58,12 +58,12 @@ impl<'w, 's> SystemParamFetch<'w, 's> for ParallelCommandsState {
unsafe fn get_param(
state: &'s mut Self,
_: &crate::system::SystemMeta,
world: &'w World,
world: MaybeUnsafeCell<'w, World>,
Copy link
Member

Choose a reason for hiding this comment

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

I'm generally on board for the approach in this PR, but having thought about this problem a bit, I think all of this MaybeUnsafeCell and world_access_level validation stuff is a sign that we're doing this inside-out. If we can never safely call an exclusive system with a &World reference, we should probably try to just represent that statically.

To that end, I have put together an alternative PR that adds a new ExclusiveFunctionSystem, which implements System (backed by ExclusiveSystemParam impls and the assumption that &mut World is always passed in).

This means that we can leave all of the existing "shared access system function" stuff untouched, and we can piggyback on the existing System::run and System::run_unsafe functions to ensure that exclusive systems are only run with System::run (by panicking on System::run_unsafe calls).

This impl is smaller, less invasive, and introduces fewer safety concerns. It comes at the cost of having a hard line between functions with SystemParams and ExclusiveSystemParams, but the impl in this PR already had that constraint. It also means that we have another set of ExclusiveSystemParam tuple impls, but I'm not noticing any difference in compile times.

Copy link
Contributor Author

@maniwani maniwani Sep 24, 2022

Choose a reason for hiding this comment

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

Cool to see an alternative identified, but we should be clear on the pros and cons.

this MaybeUnsafeCell and world_access_level validation stuff is a sign that we're doing this inside-out

MaybeUnsafeCell is a workaround to &T -> &UnsafeCell<T> not being a valid cast. @BoxyUwU suggested that may change, so I'm hoping it's only temporary.

But the general idea is not inside-out. Having to prove soundness ourselves is the nature of our access model. This PR tries to complete this model so it can express any sound access, and world_access_level is the natural extension to fill that gap.

#6083 trades completeness (maybe "consistent API" is better wording) for the static guarantees that users can't write functions with unsound access on this level + no anxiety about us having missed some UB edge case.

fewer safety concerns

It's a little unfair to say that what's implemented here is less safe. While true, the contract of SystemParamState is that params declare their access truthfully. UB can only result from violating that contract. That's still true.

And while not statically prevented, unsound World access here panics at system construction, which is still earlier than unsound query access.

it comes at the cost of having a hard line between functions with SystemParams and ExclusiveSystemParams the impl in this PR already had that constraint

This PR does not have a hard line exclusive/parallel trait split, which is what causes arbitrary limits / inconsistencies. E.g. #6083 would prevent a user from implementing a custom param with a &mut World field and from using &mut World in a ParamSet.

smaller, less invasive

I'll give you that. Your PR does save custom param implementers from having to change anything.

Copy link
Member

Choose a reason for hiding this comment

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

But the general idea is not inside-out. Having to prove soundness ourselves is the nature of our access model. This PR tries to complete this model so it can express any sound access, and world_access_level is the natural extension to fill that gap.

Within this context, I think it is. SystemParams exist to prove to the compiler that extracting disjoint components / resources from &World is sound. This is impossible to do in safe Rust.

MaybeUnsafeCell and world_access_level exist to ensure that &mut World is never used alongside &World derived references and that &World is never casted to &mut World. This is possible to do in safe Rust (and is kind of the selling feature of it).

These are different problem spaces solving different problems. I do think that choosing to merge them under one API is valid, but I think in this case doing that isn't worth the complexity / risk.

It's a little unfair to say that what's implemented here is less safe. While true, the contract of SystemParamState is that params declare their access truthfully. UB can only result from violating that contract. That's still true.

I don't think this is unfair. It expands the scope of the safety contract, making it harder to implement and verify. I think calling that "less safe" is a very fair assessment.

And while not statically prevented, unsound World access here panics at system construction, which is still earlier than unsound query access.

Only if as a SystemParam implementer you implement world_access_level correctly / only extract &mut World from MaybeUnsafeCell under the right circumstances.

This PR does not have a hard line exclusive/parallel trait split, which is what causes arbitrary limits / inconsistencies. E.g. #6083 would prevent a user from implementing a custom param with a &mut World field and from using &mut World in a ParamSet.

Yup I agree that putting &mut World in a ParamSet would be impossible in #6083, but I think thats actually desirable (as called out in that pr).

Copy link
Contributor Author

@maniwani maniwani Sep 26, 2022

Choose a reason for hiding this comment

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

It expands the scope of the safety contract, making it harder to implement and verify.

Only if as a SystemParam implementer you implement world_access_level correctly / only extract &mut World from MaybeUnsafeCell under the right circumstances.

The same could be said for ReadOnlySystemParamFetch and SystemParamState. I'm happy to accept this as an argument about aesthetics (i.e. "MaybeUnsafeCell is icky" and "let's not make the contract longer"), but I don't really buy the safety angle here. Ultimately, our model has to assume honesty and correctness. We can't verify soundness if one of those contracts is violated anyway.

@maniwani maniwani force-pushed the yeet-exclusive-system branch 2 times, most recently from 2e1e7f1 to 4f0b01b Compare September 24, 2022 16:55
maniwani and others added 3 commits September 24, 2022 10:30
- add `MaybeUnsafeCell` (in the future `&T -> &UnsafeCell<T>` may be OK)
- add `WorldAccessLevel`
- add concept of "all exclusive access" to `Access`
- update system_param.rs
- update system.rs
- update function_system.rs
- update system_chaining.rs
- remove ExclusiveSystem* internals
- add tests
- fix stage tests
  - in ambiguity_detection, the else branch in find_ambiguities is no longer taken because exclusive systems have a component access now. And obv two empty systems will always be compatible.
- update fixed_timestep.rs
- update extract_param.rs

Co-Authored-By: Mike <mike.hsu@gmail.com>
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) {
  }
}
```
@maniwani
Copy link
Contributor Author

Closing since issue was resolved by #6083.

@maniwani maniwani closed this Sep 26, 2022
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) {
  }
}
```
@maniwani maniwani deleted the yeet-exclusive-system branch November 16, 2022 20:42
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-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants