Repository: missionpinball/mpf

Issue:
Stop shows with sync_ms
When playing a show with sync_ms any previous show with the same key will be stopped instantly but the new show will play in sync. Delay the stop until the new show starts. 

Discussion:

Patch:
diff --git a/mpf/assets/show.py b/mpf/assets/show.py
index 341ccac7c..1ab70ade1 100644
--- a/mpf/assets/show.py
+++ b/mpf/assets/show.py
@@ -274,7 +274,7 @@ class Show(Asset):
              events_when_looped=None, events_when_paused=None,
              events_when_resumed=None, events_when_advanced=None,
              events_when_stepped_back=None, events_when_updated=None,
-             events_when_completed=None, start_time=None) -> "RunningShow":
+             events_when_completed=None, start_time=None, start_callback=None) -> "RunningShow":
         """Play a Show.
 
         There are many parameters you can use here which
@@ -382,7 +382,8 @@ class Show(Asset):
                                    events_when_advanced=events_when_advanced,
                                    events_when_stepped_back=events_when_stepped_back,
                                    events_when_updated=events_when_updated,
-                                   events_when_completed=events_when_completed)
+                                   events_when_completed=events_when_completed,
+                                   start_callback=start_callback)
 
         if not self.loaded:
             self.load(callback=running_show.show_loaded, priority=priority)
@@ -418,7 +419,7 @@ class RunningShow(object):
                  events_when_paused, events_when_resumed,
                  events_when_advanced, events_when_stepped_back,
                  events_when_updated, events_when_completed,
-                 start_time):
+                 start_time, start_callback):
         """Initialise an instance of a show."""
         self.machine = machine
         self.show = show
@@ -430,6 +431,7 @@ class RunningShow(object):
         self.start_step = start_step
         self.sync_ms = sync_ms
         self.show_tokens = show_tokens
+        self.start_callback = start_callback
 
         self.events = dict(play=events_when_played,
                            stop=events_when_stopped,
@@ -500,12 +502,14 @@ class RunningShow(object):
 
         # Figure out the show start time
         if self.sync_ms:
-            delay_secs = (self.sync_ms / 1000.0) - (self.next_step_time % (self.sync_ms / 1000.0))
-            self.next_step_time += delay_secs
+            # calculate next step based on synchronized start time
+            self.next_step_time += (self.sync_ms / 1000.0) - (self.next_step_time % (self.sync_ms / 1000.0))
+            # but wait relative to real time
+            delay_secs = self.next_step_time - self.machine.clock.get_time()
             self._delay_handler = self.machine.clock.schedule_once(
-                partial(self._run_next_step, post_events='play'), delay_secs)
+                self._start_now, delay_secs)
         else:  # run now
-            self._run_next_step(post_events='play')
+            self._start_now()
 
     def _post_events(self, action):
         if self.events[action]:  # Should make sure this is a list? todo
@@ -575,8 +579,10 @@ class RunningShow(object):
 
         self._stopped = True
 
-        if not self._show_loaded:
-            return
+        # if the start callback has never been called then call it now
+        if self.start_callback:
+            self.start_callback()
+            self.start_callback = None
 
         self._remove_delay_handler()
 
@@ -642,6 +648,13 @@ class RunningShow(object):
         if self._show_loaded:
             self._run_next_step(post_events='step_back')
 
+    def _start_now(self) -> None:
+        """Start playing the show."""
+        if self.start_callback:
+            self.start_callback()
+            self.start_callback = None
+        self._run_next_step(post_events='play')
+
     def _run_next_step(self, post_events=None) -> None:
         """Run the next show step."""
         if post_events:
diff --git a/mpf/config_players/show_player.py b/mpf/config_players/show_player.py
index 367d87592..09c1bd4af 100644
--- a/mpf/config_players/show_player.py
+++ b/mpf/config_players/show_player.py
@@ -63,6 +63,7 @@ class ShowPlayer(DeviceConfigPlayer):
             callback = queue.clear
 
         start_step = show_settings['start_step'].evaluate(placeholder_args)
+        start_callback = None
 
         if key in instance_dict and not instance_dict[key].stopped:
             # this is an optimization for the case where we only advance a show or do not change it at all
@@ -94,7 +95,12 @@ class ShowPlayer(DeviceConfigPlayer):
                     instance_dict[key].advance()
                     return
             # in all other cases stop the current show
-            instance_dict[key].stop()
+            if show_settings["sync_ms"]:
+                # stop current show in sync with new show
+                start_callback = instance_dict[key].stop
+            else:
+                # stop the current show instantly
+                instance_dict[key].stop()
         try:
             show_obj = self.machine.shows[show]
         except KeyError:
@@ -120,7 +126,8 @@ class ShowPlayer(DeviceConfigPlayer):
             events_when_stepped_back=show_settings[
                 'events_when_stepped_back'],
             events_when_updated=show_settings['events_when_updated'],
-            events_when_completed=show_settings['events_when_completed']
+            events_when_completed=show_settings['events_when_completed'],
+            start_callback=start_callback
         )
 
     @staticmethod


Test patch:
diff --git a/mpf/tests/machine_files/shows/config/test_sync_ms.yaml b/mpf/tests/machine_files/shows/config/test_sync_ms.yaml
new file mode 100644
index 000000000..af4367d37
--- /dev/null
+++ b/mpf/tests/machine_files/shows/config/test_sync_ms.yaml
@@ -0,0 +1,26 @@
+#config_version=5
+lights:
+  light:
+    number: 1
+
+shows:
+  my_show1:
+    - duration: -1
+      lights:
+        light: red
+  my_show2:
+    - duration: -1
+      lights:
+        light: blue
+
+show_player:
+  play_show_sync_ms1:
+    my_show1:
+      key: sync_show
+      sync_ms: 250
+  play_show_sync_ms2:
+    my_show2:
+      key: sync_show
+      sync_ms: 250
+  stop_show:
+    sync_show: stop
diff --git a/mpf/tests/test_Shows.py b/mpf/tests/test_Shows.py
index 869aa1bc0..2a0f73460 100644
--- a/mpf/tests/test_Shows.py
+++ b/mpf/tests/test_Shows.py
@@ -9,7 +9,10 @@ from mpf.tests.MpfTestCase import MpfTestCase
 
 class TestShows(MpfTestCase):
     def getConfigFile(self):
-        return 'test_shows.yaml'
+        if self._testMethodName == "test_sync_ms":
+            return "test_sync_ms.yaml"
+        else:
+            return 'test_shows.yaml'
 
     def getMachinePath(self):
         return 'tests/machine_files/shows/'
@@ -589,6 +592,59 @@ class TestShows(MpfTestCase):
         self.advance_time_and_run(10)
         self.assertEqual(1, self.machine.show_player.instances['_global']['show_player']['test_show1'].next_step_index)
 
+    def advance_to_sync_ms(self, ms):
+        current_time = self.clock.get_time()
+        ms = float(ms)
+        next_full_second = current_time + ((ms - (current_time * 1000) % ms) / 1000.0)
+        self.advance_time_and_run(next_full_second - current_time)
+        delta = (self.clock.get_time() * 1000) % ms
+        self.assertTrue(delta == ms or delta < 1, "Delta {} too large".format(delta))
+
+    def test_sync_ms(self):
+        self.advance_to_sync_ms(250)
+        # shortly after sync point. start initial show
+        self.advance_time_and_run(.01)
+        self.post_event("play_show_sync_ms1")
+        self.assertLightColor("light", "off")
+        # wait for first sync point
+        self.advance_to_sync_ms(250)
+        self.advance_time_and_run(.01)
+        self.assertLightColor("light", "red")
+        # shortly after sync point again
+        self.post_event("play_show_sync_ms2")
+        # old show still playing
+        self.advance_time_and_run(.1)
+        self.assertLightColor("light", "red")
+        # until next sync point is reached
+        self.advance_time_and_run(.15)
+        self.assertLightColor("light", "blue")
+        self.post_event("stop_show")
+        self.assertLightColor("light", "off")
+
+        # play show and start second show before first was synced
+        self.advance_to_sync_ms(250)
+        self.advance_time_and_run(.01)
+        self.post_event("play_show_sync_ms1")
+        self.advance_time_and_run(.01)
+        self.post_event("play_show_sync_ms2")
+        self.assertLightColor("light", "off")
+        self.advance_to_sync_ms(250)
+        self.assertLightColor("light", "blue")
+        self.post_event("stop_show")
+        self.assertLightColor("light", "off")
+
+        # play show and start second show before first was synced. stop before second is synced
+        self.advance_to_sync_ms(250)
+        self.advance_time_and_run(.01)
+        self.post_event("play_show_sync_ms1")
+        self.advance_time_and_run(.01)
+        self.post_event("play_show_sync_ms2")
+        self.assertLightColor("light", "off")
+        self.post_event("stop_show")
+        self.assertLightColor("light", "off")
+        self.advance_time_and_run(.5)
+        self.assertLightColor("light", "off")
+
     def test_pause_resume_shows(self):
         self.machine.events.post('play_test_show1')
         # make sure show is advancing