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 conditions #11316

Merged
merged 4 commits into from
Jan 13, 2024
Merged

Simplify conditions #11316

merged 4 commits into from
Jan 13, 2024

Conversation

Ixentus
Copy link
Contributor

@Ixentus Ixentus commented Jan 12, 2024

Objective

  • Conditions don't have to be closures unless they have state or mutate.

Solution

  • Simplify conditions when possible.

Changelog

The following run conditions are now regular systems:

  • resource_exists
  • resource_added
  • resource_changed
  • resource_exists_and_changed
  • state_exists<S: States>
  • state_changed<S: States>
  • any_with_component<T: Component>

Migration Guide

  • resource_exists() -> resource_exists
  • resource_added() -> resource_added
  • resource_changed() -> resource_changed
  • resource_exists_and_changed() -> resource_exists_and_changed
  • state_exists<S: States>() -> state_exists<S: States>
  • state_changed<S: States>() -> state_changed<S: States>
  • any_with_component<T: Component>() -> any_with_component<T: Component>

@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 targeted quality-of-life change that makes Bevy easier to use labels Jan 12, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jan 12, 2024
@alice-i-cecile
Copy link
Member

@soqb @doonv reviews? Also let me know if either of you want to be added to the Bevy org so I can do things like "request reviewer" (and so you can label things).

@Ixentus Ixentus marked this pull request as draft January 12, 2024 23:06
@Ixentus
Copy link
Contributor Author

Ixentus commented Jan 12, 2024

Alright fixed all the docs tests. Messed up the pr commits though -_-

Edit: Fixed hopefully

@Ixentus Ixentus marked this pull request as ready for review January 12, 2024 23:57
(cherry picked from commit d35c09f)
(cherry picked from commit 7361b05)
(cherry picked from commit 1a07abd)
Copy link
Contributor

@jdm jdm left a comment

Choose a reason for hiding this comment

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

Nice simplification!

@hymm
Copy link
Contributor

hymm commented Jan 13, 2024

They were originally made closures to keep the API consistent. i.e. you always call the built-in run conditions as functions when adding them in run_if.

@alice-i-cecile
Copy link
Member

They were originally made closures to keep the API consistent. i.e. you always call the built-in run conditions as functions when adding them in run_if.

I think this is a mistake overall: we should model the correct, simple behavior for users. See the previous PR by this author, who thought that it was required to use closures. It's not the first time I've seen that, and I was confused about where the misconception was coming from.

Copy link
Contributor

@nvdaz nvdaz left a comment

Choose a reason for hiding this comment

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

Docs should be updated on changed conditions (eg. here) to reflect new behavior. It could be something like

    /// A [`Condition`](super::Condition)-satisfying system that returns `true`
    ...

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Everything here is pretty clear, looks correct and improves readability and flexibility.

Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

LGTM, a clear ergonomic improvement. i think this also reduces confusion, since what is passed to run_if is more obviously a function, it's slightly easier for beginners to recognise that conditions aren't called once and then are called based on that stored flag.

btw @alice-i-cecile, i'd be happy to become an org member!

@matiqo15 matiqo15 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 Jan 13, 2024
@Ixentus
Copy link
Contributor Author

Ixentus commented Jan 13, 2024

Docs should be updated on changed conditions

Done

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 13, 2024
Merged via the queue into bevyengine:main with commit e2fd631 Jan 13, 2024
22 checks passed
@Ixentus Ixentus deleted the Simple_conditions branch January 13, 2024 13:53
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.

8 participants