Documentation mentionting chained .run_if(a).run_if(b) does not short-circuit, in contrast to run_if(a.and(b)) #21710
Conversation
|
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
added
C-Docs
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
I think mdlint is going to have words with you for your formatting, but I like the content for the most part.
The system_registry.rs changes look like they shouldn't be included in this PR.
| /// - Have independent change detection | ||
| /// - Update ticks even when skipped by conditions | ||
| /// | ||
| /// Later systems in the chain will see changes made by earlier systems while maintaining |
There was a problem hiding this comment.
Somewhere in here, I think it would be helpful to explicitly call out that this is equivalent to adding before / after system orderings.
| /// - Chained calls like `.run_if(a).run_if(b)` evaluate all conditions independently | ||
| /// - Single call like `run_if(a.and(b))` short-circuits (`b` won't run if `a` is false) | ||
| /// | ||
| /// For per-system condition evaluation, use [`distributive_run_if`]. |
There was a problem hiding this comment.
The "Note" section below already mentions distributive_run_if with a bit more context, so I'd be inclined to leave this line out (or else move the whole "Note" up here).
| /// Each individual condition will be evaluated at most once (per schedule run), | ||
| /// right before the corresponding system prepares to run. | ||
| /// | ||
| /// # Tick and Change Detection Behavior |
There was a problem hiding this comment.
If we're mentioning that each copy of the system keeps its own ticks, then it might also be worth mentioning that each one also keeps its own parameter state. Notably, MessageReader will have a separate cursor for each one, so copies of on_message may return different values.
And similarly for the mentions of tick state and change detection in fn chain().
| /// - Execute in sequential order | ||
| /// - Each maintain their own tick state | ||
| /// - Have independent change detection | ||
| /// - Update ticks even when skipped by conditions |
There was a problem hiding this comment.
Is this right? I thought systems only set their last_run tick when they actually ran, and not when skipped by conditions. Chaining them shouldn't affect that, should it?
janhohenheim
added
S-Ready-For-Final-Review
alice-i-cecile
added
S-Waiting-on-Author
|
@ZackaryRippee once CI is passing I'm content to merge this, but you may want to consider Checock's feedback too :) |
5 participants
Objective
solving issue #21244
Solution
Testing