add shots:delay_event_list to allow more delay options

[bosh]

not just switches! Based on the sequence_shot implementation of the same property name

[bosh]

Docs PR missionpinball/mpf-docs#626

[bosh]

I think this is just a generally useful feature to support, but I was wondering about one potential use case: could delay events on shots be a useful way to allow drop bank knockdowns (e.g. for ball search or in service mode) to bypass shot crediting? It seems like you might be able to get away with the custom dropdown knockdown event being reused as a delay event here.

[avanwinkle]

It surprises me that the sequence shots uses delay_event_list and delay_switch_list, that's a pattern not used anywhere else in the configs. Most just use the plural, e.g. delay_events or delay_switches.

Should we align on the standard pattern?

[bosh]

Can do :)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[bosh]

@avanwinkle I believe there is an actual reason to not name with _events as a suffix (which is why this build failed upon changing it).

In device manager it checks for config keys that x.endswith('_events') and parses the event_handler:ms format as though ms is the delay (before the handler is to be called.)

This can be confirmed in mode where the device manager yield values are being used to register handlers with delay --

self.add_mode_event_handler(
  event=event, handler=self._control_event_handler, callback=method,
  ms_delay=delay, blocking_facility=device.class_label)

But we want the event to process immediately, and the delay to last until the time expires. IMO delay is the wrong term for this whole feature on shots and shot sequences (not that I can think of better) but maybe delay was just used since most other control events do indeed use that field as a delay amount.

I feel like shot_sequence probably had the feature named awkwardly as it did specifically to avoid getting caught in the handler. Are you okay with reverting to the original PR with delay_event_list?

I guess another option could be to add delay_events to the exception list in device.py (where it skips control_events already)

[avanwinkle]

Bummer, I feel like we're so close to leveraging the existing _events syntax but just need a special case where the delay is a param passed to the handler rather than a delay in calling the handler. Would it be worth adding a special case to the mode code to handle the delay specially?

As for verbage, I introduced the word "mute" when I was working on drop targets and wanting to suppress switch hits while the drop was resetting. So perhaps mute_events would be appropriate?

[bosh]

Oops I meant to get back to this --

re: another special case in *_events -- this doesn't feel very good to me - I wish even control_events didnt have the special case.

At least control_events has a pretty standard structure that is very distinct from the rest of the *_events bindings; I think that our new case here has an identical structure to the standard case, so adding the different behavior could make it harder to track down game developer's bugs in the future.

I'm not usually an annotations guy, but I would be more in favor of something like extending the current annotation params. The annotation @event_handler(allows_delay=true) could add the method to the list of bindings to prepare then have device_manager configure the handler function based on the details of the annotation. Control_events wouldn't need a special case because it wouldnt show up the the list of annotated methods in the first place.

[bosh]

Re: mute verbiage - I'm down with it, but would you want to rename the existing usages?

We have:
sequence_shots - delay_event_list and delay_switch_list
and
shots - delay_switch already

I suppose a full renaming would allow us to fix the delay_switch name-is-single-but-actually-supports-multiple issue as well.

sequence_shots - mute_event/switch_list or just mute_events/switches
shots - mute_event/switch_list or just mute_events/switches

[avanwinkle]

Mm, now you've got me thinking and I feel like this new use case is actually closer to the control events than standard events. Most events just use a string or list of strings for event names to trigger a proscribed verb, while control events use a dictionary to specify the action and parameters.

What if the delay/mute behavior was controlled via control events with action: mute and duration: XXms?

This avoids the special formatting case and the default *_events behavior, and muting a switch/shot feels like a "control" kind of thing.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[bosh]

So switch the implementation target to a new control event type on shots?

The existing control_events for shots implicitly has an action we could call "set_state", which takes an event list and a state to set. So instead we would take a delay: property and expect state: to not be set? So then also an action property in the control events with a default of "set_state" so we dont break every existing shot control event on upgrade? (And an error if you set delay on a "set_state" or state on a "mute" action?)

Or use _control_events as a suffix (and change the exception in device_manager to check suffix instead of whole-property-name-match) and add a mute_control_events to shots? And then also rename delay_switches to mute_switches to match? And do the consistent renames on sequence_shots?

TBH after thinking through all this, I still think just avoiding the _events suffix is best (so, mute_event_list and mute_switch_list) because it reduces special cases and is the clearest naming to tell a developer what kind of structure theyre going to build. I think the suffix _events is confusing because most of the time, properties named like that are just going to be a single event, or a comma list of event names. Having the hint of _list might help some people pick up that it's different faster.

[avanwinkle]

I like the direction of inferring action: set_state for shot control_events because it leads us down a path that's consistent with how other devices use control events (counters, shakers, timers). I feel like that's an easy-enough to understand pattern and enabling action: mute for a shot is a clear expression of intent (I definitely want to steer away from "delay" because that means something very different elsewhere and just hearing it I assume it means "delay the reporting of this switch being hit by x time" and not "ignore hits to this switch for x time").

[bosh]

So:

shots:
  my_shot:
    ...
    control_events:
      - events: set_my_shot_to_my_state
        action: set_state
        state: 1
        force: True
        force_show: False
      - action: mute
        duration: 100ms
        events: mute_my_shot

and Shot::_register_control_event_handlers will split into two different handler binding sets based on action (or falling back to set_state).

Looks like shot control events also do not yet support priority setting -- it looks like shot actually supports priority (and is undocumented on the config reference, but represents an offset), so I would expect shot control event bindings to have priority of:

shot's mode's priority + shot's config priority + control event's priority property +7 (awkwardly, due to the @event_handler(7) annotation on the binding)

This would be a breaking change, but makes more sense to me than defaulting shot control events to priority 1 - the default per

mpf/mpf/core/events.py

Line 132 in a19c3a3

def add_handler(self, event: str, handler: Any, priority: int = 1, blocking_facility: Any = None,

[avanwinkle]

Yeah, I think that looks really good. I'd check out the logic_blocks to see how the control_events handle different action types.

Thanks for bearing through on this one!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud