Fixed spectator mode VGUI panel

chinese-soup commented 3 years ago

Hello. Useless fixes galore!

This PR fixes a thing that's probably only been bugging me as I am probably the only person to ever try to use the VGUI spectator panel in AG and it always ticked me off that it doesn't work. This is the panel that appears when you press +duck while in spectator mode.

Let's get this out of the way first: spectating while on a server (and not on an HLTV server or on dem_forcehltv 1) is server-side, the client only sends the server commands when it wants to change the target to spectate / change the mode of spectating etc.

First: let's start with the easy stuff. To fix this menu:

This change has to be made:

sprintf(cmdstring,"spec_mode %i",iNewMainMode );

As the server DLL that we are all using implements this client->server cmd as "spec_mode" instead of "specmode", see here: https://github.com/martinwebrant/agmod/blob/master/src/dlls/agclient.cpp#L126

Second: these two arrows were already working, but didn't make sense, they were flipped, i.e. the NEXT did previous player and PREV did next player, I fixed that in VGUI_SpectatorPanel.cpp**

Third of all: the worst offender - the player picker

This panel is supposed to just send the name of the player to FindPlayer(name) that sends the follow %s command to the AG server dll, but the AG server dll removed this and doesn't implement this at all, it only implements the follownext %i command (that's why clicking with the +attack & +attack2 works and also why the arrows mentioned above work) as can be seen here: https://github.com/martinwebrant/agmod/blob/master/src/dlls/agclient.cpp#L120

The workaround for this is to calculate how many times the client needs to send the follownext %i command where %i decides whether to go backwards (previous player) (1) or forwards (next player) (0).

Quirks & stuff:

The hell with this was that the g_iUser2 variable doesn't get updated as it's only updated once the server messages the client about the change and we don't get that until after the FindPlayer function already returns. This is the reason why we can't just do while(g_iUser2 != target_player_id) as we'd just get stuck in an infinite loop.
Another hell is that we don't know what players are active (i.e. are playing and not spectating/are on the server/etc.) until I go through all of them, hence the requirement of the first for loop.
I couldn't think of a better way to implement this than having two seperate vectors, for now anyway, if it's requested I will try to rewrite.
The main reason for the two vectors is that it was easier to me than "saving an index, iterating forwards, and the player wasn't found, iterating backwards again" in on a single vector.
You can't just do a difference between the g_iUser2 and the target player id as the server can have player ids that are not consecutive (e.g. players 1,2,6,9), not to mention that not all connected players are spectatable (i.e. they are also spectating).
Or, I do realize I could just overflow(? dunno if it's the right word -- basically start the index at and if I get to the end just reset the index from the beginning and try find the player there) the number of times the follownext 0 (just going forwards) is called until I get to the players that are behind the one I'm currently spectating, but I just couldn't get it to work that way for way too long, so I literally gave up and did this stupidity with forwards/backwards instead.
But like I've said, if a rewrite is requested, I'm willing to try it again. :smile_cat:
This has been tested with player IDs that are not consecutive (e.g. 1,2,3,5) which can be seen in the example video below where the player with the id = 4 has been kicked.
This also works if the user has colors in the name, as the VGUI SpectatorPanel doesn't show the colors in the combobox, but its action handler sends the original nickname including colors.
This also works if a player we're spectating is kicked/quit/changed to spectator, as that's handled elsewhere and makes us spectate the next available player (or switches to Free look etc.)
This doesn't break +attack or +attack2 as those just call hud_spectator.cpp:FindNextPlayer(), not this function.

All the fixes can be seen featured in this video: https://user-images.githubusercontent.com/5108747/115096852-5562a080-9f27-11eb-8357-d83127154249.mp4

Review welcome, I am a big C++ (or well, HLSDK is mostly just C with OOP but whatever) noob and this is a learning experience for me.

PS: The player picker could obviously be fixed server side by adding back the "follow" command that the AGMod author deleted. But for now, here's this.

YaLTeR commented 3 years ago

@tmp64 would you mind reviewing this too?

YaLTeR commented 3 years ago

This should be good to merge now, right?

chinese-soup commented 3 years ago

Not yet, sorry.

chinese-soup commented 3 years ago

Never mind, it is. Turns out the "follow" command isn't implemented in vanilla Half-Life server DLL either, so there's no need to check for gamemode there. This is good to go now.

YaLTeR commented 3 years ago

Alright, cool, thanks!

YaLTeR / OpenAG