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] - Optional .system #2398

Closed
wants to merge 4 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Jun 26, 2021

This can be your 6 months post-christmas present.

Objective

  • Make .system optional
  • yeet
  • It's ugly
  • Alternative title: .system is dead; long live .system
  • yeet

Solution

  • Use a higher ranked lifetime, and some trait magic.

N.B. This PR does not actually remove any .systems, except in a couple of examples. Once this is merged we can do that piecemeal across crates, and decide on syntax for labels.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 26, 2021
@DJMcNab DJMcNab added A-Core Common functionality for all bevy apps A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible P-High This is particularly urgent, and deserves immediate attention C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled C-Feature A new feature, making something new possible labels Jun 26, 2021
@DJMcNab DJMcNab changed the title Optional systems Optional .system Jun 26, 2021
@BoxyUwU
Copy link
Member

BoxyUwU commented Jun 26, 2021

this code doesnt look horribly wrong to me after a quick skim 👍

@cart
Copy link
Member

cart commented Jun 27, 2021

bors r+

bors bot pushed a commit that referenced this pull request Jun 27, 2021
This can be your 6 months post-christmas present.

# Objective

- Make `.system` optional
- yeet
- It's ugly
- Alternative title: `.system` is dead; long live `.system`
- **yeet**

## Solution

- Use a higher ranked lifetime, and some trait magic.

N.B. This PR does not actually remove any `.system`s, except in a couple of examples. Once this is merged we can do that piecemeal across crates, and decide on syntax for labels.
@bors bors bot changed the title Optional .system [Merged by Bors] - Optional .system Jun 27, 2021
@bors bors bot closed this Jun 27, 2021
@Nilirad
Copy link
Contributor

Nilirad commented Jun 27, 2021

poggers

bors bot pushed a commit that referenced this pull request Jun 29, 2021
# Objective

- Extend work done in #2398.
- Make `.system()` syntax optional when using system descriptor API.

## Solution

- Slight change to `ParallelSystemDescriptorCoercion` signature and implementors.

---

I haven't touched exclusive systems, because it looks like the only two other solutions are going back to doubling our system insertion methods, or starting to lean into stageless. The latter will invalidate the former, so I think exclusive systems should remian pariahs until stageless.

I can grep & nuke `.system()` thorughout the codebase now, which might take a while, or we can do that in subsequent PR(s).
bors bot pushed a commit that referenced this pull request Jul 1, 2021
# Objective

- Continue work of #2398 and #2403.
- Make `.system()` syntax optional when using `.config()` API.

## Solution

- Introduce new prelude trait, `ConfigurableSystem`, that shorthands `my_system.system().config(...)` as `my_system.config(...)`.
- Expand `configure_system_local` test to also cover the new syntax.
bors bot pushed a commit that referenced this pull request Jul 8, 2021
# Objective

- Continue work of #2398 and friends.
- Make `.system()` optional in run criteria APIs.

## Solution

- Slight change to `RunCriteriaDescriptorCoercion` signature and implementors.
- Implement `IntoRunCriteria` for `IntoSystem` rather than `System`.
- Remove some usages of `.system()` with run criteria in tests of `stage.rs`, to verify the implementation.
@cart cart mentioned this pull request Jul 9, 2021
bors bot pushed a commit that referenced this pull request Jul 17, 2021
# Objective

- Continue work of #2398 and friends.
- Make `.system()` optional in chaining.

## Solution

- Slight change to `IntoChainSystem` signature and implementation.
- Remove some usages of `.system()` in the chaining example, to verify the implementation.

---

I swear, I'm not splitting these up on purpose, I just legit forgot about most of the things where `System` appears in public API, and my trait usage explorer mingles that with the gajillion internal uses.

In case you're wondering what happened to part 5, #2446 ate it.
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
This can be your 6 months post-christmas present.

# Objective

- Make `.system` optional
- yeet
- It's ugly
- Alternative title: `.system` is dead; long live `.system`
- **yeet**

## Solution

- Use a higher ranked lifetime, and some trait magic.

N.B. This PR does not actually remove any `.system`s, except in a couple of examples. Once this is merged we can do that piecemeal across crates, and decide on syntax for labels.
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
# Objective

- Extend work done in bevyengine#2398.
- Make `.system()` syntax optional when using system descriptor API.

## Solution

- Slight change to `ParallelSystemDescriptorCoercion` signature and implementors.

---

I haven't touched exclusive systems, because it looks like the only two other solutions are going back to doubling our system insertion methods, or starting to lean into stageless. The latter will invalidate the former, so I think exclusive systems should remian pariahs until stageless.

I can grep & nuke `.system()` thorughout the codebase now, which might take a while, or we can do that in subsequent PR(s).
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
# Objective

- Continue work of bevyengine#2398 and bevyengine#2403.
- Make `.system()` syntax optional when using `.config()` API.

## Solution

- Introduce new prelude trait, `ConfigurableSystem`, that shorthands `my_system.system().config(...)` as `my_system.config(...)`.
- Expand `configure_system_local` test to also cover the new syntax.
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
# Objective

- Continue work of bevyengine#2398 and friends.
- Make `.system()` optional in run criteria APIs.

## Solution

- Slight change to `RunCriteriaDescriptorCoercion` signature and implementors.
- Implement `IntoRunCriteria` for `IntoSystem` rather than `System`.
- Remove some usages of `.system()` with run criteria in tests of `stage.rs`, to verify the implementation.
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
# Objective

- Continue work of bevyengine#2398 and friends.
- Make `.system()` optional in chaining.

## Solution

- Slight change to `IntoChainSystem` signature and implementation.
- Remove some usages of `.system()` in the chaining example, to verify the implementation.

---

I swear, I'm not splitting these up on purpose, I just legit forgot about most of the things where `System` appears in public API, and my trait usage explorer mingles that with the gajillion internal uses.

In case you're wondering what happened to part 5, bevyengine#2446 ate it.
bors bot pushed a commit that referenced this pull request Feb 25, 2022
# Objective

- Fix the ugliness of the `config` api. 
- Supercedes #2440, #2463, #2491

## Solution

- Since #2398, capturing closure systems have worked.
- Use those instead where we needed config before
- Remove the rest of the config api. 
- Related: #2777
kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022
# Objective

- Fix the ugliness of the `config` api. 
- Supercedes bevyengine#2440, bevyengine#2463, bevyengine#2491

## Solution

- Since bevyengine#2398, capturing closure systems have worked.
- Use those instead where we needed config before
- Remove the rest of the config api. 
- Related: bevyengine#2777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use P-High This is particularly urgent, and deserves immediate attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants