Yucked/Victoria

[ BUG ] Skipping to a track doesn't play it

Gameoholic opened this issue · 8 comments


Describe the bug/issue.

Whenever I try to enqueue 2 tracks, for example:
!playy happy birthday
!playy lofi beats
It plays the first one and enqueues the second one.
However, upon skipping:
!skipp
It stops the first track but doesn't play the second one. Furthermore, when I try to get the current track's name after skipping it gives the error: Object reference not set to an instance of an object.

(Version 5.2.2)

Stacktrace / Screenshots

The code:

[Command("skipp")]
        [RequireUserPermission(GuildPermission.Administrator, ErrorMessage = "You must be an admin to use this command!")]
        public async Task Skip()
        {
            var voiceState = Context.User as IVoiceState;
            if (voiceState?.VoiceChannel == null)
            {
                await ReplyAsync("you msut be connecteed to VC");
                return;
            }
            if (!_lavaNode.HasPlayer(Context.Guild))
            {
                await ReplyAsync("I am not connected");
                return;
            }
            var player = _lavaNode.GetPlayer(Context.Guild);
            if (voiceState.VoiceChannel != player.VoiceChannel)
            {
                await ReplyAsync("You must be in the same VC as me");
                return;
            }
            if (player.Queue.Count == 0)
            {
                await ReplyAsync("there are no more songsi nt he queue");
                return;
            }
            await player.SkipAsync();
            await ReplyAsync($"skipped! now playing {player.Track.Title}"); //<- this is where the error gets thrown
        }
    [Command("Playy")]
    [RequireUserPermission(GuildPermission.Administrator, ErrorMessage = "You must be an admin to use this command!")]
    public async Task PlayAsync([Remainder] string query)
    {
        if (string.IsNullOrWhiteSpace(query))
        {
            await ReplyAsync("Please provide search terms.");
            return;
        }

        if (!_lavaNode.HasPlayer(Context.Guild))
        {
            await ReplyAsync("I'm not connected to a voice channel.");
            return;
        }

        var searchResponse = await _lavaNode.SearchAsync(Victoria.Responses.Search.SearchType.YouTube ,query);
        if (searchResponse.Status == Victoria.Responses.Search.SearchStatus.LoadFailed ||
            searchResponse.Status == Victoria.Responses.Search.SearchStatus.NoMatches)
        {
            await ReplyAsync($"I wasn't able to find anything for `{query}`.");
            return;
        }

        var player = _lavaNode.GetPlayer(Context.Guild);

        if (player.PlayerState == PlayerState.Playing || player.PlayerState == PlayerState.Paused)
        {
            var track = searchResponse.Tracks.ToList()[0];
            player.Queue.Enqueue(track);
            await ReplyAsync($"Enqueued: {track.Title}");
        }
        else
        {
            var track = searchResponse.Tracks.ToList()[0];
            await player.PlayAsync(track);
            await ReplyAsync($"Now Playing: {track.Title}");
        }
    }

What version?

5.2.2

SkipAsync returns the currentTrack and the skippedTrack.

var (oldTrack, currenTrack) = await player.SkipAsync();

Though, using your method doesn't result in null ref for me.

                var (oldTrack, currenTrack) = await player.SkipAsync();
                await ReplyAsync($"Skipped: {oldTrack.Title}\nNow Playing: {player.Track.Title}");

image

There is a weird issue with skip. Sometimes (not always) when you skip a track you can get null reference here.
Version is 5.2.3.

image

08:57:32 [Debug   ] Command       : Executing "skip" for Eloncasie??#6287 in ChipBot's Cave/what-is-that-for
08:57:32 [Debug   ] LavaService   : Victoria: {"op":"event","reason":"REPLACED","type":"TrackEndEvent","track":"QAAAmwIAL1tUcmFwXSAtIFJvZ3VlIC0gVWx0aW1hdHVtIFtNb25zdGVyY2F0IFJlbGVhc2VdABJNb25zdGVyY2F0IFVuY2FnZWQAAAAAAAPoAAALaTg3THVIcElvd1EAAQAraHR0cHM6Ly93d3cueW91dHViZS5jb20vd2F0Y2g/dj1pODdMdUhwSW93UQAHeW91dHViZQAAAAAAABBo","guildId":"314734113854717952"}
08:57:32 [Debug   ] LavaService   : Victoria: {"op":"playerUpdate","state":{"connected":true,"position":0,"time":1630821454280},"guildId":"314734113854717952"}
08:57:35 [Error   ] Exception     : Victoria: Object reference not set to an instance of an object.
   at Victoria.LavaNode`1.OnDataAsync(Byte[] data) in ..\Victoria\src\LavaNode.cs:line 462
08:57:36 [Error   ] Exception     : Victoria: Object reference not set to an instance of an object.
   at Victoria.LavaNode`1.OnDataAsync(Byte[] data) in ..\Victoria\src\LavaNode.cs:line 462
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw() in System.Private.CoreLib.dll:token 0x60035f6+0x11
08:57:38 [Error   ] LavaService   : Victoria: System.NullReferenceException: Object reference not set to an instance of an object.
   at Victoria.LavaNode`1.OnDataAsync(Byte[] data) in ..\Victoria\src\LavaNode.cs:line 462
   at Victoria.LavaSocket.ReceiveAsync() in ..\Victoria\src\LavaSocket.cs:line 203
08:57:38 [Warning ] LavaService   : Victoria: Lavalink reconnect attempt #1. Waiting 10s before next attempt.
08:57:38 [Verbose ] Command       : Executed "skip" for Eloncasie??#6287 in ChipBot's Cave/what-is-that-for
08:57:48 [Info    ] LavaService   : Victoria: Websocket connection established.

My speculation is that it sets track to null when TrackEndEvent data is received since it goes before playerUpdate in logs.
And this seems to fix it (I know it's dirty). I haven't really looked at other code so it may be completely wrong thing to do.

case "TrackEndEvent": {
  var trackEndReason = (TrackEndReason)(byte)root.GetProperty("reason").ToString()[0];
  if (trackEndReason != TrackEndReason.Replaced)
  {
    player.Track = default;
    player.PlayerState = PlayerState.Stopped;
  }

My code for skip command if needed: Module and backing Method

UPDATE:
Either something was faulty in my code or the newest version (5.2.3) fixed my issue.
Thank you! I will not close the thread due to the person above me

@Eloncase The other solution is to check in trackUpdate for null track and ignore

Hi again,

I have a proposal that will fix this issue which isn't with SkipAsync exactly, it's with PlayAsync that happens to be called on SkipAsync.
It kinda changes the logic of keeping track of the position of the player, not by much but it does change it a bit.
I'll start a feature-request issue to elaborate on it and hopefully start a bit of a brainstorm session.

Edit: Link for the issue #106

Closing this issue. Please refer to #106 discussion, 095421a commit and #108.