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

Re-name and Extend Run Conditions API #13784

Merged
merged 8 commits into from
Jun 10, 2024
Merged

Conversation

CupOfTeaJay
Copy link
Contributor

@CupOfTeaJay CupOfTeaJay commented Jun 9, 2024

Objective

Solution

Renames the and_then / or_else run condition methods to and / or, respectively.

Extends the run conditions API to include a suite of binary logical operators:

  • and
  • or
  • nand
  • nor
  • xor
  • xnor

Testing

  • Did you test these changes? If so, how?

    • The test run_condition_combinators was extended to include the added run condition combinators. A double_counter system was added to test for combinators running on even count cycles.
  • Are there any parts that need more testing?

    • I'm not too sure how I feel about the "counter" style of testing but I wanted to keep it consistent. If it's just a unit test I would prefer simply to just assert true == combinator output or false == combinator output .
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?

    • Nothing too specific. The added methods should be equivalent to the logical operators they are analogous to (&& , ||, ^, !).
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

    • Should not be relevant, I'm using Windows.

Changelog

  • What changed as a result of this PR?

    • The run conditions API.
  • If applicable, organize changes under "Added", "Changed", or "Fixed" sub-headings

    • Changed:
      • and_then run condition combinator renamed to simply and
      • or_else run condition combinator renamed to simply or
    • Added:
      • nand run condition combinator.
      • nor run condition combinator.
      • xor run condition combinator.
      • xnor run condition combinator.

Migration Guide

  • The and_then run condition method has been replaced with the and run condition method.
  • The or_else run condition method has been replaced with the or run condition method.

Copy link
Contributor

github-actions bot commented Jun 9, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide X-Contentious There are nontrivial implications that should be thought through labels Jun 9, 2024
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.

Can you add and_then and or_else back, marking them as deprecated and calling the renamed methods in them? That will make migrations a bit easier for users and we can remove it right after it gets into a release.

I much prefer these names though, and the other logical combinations are useful enough.

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Jun 9, 2024
@CupOfTeaJay
Copy link
Contributor Author

Can you add and_then and or_else back, marking them as deprecated and calling the renamed methods in them? That will make migrations a bit easier for users and we can remove it right after it gets into a release.

I much prefer these names though, and the other logical combinations are useful enough.

Yes - will do

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 9, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 10, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Jun 10, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 10, 2024
@alice-i-cecile alice-i-cecile removed this pull request from the merge queue due to a manual request Jun 10, 2024
Co-authored-by: Andres O. Vela <andresovela@users.noreply.github.com>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 10, 2024
Merged via the queue into bevyengine:main with commit 70a38ab Jun 10, 2024
27 checks passed
@JoJoJet
Copy link
Member

JoJoJet commented Jun 10, 2024

I don't think we should rename these. The then keyword is intentionally used here to draw attention to the fact that these combinators short-circuit, which can already lead to surprising behavior when it causes systems to skip runs and retain old change ticks

@alice-i-cecile
Copy link
Member

While that's an important fact about how these work, I don't think this naming convention was doing a good job conveying this information to users. As evidenced by the fact that users regularly asked "why is it called and_then", and answerers didn't point to "oh that's because it's short-circuiting".

IMO we should use clear, simple names, and explain this behavior better in documentation and other learning material.

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-Enhancement A new feature C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants