Re-evaluate loops argument in Mixer::playMusic function

[cugone]

Ref: #923 comment

[cugone]

The playMusic loop arguments are specific to and part of the SDL Mixer API. Requiring the loop arguments in the base Interface should be re-evaluated.

[ldicker83]

I thought I commented on this already... hrmm.

Well anyway,

I'm in favor of removing the loops argument from playMusic and fadeInMusic -- I don't think I've ever used anything other than the default value there and it's fair to assume that music will be played continuously.

That stated, there are cases where the user will want to play a music track only once -- I can think of the case in OPHD with multiple tracks. When a track finishes, a signal is emitted and another track is started. In these cases it might make sense to just have a separate function, something like playMusicOnce. Or maybe playMusic and playMusicContinous.

[cugone]

I can see a music player-like functionality similar to Command and Conquer. The repeat setting is part of the player and can be turned on/off. The value of the button would determine if looping were enabled or not and by being an on/off toggle (like, how all music players do) it would be be default "continuous" (i.e., "as many times as allowed").

Regardless, I agree, separating out looping vs non-looping into their own functions would make things cleaner.

That said, a music player is game-specific so that'd be an OPHD thing with only supporting functions like getting loaded/available music tracks implemented on engine-side.

[DanRStevens]

Issue #944 doesn't have a clear distinction from this issue. Perhaps we should close one of them. They kind of look like duplicate issues.

[DanRStevens]

I'd be tempted to remove loop support from the backend, and dump that responsibility on the front end for playlist control.

For the simple case, to loop music they could setup a music complete callback that would just start playing the same track again.

For the more complicated case of having full playlist control, such as in Command and Conquer, the loop feature of the backend wouldn't likely be of much use, and would probably just be ignored.

I sort of feel like adding looping support to the API may actually just be adding needless complexity.

[cugone]

From an engine standpoint, looping or not would be defined at the asset level. Unreal Engine has Audio Assets that encode whether or not they are looping and that determines how the buffer is initialized for playing. NAS2D doesn't have an asset system. This allows us to punt on looping support engine-side for now and let the user use the callback system to restart the music. It might introduce a small delay but "meh".

[DanRStevens]

Was revisiting issues we haven't touched in awhile.

I'm in favour of removing looping support from the API, and instead rely on the caller to make use of the callback system to either restart the track for looped music, or to play a new track.

In terms of impact, I think such a change would not really be noticed. OPHD doesn't current have music playing during gameplay. It might be relevant if someone spends enough time at the main menu. Though, infinitely looping a single track can get pretty annoying.

Many years ago, I was at a LAN party, and someone left a DVD on the main menu, that just kept looping some intro sequence. Everyone was too busy playing some long intense game to stop the DVD, and as the game dragged on, the looped intro music in the background got really really annoying. I remember someone in a different room attempting to start a second round, and someone pleading for a pause while that damn DVD was shut off. Let's not be like that.

[ldicker83]

I'm ultimately not opposed to taking it out and requiring the user to use some form of callback. I'd rather keep it out of the assets to make it really easy to deal with stuff like this. I'd rather avoid needing to have additional tools to encode stuff like that.

SDL1/2/3 provides callback methods so we can just build that into NAS2D and have the end-user code hook into that as an event and go from there. Would also eliminate the need for the playMusicWithIntro() type function that I used to have (might still be there? Don't remember).