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

Fix events on sounds #378

Merged
merged 7 commits into from Jul 18, 2019

Conversation

@jabdoa2
Copy link
Collaborator

commented Jul 14, 2019

No description provided.

@jabdoa2 jabdoa2 added the bug label Jul 14, 2019

@jabdoa2 jabdoa2 added this to the 0.53 milestone Jul 14, 2019

@jabdoa2 jabdoa2 requested review from avanwinkle and qcapen Jul 14, 2019

@qcapen

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

This fix will work if None is not a setting value that can be overridden in the sound_player (frequently desirable with some settings like max_queue_time). My original goal with the sound_player was to allow the user to override just about any setting in the sound, including None values. To do that, I created default values that would make it obvious that they were not provided by the user, but rather were provided by default during the config validation process. Then the sound_player config validation code could simply delete them from the config. I really thought this was working properly at one time. What is strange is it seems those dummy default values (like 'use_sound_setting') would get deleted from the sound_player config during validation, but then were somehow still getting posted during the config player event. That is the part I could not track down yet. One easy (but not so elegant) solution is to put the dummy value detection code in the SoundInstance constructor (rather than in the validate_config_item code in the sound_player). Perhaps I'm overthinking this and not allowing None as an overridden value is just fine.

@jabdoa2

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 14, 2019

Good point. Maybe we should just change the semantics and disallow "None" as a valid option? max_queue_time 0 makes sense. -1 somehow makes sense but "unlimited" would be better. More me "None" does not have a clear semantic except that is is not set. What would overwriting with "not set" mean? Not sure

@qcapen

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2019

I'm going to make a few more changes to this branch to get the behavior I originally had in mind (there will be a corresponding branch in mpf for a few config_spec changes).

qcapen added some commits Jul 17, 2019

Remove custom _validate_config_item function
This functionality was moved into SoundInstance constructor
@qcapen

qcapen approved these changes Jul 17, 2019

Copy link
Collaborator

left a comment

Made some additional changes to restore the original behavior I had intended (ability to override using a None value for settings where it makes sense). There are corresponding changes to the config_spec in mpf (same branch name).

@avanwinkle
Copy link
Collaborator

left a comment

Cool! I like big blocks of red that clear up old functionality!

mpfmc/assets/sound.py Outdated Show resolved Hide resolved

@qcapen qcapen merged commit 9fcfe03 into dev Jul 18, 2019

2 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@qcapen qcapen deleted the fix_events_on_sounds branch Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.