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

[Merged by Bors] - Down with the system! #2496

Closed
wants to merge 12 commits into from
Closed

[Merged by Bors] - Down with the system! #2496

wants to merge 12 commits into from

Conversation

mockersf
Copy link
Member

Objective

  • Remove all the .system() possible.
  • Check for remaining missing cases.

Solution

  • Remove all .system(), fix compile errors
  • 32 calls to .system() remains, mostly internals, the few others should be removed after Run criteria multipiping #2446

@github-actions github-actions bot added S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 17, 2021
@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 and removed S-Needs-Triage This issue needs to be labelled S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work labels Jul 17, 2021
@alice-i-cecile alice-i-cecile added this to the Bevy 0.6 milestone Jul 17, 2021
@cart
Copy link
Member

cart commented Jul 17, 2021

It's so ... beautiful.

@alice-i-cecile
Copy link
Member

What sort of internal .system calls do we have left that won't be cleaned up by #2446?

@Ratysz
Copy link
Contributor

Ratysz commented Jul 17, 2021

Hahah, I'm sitting on a very similar changeset. Couple differences: started on updating the docs and removed IntoSystem from the prelude, which soft-deprecates it. Wanted to also rewrite make_parallel!() into less of an atrocity.

What sort of internal .system calls do we have left that won't be cleaned up by #2446?

Code that tests if a function is a system.

MinerSebas
MinerSebas reviewed Jul 17, 2021
View reviewed changes
@mockersf
Copy link
Member Author

Wanted to also rewrite make_parallel!() into less of an atrocity.

Ended up doing that to remove the macro, but there's still an issue: system dependency cycles now stack overflow instead of panicking

@Ratysz
Copy link
Contributor

Ratysz commented Jul 17, 2021

I have no idea why that's happening... Have to sleep first, I guess.

@mockersf
Copy link
Member Author

there was an infinite loop of two traits calling each other name method that needed a .system() to return something concrete at some point, fixed 👍

53 .system() now!

@Ratysz
Copy link
Contributor

Ratysz commented Jul 18, 2021

Ah, yeah: that's not the .system() that needs to be removed, it's just named the same way. Weird it didn't come up sooner.

@Nilirad
Copy link
Contributor

Nilirad commented Jul 18, 2021

Well, don't mind me...

proceeds to sneak in .system() in every doctest

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@mockersf mockersf force-pushed the down-with-the-system branch 2 times, most recently from 0b3f766 to b51c03c Compare July 27, 2021 22:38
@cart
Copy link
Member

cart commented Jul 27, 2021

This re-adds AppBuilder (now that we've removed it on main). Can you remove it?

@mockersf mockersf force-pushed the down-with-the-system branch from b51c03c to af53035 Compare July 27, 2021 23:17
@mockersf
Copy link
Member Author

oh messed up my rebase and re-added it, it should be good now 👍

@cart
Copy link
Member

cart commented Jul 27, 2021

bors r+

bors bot pushed a commit that referenced this pull request Jul 27, 2021
@mockersf
Down with the system! (#2496)
b724a0f 
# Objective

- Remove all the `.system()` possible.
- Check for remaining missing cases.

## Solution

- Remove all `.system()`, fix compile errors
- 32 calls to `.system()` remains, mostly internals, the few others should be removed after #2446
@bors
Copy link
Contributor

bors bot commented Jul 27, 2021

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Down with the system! [Merged by Bors] - Down with the system! Jul 27, 2021
@bors bors bot closed this Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MinerSebas MinerSebas MinerSebas left review comments

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
Projects
None yet
Milestone
Bevy 0.6
Development

Successfully merging this pull request may close these issues.
6 participants
@mockersf @cart @alice-i-cecile @Ratysz @Nilirad @MinerSebas