Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

score reels no longer activate chime on score reset. #1784

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

mrechte
Copy link
Contributor

@mrechte mrechte commented Mar 22, 2024

This is to avoid noise during score reset.

Copy link

sonarcloud bot commented Mar 22, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@avanwinkle avanwinkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the improvement! I think it can be abstracted a little more to be useful in a wider range of situations, but it's a great fix!

@@ -154,7 +157,8 @@ async def _advance_reel_if_position_does_not_match(self):
while self._destination_value != self.assumed_value:
self.machine.events.post('reel_{}_will_advance'.format(self.name))
wait_ms = self.config['coil_inc'].pulse(max_wait_ms=500)
self.machine.events.post('reel_{}_advancing'.format(self.name))
if not self._quiet:
self.machine.events.post('reel_{}_advancing'.format(self.name))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice fix, but limited to your specific application. An alternative approach would be to post the event with an argument like reset: true to indicate whether the reel is resetting or not. This way there's still an event for people who want/need to know the advancement of the reel, and chimes can be made conditional on that argument to determine whether to chime or not.

@@ -75,7 +75,7 @@ def chime(cls, chime, **kwargs):
del kwargs
chime.pulse()

def set_value(self, value):
def set_value(self, value, quiet=False):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aligned with my previous comment, changing the argument from quiet to is_reset will provide usecase-independent context and help future developers understand what's happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello. Please could you help me ?
Everything starts from the score_reel_controller, that will call set_value() of ScoreReelGroup class in 3 situations:

  1. _game_starting
  2. _rotate_player
  3. score_change
    If I understand correctly, ScoreReel would always 'post reel
    {}_advancing', with an extra boolean argument 'reset'.
    Then we would modify the chime handler of ScoreReelGroup, that would not pulse if reset is True.
    How do I retrieve the reset argument in chime method ? Is it in kwargs, or do I just add a reset arg after chime ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you post an event, you can include kwargs like so:

self.machine.events.post(f'reel_{self.name}_advancing', is_reset=False)

And then in an event listener, you can make it conditional:

event_player:
  reel_myreel_advancing{not is_reset}: play_chime

Copy link

sonarcloud bot commented Jun 10, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
21.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants