Fix FlxSound fade problems

[IQAndreas]

Fixes a few "fade" problems I forgot to address in the previous pull request.

If the sound is paused or stops, all fade modifiers are reset.

[IQAndreas]

I'm still not sure what to do in some cases.

What if:

• The user runs fadeIn() while the sound is already playing?
• The user pauses or stops while fading?
• The user runs play() while fading? Should it cancel the fade and return to normal?
• The user fades in while it is fading out. Should it "reset to 0 volume" and fade in from there, or continue from where "fade out" left off?

[Dovyski]

I think some of your questions can be answered if we add a force param to all methods. That param means something like "just do it right now".

About your questions (assuming the force param was not added yet):

The user runs fadeIn() while the sound is already playing?

Do nothing.

The user pauses or stops while fading?

Save the current fading state. If the user resumes the sound, it keeps fading from where it was before. If play() is called, the fade state is removed and the sound plays normally.

The user runs play() while fading? Should it cancel the fade and return to normal?

Stop the fading and start playing from the beginning.

The user fades in while it is fading out. Should it "reset to 0 volume" and fade in from there, or continue from where ">fade out" left off?

Continue from where "fade out" left off.

[IQAndreas]

All of those sound like good solutions. I'll go ahead and add them to this commit when I get a few minutes (don't merge it yet).

If the user fades in while it is fading out?

Continue from where "fade out" left off.

Let's say the user is in the process of fading out, so the volume is at 0.5 (of between 0 and 1). If the user chooses to "fade in" with a duration of 2 seconds, should it take 2 seconds to go from 0.5 to 1, or calculate the time it would have taken to go from 0 to 1 and extrapolate how long it should fade in from there? (resulting in only taking 1 second to fade in)

Sorry for the hassle, but I prefer having a clear direction before coding.

[Dovyski]

Using your example, I think the "fade in" should last 2 seconds and go from 0.5 to 1. It will be simpler to code and I think is a good approach.

If I were going to call fadeIn(n), I would expect it to take n seconds to completely fade the sound in, no matter what is happening. Again if there was a force param, I would call fadeIn(n, true) to make it fade in from 0 to 1, canceling any other on going fade effect.

[IQAndreas]

It will be simpler to code and I think is a good approach.

Hehe, I agree with you that it is a better approach, but I was hoping you wouldn't think so. :P It's actually more difficult due to the way the code is set up. It's hard-coded to adjust the volume linearly:

protected var _fadeInTimer:Number;
protected var _fadeTotal:Number;

// Inside of `update()`
if(_fadeInTimer > 0)
{
    _fadeInTimer -= FlxG.elapsed;
    fade = _fadeInTimer/_fadeTotal;
    if(fade < 0) fade = 0;
    fade = 1 - fade;
}

If it were me, I would have all the fading logic and variables separated off into a separate class with a getFadeMultiplier() variable, having the class work and function internally much more like a Tween (including allowing you to adjust the easing function). Though, realistically, how often will developers want to change the easing function of a sound's volume? In such a case it would be easier to tween the "volume" property of the sound manually.

[IQAndreas]

Actually, with some clever algebra, I think I can pull it off without adding a bunch of extra variables.

This code should automatically calculate the _fadeTotal value the fade should set in order to make it seem like it continued from a certain volume rather than 0.0 (it will be shortened down and optimized once it's in the final product)

public function fadeIn(Seconds:Number):void
{
    if (_fadeOutTimer > 0)
    {
        var currentVolume:Number = _fadeInTimer/_fadeTotal;

        // Just to avoid "infinity" value for `_fadeTotal`
        //  which may occur if they fade out immediately after fading in
        //  (without an `update()` in between)
        if (currentVolume >= 1)
        {
            _fadeInTimer = 0;
            _fadeTotal = 0;
        }
        else
        {
            _fadeInTimer = Seconds;
            _fadeInTotal = Seconds / (1-currentVolume);
        }
    }
    else
    {
        _fadeInTimer = Seconds;
        _fadeTotal = _fadeInTimer;
    }

    _fadeOutTimer = 0;

    play();
}

Could someone double-check my math before I put this in a pull request?

Or is it better just to add an additional variable _fadeStartVolume?

[Dovyski]

I think this approach is better than adding another variable. About your math, it seems ok to me, but I think I found a typo:

 var currentVolume:Number = _fadeInTimer/_fadeTotal;

Shouln't it be?

 var currentVolume:Number = _fadeOutTimer/_fadeTotal;

[IQAndreas]

I added a few commits based on the points discussed earlier (it does not include this needlessly complex workaround for fading from a certain point).

One of the commits adds a public playing getter. I'm hesitant to add/remove/edit public members in this "bug fix" version of Flixel, but considering how it won't affect existing code, it's still reverse-compatible, so I added it anyway.

I also included a quick bugfix for the amplitude value. I hope no one minds that I snuck it in here, but I found no reason creating a new pull request for such a tiny issue, even though it was not really related to this one.

Shouln't it be _fadeOutTimer/_fadeTotal?

You are entirely correct. Good catch.

[IQAndreas]

Another tiny question, what if the sound has 2 seconds remaining, but the user sets the sound to fade out over a span of 4 seconds, should the sound end at 0.5 volume, or automatically adjust the time so it ends at 0 volume?

Shall we assume it's more important for the user to end the sound at volume 0, or to take n amount of seconds, regardless of whether the sound ends prematurely?

[Dovyski]

I see no problem with the playing and amplitude addition. About your question, I'm really in doubt. I think the best approach would be to automatically adjust the time so it ends at 0 volume, but it would not be the case if the sound is set to loop.

This approach, however, might add some (needless) extra complexity to the sound class. Maybe we should just end the fade process at the current volume when the sound ends.

[IQAndreas]

Actually, with some clever algebra, I think I can pull it off without adding a bunch of extra variables.

[IQAndreas]

I have been walking on eggshells to avoid adding extra variables to this function, but quite frankly, the code for tweening the volume value doesn't even belong in this class in the first place.

So I went so far as to separate off all the "tweening" values to a new, separate class, and made the code oh so much nicer and tidier. :) (it even removed several extra variables!)

