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] - make WorldQuery very flat #5205

Closed
wants to merge 3 commits into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Jul 4, 2022

Objective

Simplify the worldquery trait hierarchy as much as possible by putting it all in one trait. If/when gats are stabilised this can be trivially migrated over to use them, although that's not why I made this PR, those reasons are:

  • Moves all of the conceptually related unsafe code for a worldquery next to eachother
  • Removes now unnecessary traits simplifying the "type system magic" in bevy_ecs

Changelog

All methods/functions/types/consts on FetchState and Fetch traits have been moved to the WorldQuery trait and the other traits removed. WorldQueryGats now only contains an Item and Fetch assoc type.

Migration Guide

Implementors should move items in impls to the WorldQuery/Gats traits and remove any Fetch/FetchState impls
Any use sites of items in the Fetch/FetchState traits should be updated to use the WorldQuery trait items instead

@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 C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Jul 4, 2022
@alice-i-cecile
Copy link
Member

This might be easier if you merge in #4800 first and then base off main + 4800 + 5170?

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.

Note to reviewers: this must be reviewed commit-by-commit in IDE or in split view to make any sense.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Jul 4, 2022

Yeah honestly no point reviewing this rn because it needs my other PR to get merged anyway 🤷‍♀️ mostly just opening as proof of concept "look we can actually make the traits 1000x simpler even without gats"

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 4, 2022

I'm very impressed that this is possible, and think this is important to prioritize. We even get nicer user-facing error messages!

I've done a full review of the second commit and it's just shuffling code around in a way that is IMO significantly clearer.

@cart, when you're ready I think we should do:

  1. [Merged by Bors] - remove QF generics from all Query/State methods and types #5170
  2. [Merged by Bors] - make WorldQuery very flat #5205
  3. [Merged by Bors] - Clean up Fetch code #4800

in order to reduce rebasing pain.

@BoxyUwU BoxyUwU force-pushed the fix_worldquery_forever branch 2 times, most recently from 2815e69 to 91d2c8d Compare July 4, 2022 23:43
@james7132 james7132 mentioned this pull request Jul 6, 2022
@cart cart modified the milestone: Bevy 0.8 Jul 19, 2022
@BoxyUwU BoxyUwU force-pushed the fix_worldquery_forever branch 2 times, most recently from cf4e8e3 to 16abf5c Compare July 29, 2022 03:26
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I dig these changes! Moving methods that are clearly "fetch methods" to WorldQuery does make me a bit sad, but I agree that in the global context the flattening does make things nicer. Interacting with WorldQuery types directly is less "type wizard-ey" / we push that concern onto WorldQuery implementors (which will be less common, so this feels like a good tradeoff). Looking forward to gats making this even better!

Also: I just converted all of your "match on bools" to "if / else statements". Next time you get the urge, just assume I'll revert and then we can skip this step in the future ❤️
(also removed some unnecessary clones)

@cart
Copy link
Member

cart commented Aug 4, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 4, 2022
# Objective

Simplify the worldquery trait hierarchy as much as possible by putting it all in one trait. If/when gats are stabilised this can be trivially migrated over to use them, although that's not why I made this PR, those reasons are:
- Moves all of the conceptually related unsafe code for a worldquery next to eachother
- Removes now unnecessary traits simplifying the "type system magic" in bevy_ecs

---

## Changelog

All methods/functions/types/consts on `FetchState` and `Fetch` traits have been moved to the `WorldQuery` trait and the other traits removed. `WorldQueryGats` now only contains an `Item` and `Fetch` assoc type.

## Migration Guide
Implementors should move items in impls to the `WorldQuery/Gats` traits and remove any `Fetch`/`FetchState` impls
Any use sites of items in the `Fetch`/`FetchState` traits should be updated to use the `WorldQuery` trait items instead


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors bors bot changed the title make WorldQuery very flat [Merged by Bors] - make WorldQuery very flat Aug 4, 2022
@bors bors bot closed this Aug 4, 2022
maccesch pushed a commit to Synphonyte/bevy that referenced this pull request Aug 5, 2022
# Objective

Simplify the worldquery trait hierarchy as much as possible by putting it all in one trait. If/when gats are stabilised this can be trivially migrated over to use them, although that's not why I made this PR, those reasons are:
- Moves all of the conceptually related unsafe code for a worldquery next to eachother
- Removes now unnecessary traits simplifying the "type system magic" in bevy_ecs

---

## Changelog

All methods/functions/types/consts on `FetchState` and `Fetch` traits have been moved to the `WorldQuery` trait and the other traits removed. `WorldQueryGats` now only contains an `Item` and `Fetch` assoc type.

## Migration Guide
Implementors should move items in impls to the `WorldQuery/Gats` traits and remove any `Fetch`/`FetchState` impls
Any use sites of items in the `Fetch`/`FetchState` traits should be updated to use the `WorldQuery` trait items instead


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Nilirad added a commit to Nilirad/bevy that referenced this pull request Aug 8, 2022
Moved all links to the bottom of the doc comment.
Links have been alphabetically ordered.

Rebase notes:

- After bevyengine#5205 got merged,
  the “Safety” section of `WorldQuery` has changed.
  I listed the mentioned methods for a better layout.
  I also removed the `WorldQuery::` prefix for readability.
- I don't have enough knowledge of the previous and current state of `WorldQuery` types,
  so the documentation may be outdated somewhere.
  `WorldQuery` needs another review round.
@Nilirad Nilirad mentioned this pull request Aug 8, 2022
2 tasks
Nilirad added a commit to Nilirad/bevy that referenced this pull request Aug 19, 2022
Moved all links to the bottom of the doc comment.
Links have been alphabetically ordered.

Rebase notes:

- After bevyengine#5205 got merged,
  the “Safety” section of `WorldQuery` has changed.
  I listed the mentioned methods for a better layout.
  I also removed the `WorldQuery::` prefix for readability.
- I don't have enough knowledge of the previous and current state of `WorldQuery` types,
  so the documentation may be outdated somewhere.
  `WorldQuery` needs another review round.
inodentry pushed a commit to BroovyJammy/bevy that referenced this pull request Aug 19, 2022
# Objective

Simplify the worldquery trait hierarchy as much as possible by putting it all in one trait. If/when gats are stabilised this can be trivially migrated over to use them, although that's not why I made this PR, those reasons are:
- Moves all of the conceptually related unsafe code for a worldquery next to eachother
- Removes now unnecessary traits simplifying the "type system magic" in bevy_ecs

---

## Changelog

All methods/functions/types/consts on `FetchState` and `Fetch` traits have been moved to the `WorldQuery` trait and the other traits removed. `WorldQueryGats` now only contains an `Item` and `Fetch` assoc type.

## Migration Guide
Implementors should move items in impls to the `WorldQuery/Gats` traits and remove any `Fetch`/`FetchState` impls
Any use sites of items in the `Fetch`/`FetchState` traits should be updated to use the `WorldQuery` trait items instead


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Simplify the worldquery trait hierarchy as much as possible by putting it all in one trait. If/when gats are stabilised this can be trivially migrated over to use them, although that's not why I made this PR, those reasons are:
- Moves all of the conceptually related unsafe code for a worldquery next to eachother
- Removes now unnecessary traits simplifying the "type system magic" in bevy_ecs

---

## Changelog

All methods/functions/types/consts on `FetchState` and `Fetch` traits have been moved to the `WorldQuery` trait and the other traits removed. `WorldQueryGats` now only contains an `Item` and `Fetch` assoc type.

## Migration Guide
Implementors should move items in impls to the `WorldQuery/Gats` traits and remove any `Fetch`/`FetchState` impls
Any use sites of items in the `Fetch`/`FetchState` traits should be updated to use the `WorldQuery` trait items instead


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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
…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

Simplify the worldquery trait hierarchy as much as possible by putting it all in one trait. If/when gats are stabilised this can be trivially migrated over to use them, although that's not why I made this PR, those reasons are:
- Moves all of the conceptually related unsafe code for a worldquery next to eachother
- Removes now unnecessary traits simplifying the "type system magic" in bevy_ecs

---

## Changelog

All methods/functions/types/consts on `FetchState` and `Fetch` traits have been moved to the `WorldQuery` trait and the other traits removed. `WorldQueryGats` now only contains an `Item` and `Fetch` assoc type.

## Migration Guide
Implementors should move items in impls to the `WorldQuery/Gats` traits and remove any `Fetch`/`FetchState` impls
Any use sites of items in the `Fetch`/`FetchState` traits should be updated to use the `WorldQuery` trait items instead


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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 C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants