-
Notifications
You must be signed in to change notification settings - Fork 289
docs: Add Sound Effects documentation. #753
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
docs: Add Sound Effects documentation. #753
Conversation
d8a058d
to
b0edf5c
Compare
b0edf5c
to
e7ff826
Compare
@dpgeorge @jaustin @microbit-giles This PR is ready for review. As we'd like not to delay the implementation too much it'd be appreciated if any feedback could be provided soonish. |
Oh, and @microbit-matt-hillsdon, I think you might be interested to have a look at these as well at some point for the stubs. |
My main concern is that the name |
Thanks, yes, I think I'd like to have a stubs branch for this so we can spot any issues typing the API early. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great. I think SoundEffect overall is better than Effect, but one minor counter argument amongst the comments (that it confuses it more with the built-in sounds).
Still open to other suggestions for the name. I think if it were a function to create these I'd expect it to be something like audio.generate(...)
or something related to a synth. It's the act of having to name the object that's hard.
docs/audio.rst
Outdated
==================== | ||
|
||
.. py:class:: | ||
Effect(preset=None, freq_start=400, freq_end=200, duration=500, vol_start=100, vol_end=255, wave=WAVE_SQUARE, fx=None, interpolation=INTER_LINEAR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth giving some thought to how this relates to the MakeCode interface and JS function calls.
In those the wave type parameter comes first, and there's a slightly different ordering between the block presentation and the JS one. The ordering here matches the block display (aside from wave type).
I'd be in favour of moving wave=
to the first parameter and move preset=
to the end of the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that on first impressions is really attractive to duplicate the MakeCode parameter order, as that would technically offer us the advantage of copy/paste arguments from the output of the really nice MakeCode widget:
However, as the wave
, fx
, and interpolation
parameters depend on constants defined in MicroPython this is not possible, so I think we should prioritise an order that makes more sense for MicroPython users.
Although it is true, that using keyword arguments we do have the flexibility on how tutorials or the users order them.
* ``audio.Effect.CHIRP`` | ||
* ``audio.Effect.CROAK`` | ||
* ``audio.Effect.CLICK`` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really happy with including these, but I think their relationship to the built-in sounds is curious to explain. I don't know that this means we shouldn't have them, but I'm struggling to work out what to call them.
audio.SoundEffect.CROAK
for example is sort of weird to distinguish from Sound.TWINKLE
- why can one of them be used as a preset and not the other? ("because one is a SoundEffect and one is a Sound"?)
This is one downside to "SoundEffect" over pure "Effect" - it ties the built-in sounds more closely to the SoundEffects - though @microbit-carlos points out that it does make sense that a 'Sound' is made out of SoundEffects...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing built-in sounds could be rewritten as a (constant) tuple of (constant) SoundEffect
instances. This is similar to sequences of images, like all the clock faces.
Eg:
>>> audio.Sound.SPRING
(SoundEffect(...), SoundEffect(...))
Then users could really see how Sound
's are made and tweak them if they like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is something that was quite desirable, our only concern is all the extra flash usage, specially if the strings are already in CODAL and probably cannot be excluded as they are.
Would be good to get an estimation of how much extra flash space this would consume?
docs/audio.rst
Outdated
fx=audio.FX_VIBRATO, | ||
interpolation=audio.LOG | ||
)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, it's nice and clear, but also think we should give some thought to when to document the positional, not keyword options.
@microbit-matt-hillsdon when autocomplete works well with positional arguments, it can be a really smooth editing experience - are there ways we could document the code here so that we get good positional argument help during autocomplete as long as the user hasn't used any keyword arguments
(for example, if I start typing audio.play(Effect(400
will the autocomplete in the editor be showing the parameter help from freq_start (as long as we re-order the parameters so start is first))
docs/audio.rst
Outdated
while True: | ||
my_effect.freq_start=accelerometer.get_x() | ||
my_effect.freq_end=accelerometer.get_y() | ||
audio.play(my_effect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice and much clearer than also containing all the parameters and @microbit-giles I'd like us to make sure we include some nice lean examples in the edu content that encourage people to use this cleaner interface.
Okay, based on @microbit-giles and @jaustin suggestion I've renamed Also updated the default values in ce19f55 to produce this sound: https://makecode.microbit.org/_ajAW04hDL0MP |
@dpgeorge I think we are at a point were we can implement the current version of the docs (minus the addition of the pre-made built-in SoundEffects), so that we can start playing with the feature and get a feeling for it. For that I think it might be good to keep the implementation in a feature branch, so that we can still tweak it before merging it. Also we would like to of bringing the Python Editor beta with a MicroPython build with Sound Effects for workshops at EuroPython. I think it would be really cool if we could play with the SoundEffects with people at the conference and if they'd like to contribute their sounds to an example file, we could then pick some of those to use as the pre-build SoundEffects and credit them. |
How about |
docs/audio.rst
Outdated
|
||
:param preset: An existing SoundEffect instance to use as a base, its values | ||
are cloned in the new instance, and any additional arguments provided | ||
overwrite the base values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interaction with a preset and default keyword arguments is problematic.
Consider SoundEffect()
. Does that create a new instance with all default values?
Consider SoundEffect(vol_end=255)
. Does that create a new instance with all default values except vol_end
which is overridden to 255?
Consider SoundEffect(audio.SoundEffect.CLICK)
. Does that copy CLICK
and then override everything with the default values? If not, how do you explain the difference between that and SoundEffect()
behaviour?
How about changing the constructor signature to:
SoundEffect(preset=SoundEffect.DEFAULT, *, freq_start, freq_end, duration, vol_start, vol_end, wave, fx, interpolation)
Then SoundEffect.DEFAULT
has the default values for the keyword-only args as previously specified above (freq_start=500 etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider SoundEffect(). Does that create a new instance with all default values?
Yes, I think this one is straight forward.
Consider SoundEffect(vol_end=255). Does that create a new instance with all default values except vol_end which is overridden to 255?
Yes, as it would if the preset
argument didn't exist.
Consider
SoundEffect(audio.SoundEffect.CLICK)
. Does that copyCLICK
and then override everything with the default values? If not, how do you explain the difference between that andSoundEffect()
behaviour?
Okay, I could see how maybe that is a bit less obvious.
The way I would see it is that SoundEffect(audio.SoundEffect.CLICK)
is like a copy constructor, so it ignores any of the __init__
default values.
I think SoundEffect(audio.SoundEffect.CLICK, duration=2000)
could be easily read as "copy all audio.SoundEffect.CLICK
values, but overwrite duration
to 2000
, but I could see how that might not be as obvious if written as SoundEffect(duration=2000, preset=audio.SoundEffect.CLICK)
.
Then SoundEffect.DEFAULT has the default values for the keyword-only args as previously specified above (freq_start=500 etc).
The disadvantage of that is we wouldn't be able to created shorter strings for serialisation with positional arguments.
So this shorter string wouldn't eval:
SoundEffect(988, 440, 190, 255, 255, 0, 1, 8)
If the confusion arising from having a preset
argument in the constructor, what if the only way to duplicate and modify would be to use a copy()
method?
my_effect = audio.SoundEffect.CLICK.copy()
my.effect.duration = 2000
It's an extra line, but might be easier to understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SoundEffect(988, 440, 190, 255, 255, 0, 1, 8)
This won't work because 988 is not a valid preset. It would need to be SoundEffect(None, 988, 440, 190, 255, 255, 0, 1, 8)
.
Actually, the way it's implemented at the moment is that if you pass None
as the preset argument then the preset is the default set of values. And any extra keyword args override that default /None preset value.
The signature for that is:
SoundEffect(preset=None, *, freq_start, freq_end, duration, vol_start, vol_end, wave, fx, interpolation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SoundEffect(988, 440, 190, 255, 255, 0, 1, 8)
This won't work because 988 is not a valid preset. It would need to be
SoundEffect(None, 988, 440, 190, 255, 255, 0, 1, 8)
.
Yes, sorry, that would only work like that if we moved preset
to be the last parameter, as it was suggested in #753 (comment). I thought I had added a comment somewhere to indicate that having it as the last parameter would help with __repr__
, but I couldn't find it, so I probably forgot to write it down.
The more I think about it, the better the copy()
method sounds. Do you have any thoughts about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been agreed to remove the preset
constructor parameter, and add a copy()
method to be able to clone SoundEffects.
The existing |
The original proposal was to have these inside the The reason we ended up putting
For now the test branch has it as |
e36b5d8
to
14860a7
Compare
14860a7
to
63fe6a0
Compare
Merging this as it is the current state of SoundEffect in the v2.1.0-beta.1 release. |
* docs: Add Sound Effects documentation. * docs: Change proposed audio.Effect() to audio.SoundEffect() * docs: Update the proposed default values for audio.SoundEffect(). * docs: Move the SoundEffects constants from `audio` to `audio.SoundEffect`. * docs: Remove SoundEffect `preset` constructor parameter, and add `copy()` method. * docs: Change SoundEffect() `interporlation` parameter to `shape`. * docs: Remove mentions of SoundEffect presets, add ranges, example to file.
* docs: Add Sound Effects documentation. * docs: Change proposed audio.Effect() to audio.SoundEffect() * docs: Update the proposed default values for audio.SoundEffect(). * docs: Move the SoundEffects constants from `audio` to `audio.SoundEffect`. * docs: Remove SoundEffect `preset` constructor parameter, and add `copy()` method. * docs: Change SoundEffect() `interporlation` parameter to `shape`. * docs: Remove mentions of SoundEffect presets, add ranges, example to file.
* docs: Add Sound Effects documentation. * docs: Change proposed audio.Effect() to audio.SoundEffect() * docs: Update the proposed default values for audio.SoundEffect(). * docs: Move the SoundEffects constants from `audio` to `audio.SoundEffect`. * docs: Remove SoundEffect `preset` constructor parameter, and add `copy()` method. * docs: Change SoundEffect() `interporlation` parameter to `shape`. * docs: Remove mentions of SoundEffect presets, add ranges, example to file.
* docs: Add Sound Effects documentation. * docs: Change proposed audio.Effect() to audio.SoundEffect() * docs: Update the proposed default values for audio.SoundEffect(). * docs: Move the SoundEffects constants from `audio` to `audio.SoundEffect`. * docs: Remove SoundEffect `preset` constructor parameter, and add `copy()` method. * docs: Change SoundEffect() `interporlation` parameter to `shape`. * docs: Remove mentions of SoundEffect presets, add ranges, example to file.
* docs: Add Sound Effects documentation. * docs: Change proposed audio.Effect() to audio.SoundEffect() * docs: Update the proposed default values for audio.SoundEffect(). * docs: Move the SoundEffects constants from `audio` to `audio.SoundEffect`. * docs: Remove SoundEffect `preset` constructor parameter, and add `copy()` method. * docs: Change SoundEffect() `interporlation` parameter to `shape`. * docs: Remove mentions of SoundEffect presets, add ranges, example to file.
* docs: Add Sound Effects documentation. * docs: Change proposed audio.Effect() to audio.SoundEffect() * docs: Update the proposed default values for audio.SoundEffect(). * docs: Move the SoundEffects constants from `audio` to `audio.SoundEffect`. * docs: Remove SoundEffect `preset` constructor parameter, and add `copy()` method. * docs: Change SoundEffect() `interporlation` parameter to `shape`. * docs: Remove mentions of SoundEffect presets, add ranges, example to file.
* docs: Add Sound Effects documentation. * docs: Change proposed audio.Effect() to audio.SoundEffect() * docs: Update the proposed default values for audio.SoundEffect(). * docs: Move the SoundEffects constants from `audio` to `audio.SoundEffect`. * docs: Remove SoundEffect `preset` constructor parameter, and add `copy()` method. * docs: Change SoundEffect() `interporlation` parameter to `shape`. * docs: Remove mentions of SoundEffect presets, add ranges, example to file.
Proposal
The current proposal adds a new class in the audio module named
SoundEffect
, which can be played using theaudio.play()
function.The user can then create new instances, configuring them via constructor arguments, and after creation they can be modified via attributes.
A collection of a Sound Effects could be played by grouping them in a list (or something more advanced like a generator, so essentially an iterable).
Preview build of the docs: https://microbit-micropython--753.org.readthedocs.build/en/753/audio.html
In progress
Things Still Under Consideration
Effect()
might not be the best name for this featureSoundEffect()
is better but also longer (and we prefer shorter names that are easier to type for young learners)SoundEffect
name as described in comment docs: Add Sound Effects documentation. #753 (comment)interpolation
is too long, we are still looking for alternatives.curve
is being considered, but one of the options is actuallyINTER_CURVE
, so we might have to rename that to something elseramp
?Alternative Proposals
A new function to play an effect instead of creating a class instance
We know that some beginner programmers struggle with the concept of instantiating classes in variables, and then using attributes to modify them. An example of this would be NeoPixels, although on the other hand things like
Images
where a new instance is immediately used in a function has generally been okay. So:So we are considering providing a function to play a single Sound Effect:
If we adopt this we might need a better name that
play_effect(...)
as it is not that different thanplay(Effect(...))
.Implementation details not shown in the docs
Image
->MicroBitImage
, so theEffect
class could have the longerMicroBitSoundEffect
internal nameimport audio
vsfrom audio import *
vsfrom microbit import *
), so their value would have to be used directlyprint(audio.Effect(vol_start=100)
->Effect(freq_start=400, freq_end=2000, duration=500, vol_start=100, vol_end=255, wave="triangle", fx="vibrato", interpolation="log")
__repr__
and keyword arguments for__str__
?Effect(None, 400, 2000, 500, 100, 255, "triangle", "vibrato", "log")
fx
has parameters that can be tweaked.audio.play()
setswait=False
so it's a blocking function. If the CODAL call async version of play is called, and multiple calls are done inmediately one after another, they will not overlap.