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

Simplify system piping and make it more flexible #8377

Merged
merged 6 commits into from
Apr 17, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Apr 13, 2023

Objective

  • Currently, it is not possible to call .pipe on a system that takes any input other than ().
  • The IntoPipeSystem trait is currently very difficult to parse due to its use of generics.

Solution

Remove the IntoPipeSystem trait, and move the pipe method to IntoSystem.


Changelog

  • System piping has been made more flexible: it is now possible to call .pipe on a system that takes an input.

Migration Guide

The IntoPipeSystem trait has been removed, and the pipe method has been moved to the IntoSystem trait.

// Before:
use bevy_ecs::system::IntoPipeSystem;
schedule.add_systems(first.pipe(second));

// After:
use bevy_ecs::system::IntoSystem;
schedule.add_systems(first.pipe(second));

@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 13, 2023

Not quite sure what to put for the migration guide. While this is technically an API breakage, I can't image a case in which anyone would actually have their code broken to use as an example.

@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 C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Apr 13, 2023
@james7132 james7132 added this to the 0.11 milestone Apr 13, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 13, 2023

I took @james7132's suggestion of merging IntoPipeSystem with IntoSystem. Note that the module hierarchy no longer makes much sense, so we should reorganize it. In particular, IntoSystem shouldn't be in function_system.rs anymore, and system_piping.rs probably doesn't need to exist.

I'm saving these changes for a follow-up so the diffs won't be horrible, as they tend to be when moving files around.

@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 Apr 14, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2023
@alice-i-cecile
Copy link
Member

I took @james7132's suggestion of merging IntoPipeSystem with IntoSystem. Note that the module hierarchy no longer makes much sense, so we should reorganize it. In particular, IntoSystem shouldn't be in function_system.rs anymore, and system_piping.rs probably doesn't need to exist.

I'm saving these changes for a follow-up so the diffs won't be horrible, as they tend to be when moving files around.

Totally agree on both the need for reorganization and to do those in separate PRs that we merge ASAP.

@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 17, 2023

Totally agree on both the need for reorganization and to do those in separate PRs that we merge ASAP.

Sounds like a plan :)

Merged via the queue into bevyengine:main with commit b03b7b5 Apr 17, 2023
@JoJoJet JoJoJet deleted the system-pipe-flexibility branch April 17, 2023 16:37
cart pushed a commit that referenced this pull request Apr 17, 2023
# Objective

Follow-up to #8377.

As the system module has been refactored, there are many types that no
longer make sense to live in the files that they do:
- The `IntoSystem` trait is in `function_system.rs`, even though this
trait is relevant to all kinds of systems. Same for the `In<T>` type.
- `PipeSystem` is now just an implementation of `CombinatorSystem`, so
`system_piping.rs` no longer needs its own file.

## Solution

- Move `IntoSystem`, `In<T>`, and system piping combinators & tests into
the top-level `mod.rs` file for `bevy_ecs::system`.
- Move `PipeSystem` into `combinator.rs`.
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 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.

3 participants