There are so many individual success/failure commands in the lobby server protocol now that it's kind of silly trying to implement it. I strongly suggest we go a different way: using the generic "Failed" command and adding a generic "Success" command. Something like…
Success [COMMAND] (command argument is optional – preferably clients would be using message IDs. wouldn't be optional in the final spec – would be there or not)
Failure [Command] {Reason}
Having this as a standard response to *all* fail/success commands would also make a nice alternative to the commands which *may* get a ServerMessage response (e.g. ChangePassword) and make it easier to deliberately display client behaviour based on responses of an expected format that we know we can count on, without having to try to "guess" whether it's a failure or a success.
Similarly, why are these all separate commands?
Is there room to update these commands? I'm willing to do the work, if you can assure me it would be considered.
I'm very sorry about the last link but I had one spare and had to do something with it.
Can we please simplify the protocol
Moderators: Moderators, Lobby Developers
Re: Can we please simplify the protocol
the issue was, that changing the protocol mostly broke clients. thats why mostly new commands where added when some feature was missing...
the main issue i guess is, that lobby server and lobby client development doesn't happen in sync.
if you really want to improve stuff i guess you have to start client and server development.
the main issue i guess is, that lobby server and lobby client development doesn't happen in sync.
if you really want to improve stuff i guess you have to start client and server development.
Re: Can we please simplify the protocol
I say, sir, those who demand a lower protocol level, stoop to be lower class.
Srsly, protocols are always a first draft expanded on and then walled in. Licho and
Srsly, protocols are always a first draft expanded on and then walled in. Licho and
Re: Can we please simplify the protocol
The others should agree though
Re: Can we please simplify the protocol
I still can't get SpringLobby compiling on my machine. Setting up WxWidgets stuff is permanently a pain. (It hurts more that I've got it working once before in the past, and can't work it out now).
I should be able to (at least mostly) deal with things with Chobby/ Uberserver, but I'd need someone's help with SpringLobby. But if you (or someone else) is interested in helping out on lobby side, and there are no complaints from a protocol quality standpoint, I'll start doing some testing.
- Silentwings
- Posts: 3720
- Joined: 25 Oct 2008, 00:23
Re: Can we please simplify the protocol
Potentially all of SL, Chobby, SPADS, SLDB, Maddox's autohosts, theIRC bridge, the matrix bridge, and Notalobby suffer fully breaking changes here, so you're looking at a potentially huge amount of favours/PRs. All you're gaining is merging some commands - which on its own is imo a questionable idea of simplification, it would move lots of "if" statements from server<->client code and vice versa, but afaics without any reduction in the complexity of what information has to be communicated.
Wanting "all" requests to be sent a generic pass/fail response simply doesn't work e.g. successful commands mostly need non-binary responses, in the form of state change messages to several clients, after which a generic success message would be pointless. There are also failure cases on server side where the server cannot know if the client would consider the result a success or not (e.g. request to set team id to current team id, which is denied because client does not have battle host rights). A generic FAILED command already exists, used in >50 places, often to inform client devs of invalid/ill-formatted requests that they should have known not to ask (if backwards compat was forgotten, it could be used more widely). It's unclear to me why you mention messageIDs - do you see logical inconsistencies if they remained optional? Do you know that there wouldn't be logical inconsistencies if they didn't?Having this as a standard response to *all* fail/success commands would ...
Bandwidth, older parts of the protocol try harder to minimize it.Similarly, why are these all separate commands?
This seems like a more genuine issue, if you can simply list the troublesome cases that would be better.commands which *may* get a ServerMessage response (e.g. ChangePassword) and make it easier to deliberately display client behaviour based on responses
Re: Can we please simplify the protocol
Good point. Thanks for the heads up.Silentwings wrote: ↑19 Jun 2020, 09:58 Potentially all of SL, Chobby, SPADS, SLDB, Maddox's autohosts, theIRC bridge, the matrix bridge, and Notalobby suffer fully breaking changes here, so you're looking at a potentially huge amount of favours/PRs.
With my protocol implementation I'd only see simplifications on clientside. I don't think it's reducing complexity, but it's reducing repetition and (in theory) increasing extensibility.Silentwings wrote: ↑19 Jun 2020, 09:58 it would move lots of "if" statements from server<->client code and vice versa, but afaics without any reduction in the complexity of what information has to be communicated.
MessageIDs are useful for ascertaining which success/fail replies to which request, and would provide extra opportunity for simplifying the protocol… in theory.Silentwings wrote: ↑19 Jun 2020, 09:58 Wanting "all" pass/fail replies to switch to a generic version simply doesn't work e.g. successful commands mostly need non-binary responses, in the form of state change messages to several clients, after which a generic success message would be pointless. There are cases (e.g. request to kick a non-existing user) where the server does not know if the client would consider the state change a success or not. A generic FAILED command already exists, used in >50 places, often to inform client devs of invalid/ill-formatted requests (if backwards compat was forgotten, it could be used more widely). It's unclear to me why you mention messageIDs - do you see logical inconsistencies if they remained optional? Do you know that there wouldn't be logical inconsistencies if they didn't?
This seems like a more genuine issue, if you can simply list the troublesome cases that would be better.commands which *may* get a ServerMessage response (e.g. ChangePassword) and make it easier to deliberately display client behaviour based on responses
I'm primarily thinking of messages that get the Succeeded/Failed(with a reason) responses that don't correlate to state changes which would be sent out to everyone.
And now for the lists of commands that may benefit:
Login commands:
- Register
- Login (Questionable because Agreement-related commands are also in there)
- ConfirmAgreement
- RenameAccount (currently ServerMSG response)
- ChangePassword (currently ServerMSG response)
- ChangeEmailRequest
- ChangeEmail
- ResendVerification
- ResetPasswordRequest
- ResetPassword
- Join (possibly could remove the Join response from the server and just rely on the "Joined" which would now also be sent to the new client. – like how JoinFrom appears to work.)
- BridgeClientFrom (Already uses the Failed command)
- UnBridgeClientFrom (Already uses the Failed command)
- Join (Kind of. Also in the above category)
- JoinFrom
- LeaveFrom
- SayFrom
- OpenBattle
- JoinBattle
Also somewhat less related, the JSON command could use more description. I had a look into it, I think I won't be qualified to write the description until I've actually implemented a few JSON commands on the client-side.
- Silentwings
- Posts: 3720
- Joined: 25 Oct 2008, 00:23
Re: Can we please simplify the protocol
After some thought, a generic "SUCCESS" command is not sensible, better that the server just informs the client of what changed, and leaves open the option that the server may not react exactly as the client requested/expected. FAILED is ok, to be used in cases where the server changes nothing, and sends back a human readable error/explanation instead. That is what's most extensible/future proof.
Re your list, the login sequence & JOIN/**BATTLE & messageIDs are not worth touching for this, breaks far too much for too little gain / are not suitable. The **FROM set already uses the nicest format for "simple" commands imo. That leaves the user management stuff
Two possible changes here:
(1) Add ACCEPTED/DENIED replies to RENAMEACCOUNT/CHANGEPASSWORD, to remove the (only) amibiguity on client side. This only requires a tiny PR to uberserver & the LobbyProtocol docs.
(2) Move the failure replies of all the above to generic FAILED command, and have the successful replies put into the reflection style with RENAMEDACCOUNT/CHANGEDPASSWORD/CHANGEDEMAILREQUEST/etc and add suitable reflected args specifying what changed (ie. consistent with **FROM syntax). I guess that requires
- probably a compat flag
- PRs to uberserver and the LobbyProtocol docs
- PR to SL (iirc it knows only CHANGEPASSWORD and RENAMEACCOUNT)
- possible PRs/favour requests to Chobby, NotaLobby and Bibim's IrcBridge (if they use any of them, likely they do but I don't remember)
I probably should have done (2) myself, at the point where I added half of those commands, but I don't plan to touch it rn.
Re your list, the login sequence & JOIN/**BATTLE & messageIDs are not worth touching for this, breaks far too much for too little gain / are not suitable. The **FROM set already uses the nicest format for "simple" commands imo. That leaves the user management stuff
Code: Select all
RenameAccount
ChangePassword
ChangeEmailRequest
ChangeEmail
ResendVerification
ResetPasswordRequest
ResetPassword
(1) Add ACCEPTED/DENIED replies to RENAMEACCOUNT/CHANGEPASSWORD, to remove the (only) amibiguity on client side. This only requires a tiny PR to uberserver & the LobbyProtocol docs.
(2) Move the failure replies of all the above to generic FAILED command, and have the successful replies put into the reflection style with RENAMEDACCOUNT/CHANGEDPASSWORD/CHANGEDEMAILREQUEST/etc and add suitable reflected args specifying what changed (ie. consistent with **FROM syntax). I guess that requires
- probably a compat flag
- PRs to uberserver and the LobbyProtocol docs
- PR to SL (iirc it knows only CHANGEPASSWORD and RENAMEACCOUNT)
- possible PRs/favour requests to Chobby, NotaLobby and Bibim's IrcBridge (if they use any of them, likely they do but I don't remember)
I probably should have done (2) myself, at the point where I added half of those commands, but I don't plan to touch it rn.
https://github.com/spring/LobbyProtocol/issues/newerror in the documentation
There is some minor oddity in the reflection of JOIN/JOINED, no idea what history caused it, but trying to change it is not worth the trouble (I once removed this oddity by accident and broke almost everything that tried to connect).Join (Kind of. Also in the above category)