Skip to content

Remove OnUpdate system set #8260

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
merged 5 commits into from
Apr 4, 2023
Merged

Remove OnUpdate system set #8260

merged 5 commits into from
Apr 4, 2023

Conversation

lewiszlw
Copy link
Member

@lewiszlw lewiszlw commented Mar 30, 2023 •
edited
Loading

Objective

Solution

  • Replace OnUpdate with run_if(in_state(xxx)).

Migration Guide

  • Replace OnUpdate with run_if(in_state(xxx)).

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Mar 30, 2023
alice-i-cecile
alice-i-cecile reviewed Mar 30, 2023
View reviewed changes
@alice-i-cecile
Copy link
Member

Let's merge #7676 first, then this will no longer be controversial.

@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Mar 30, 2023
@alice-i-cecile alice-i-cecile 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 Mar 30, 2023
cart
cart approved these changes Mar 30, 2023
View reviewed changes
Copy link
Member

@cart cart left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets hold off until #7676 is merged (its currently in the merge queue). After that merge, we should adapt the distributed_run_if cases to use run_if

@alice-i-cecile alice-i-cecile removed 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 Mar 30, 2023
alice-i-cecile
alice-i-cecile reviewed Mar 30, 2023
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Mar 30, 2023
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Mar 30, 2023
View reviewed changes
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 30, 2023 •
edited
Loading

@lewiszlw could you look over the changes I made to remove distributive_run_if + the merge conflict resolution? :)

@lewiszlw
Copy link
Member Author

Yeah, definitely agreed.

@alice-i-cecile alice-i-cecile 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 Mar 31, 2023
@cart cart added this pull request to the merge queue Apr 3, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 3, 2023
@cart cart added this pull request to the merge queue Apr 4, 2023
Merged via the queue into bevyengine:main with commit 5c7abb0 Apr 4, 2023
@joseph-gio joseph-gio mentioned this pull request Apr 12, 2023
Closed
@hymm hymm mentioned this pull request Apr 16, 2023
Closed
@andriyDev andriyDev mentioned this pull request Jun 12, 2023
Closed
This was referenced Jun 28, 2023
Closed
Merged
@Selene-Amanita Selene-Amanita added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 2023
Estus-Dev pushed a commit to Estus-Dev/bevy that referenced this pull request Jul 10, 2023
@lewiszlw @alice-i-cecile
Remove OnUpdate system set (bevyengine#8260)
4441265 
# Objective

- Fixes bevyengine#8239.

## Solution

- Replace `OnUpdate` with `run_if(in_state(xxx))`.

---

## Migration Guide

- Replace `OnUpdate` with `run_if(in_state(xxx))`.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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 approved these changes

@cart cart cart approved these changes

@maniwani maniwani Awaiting requested review from maniwani

+1 more reviewer

@joseph-gio joseph-gio joseph-gio approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove OnUpdate system set
6 participants
@lewiszlw @alice-i-cecile @cart @joseph-gio @james7132 @Selene-Amanita