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

Add non-short-circuiting run conditions and re-rename short-circuiting run conditions to be explicit #13793

Open
alice-i-cecile opened this issue Jun 10, 2024 · 9 comments
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation C-Usability A simple quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through
Milestone

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jun 10, 2024

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.

Originally posted by @alice-i-cecile in #13784 (comment)

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Jun 10, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Jun 10, 2024
@alice-i-cecile
Copy link
Member Author

@JoJoJet, spinning this out into an issue. We should resolve this one way or another for 0.15. I definitely agree with your concern, I just don't think that the names are a good way to communicate this :)

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 10, 2024
@JoJoJet
Copy link
Member

JoJoJet commented Jun 10, 2024

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 that's the name having its intended effect. The problem is people not answering with the actual reason. The name is supposed to make you go 'huh that's weird, I wonder why it's named like that' so you go looking. If there's a simple name then it's really not clear that there's a huge asterisk when it comes to the system's heavior

@CupOfTeaJay
Copy link
Contributor

CupOfTeaJay commented Jun 10, 2024

@JoJoJet
@alice-i-cecile

I would argue that most are using the combinators to just achieve some logical output, that is without taking advantage of the short-circuiting behavior - but maybe there is a better solution here:

Maybe refactor interface to be:

Combinator Short-Circuits Logical equivalent
and No A & B
and_short Yes A && B
nand No !(A & B)
nand_short Yes !(A && B)
or No A | B
or_short Yes A || B
nor No !(A | B)
nor_short Yes !(A || B)
xor No A ^ B
xnor No !(A ^ B)

This has the following advantages:

  • The name of the combinator used explicitly indicates whether or not it is short-circuiting by virtue of the _short suffix. This is much less ambiguous than _then or _else, enough to where I imagine if anybody is curious about what _short means other users who know will probably give the "because it's short-circuiting" answer.

  • No suffixes are added for non short-circuiting combinators. This should be straightforward for users who just want a simple logical output, and the intent (to my understanding) of the original pull request.

Disadvantages:

  • This would probably bloat the interface some more.

@alice-i-cecile
Copy link
Member Author

For context, the short circuiting is actually mostly a performance optimization, rather than a deliberate behavioral choice.

@CupOfTeaJay
Copy link
Contributor

For context, the short circuiting is actually mostly a performance optimization, rather than a deliberate behavioral choice.

Right - that's exactly what I thought. I imagine @JoJoJet 's concern is that perhaps there are users who deliberately want to take advantage of the behavior, and have it not be ambiguous.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Jun 10, 2024

If we end up introducing non-short-circuiting variants, I think those should be the ones to get the longer name, as they're generally going to be dispreferred. Shortcircuiting is a real footgun, but it's rarely stumbled on.

After seeing how run conditions are used in practice, most conditions don't use change detection or event readers, which are the main worry.

We could add docs to the built in combinators that do as well to help warn users?

@JoJoJet
Copy link
Member

JoJoJet commented Jun 10, 2024

If we end up introducing non-short-circuiting variants, I think those should be the ones to get the longer name, as they're generally going to be dispreferred

I really don't agree with this. Non short-circuiting should get the shorter name since it's more reliable and generally has fewer considerations. In general, I believe any option that is faster but requires you to know what you're doing should get a longer, more descriptive name

@alice-i-cecile
Copy link
Member Author

Hmm, fair. Let me bikeshed some names. If we add non-shortcircuiting nicely named variants I think I'm on board.

Candidates:

  • and_short: mysterious, but short to read
  • and_short_circuit: very clear, rather a mouthful
  • and_then: not obvious, same can't be used for or
  • and_&&: is this even legal? It's definitely unidiomatic
  • and_if: short, legible, weird enough to pick up that something weird is going on

I think and_if is my favorite, followed by and_short_circuit. and_short is fine, but meh. Everything else is pretty bad.

@alice-i-cecile alice-i-cecile changed the title Explain that run condition combinators are short-circuiting Add non-short-circuiting run conditions and re-rename short-circuiting run conditions to be explicit Jun 10, 2024
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR labels Jun 10, 2024
@JoJoJet
Copy link
Member

JoJoJet commented Jun 11, 2024

I think or_else and and_then are fine. It corresponds to methods on Option with similar behavior so many users will already know how it works, plus it's a non-breaking change to go back to those names

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-Docs An addition or correction to our documentation C-Usability A simple quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

No branches or pull requests

3 participants