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.

Sign up for GitHub

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

Jump to bottom

Make system chaining use a normal system #4666

Closed
wants to merge 4 commits into from
Closed

Make system chaining use a normal system #4666

wants to merge 4 commits into from

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented May 5, 2022

Objective

  • The API for our system abstraction is functions, not the System trait.
  • Demonstrate that this can be used for higher order systems
  • Allow it to be used for higher order systems

Solution

  • Change SystemParamFunction to allow higher-order systems to be created in terms of functions rather than Systems.
  • Migrate ChainSystem to this.

Please note, that without GATs, as far as I know, we cannot have chain remain as a method. However, GATs are on the path to stabilisation, hopefully in a form sufficiently powerful for this case.

Open questions

Is chain the correct name? I think it's probably a fine name, but the idiomatic style is for functions which return systems to have the _system suffix. Do we make an exception for chain due to the old name or higher-order nature?

Changelog

Changed

The chain method on system functions has been removed. Use the bevy_ecs::system::chain function instead (which is in the bevy_ecs prelude).

Migration Guide

The chain method on system functions has been removed. Use the bevy_ecs::system::chain function instead (which is in the bevy_ecs prelude). If previously you used chain as:

do_thing.chain(handler)

you should instead use:

chain(do_thing, handler)

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label May 5, 2022
@DJMcNab DJMcNab 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 and removed S-Needs-Triage This issue needs to be labelled labels May 5, 2022
@TheRawMeatball TheRawMeatball self-requested a review May 5, 2022 09:19
@DJMcNab DJMcNab mentioned this pull request May 24, 2022
Closed
bors bot pushed a commit that referenced this pull request May 30, 2022
@DJMcNab
Allow higher order systems (#4833)
80b08ea 
# Objective

- Higher order system could not be created by users.
- However, a simple change to `SystemParamFunction` allows this.
- Higher order systems in this case mean functions which return systems created using other systems, such as `chain` (which is basically equivalent to map)

## Solution

- Change `SystemParamFunction` to be a safe abstraction over `FnMut([In<In>,] ...params)->Out`.
- Note that I believe `SystemParamFunction` should not have been counted as part of our public api before this PR.
    - This is because its only use was an unsafe function without an actionable safety comment.
    - The safety comment was basically 'call this within bevy code'.
    - I also believe that there are no external users in its current form. 
        - A quick search on Google and in the discord confirmed this.

## See also

- #4666, which uses this and subsumes the example here

---

## Changelog

### Added

- `SystemParamFunction`, which can be used to create higher order systems.
alice-i-cecile
alice-i-cecile approved these changes May 30, 2022
View reviewed changes
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 is a much simpler API, and should significantly simplify working with system chaining elsewhere.

It makes sense that we can do this now that systems as closures is supported.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label May 30, 2022
@alice-i-cecile
Copy link
Member

nil / nile 🧪: it'd need TAIT, not gat s
[2:14 PM] nil / nile 🧪: and so yes, personally I don't think it should be merged in yet
[2:15 PM] nil / nile 🧪: it

  • doesn't work for dynamic systems
  • worsens the userspace ergonomics
    [2:16 PM] nil / nile 🧪: and while cleaner code is always a plus, given that it's a piece of code not much else depends on i don't see the point in merging it yet

From @TheRawMeatball on Discord

In light of that, I think it's reasonable to block this on the TAIT RFC and the associated Rustlang issue.

james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
@DJMcNab @james7132
Allow higher order systems (bevyengine#4833)
f44721c 
# Objective

- Higher order system could not be created by users.
- However, a simple change to `SystemParamFunction` allows this.
- Higher order systems in this case mean functions which return systems created using other systems, such as `chain` (which is basically equivalent to map)

## Solution

- Change `SystemParamFunction` to be a safe abstraction over `FnMut([In<In>,] ...params)->Out`.
- Note that I believe `SystemParamFunction` should not have been counted as part of our public api before this PR.
    - This is because its only use was an unsafe function without an actionable safety comment.
    - The safety comment was basically 'call this within bevy code'.
    - I also believe that there are no external users in its current form. 
        - A quick search on Google and in the discord confirmed this.

## See also

- bevyengine#4666, which uses this and subsumes the example here

---

## Changelog

### Added

- `SystemParamFunction`, which can be used to create higher order systems.
@alice-i-cecile alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! and removed X-Controversial There is active debate or serious implications around merging this PR labels Jan 8, 2023
alice-i-cecile
alice-i-cecile requested changes Jan 8, 2023
View reviewed changes
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.

We should swap this back to a method, as GATs have landed 🎉

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Jan 8, 2023
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@DJMcNab @ItsDoot
Allow higher order systems (bevyengine#4833)
59a732e 
# Objective

- Higher order system could not be created by users.
- However, a simple change to `SystemParamFunction` allows this.
- Higher order systems in this case mean functions which return systems created using other systems, such as `chain` (which is basically equivalent to map)

## Solution

- Change `SystemParamFunction` to be a safe abstraction over `FnMut([In<In>,] ...params)->Out`.
- Note that I believe `SystemParamFunction` should not have been counted as part of our public api before this PR.
    - This is because its only use was an unsafe function without an actionable safety comment.
    - The safety comment was basically 'call this within bevy code'.
    - I also believe that there are no external users in its current form. 
        - A quick search on Google and in the discord confirmed this.

## See also

- bevyengine#4666, which uses this and subsumes the example here

---

## Changelog

### Added

- `SystemParamFunction`, which can be used to create higher order systems.
@DJMcNab DJMcNab added S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Feb 6, 2023
@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Mar 2, 2023
@alice-i-cecile
Copy link
Member

This is no longer blocked! Want to rebase, or should I stick an Adopt-Me on this?

@DJMcNab
Copy link
Member Author

DJMcNab commented Mar 2, 2023

Can you clarify what changed to unblock it?

@alice-i-cecile
Copy link
Member

Stageless merged :)

@DJMcNab
Copy link
Member Author

DJMcNab commented Mar 2, 2023

I must admit, I'm still not following how that solves the issue that we can't add the chain method without TAIT, unless we do boxing?

Was the decision made in stageless to just do the boxing?

@alice-i-cecile
Copy link
Member

Oh right, I remember this discussion now. Sorry for the confusion. Still blocked unfortunately.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Mar 2, 2023
@richchurcher
Copy link
Contributor

And, sadly still blocked! Closing as part of backlog cleanup. Feels like the .chain() syntax is fairly stable now, I guess? Last comment on the tracking thread was Dec '23, apparently there are still issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile requested changes

@TheRawMeatball TheRawMeatball Awaiting requested review from TheRawMeatball

@james7132 james7132 Awaiting requested review from james7132

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change D-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Blocked This cannot move forward until something else changes
Projects
Scheduling
Status: Open PRs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DJMcNab @alice-i-cecile @richchurcher