Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement single-song repeat mode #72

Merged
merged 1 commit into from
Dec 30, 2021
Merged

Implement single-song repeat mode #72

merged 1 commit into from
Dec 30, 2021

Conversation

ProfOak
Copy link
Contributor

@ProfOak ProfOak commented Dec 23, 2021

Hey all, not sure how important this is to anyone else but I sometimes like to loop a song.

Using the command line flag -L will now loop a single song repeatedly.

Using the command line flag `-L` will now loop a single song repeatedly.
@codecov
Copy link

codecov bot commented Dec 24, 2021

Codecov Report

Merging #72 (128c633) into master (779e4fe) will decrease coverage by 0.06%.
The diff coverage is 50.00%.

Impacted Files Coverage Δ
player.c 40.35% <50.00%> (-0.58%) ⬇️

@ranma
Copy link
Collaborator

ranma commented Dec 25, 2021

Instead of adding a new flag, I think it would be reasonable that if you set -l for loop mode and request a specific subsong, that subsong will be played on loop.

@ranma
Copy link
Collaborator

ranma commented Dec 25, 2021

Actually, you can already loop a single song, e.g.: gbsplay -l foo.gbs 2 2 will loop subsong 2 repeatedly

@ProfOak
Copy link
Contributor Author

ProfOak commented Dec 26, 2021

Oh wow I totally missed that. Thanks for the tip!

@ProfOak ProfOak closed this Dec 26, 2021
@mmitch
Copy link
Owner

mmitch commented Dec 27, 2021

Actually there is a subtle difference between -L $file 3 and -l $file 3 3:

  • With -L you can switch the subsong with the n and p keys and the newly chosen subsong will also loop indefinitely.
  • With -l the other subsongs will play once until you reach song 3 again which will then loop indefinitely.

Is this a use-case that would warrant the -L?
It might be useful for situations like repeat it until you can't hear it any more; manually change subsong; goto 1.

@ProfOak
Copy link
Contributor Author

ProfOak commented Dec 29, 2021

I'll re-open and let you make the call as the project lead. As long as there's some way to loop songs I'll be happy.

For some reason this is the only gbs player that properly emulates everything thrown at it. Music players that integrate Game Music Emu fail to play songs correctly and even in some cases crash the music application. So thanks for creating and maintaining a great program!

@ProfOak ProfOak reopened this Dec 29, 2021
@ProfOak
Copy link
Contributor Author

ProfOak commented Dec 29, 2021

Is this a use-case that would warrant the -L?

Sorry I wasn't clear before. Yes your assumption is correct that this is the intended behavior. You can keep single-song-repeat mode on and change the subsong without restarting the application.

@mmitch
Copy link
Owner

mmitch commented Dec 29, 2021

I think it's a useful feature and it also seems legit to use both -l and -L together.
I'm currently thinking about how to describe the difference between both options.
What about:

-l    loop playlist
-L    loop current subsong

My intention with current subsong is to point out that it is still possible to change the subsong manually.
Would anybody besides me understand that?
What are your thoughts?

@ProfOak
Copy link
Contributor Author

ProfOak commented Dec 30, 2021

Are you suggesting a sub-range of subsongs played as a playlist? For example, suppose a gbs pack had 10 subsongs. Then the user runs the following command.

$ gbsplay -L -l songs.gbs 2 4

subsong 2 begins playing and doesn't change until the user presses n or p
n is pressed, subsong 3 plays
n is pressed, subsong 4 plays
n is pressed, would subsong 2 play in this scenario?

If that's the case I don't think this is how the -l flag currently works and might warrant a new GitHub issue to be made.

Running the command then pressing n a few times will take you to subsong 5 or later.

$ gbsplay -l songs.gbs 2 4

Only once the current subsong reaches 2 through 4, it will loop through subsongs (2-4] again.

@mmitch
Copy link
Owner

mmitch commented Dec 30, 2021

You're right: -l only loops the subsong playlist when the subsong change happens automatically. -L prevents the automatic change. So using both flags at the same time is bogus.

-l has always acted this way: First and last selected subsong could be 'overridden' by manual subsong changes. I don't know if this should change. Every change breaks somebody's workflow ;-)

I think the easiest course of action would be to just add something like -l and -L are mutually exclusive to the manpage.

But I don't know about the short help text. Should that be mentioned there? Or would it be clear that both options don't work together? (cough cough in my last post I wrote the opposite ;-)

I still like -L loop current subsong.
What about -l loop between first and last selected subsong instead of -l loop playlist?

I'll merge the change, but we could still tweak the wording.

@ranma
Copy link
Collaborator

ranma commented Jan 9, 2022

BTW I think a fair number of GBS have the subsong run forever, so if you disable the gbsplay subsong timeout using -t 0 you'll get better looping behavior for those.

With -l the other subsongs will play once until you reach song 3 again which will then loop indefinitely

That's fairly broken behavior, it should just behave like the -L option added here. :)

@mmitch
Copy link
Owner

mmitch commented Jan 9, 2022

Should the "loop single subsong" mode (-L) temporarily (regarding #74 which will add toggling of loop modes) reduce the subsong timeout (-t) to 0?
If we want to loop the subsong, we don't have to cut it short just to restart it.

ranma added a commit that referenced this pull request Jan 10, 2022
This way we can modify the timeout behavior based on loop mode.
For LOOP_SINGLE the subsong timeout is ignored since many GBS subsongs
just play forever anyway.
By letting them continue indefinitely there is no fadeout break in the
song in single song loop mode.

See #72 (comment)
@ranma
Copy link
Collaborator

ranma commented Jan 10, 2022

Should the "loop single subsong" mode (-L) temporarily (regarding #74 which will add toggling of loop modes) reduce the subsong timeout (-t) to 0?

Yes, I think it should. Implemented in 39aeb12

@mmitch
Copy link
Owner

mmitch commented Jan 22, 2022

I've added a description of the loop modes to the man page with 04d709f.
I've also changed the help text wording to match the interactive display with 857eaf9. (This also satisfies my urge to find a different wording.)

I think we're ready to close this issue or am I missing anything?
In any case thanks for the nice feature!

@mmitch
Copy link
Owner

mmitch commented Jan 25, 2022

d'oh: "merged" already means closed, I just have to set the thread to done in my notifications ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants