shipgirlproject / Shoukaku

A stable, powerful and updated wrapper around Lavalink
https://guide.shoukaku.shipgirl.moe/
MIT License
273 stars 84 forks source link

Lavalink v4 #158

Closed thaddeuskkr closed 1 year ago

thaddeuskkr commented 1 year ago

Here's my attempt at implementing Lavalink v4. I followed the implementation documentation, but I can't test these changes. There are probably issues with this PR, but it should mostly work. I think. The resuming via session ID kinda confuses me too. Not too experienced with TS, merge / change / close this if y'all wish!

Quoted from Lavalink IMPLEMENTATION.md:

Significant changes v3.7.0 -> v4.0.0

v4.0.0 Migration Guide All websocket ops are removed as of `v4.0.0` and replaced with the following endpoints and json fields: * `play` -> [Update Player Endpoint](#update-player) `encodedTrack` or `identifier` field * `stop` -> [Update Player Endpoint](#update-player) `encodedTrack` field with `null` * `pause` -> [Update Player Endpoint](#update-player) `pause` field * `seek` -> [Update Player Endpoint](#update-player) `position` field * `volume` -> [Update Player Endpoint](#update-player) `volume` field * `filters` -> [Update Player Endpoint](#update-player) `filters` field * `destroy` -> [Destroy Player Endpoint](#destroy-player) * `voiceUpdate` -> [Update Player Endpoint](#update-player) `voice` field * `configureResuming` -> [Update Session Endpoint](#update-session)
1intan99 commented 1 year ago

The sessionId to resume the player should be array / object since each player has different sessionId.

And maybe need more impelentation to the sessionId resuming, I don't know how the resuming thing work on v4 lavalink right now. But maybe it sending a REST to lavalink for each player to resume the state

Deivu commented 1 year ago

The sessionId to resume the player should be array / object since each player has different sessionId.

And maybe need more impelentation to the sessionId resuming, I don't know how the resuming thing work on v4 lavalink right now. But maybe it sending a REST to lavalink for each player to resume the state

or just put it in the node instead, it shoudnt be on the main shoukaku class anyways as the sessionId is unique per each node

thaddeuskkr commented 1 year ago

resolved, but the session ids do confuse me, how would shoukaku resume without a user input - of like session id or something? the current state of the branch does work, i cloned kongou and tweaked the code abit to test. but im not sure if resuming works, don't know how to test it.

thaddeuskkr commented 1 year ago

Additional note, Line 38 https://github.com/thaddeuskkr/Shoukaku/blob/bdd4caffe150895e8a20306ba3814fb21659b171/src/node/Node.ts#L38

Should also reflect the new changes

Fixed in d4735d5.
I've also updated the load types to use enums, as you suggested.

thaddeuskkr commented 1 year ago

Could resuming be configured via NodeOption by any chance? Then in the constructor for Node:

this.sessionId = options.sessionId || null;
Deivu commented 1 year ago

Could resuming be configured via NodeOption by any chance? Then in the constructor for Node:

this.sessionId = options.sessionId || null;

that's what i plan, and probably players as well if you saved it

thaddeuskkr commented 1 year ago

Could resuming be configured via NodeOption by any chance? Then in the constructor for Node:

this.sessionId = options.sessionId || null;

that's what i plan, and probably players as well if you saved it

Would this be simply fixed by adding two new NodeOptions then? In that case, I'd be able to do it as well.

    constructor(manager: Shoukaku, options: NodeOption) {
        ...
        this.players = options.players ?? new Map();
        ...
        this.sessionId = options.sessionId || null;
    }
export interface NodeOption {
    ...
    /**
     * A map of guild IDs to players to resume
     */
    players?: Map<string, Player>;
    /**
     * The session id for lavalink to resume
     */
    sessionId?: string;
}
Deivu commented 1 year ago

Could resuming be configured via NodeOption by any chance? Then in the constructor for Node:

this.sessionId = options.sessionId || null;

that's what i plan, and probably players as well if you saved it

Would this be simply fixed by adding two new NodeOptions then? In that case, I'd be able to do it as well.

    constructor(manager: Shoukaku, options: NodeOption) {
        ...
        this.players = options.players ?? new Map();
        ...
        this.sessionId = options.sessionId || null;
    }
export interface NodeOption {
    ...
    /**
     * A map of guild IDs to players to resume
     */
    players?: Map<string, Player>;
    /**
     * The session id for lavalink to resume
     */
    sessionId?: string;
}

shoudn't be options.players, this should be some sort of a structure then add it manually one by one

thaddeuskkr commented 1 year ago

shoudn't be options.players, this should be some sort of a structure then add it manually one by one

what do you mean some sort of structure? like set on ShoukakuOptions.structures?

Deivu commented 1 year ago

shoudn't be options.players, this should be some sort of a structure then add it manually one by one

what do you mean some sort of structure? like set on ShoukakuOptions.structures?

Nope basically you build it from scratch from raw data. Look at player class and voice connection class, what do they need to be functional. Im pretty sure all the data from the Connection Class must be present, as for player class,

thaddeuskkr commented 1 year ago

shoudn't be options.players, this should be some sort of a structure then add it manually one by one

what do you mean some sort of structure? like set on ShoukakuOptions.structures?

Nope basically you build it from scratch from raw data. Look at player class and voice connection class, what do they need to be functional. Im pretty sure all the data from the Connection Class must be present, as for player class,

so what i should be doing is instantiating the player + connection class with a set of provided data from a NodeOption, then setting it on shoukaku?

Deivu commented 1 year ago

shoudn't be options.players, this should be some sort of a structure then add it manually one by one

what do you mean some sort of structure? like set on ShoukakuOptions.structures?

Nope basically you build it from scratch from raw data. Look at player class and voice connection class, what do they need to be functional. Im pretty sure all the data from the Connection Class must be present, as for player class,

so what i should be doing is instantiating the player + connection class with a set of provided data from a NodeOption, then setting it on shoukaku?

yes thats how it will work technically pre create the player and connection BEFORE connecting to lavalink with the sessionId

thaddeuskkr commented 1 year ago

I'll work on the resuming thing in my free time, or someone else can. But for now this should be a stable enough implementation of v4 (without resuming).

Deivu commented 1 year ago

you can remove the code associated with it and i can work on it on my free time as well to get this merged

thaddeuskkr commented 1 year ago

you can remove the code associated with it and i can work on it on my free time as well to get this merged

i haven't pushed anything past what i did earlier with the enums, so there shouldn't be any code associated with resuming

Deivu commented 1 year ago

Is the changes tested and works well with the v4?

thaddeuskkr commented 1 year ago

Is the changes tested and works well with the v4?

I cloned Kongou to test, and I did /play. That works. So the other stuff should as well, I can look into that more later today.

thaddeuskkr commented 1 year ago

Tested all of Kongou's music commands, after tweaking Kongou's code a bit. Works fine. So I think the last thing to add is resuming.