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] - Simplify trait hierarchy for SystemParam #6865

Closed
wants to merge 20 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Dec 5, 2022

Objective

  • Implementing a custom SystemParam by hand requires implementing three traits -- four if it is read-only.
  • The trait SystemParamFetch<'w, 's> is a workaround from before we had generic associated types, and is no longer necessary.

Solution

  • Combine the trait SystemParamFetch with SystemParamState.
    • I decided to remove the Fetch name and keep the State name, since the former was consistently conflated with the latter.
  • Replace the trait ReadOnlySystemParamFetch with ReadOnlySystemParam, which simplifies trait bounds in generic code.

Changelog

  • Removed the trait SystemParamFetch, moving its functionality to SystemParamState.
  • Replaced the trait ReadOnlySystemParamFetch with ReadOnlySystemParam.

Migration Guide

Merged with the guide for #6919.

@james7132
Copy link
Member

james7132 commented Dec 5, 2022

Could we go one step further, as we have with WorldQuery and further combine SystemParamState with SystemParam? What used to be FetchState is now just a type State: Send + Sync + 'static on WorldQuery.

@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 5, 2022

Unfortunately that is not possible due to how GATs work for now. You are forced to add the where Self: 'a bound on every lifetime GAT, which messes up any SystemParam with lifetimes.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Dec 5, 2022
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.

LGTM. Just one style nit.

crates/bevy_ecs/src/system/system_param.rs Outdated Show resolved Hide resolved
@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 6, 2022

You are forced to add the where Self: 'a bound on every lifetime GAT

Nevermind, I misunderstood how required bounds work the first time I tried this. Working on some more improvements.

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.

This change results in a serious simplification, and appears to be correctly done. @BoxyUwU can you double check this?

@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 7, 2022

My ultimate goal is to merge SystemParamState with SystemParam, which would flatten the hierarchy akin to WorldQuery (like James' suggestion above). It would also allow removing types like ResState, which will be amazing.

It seems doable, but unfortunately there is a nightmare of trait bounds that may take a while to untangle. It will also have hundreds of additional lines of churn, so I think I will split it off into a future PR.

@alice-i-cecile alice-i-cecile 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 Dec 7, 2022
@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 7, 2022

One last change: I replaced ReadOnlySystemParamState with ReadOnlySystemParam, which feels like a better API to present. It will also reduce churn later.

@JonahPlusPlus
Copy link
Contributor

@JoJoJet If you are planning to merge SystemParam and SystemParamState, should I wait till after? It won't take me long to implement my changes (hopefully, I've got everything planned out exactly except Resultful (I think I've settled on that as a name, since it's an actual word, unlike Resultable)).

Basically, I want to make both generic for T: Fallibility, where Fallibility is a sealed trait implemented for markers Infallible, Optional and Resultful.

I don't think this would have any significant effect on your implementation. I'm guessing you just want to make it so SystemParam has an associated type that it uses for the SystemParamState functions and you would still have a state struct?

@james7132 james7132 removed 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 Dec 7, 2022
@JoJoJet
Copy link
Member Author

JoJoJet commented Dec 7, 2022

If you are planning to merge SystemParam and SystemParamState, should I wait till after?

@JonahPlusPlus There's still a few things left to figure out for that PR, so it's not a sure thing for it to get merged. I wouldn't wait, so long as you're okay with a little rebasing if needed.

@JoJoJet JoJoJet requested review from james7132 and removed request for BoxyUwU December 7, 2022 13:16
@@ -136,8 +136,8 @@ impl<'w, 's, Q: WorldQuery + 'static, F: ReadOnlyWorldQuery + 'static> SystemPar
}

// SAFETY: QueryState is constrained to read-only fetches, so it only reads World.
unsafe impl<'w, 's, Q: ReadOnlyWorldQuery, F: ReadOnlyWorldQuery> ReadOnlySystemParam
for Query<'w, 's, Q, F>
unsafe impl<'w, 's, Q: ReadOnlyWorldQuery + 'static, F: ReadOnlyWorldQuery + 'static>
Copy link
Member

Choose a reason for hiding this comment

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

Was this required by the compiler? This seems incorrect to me.

Copy link
Member Author

@JoJoJet JoJoJet Dec 7, 2022

Choose a reason for hiding this comment

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

These bounds are required because ReadOnlySystemParam: SystemParam -- same reason I had to add them to StaticSystemParam before. The bounds already exist on impl SystemParam for Query<>, so nothing is lost here.

Copy link
Member

Choose a reason for hiding this comment

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

oh awesome I didn't realise we were already requiring Q: 'static and F: 'static on the SystemParam impl. We should just require trait WorldQuery: 'static then

Copy link
Member

Choose a reason for hiding this comment

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

Oh man that's nutty. Seems counterintuitive in this case, though probably for the best. LGTM then, we can address the WorldQuery: 'static thing in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should just require trait WorldQuery: 'static then

I agree -- I just tried this though and it seems like it'll add a bunch of churn, so I'll leave it for another PR.

@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 Dec 7, 2022
@james7132 james7132 added this to the 0.10 milestone Dec 7, 2022

/// The state of a [`SystemParam`].
///
/// # Safety
///
/// It is the implementor's responsibility to ensure `system_meta` is populated with the _exact_
/// [`World`] access used by the [`SystemParamState`] (and associated [`SystemParamFetch`]).
/// [`World`] access used by the [`SystemParamState`].
/// Additionally, it is the implementor's responsibility to ensure there is no
/// conflicting access across all [`SystemParam`]'s.
pub unsafe trait SystemParamState: Send + Sync + 'static {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this and the function get_param have to both be marked unsafe?

Copy link
Member Author

@JoJoJet JoJoJet Dec 8, 2022

Choose a reason for hiding this comment

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

Yes. unsafe has different implications for fns and traits: unsafe fn requires the caller of the fn to uphold safety invariants, whereas unsafe trait requires the implementer of the trait to uphold safety invariants. In this case, we want it to be both unsafe to implement and to call, so we mark both as unsafe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so the other methods are unsafe to implement, but get_param is unsafe to call. Thanks for the clarification, I couldn't find anything online explaining why you would prefer one over the other.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Dec 11, 2022
# Objective

* Implementing a custom `SystemParam` by hand requires implementing three traits -- four if it is read-only.
* The trait `SystemParamFetch<'w, 's>` is a workaround from before we had generic associated types, and is no longer necessary.

## Solution

* Combine the trait `SystemParamFetch` with `SystemParamState`.
    * I decided to remove the `Fetch` name and keep the `State` name, since the former was consistently conflated with the latter.
* Replace the trait `ReadOnlySystemParamFetch` with `ReadOnlySystemParam`, which simplifies trait bounds in generic code.

---

## Changelog

- Removed the trait `SystemParamFetch`, moving its functionality to `SystemParamState`.
- Replaced the trait `ReadOnlySystemParamFetch` with `ReadOnlySystemParam`.

## Migration Guide

The trait `SystemParamFetch` has been removed, and its functionality has been transferred to `SystemParamState`.

```rust
// Before
impl SystemParamState for MyParamState {
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam<'w, 's>;
    fn get_param(...) -> Self::Item;
}

// After
impl SystemParamState for MyParamState {
    type Item<'w, 's> = MyParam<'w, 's>; // Generic associated types!
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
    fn get_param<'w, 's>(...) -> Self::Item<'w, 's>;
}
```

The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.

```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}

// After
unsafe impl<'w, 's> ReadOnlySystemParam for MyParam<'w, 's> {}
```
@bors bors bot changed the title Simplify trait hierarchy for SystemParam [Merged by Bors] - Simplify trait hierarchy for SystemParam Dec 11, 2022
@bors bors bot closed this Dec 11, 2022
@JoJoJet JoJoJet deleted the system-param-gats branch December 12, 2022 17:15
bors bot pushed a commit that referenced this pull request Jan 7, 2023
…6919)

Spiritual successor to #5205.
Actual successor to #6865.

# Objective

Currently, system params are defined using three traits: `SystemParam`, `ReadOnlySystemParam`, `SystemParamState`. The behavior for each param is specified by the `SystemParamState` trait, while `SystemParam` simply defers to the state.

Splitting the traits in this way makes it easier to implement within macros, but it increases the cognitive load. Worst of all, this approach requires each `MySystemParam` to have a public `MySystemParamState` type associated with it.

## Solution

* Merge the trait `SystemParamState` into `SystemParam`.
* Remove all trivial `SystemParam` state types. 
  * `OptionNonSendMutState<T>`: you will not be missed.

---

- [x] Fix/resolve the remaining test failure.

## Changelog

* Removed the trait `SystemParamState`, merging its functionality into `SystemParam`.

## Migration Guide

**Note**: this should replace the migration guide for #6865.
This is relative to Bevy 0.9, not main.

The traits `SystemParamState` and `SystemParamFetch` have been removed, and their functionality has been transferred to `SystemParam`.


```rust
// Before (0.9)
impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
}
unsafe impl SystemParamState for MyParamState {
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
unsafe impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam<'w, 's>;
    fn get_param(&mut self, ...) -> Self::Item;
}
unsafe impl ReadOnlySystemParamFetch for MyParamState { }

// After (0.10)
unsafe impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
    type Item<'w, 's> = MyParam<'w, 's>;
    fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { ... }
    fn get_param<'w, 's>(state: &mut Self::State, ...) -> Self::Item<'w, 's>;
}
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> { }
```

The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.

```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}

// After
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> {}
```
james7132 pushed a commit to james7132/bevy that referenced this pull request Jan 21, 2023
…evyengine#6919)

Spiritual successor to bevyengine#5205.
Actual successor to bevyengine#6865.

# Objective

Currently, system params are defined using three traits: `SystemParam`, `ReadOnlySystemParam`, `SystemParamState`. The behavior for each param is specified by the `SystemParamState` trait, while `SystemParam` simply defers to the state.

Splitting the traits in this way makes it easier to implement within macros, but it increases the cognitive load. Worst of all, this approach requires each `MySystemParam` to have a public `MySystemParamState` type associated with it.

## Solution

* Merge the trait `SystemParamState` into `SystemParam`.
* Remove all trivial `SystemParam` state types. 
  * `OptionNonSendMutState<T>`: you will not be missed.

---

- [x] Fix/resolve the remaining test failure.

## Changelog

* Removed the trait `SystemParamState`, merging its functionality into `SystemParam`.

## Migration Guide

**Note**: this should replace the migration guide for bevyengine#6865.
This is relative to Bevy 0.9, not main.

The traits `SystemParamState` and `SystemParamFetch` have been removed, and their functionality has been transferred to `SystemParam`.


```rust
// Before (0.9)
impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
}
unsafe impl SystemParamState for MyParamState {
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
unsafe impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam<'w, 's>;
    fn get_param(&mut self, ...) -> Self::Item;
}
unsafe impl ReadOnlySystemParamFetch for MyParamState { }

// After (0.10)
unsafe impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
    type Item<'w, 's> = MyParam<'w, 's>;
    fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { ... }
    fn get_param<'w, 's>(state: &mut Self::State, ...) -> Self::Item<'w, 's>;
}
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> { }
```

The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.

```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}

// After
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> {}
```
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

* Implementing a custom `SystemParam` by hand requires implementing three traits -- four if it is read-only.
* The trait `SystemParamFetch<'w, 's>` is a workaround from before we had generic associated types, and is no longer necessary.

## Solution

* Combine the trait `SystemParamFetch` with `SystemParamState`.
    * I decided to remove the `Fetch` name and keep the `State` name, since the former was consistently conflated with the latter.
* Replace the trait `ReadOnlySystemParamFetch` with `ReadOnlySystemParam`, which simplifies trait bounds in generic code.

---

## Changelog

- Removed the trait `SystemParamFetch`, moving its functionality to `SystemParamState`.
- Replaced the trait `ReadOnlySystemParamFetch` with `ReadOnlySystemParam`.

## Migration Guide

The trait `SystemParamFetch` has been removed, and its functionality has been transferred to `SystemParamState`.

```rust
// Before
impl SystemParamState for MyParamState {
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam<'w, 's>;
    fn get_param(...) -> Self::Item;
}

// After
impl SystemParamState for MyParamState {
    type Item<'w, 's> = MyParam<'w, 's>; // Generic associated types!
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
    fn get_param<'w, 's>(...) -> Self::Item<'w, 's>;
}
```

The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.

```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}

// After
unsafe impl<'w, 's> ReadOnlySystemParam for MyParam<'w, 's> {}
```
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…evyengine#6919)

Spiritual successor to bevyengine#5205.
Actual successor to bevyengine#6865.

# Objective

Currently, system params are defined using three traits: `SystemParam`, `ReadOnlySystemParam`, `SystemParamState`. The behavior for each param is specified by the `SystemParamState` trait, while `SystemParam` simply defers to the state.

Splitting the traits in this way makes it easier to implement within macros, but it increases the cognitive load. Worst of all, this approach requires each `MySystemParam` to have a public `MySystemParamState` type associated with it.

## Solution

* Merge the trait `SystemParamState` into `SystemParam`.
* Remove all trivial `SystemParam` state types. 
  * `OptionNonSendMutState<T>`: you will not be missed.

---

- [x] Fix/resolve the remaining test failure.

## Changelog

