[Merged by Bors] - Fixed criteria-less systems being re-ran unnecessarily #1754
Conversation
Ratysz
added
C-Bug
|
That one looks like a new lint. I don't think I should fix that as part of this PR. |
Ratysz
added
the
S-Ready-For-Final-Review
| } | ||
| } | ||
| } | ||
| if let ShouldRun::YesAndCheckAgain = stage_should_run { |
There was a problem hiding this comment.
doesn't this double-run stage_run_criteria if line 843 returns YesAndCheckAgain
There was a problem hiding this comment.
Yes... yes it does. The perils of rushed single-commit bugfixes. 6e7a51b should take care of that.
To elaborate, this is a remnant of the usual fiddling during implementation. It doesn't break any explicit invariants, hence it passing the tests; but it shouldn't have been there - I simply missed it while going over the fix one last time.
| self.stage_run_criteria.should_run(world) | ||
| { | ||
| let mut stage_should_run = self.stage_run_criteria.should_run(world); | ||
| if let ShouldRun::No | ShouldRun::NoAndCheckAgain = stage_should_run { |
There was a problem hiding this comment.
Doesn't returning here on NoAndCheckAgain mean we don't check again?
There was a problem hiding this comment.
Yes: if we check again without running anything (which a No* implies) we're stuck in a loop.
|
Tests pass and this behaves as expected for everything i throw it at. Normally I would wait for your feedback here but I'd like to test on our users asap (and you did give the go-ahead on discord for changes needed). Feel free to follow up if you have suggestions. |
|
bors r+ |
Fixes #1753. The problem was introduced while reworking the logic around stages' own criteria. Before #1675 they used to be stored and processed inline with the systems' criteria, and systems without criteria used that of their stage. After, criteria-less systems think they should run, always. This PR more or less restores previous behavior; a less cludge solution can wait until after 0.5 - ideally, until stageless. Co-authored-by: Carter Anderson <mcanders1@gmail.com>
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
|
It's much cleaner now 👍 The only thing I can add is future plans, just to have them written down somewhere relevant: with stageless, this nested looping won't be necessary, because the outer loop would be the entire schedule itself. It should make the model that much simpler, but might have implications that are hard to see right now. |
Fixes #1753.
The problem was introduced while reworking the logic around stages' own criteria. Before #1675 they used to be stored and processed inline with the systems' criteria, and systems without criteria used that of their stage. After, criteria-less systems think they should run, always. This PR more or less restores previous behavior; a less cludge solution can wait until after 0.5 - ideally, until stageless.