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

Separate state crate #13216

Merged
merged 23 commits into from
May 9, 2024
Merged

Conversation

lee-orr
Copy link
Contributor

@lee-orr lee-orr commented May 3, 2024

Objective

Extracts the state mechanisms into a new crate called "bevy_state".

This comes with a few goals:

  • state wasn't really an inherent machinery of the ecs system, and so keeping it within bevy_ecs felt forced
  • by mixing it in with bevy_ecs, the maintainability of our more robust state system was significantly compromised

moving state into a new crate makes it easier to encapsulate as it's own feature, and easier to read and understand since it's no longer a single, massive file.

Solution

move the state-related elements from bevy_ecs to a new crate

Testing

  • Did you test these changes? If so, how? all the automated tests migrated and passed, ran the pre-existing examples without changes to validate.

Migration Guide

Since bevy_state is now gated behind the bevy_state feature, projects that use state but don't use the default-features will need to add that feature flag.

Since it is no longer part of bevy_ecs, projects that use bevy_ecs directly will need to manually pull in bevy_state, trigger the StateTransition schedule, and handle any of the elements that bevy_app currently sets up.

@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 C-Usability A simple quality-of-life change that makes Bevy easier to use X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels May 3, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 3, 2024
@alice-i-cecile alice-i-cecile added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label May 3, 2024
@alice-i-cecile alice-i-cecile self-requested a review May 3, 2024 20:09
Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

Looks good, much cleaner now that it's in separate files.

Proposal: move state related examples to separate folder?

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved
crates/bevy_app/src/main_schedule.rs Outdated Show resolved Hide resolved
crates/bevy_state/macros/Cargo.toml Show resolved Hide resolved
examples/ecs/sub_states.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

Yeah, we should move the examples to a separate folder now :)

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.

You need to update the publish.sh script, but otherwise this LGTM.

https://github.com/bevyengine/bevy/blob/main/tools/publish.sh

@lee-orr
Copy link
Contributor Author

lee-orr commented May 6, 2024

You need to update the publish.sh script, but otherwise this LGTM.

https://github.com/bevyengine/bevy/blob/main/tools/publish.sh

Done!

@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 May 7, 2024
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Just one quick thing before this gets merged!

tools/publish.sh Show resolved Hide resolved
@BD103 BD103 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 May 7, 2024
@BD103 BD103 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 May 7, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 8, 2024
@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label May 8, 2024
@@ -11,7 +11,7 @@ proc-macro = true
[dependencies]
bevy_macro_utils = { path = "../../bevy_macro_utils", version = "0.14.0-dev" }

syn = { version = "2.0", features = ["full"] }
syn = "2.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bad merge from main where the same thing was fixed. I think the merge might go through if this change is removed.

lee-orr and others added 2 commits May 8, 2024 14:33
@alice-i-cecile alice-i-cecile removed the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label May 9, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 9, 2024
Merged via the queue into bevyengine:main with commit 42ba9df May 9, 2024
31 checks passed
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants