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

[ BUG ] "reason":"STOPPED" instead of "reason":"FINISHED" #78

Closed
dakata1337 opened this issue Nov 12, 2020 · 13 comments
Closed

[ BUG ] "reason":"STOPPED" instead of "reason":"FINISHED" #78

dakata1337 opened this issue Nov 12, 2020 · 13 comments
Assignees
Labels
🐞Buggy Bug Buggy bug buggies.

Comments

@dakata1337
Copy link


Describe the bug/issue.

My discord bot has TrackEnded function. This function is triggered by Victoria when a track ends. In this function i have a check like this:

if (!args.Reason.ShouldPlayNext())
          return;

With the latest version of Lavalink when the track ends and the reason is "STOPPED". I downgraded to Lavalink 3.3.1.4 and the reason changed to "FINISHED" (as it should be).
No errors shown in Lavalink.

Stacktrace / Screenshots

With Lavalink 3.3.2.1:
[Victoria] {"op":"event","reason":"STOPPED","type":"TrackEndEvent","track":"","guildId":""}

With Lavalink 3.3.1.4:
[Victoria] {"op":"event","reason":"FINISHED","type":"TrackEndEvent","track":"","guildId":""}

@dakata1337 dakata1337 added the 🐞Buggy Bug Buggy bug buggies. label Nov 12, 2020
@dakata1337 dakata1337 changed the title [ BUG ] [ BUG ] "reason":"STOPPED" instead of "reason":"FINISHED" Nov 12, 2020
@FireController1847
Copy link

Was this issue resolved then? What's the status?

@Yucked
Copy link
Owner

Yucked commented Dec 2, 2020

I thought it got pushed but apparently not. Lemme get on that.

@Yucked Yucked reopened this Dec 2, 2020
@Yucked
Copy link
Owner

Yucked commented Dec 2, 2020

Was this issue resolved then? What's the status?

It should be live now. Apparently nuget key expired.

@Yucked Yucked closed this as completed Dec 2, 2020
@FireController1847
Copy link

Glad to hear, I'll test it soon. Thanks a ton :)

@dakata1337
Copy link
Author

@Yucked I updated to 5.1.10 still same issues.

@Yucked Yucked reopened this Dec 5, 2020
@Yucked
Copy link
Owner

Yucked commented Dec 5, 2020

Just realized you failed to provide crucial information: https://github.com/Frederikam/Lavalink/issues/408

@Yucked
Copy link
Owner

Yucked commented Dec 5, 2020

@FireController1847 @dakata1337 This isn't an issue on Victoria's end. ShouldPlayNext() is an extension method. You are not required to use it which is why Victoria gives you the flexibility you need. You can create your own extension method or handle each case the way you want to:

The code of extension method is fairly straight forward:

        public static bool ShouldPlayNext(this TrackEndReason trackEndReason) {
            return trackEndReason == TrackEndReason.Finished || trackEndReason == TrackEndReason.LoadFailed;
        }

You can add your own check if you want, you can use switch clause, if-else statements. The provided is default. Most of the stuff provided in Victoria is default such as DefaultQueue, ArtworkResolver, LyricsResolver.

Lavalink also doesn't mention anything in changelogs regarding that: https://github.com/Frederikam/Lavalink/commits/master/IMPLEMENTATION.md

Now if I were to also include TrackEndReason.Stopped, if someone was to call StopAsync it will start playing the next song if any. That's not what STOPPED is meant to do.

I apologize about me taking so long to get on it as I've work and college but please next time do provide complete info as to what you're doing. If Lavalink implements a breaking change, doesn't mention it in implementation or change log, there is no way for library supporters to know what got changed.

Changelog doesn't mention that change either if https://github.com/Frederikam/Lavalink/blob/master/CHANGELOG.md

@FireController1847
Copy link

Very interesting. I didn't know about this misunderstanding of stopped vs finished. This may be unrelated to the issue I was having, then. I use ShouldPlayNext() to determine, of course, whether or not to play the next song, or to just stop playing entirely. I'm still not sure I understand when stopped vs finished is called, however. Though maybe my issue is unrelated to this issue... I'll do some more investigation and if I see my issue continue I will report a proper bug report to the appropriate place, if it is even a bug on this end or on my end.

What I do know, however, is that updating fixed the issue for me :)

@Yucked
Copy link
Owner

Yucked commented Dec 5, 2020

I'm currently following the Lavalink issue dakata1337 opened and did ask him few questions. STOPPED was supposed to send only and only if you called StopAsync which meant to completely stop the player. But now, Lavalink is sending STOPPED for every normal track as well which is obviously going to make it extremely difficult to determine whether a user wants to stop the player all together or play the next song.

@dakata1337
Copy link
Author

I'm currently following the Lavalink issue dakata1337 opened and did ask him few questions. STOPPED was supposed to send only and only if you called StopAsync which meant to completely stop the player. But now, Lavalink is sending STOPPED for every normal track as well which is obviously going to make it extremely difficult to determine whether a user wants to stop the player all together or play the next song.

Any updates?

@Yucked
Copy link
Owner

Yucked commented Dec 15, 2020

@dakata1337

I don't manage Lavalink. According to that thread, Lavalink is sending wrong TrackFinished for tracks less than <=5 seconds.

The solution was also provided above.

@dakata1337 This isn't an issue on Victoria's end. ShouldPlayNext() is an extension method. You are not required to use it which is why Victoria gives you the flexibility you need. You can create your own extension method or handle each case the way you want to:

The code of extension method is fairly straight forward:

        public static bool ShouldPlayNext(this TrackEndReason trackEndReason) {
            return trackEndReason == TrackEndReason.Finished || trackEndReason == TrackEndReason.LoadFailed;
        }

You can add your own check if you want, you can use switch clause, if-else statements. The provided is default. Most of the stuff provided in Victoria is default such as DefaultQueue, ArtworkResolver, LyricsResolver.

Lavalink also doesn't mention anything in changelogs regarding that: https://github.com/Frederikam/Lavalink/commits/master/IMPLEMENTATION.md

Now if I were to also include TrackEndReason.Stopped, if someone was to call StopAsync it will start playing the next song if any. That's not what STOPPED is meant to do.

I apologize about me taking so long to get on it as I've work and college but please next time do provide complete info as to what you're doing. If Lavalink implements a breaking change, doesn't mention it in implementation or change log, there is no way for library supporters to know what got changed.

Changelog doesn't mention that change either if https://github.com/Frederikam/Lavalink/blob/master/CHANGELOG.md

@Yucked
Copy link
Owner

Yucked commented Dec 22, 2020

Please make sure v5.1.11 fixes your issue.

@dakata1337
Copy link
Author

Please make sure v5.1.11 fixes your issue.
v5.1.11 fixed all the issues i've had.
Thank you for all the help @Yucked!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞Buggy Bug Buggy bug buggies.
Projects
None yet
Development

No branches or pull requests

3 participants