FlxSound: Add loopCount and loopUntil

[Geokureli]

Alternative to #3478

Test state:

import flixel.FlxG;
import flixel.sound.FlxSound;

class SoundLoopTestState extends flixel.FlxState
{
    var sound:FlxSound;
    override function create()
    {
        super.create();
        
        sound = FlxG.sound.load("assets/sounds/long-sound.mp3", 1.0);
        sound.looped = true;
        sound.loopUntil = 2;
        sound.onComplete = ()->trace('Completed, loops: ${sound.loopCount} < ${sound.loopUntil}');
        sound.play();
    }
    
    override function update(elapsed)
    {
        super.update(elapsed);
        
        if (FlxG.keys.justPressed.SPACE)
            sound.play(true); // sets loopCount back to 0
    }
}

We can't unit test sounds, for now

[MondayHopscotch]

Haven't run the code locally (looking at this from my phone), but the code looks good. I use my own sound middleware, so I am not experienced with FlxSound - but this seems very nice to spare folks from having to track loop counts themselves.

👍

[glowsoony]

As in the original PR I made, it would technically make sense to keep the sound looped instead of freely allowing the loopUtil and loopCount. This would otherwise be confusing due to the fact the variables are connected to looped, in a way, but that's just my opinion

[Geokureli]

As in the original PR I made, it would technically make sense to keep the sound looped instead of freely allowing the loopUtil and loopCount. This would otherwise be confusing due to the fact the variables are connected to looped, in a way, but that's just my opinion

I don't follow, I don't quite know what "freely allow" means in this context and I don't see any significant difference in behavior between this approach and yours. if possible use a code snippet and say what the behavior is compared to what you would prefer

[BernardoGP4504]

As in the original PR I made, it would technically make sense to keep the sound looped instead of freely allowing the loopUtil and loopCount. This would otherwise be confusing due to the fact the variables are connected to looped, in a way, but that's just my opinion

I don't follow, I don't quite know what "freely allow" means in this context and I don't see any significant difference in behavior between this approach and yours. if possible use a code snippet and say what the behavior is compared to what you would prefer

He means like, make loopUntil be related to looped.

Either only have loopUntil and loopCount work when looped is true, or set looped to true when any sort of looping happens because of loopUntil. This all so looped doesn't lose its meaning

[Geokureli]

When making this I thought requiring looped = true seems like an unnecessary step, and requiring both would lead to confusion. If people feel the opposite is more confusing, I can revisit this.

For example the following code would not loop, but IMO the dev had the intent to loop twice:

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.loopUntil = 2;
sound.play();

I can't foresee a case where a dev might ever want to set looped to false while also setting a loop count. @MondayHopscotch I'd appreciate your thoughts as well

Whichever we go with, I think a clarifying doc will help avoid most confusion

[BernardoGP4504]

When making this I thought requiring looped = true seems like an unnecessary step, and requiring both would lead to confusion. If people feel the opposite is more confusing, I can revisit this.

For example the following code would not loop, but IMO the dev had the intent to loop twice:

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.loopUntil = 2;
sound.play();

I can't foresee a case where a dev might ever want to set looped to false while also setting a loop count. @MondayHopscotch I'd appreciate your thoughts as well

Then ig the least confusing approach would be automatically toggling looped once loopUntil gets changed or finishes

[Geokureli]

Then ig the least confusing approach would be automatically toggling looped once loopUntil gets changed or finishes

That's a fair point, I automatically toggle looped off when the loopCount reaches loopUtil, but not any other time. I think I'll keep looped alone, and require looped == true as suggested

[MondayHopscotch]

I'm just thinking through how you'd use it. Seems like these are the intended uses:

Play a sound 3 times in row (initial play + 2 loops):

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.looped = true;
sound.loopUntil = 2;
sound.play();

Play sound that loops forever:

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.looped = true;
sound.play();

To re-play a sound, since it resets, you just call play again:

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.looped = true;
sound.loopUntil = 2;
sound.play();
.
. // Time passes
.
sound.play();

Does that sound right?

[BernardoGP4504]

I'm just thinking through how you'd use it. Seems like these are the intended uses:

Play a sound 3 times in row (initial play + 2 loops):

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.looped = true;
sound.loopUntil = 2;
sound.play();

Play sound that loops forever:

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.looped = true;
sound.play();

To re-play a sound, since it resets, you just call play again:

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.looped = true;
sound.loopUntil = 2;
sound.play();

// Time passes

sound.play();

Does that sound right?

Yep, that's it

[Geokureli]

To re-play a sound, since it resets, you just call play again:

sound = new FlxSound();
sound.load("assets/sounds/my-sound.mp3");
sound.looped = true;
sound.loopUntil = 2;
sound.play();

// Time passes

sound.play();

Does that sound right?

Note that in the last example the second play() will not play 3 more loops, it will play once. If you want to play all three again you would call sound.play(true);

[Geokureli]

I don't know what side eye emoji means, can I some more concrete feedback on that?

[glowsoony]

I don't know what side eye emoji means, can I some more concrete feedback on that?

It isn't side eye, it just represents observing and in this context, it's about how the topic just interesting and amazing to acknowledge, so in this case, the feedback would be that the code looks very good and seems ready to already be implemented

[MondayHopscotch]

I meant it as I'll look at it... but then I didn't go look at it because I got busy/distracted. If I'm understanding correctly, the first time you play a sound that was set up to be looped, it loops.

After that, play() will play it once, and you have to do play(true) to have it do the "looped" version of it again.

It feels a little weird, but at the same time not something that is hard to manage. I feel that a perfect world is that calling play() would do the looped version every time, since that's what the sound was configured to do.

[glowsoony]

Not really, since play just replays the sound, it shouldn't impact the loops at all, with true it restarts it forcibly which would make sense to then have the looped version again because it cleansup the sounds, hence the loops reset to 0 so it would make sense

[MondayHopscotch]

It feels odd to me that:

trace(sound.looped); // true
trace(sound.loopUntil); // 2
sound.play();

Means it plays and loops twice (plays 3 times)

But later on

trace(sound.looped); // true
trace(sound.loopUntil); // 2
sound.play();

Means it plays just once.

If the key here that I'm missing is that loopCount == 2 when the second call is made, I think I could be convinced that the pattern reasonable. That makes the assumption based on the current state of the sound rather than the input configuration of the sound.

[Geokureli]

It feels odd to me that:

trace(sound.looped); // true
trace(sound.loopUntil); // 2
sound.play();

Means it plays and loops twice (plays 3 times)

But later on

trace(sound.looped); // true
trace(sound.loopUntil); // 2
sound.play();

Means it plays just once.

If the key here that I'm missing is that loopCount == 2 when the second call is made, I think I could be convinced that the pattern reasonable. That makes the assumption based on the current state of the sound rather than the input configuration of the sound.

A fair point to bring up, my reasoning behind this is that when you call play() with a forceRestart of false while it's playing, nothing changes, so play already checks the current state.

Another option, is to reset loopCount to 0 on play calls after it finished looping. This matches the existing behavior where play(false) calls on completed sounds behave the same as play(true). The arg is forceRestart, and, currently play(false) on a completed sound "restarts" it. (actually it is just started, and it's time is set to 0 on completion)

Curent way:

// restart loops before completion
play(true);
// restart loops after completion
play(true);
// play once more after completing(assuming anyone specifically wants this)
play();

The way mentioned above:

// restart loops before completion
play(true);
// restart loops after completion
play(); // or play(true);
// play once more after completing loops
loopsUntil = 0;
play(true);

The existing doc for play is ambiguous, here. But I notice that play(false) on a completed sound starts at 0 by default and when it's playing, they simply unpause at their current point.

[Geokureli]

Another option, is to reset loopCount to 0 on play calls after it finished looping. This matches the existing behavior where play(false) calls on completed sounds behave the same as play(true). The arg is forceRestart, and, currently play(false) on a completed sound "restarts" it. (actually it is just started, and it's time is set to 0 on completion)

done