mmitch / gbsplay

gameboy sound player
https://mmitch.github.io/gbsplay/
Other
98 stars 20 forks source link

Bugfix/repeat finite subsongs #80

Closed Erhannis closed 2 years ago

Erhannis commented 2 years ago

Finite subsongs - ones that have an actual end, rather than automatically looping - weren't looping right in "LOOP_SINGLE" mode, when initiated by the hotkey. Somehow we ended up with two places where loop_mode is stored. While I'd prefer there be only one, I'm not sure how to combine them, so I made the less invasive change of making sure both are updated together when the hotkey is pressed.

codecov[bot] commented 2 years ago

Codecov Report

Merging #80 (8c79fb5) into master (2391fee) will decrease coverage by 0.10%. The diff coverage is 0.00%.

Impacted Files Coverage Δ
gbsplay.c 37.38% <0.00%> (-0.36%) :arrow_down:
player.c 39.32% <0.00%> (-0.82%) :arrow_down:
mmitch commented 2 years ago

Thanks for reporting this – from a quick glance and first, I'd say the gbs library should only export a set_loop_mode() function, while the player code can implement the loop mode cycling. To remove the duplication, the variable in the player code should be replaced my a function call to the gbs library to retrieve the current loop mode. I'll look at it in detail when I have some more time on my hands.

mmitch commented 2 years ago

I think I have found a simple two-line fix for this: 2979b76f (where my commit message is one magnitude bigger than the diff) Could you please test the current master if it fixed the problem for you?

mmitch commented 2 years ago

ping @Erhannis : Does the fix on master solve the problem for you?

I don't want to reject your Pull Request (you put work in it), but if the master already fixes the problem, I'd prefer the smaller fix done there because it does not duplicate the cycle_loop_mode() function.

Erhannis commented 2 years ago

Oh, sorry. Yeah, it seems to; thanks. So, then - the second loop_mode in player.c is there to hold the command line flag until it can be copied into the status structure? And actually, looks like there's a number of flags like that. Seems kinda weird to store those forever, when they only need to survive common_init - dunno if it's worth messing with, though. Anyway; I think we're good, now; thanks!