Non-intuitive loop setup ("infinite" vs "loop X times")
Wohlstand opened this issue · 8 comments
Original report by Anonymous.
Current API is not intuitive to set setup of looping. I mean, the setting that enables infinite loop is hidden into non-related "gme_set_fade()" call...
For example, after this commit: https://bitbucket.org/mpyne/game-music-emu/commits/49238f9ae5792e16481228916cef50c4a6c6c5a0
That causes this ZDoom/gzdoom@df2bef2 because of unknowledge of set gme_set_fade to -1 to enable infinite loop which is non-intuitive.
My suggestion is:
- Provide a separate function "gme_set_loops_count()" that will receive count of loops and -1 to loop infinite.
- Why not to keep infinite loop behavior be default to keep compatibility with software that relies on this behavior? And let everyone who want "play X times", enable that by manual way.
Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).
I'm supportive of breaking this out into a dedicated method rather than overloading gme_set_fade.
Are you saying that gme should default to looping infinitely? Or are you referring to older behavior of gme_set_fade when you talk about restoring compatibility?
Original comment by Wohlstand (Bitbucket: Wohlstand, GitHub: Wohlstand).
Yes, make the infinite loop be default behavior which was originally. Most of software are used this version before this update, rely on that it’s playing infinitely.
It’s bad strategy to overload unrelated feature into gme_set_fade as it’s NOT obvious and very hard to figure out “where is to disable/enable loop“, the fade must control the fade time only. For an ability to control loops, you better to introduce the gme_set_loop call that will not only on/off looping, and will also give an ability to choose count of loops (for now it’s possible to run only one loop).
- Adding new functions is safe for ABI
- Behavior change of existing functions - is incompatibility that will affect runtime
Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).
I looked into this a bit over the past weekend and this change was actually intentional to address issue #25.
In that case, to support acting as a music decoder for plugin-based usage, infinite looping by default is problematic.
Instead of trying to control looping a track a specific number of times, maybe we can make the default loading of track playback times optional within the library (as a global function) that way it can be set once, when the libgme is initially created.
Then the VLC devs might still need to make a fix once the previous default behavior is restored, but it wouldn’t have to be a difficult fix.
Original comment by Wohlstand (Bitbucket: Wohlstand, GitHub: Wohlstand).
Anyway, yeah, you can make the "don't loop by default” like is made on my libADLMIDI/libOPNMIDI libraries, but instead of overloading gme_set_fade please create new function(s) to control loops (for example, `gme_set_loop()` )are will be obvious to understand even you’ll keep that overloading on `gme_set_fade` for compatibility.
Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).
WIP: Add a function to avoid automatic track ending.
This maintains automatic track ending as the default to address issue #25
while making it easier to restore the previous default behavior of
having music loop infinitely.
To maintain binary compatibility on Music_Emu and its subclasses, I make
this a global library setting that can be queried and set (or reset),
instead of specific to a Music_Emu class.
This should resolve issue #29.
Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).
WIP: Add a function to avoid automatic track ending.
This maintains automatic track ending as the default to address issue #25
while making it easier to restore the previous default behavior of
having music loop infinitely.
To maintain binary compatibility on Music_Emu and its subclasses, I make
this a global library setting that can be queried and set (or reset),
instead of specific to a Music_Emu class.
This should resolve issue #29.
Original comment by Michael Pyne (Bitbucket: mpyne, GitHub: mpyne).
Add a function to avoid automatic track ending.
This maintains automatic track ending as the default to address issue #25
while making it easier to restore the previous default behavior of
having music loop infinitely.
This changes the structure of the Music_Emu object itself, but that has
not been part of the binary interface (the C API from gme.h treats
Music_Emu as an opaque handle). So as long as you don't mix two versions
of the same library in the same application somehow then this shouldn't
cause existing compiled apps to break.
Fixes issue #29. Thanks to Wohlstand for reviewing the patch!