* Removed the trait `SystemParamState`, merging its functionality into `SystemParam`.

## Migration Guide

**Note**: this should replace the migration guide for bevyengine#6865.
This is relative to Bevy 0.9, not main.

The traits `SystemParamState` and `SystemParamFetch` have been removed, and their functionality has been transferred to `SystemParam`.


```rust
// Before (0.9)
impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
}
unsafe impl SystemParamState for MyParamState {
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
unsafe impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam<'w, 's>;
    fn get_param(&mut self, ...) -> Self::Item;
}
unsafe impl ReadOnlySystemParamFetch for MyParamState { }

// After (0.10)
unsafe impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
    type Item<'w, 's> = MyParam<'w, 's>;
    fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { ... }
    fn get_param<'w, 's>(state: &mut Self::State, ...) -> Self::Item<'w, 's>;
}
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> { }
```

The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.

```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}

// After
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> {}
```
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

* Implementing a custom `SystemParam` by hand requires implementing three traits -- four if it is read-only.
* The trait `SystemParamFetch<'w, 's>` is a workaround from before we had generic associated types, and is no longer necessary.

## Solution

* Combine the trait `SystemParamFetch` with `SystemParamState`.
    * I decided to remove the `Fetch` name and keep the `State` name, since the former was consistently conflated with the latter.
* Replace the trait `ReadOnlySystemParamFetch` with `ReadOnlySystemParam`, which simplifies trait bounds in generic code.

---

## Changelog

- Removed the trait `SystemParamFetch`, moving its functionality to `SystemParamState`.
- Replaced the trait `ReadOnlySystemParamFetch` with `ReadOnlySystemParam`.

## Migration Guide

The trait `SystemParamFetch` has been removed, and its functionality has been transferred to `SystemParamState`.

```rust
// Before
impl SystemParamState for MyParamState {
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam<'w, 's>;
    fn get_param(...) -> Self::Item;
}

// After
impl SystemParamState for MyParamState {
    type Item<'w, 's> = MyParam<'w, 's>; // Generic associated types!
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
    fn get_param<'w, 's>(...) -> Self::Item<'w, 's>;
}
```

The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.

```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}

// After
unsafe impl<'w, 's> ReadOnlySystemParam for MyParam<'w, 's> {}
```
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…evyengine#6919)

Spiritual successor to bevyengine#5205.
Actual successor to bevyengine#6865.

# Objective

Currently, system params are defined using three traits: `SystemParam`, `ReadOnlySystemParam`, `SystemParamState`. The behavior for each param is specified by the `SystemParamState` trait, while `SystemParam` simply defers to the state.

Splitting the traits in this way makes it easier to implement within macros, but it increases the cognitive load. Worst of all, this approach requires each `MySystemParam` to have a public `MySystemParamState` type associated with it.

## Solution

* Merge the trait `SystemParamState` into `SystemParam`.
* Remove all trivial `SystemParam` state types. 
  * `OptionNonSendMutState<T>`: you will not be missed.

---

- [x] Fix/resolve the remaining test failure.

## Changelog

* Removed the trait `SystemParamState`, merging its functionality into `SystemParam`.

## Migration Guide

**Note**: this should replace the migration guide for bevyengine#6865.
This is relative to Bevy 0.9, not main.

The traits `SystemParamState` and `SystemParamFetch` have been removed, and their functionality has been transferred to `SystemParam`.


```rust
// Before (0.9)
impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
}
unsafe impl SystemParamState for MyParamState {
    fn init(world: &mut World, system_meta: &mut SystemMeta) -> Self { ... }
}
unsafe impl<'w, 's> SystemParamFetch<'w, 's> for MyParamState {
    type Item = MyParam<'w, 's>;
    fn get_param(&mut self, ...) -> Self::Item;
}
unsafe impl ReadOnlySystemParamFetch for MyParamState { }

// After (0.10)
unsafe impl SystemParam for MyParam<'_, '_> {
    type State = MyParamState;
    type Item<'w, 's> = MyParam<'w, 's>;
    fn init_state(world: &mut World, system_meta: &mut SystemMeta) -> Self::State { ... }
    fn get_param<'w, 's>(state: &mut Self::State, ...) -> Self::Item<'w, 's>;
}
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> { }
```

The trait `ReadOnlySystemParamFetch` has been replaced with `ReadOnlySystemParam`.

```rust
// Before
unsafe impl ReadOnlySystemParamFetch for MyParamState {}

// After
unsafe impl ReadOnlySystemParam for MyParam<'_, '_> {}
```
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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