microsoft / pxt-microbit

A Blocks / JavaScript code editor for the micro:bit built on Microsoft MakeCode
https://makecode.microbit.org
Other
721 stars 594 forks source link

Request: Use CODAL defaults for SoundEffect vibrato, tremolo, and warble steps and params #5569

Closed microbit-carlos closed 3 months ago

microbit-carlos commented 7 months ago

Currently the Sound Effect fxnSteps and fxParam are values selected by the MakeCode team in https://github.com/microsoft/pxt/blob/5e2ff19c7d44bc659b997530c96b7ac9ad6a6551/webapp/src/components/soundEffectEditor/soundUtil.ts#L24-L42:

    // These values were chosen through trial and error to get something
    // that sounded pleasing and is most like the intended effect
    switch (sound.effect) {
        case "vibrato":
            codalSound.fx = pxsim.codal.music.Effect.Vibrato;
            codalSound.fxnSteps = 512;
            codalSound.fxParam = 2;
            break;
        case "tremolo":
            codalSound.fx = pxsim.codal.music.Effect.Tremolo;
            codalSound.fxnSteps = 900;
            codalSound.fxParam = 3;
            break;
        case "warble":
            codalSound.fx = pxsim.codal.music.Effect.Warble;
            codalSound.fxnSteps = 700;
            codalSound.fxParam = 2;
            break;
    }

These values work quite well and to ensure consistency across micro:bit implementations of SoundEffect it'd be good to have them as defaults in CODAL.

We've added them as defines, so it'd be great if the MakeCode team could adopt their usage: https://github.com/lancaster-university/codal-microbit-v2/commit/9a03c691c12474852819843f21e32908ab405a2f

abchatra commented 3 months ago

@carlosperate would you be able to make a PR for this? Change looks good to me.

microbit-carlos commented 3 months ago

Unsure what's the preferred method to expose C++ define values to files in typescript. If could you let me know how you'd prefer to do that I am happy to send a PR, but the change is small enough that it might be quicker for somebody from the makecode team to create the PR themselves.

riknoll commented 3 months ago

@microbit-carlos we actually have a script to surface all of the defines in CODAL in an enum in libs/core/dal.d.ts!

No need to run it though, see the PRs I just opened for a fix for this issue

microbit-carlos commented 3 months ago

Perfect, thanks @riknoll!