Issue/#786 send player update on disconnect

[Askaholic]

This should let the client more accurately determine when a player has disconnected. It could also be used to rewrite the player state determination to be more accurate (for displaying swords and blocking certain actions).

The state names could change if someone has better ideas. I just didn't want to make them too long since they will already add quite a few bytes to the network traffic. We could save the most bytes by using integer codes, but I think it's ok to sacrifice some bytes to have a more expressive protocol.

Closes #786

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ea2b7d3) 94.61% compared to head (66287f9) 96.68%.

❗ Current head 66287f9 differs from pull request most recent head 76d7944. Consider uploading reports for the commit 76d7944 to get more accurate results

Additional details and impacted files

Files	Coverage Δ
server/player_service.py	99.21% <100.00%> (+2.10%)	⬆️
server/players.py	100.00% <100.00%> (ø)

... and 65 files with indirect coverage changes

[Sheikah45]

When I was testing on the test server I ended up seeing a few player_info messages without a state and it also seemed like the state did not always match with the game status. Should the state always be included?

[Askaholic]

Yea the state should always be there. If it happens again in future testing we'll need to look in the server logs to see what's going on as the contents of the messages is logged there.

[Sheikah45]

This seems a little counterintuitive as to what the expected value of self.state is when the player is offline. Might it not be better to add a playerState offline?

[Askaholic]

What is counterintuitive about it? If the player is offline then the state is "offline". If I add an "OFFLINE" state to the enum then I have to make sure that's kept in sync with the value of lobby_connection which complicates things and introduces the possibility of weird bugs.

[Sheikah45]

Couldn't you just do it the line after you delete the lobby_connection? How does it complicate things more than that?
And it would also be sufficient I think to just at least add the offline to the enum. As I had expected that to define all possible states a player could be in but then you find out there is another state hidden in the dict method. So it makes it harder to be sure what the allowable values are

[Askaholic]

Maybe, but I really don't want any other places in the code suddenly getting a player state of 'OFFLINE', since that isn't really a state... Internally you are offline when your LobbyConnection is gone which will cause an exception to be thrown when the server tries to send a message to you, thats how the code expects to be signaled. But I needed something to signal to the client that the player isn't connected anymore, and putting that in state seemed to make sense.

[Askaholic]

Its the same with the game_info message. The states that we send to the client aren't actually the same as the internal states.

[Askaholic]

Maybe for now it would be better to simplify things and just have 'online' and 'offline'. Unless you are actually planning on making use of the other states in the client. That's probably safer too because otherwise the internal state tracking becomes kindof part of the server protocol, which makes changing it a lot more annoying.

[Sheikah45]

Well with the addition of the states I would probably switch to using those to determine things like is a player available to join a queue or host or join a game as I would be trusting those are more consistent than perusing the games list in the client because the client may not necessarily even see all the game messages

[Eternal-ll]

Any updates on that? I am missing this feature =)

[Askaholic]

I can take another look at this. I think the main thing for me is I didn't like the state names and I am not quite comfortable with exposing the internal state over the protocol. But we might be able to start with just two state 'offline' and 'online', then you will at least be able to tell when people disconnect.

[Sheikah45]

As an update we are soon going to turn on always-on for the irc client to facilitate offline messages. This will make state tracking using the irc server much harder so it would be nice if something like this could be added.

[Sheikah45]

Tested on the test server and this works for me