Add restart grace period to multiball #1862
Conversation
Allow multiballs to be restarted during grace period via add-a-ball.
mpf/devices/multiball.py
Outdated
| @@ -284,6 +302,15 @@ def event_add_a_ball(self, **kwargs): | |||
|
|
|||
| def add_a_ball(self): | |||
| """Add a ball if multiball has started.""" | |||
|
|
|||
| # remove if it exists as multiball now has multiple balls | |||
| if self.delay.check(f"multiball_{self.name}_grace_period_restart"): | |||
There was a problem hiding this comment.
I feel like this name implies that it's restarting the grace period, rather than being a grace period for a restart. For consistency, this name should be restart_grace_period—that's still a bit ambiguous, but I can't think of anything better :)
Because the delay's scope is encapsulated to self.delay, there's no need for multiball_<name>_ in the delay name.
mpf/devices/multiball.py
Outdated
| # remove if it exists as multiball now has multiple balls | ||
| if self.delay.check(f"multiball_{self.name}_grace_period_restart"): | ||
| self.delay.remove(f"multiball_{self.name}_grace_period_restart") | ||
| self.machine.events.post("multiball_{}_will_resume".format(self.name)) |
There was a problem hiding this comment.
Might as well use f-string formatting here
mpf/devices/multiball.py
Outdated
| if self.machine.game.balls_in_play == 0: | ||
| self.delay.run_now(f"multiball_{self.name}_grace_period_restart") | ||
| elif self.machine.game.balls_in_play - balls < 1: | ||
| self.machine.events.post("multiball_{}_will_end".format(self.name)) |
There was a problem hiding this comment.
I've never encountered/noticed this feature before, so I'm not sure how its used or communicated (if at all). Would there be value in including the grace period duration as an event kwarg? Does it matter to anyone?
There was a problem hiding this comment.
I'll add it in. It could be useful if the grace period is dynamically set.
Added unit tests, added argument to event, minor name changes.
There was a problem hiding this comment.
This is looking great! A few tweaks and if you are setup to run testing and prospector locally, that'll be a good safety check.
python -m unittest discover mpf/tests (from root)
prospector (also from root, after pip install prospector)
If not that's okay, we can trigger the CI to run the tests after addressing my few little comments. Thanks for this nice improvement!
mpf/devices/multiball.py
Outdated
| self.debug_log("Ball drained. MB ended.") | ||
| restart_grace_period_ms = self.config['restart_grace_period'].evaluate([]) | ||
|
|
||
| if not self.machine.game or (self.machine.game.balls_in_play - balls < 1 and restart_grace_period_ms==0): |
There was a problem hiding this comment.
The linter is going to complain here that you need spaces between values and comparators, i.e. restart_grace_period_ms == 0 and two lines down, restart_grace_period_ms > 0
There was a problem hiding this comment.
I ran the linter... Never used so added to the toolbelt for the future.
mpf/devices/multiball.py
Outdated
| self._multiball_end() | ||
| elif restart_grace_period_ms>0: | ||
| if self.machine.game.balls_in_play == 0: | ||
| self.delay.run_now(f"{self.name}_restart_grace_period") |
There was a problem hiding this comment.
Please remove {self.name} from the delay name, it's a computational cost to generate the formatted string and unnecessary because self.delay is isolated in scope to just this particular instance of multiball.
| self.assertEventCalled("multiball_mb_grace_period_restart_ended", 0) | ||
|
|
||
| self.advance_time_and_run(5) | ||
| self.assertEventCalled("multiball_mb_grace_period_restart_ended", 1) |
There was a problem hiding this comment.
For thoroughness, this test should validate the number of balls on the playfield (assertAvailableBallsOnPlayfield(1)) and that the multiball_mb_grace_period_restart_will_resume event did not get called.
mpf/tests/test_MultiBall.py
Outdated
| self.advance_time_and_run(5) | ||
|
|
||
| self.assertEventCalled("multiball_mb_grace_period_restart_will_end", 1) | ||
| self.assertEventCalled("multiball_mb_grace_period_restart_ended", 0) |
There was a problem hiding this comment.
For readability, prefer assertEventNotCalled() instead of assertEventCalled zero times. Just easier to scan the code and understand what's happening :)
mpf/tests/test_MultiBall.py
Outdated
| self.assertEventCalled("multiball_mb_grace_period_restart_ended", 0) | ||
|
|
||
| self.post_event("add_ball") | ||
| self.assertEventCalled("multiball_mb_grace_period_restart_ended", 0) |
There was a problem hiding this comment.
Best to add a single-cycle clock loop after post_event just to make sure every handler has processed. That can be run by passing no time argument: self.advance_time_and_run()
| self.assertEventCalled("multiball_mb_grace_period_restart_will_resume", 1) | ||
| self.advance_time_and_run(5) | ||
| self.assertAvailableBallsOnPlayfield(2) | ||
| self.assertEventCalled("ball_save_mb_grace_period_restart_timer_start") |
There was a problem hiding this comment.
And for thoroughness, best to test that the multiball ending events did not post here
mpf/devices/multiball.py
Outdated
| if self.machine.game.balls_in_play == 0: | ||
| self.delay.run_now(f"{self.name}_restart_grace_period") | ||
| elif self.machine.game.balls_in_play - balls < 1: | ||
| self.machine.events.post(f"multiball_{self.name}_will_end", duration=restart_grace_period_ms) |
There was a problem hiding this comment.
For clarity, I think it'd be best to name the kwarg "grace_period" instead of "duration", so there's no confusion about what that value applies to.
In this setup, the multiballwill_end event is only posted if there is a grace period, otherwise it gets skipped and just the multiballended event is sent. That seems fine to me, a designer will know whether or not a grace period exists and can manage event handlers accordingly, but I just wanted to check with you that that's your intention.
|
Happy to do it. I don't mind making the changes. It's going into a code base used by many. I haven't used the tools but will run it and get the changes made as required. |
Fixed linting, updated unit tests and renamed event kwarg names.
|
|
PR updated. |
|
Oh, I just saw that this is targeting the 0.80 branch. I tried targeting at |
Currently Stern pinball machines allow a grace period to restart a multiball for a mode that is in-progress. There are work-arounds to get this to function within the configuration but is not elegant nor very reusable. I propose this change to allow multiballs to be restarted during a grace period via add-a-ball. This grace period by default is set to 0 therefore existing configurations will continue to function as normal behavior.