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

SoundEffect fx value doesn't have an effect #155

Closed
microbit-carlos opened this issue May 4, 2023 · 10 comments
Closed

SoundEffect fx value doesn't have an effect #155

microbit-carlos opened this issue May 4, 2023 · 10 comments
Milestone

Comments

@microbit-carlos
Copy link
Contributor

microbit-carlos commented May 4, 2023

No matter what the fx value is, it doesn't change the sound.

We can see in this example code (also runnable in the Python Editor simulator), that the sound is the same in all cases.

from microbit import *

fx_test = audio.SoundEffect(
    freq_start=3041,
    freq_end=3923,
    vol_start=59,
    vol_end=255,
    duration=500,
    waveform=audio.SoundEffect.WAVEFORM_SINE,
    fx=audio.SoundEffect.FX_NONE,
    shape=audio.SoundEffect.SHAPE_LINEAR,
)
audio.play(fx_test)
sleep(100)

fx_test.fx = audio.SoundEffect.FX_TREMOLO
audio.play(fx_test)
sleep(100)

fx_test.fx = audio.SoundEffect.FX_WARBLE
audio.play(fx_test)
sleep(100)

fx_test.fx = audio.SoundEffect.FX_VIBRATO
audio.play(fx_test)

fx_test.hex.zip

@microbit-carlos microbit-carlos added this to the 2.2.0-beta.1 milestone May 4, 2023
@microbit-carlos microbit-carlos added the bug Something isn't working label May 4, 2023
@dpgeorge
Copy link
Collaborator

dpgeorge commented May 8, 2023

I had a play around with this. The fx parameter is not doing anything because the "fx steps" and "fx param" parameters default to bad values. Their defaults are 24 and 1 respectively, and they cannot be changed (from Python).

Do we need to expose these "fx steps" and "fx param" parameters to Python? Or maybe give better defaults? (Or both).

@microbit-carlos
Copy link
Contributor Author

microbit-carlos commented May 15, 2023

We can use the default values used by MakeCode, but if these are going to be the "unofficial defaults" and are not exposing them to the user, we should probably include them in CODAL instead.

https://github.com/microsoft/pxt/blob/5e2ff19c7d44bc659b997530c96b7ac9ad6a6551/webapp/src/components/soundEffectEditor/soundUtil.ts#L24-L42

@microbit-carlos
Copy link
Contributor Author

@microbit-carlos microbit-carlos added CODAL and removed bug Something isn't working labels May 15, 2023
@martinwork
Copy link
Collaborator

Reported in support ticket https://support.microbit.org/helpdesk/tickets/70448 (private)

@microbit-carlos
Copy link
Contributor Author

@dpgeorge because these settings are different depending on the type of fx applied (tremolo, warble or vibrato):
https://github.com/microsoft/pxt/blob/5e2ff19c7d44bc659b997530c96b7ac9ad6a6551/webapp/src/components/soundEffectEditor/soundUtil.ts#L24-L42

CODAL doesn't have a way to set the same values as MakeCode without changing something in the CODAL API. We could either update CODAL to have something like an "effect factory", or maybe we could set up these fx-steps and fx-param values as macros defined in CODAL?
Do you have any thoughts, preferences or ideas?

@dpgeorge
Copy link
Collaborator

maybe we could set up these fx-steps and fx-param values as macros defined in CODAL?

Having simply a set of #define's for the default fx-steps/fx-param values for each of tremolo, warble, vibrato, is a good solution. That's very simple.

Then I guess we will not expose these fx-steps/fx-param values to the (Python) user, at least for now. They will be set automatically when the fx is set.

dpgeorge added a commit that referenced this issue Mar 25, 2024
Fixes issue #155.

Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
Copy link
Collaborator

In 8d9067d I made it so that when the fx is changed the param and steps values are updated to use default values corresponding to the given fx choice. These default values follow what's used by MakeCode.

@microbit-carlos
Copy link
Contributor Author

@microbit-carlos
Copy link
Contributor Author

Once the PR #213 is merged, the macros can be included from lancaster-university/codal-microbit-v2@9a03c69

@dpgeorge
Copy link
Collaborator

The above PR is merged and I think this issue can now be closed.

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

No branches or pull requests

3 participants