Since this additional change is going to be rather different than the changes presented in this pull request, if everything so far looks good and functional, shall we merge this request and add the larger changes to a different pull request, or should I just append them to this one?

[Dovyski]

(loved the image!) 😄

I think you should append them to this one (since they are connected). Separating the tweening code will certainly make the sound class easier to undersand regarding all those fading stuff. Commit your changes so we can check it.

[IQAndreas]

Alrighty then. The proposed changes have been pushed. I also updated the Flixel Sandbox to make sure this worked properly, and it seems to be working beautifully (try interrupting a "fade out" with a "fade in" and see how it handles it just fine).

The new FlxTween system does call functions which is slightly more performance heavy than using "inline code".

However, Flash has a cap of only being able to play 32 sounds at a time. So, at the most, you will be calling 64 extra functions each frame (assuming every single sound is fading at the same time) compared to using the inline code. I don't really see this tiny performance change as an issue, but let me know if you guys disagree.

Since all the tweening is delegated to the new FlxTween class, developers can also potentially change the "tweening function" (which right now is linear, just as the FlxSound fade code was set to).

[IQAndreas]

I added the fix for Issue 120 to the pull request.

FlixelSandbox has been updated as well to test the changes (you can checkout previous versions of the sandbox if you want to switch between Flixel versions).

[IQAndreas]

Any thoughts on FlxTween? Does it make it clearer, or needlessly complex?

[Dovyski]

I really liked FlxTween. It is much better than lots of temp vars and fragmented interpolation code. It can be used in other places, such as fade effects.

This pull request was tested and looks good to me. I think you can proceed merging.

[IQAndreas]

I really liked FlxTween. It is much better than lots of temp vars and fragmented interpolation code. It can be used in other places, such as fade effects.

Great.

This pull request was tested and looks good to me. I think you can proceed merging.

It was brought to my attention that there are a few items that still need to be disposed of in the destroy() function. I'll get those last, minor changes added in first (I should have some free time to do so some time tonight).

[Dovyski]

Ok, great!

[IQAndreas]

There! All done. Every object should now clean up nicely (and without too much repetitive code).

I haven't tested the code in the Sandbox, but it compiles to a SWC without problems.

Sadly, it seems up until my last commit the pull request was automatically mergeable; now it is not. This is despite the fact that no other commit has edited FlxSound or the new class, FlxTween. Perhaps GitHub has a safeguard at 12 commits in place preventing massive commits from being made with the click of a button, requiring manual merging to prevent problems.

Just say the word and I can do the manual merge when you guys have approved the new code.

[moly]

Feel to start manually merging.

[Dovyski]

This code has already been tested, it is good to go.

[IQAndreas]

In case anyone was curious, there was actually a merge conflict in FlxSound.as (and was not a limit added by GitHub). Apparently, there had been a minor change to the class in one of the already pulled requests. Resolving the merge was a simple matter and went smoothly.

[Dovyski]

I guess it was me fixing the pan problems in the past :)

[IQAndreas]

I guess it was me fixing the pan problems in the past :)

Actually, it was the following two lines inside of FlxSound::destroy():

kill();

_sound.removeEventListener(Event.ID3, gotID3);

Which if you use "blame" (also available as a command, git blame):
https://github.com/FlixelCommunity/flixel/blame/d145e29e5815bc77967bf0fa5a70c29c51a848f5/org/flixel/FlxSound.as
line 172 in fact shows that it was @moly's fault. ;)