[ PROPOSAL ] Track the player position on LavaPlayer rather than the LavaTrack.
RofLBotZ94 opened this issue · 4 comments
Proposal Description:
I noticed the issue where if there happens to be a playerUpdate event in between a TrackEndEvent and a TrackStartEvent it breaks the playback. I wondered about instead of keeping track of the player position on the LavaTrack if it wouldn't make more sense to keep track of it on the player, especially because it didn't feel right to keep track of the time when the last player update was received from LavaLink on the player but not the position itself.
So I looked at the LavaLink client example implementation provided in the IMPLEMENTATION.MD file from the LavaLink repo and sure enough they keep track of the player position in the player.
Here are the links that highlight those points in the implementation:
How it handles playerUpdate event and the method implementation that gets called to update the player
The position field on the Track seems to be used only as an indicator for its startTime (probably more helpful in the case of a livestream where you probably wouldn't want to play the track from the beginning rather than its current position on the platform it's hosted on), and also seems to give the option for the user to overwrite it.
You make a good case though Victoria has never kept track of track updates in player. I checked code prior to v5.2 and noticed I was keeping a check before setting the track property to default (095421a). That will most likely resolve it since nothing broke in 5.1.x.
I believe it will fix the issue as well. It's just that like @Eloncase mentioned, the fix feels a bit "dirty" and to me it also doesn't feel right to keep track of the player's LastUpdate on the player but not the position. The best way that I can explain it is with the analogy of reading a file with a FileStream, where the stream is the one that holds the position of the file it is currently reading/writing and this feels like the file is the one that's holding the position. Either way as long as the issue is fixed that's what matters and it's your repo so it's up to you of course, this was just more of a little brainfart that I wanted to let out. 😂
EDIT: Also if for some reason Lavalink were to have a new reason for the TrackEndEvent or simply just change the name "replace" to something else, it will probably break the playback again, until you change the code to check for the new reason, whilst if you keep track of the position on the player it wouldn't break anything, so it would possibly make it a bit more maintainable. I know it's a bit of a long shot of any of this happening and even so it might not go exactly the way I'm describing it, again just another one of those old brainfarts. 😅
I'll add it in 6.0 and keep it that way moving forward. Thank you!
Glad I could help and thank you for taking the time to hear my suggestion.