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

Reorganize system modules #8419

Merged
merged 8 commits into from
Apr 17, 2023
Merged

Reorganize system modules #8419

merged 8 commits into from
Apr 17, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented 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.

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Apr 17, 2023
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.

I'd like to break up the top level mod.rs a great deal further, but this is incrementally better.

@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 17, 2023

Is there anything specific you'd like to be moved into different modules? As of this PR, mod.rs only contains items that, in my opinion, don't really make sense to be in their own files: assert_is_system, IntoSystem, In<T>, and the system_adapter module, plus a bunch of unit tests.

@alice-i-cecile
Copy link
Member

I think the system_adapter module should be split out: it's self-contained and quite different from the rest of the internals.

@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 17, 2023

I can split it out if you'd prefer, but I don't think it's necessary. It'd be easy to move that module into a different file but I don't think there's much reason to -- that module isn't doing anything complicated or novel, it's just a relatively short list of trivial functions, similar to common_conditions which does not have its own file.

Also, this may not go anywhere, but note that I've been working on a PR that would deprecate the system_adapter module entirely and replace it with a zero-cost abstraction (which .pipe() is not).

@alice-i-cecile
Copy link
Member

Sounds good; I'm fine to leave it alone.

@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 17, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 17, 2023
@cart cart added this pull request to the merge queue Apr 17, 2023
Merged via the queue into bevyengine:main with commit ce252f8 Apr 17, 2023
@JoJoJet JoJoJet deleted the system-modules branch April 18, 2023 14:05
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 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.

4 participants