Refactor server orders #19447
Refactor server orders #19447
Conversation
teinarss
force-pushed
the
server_dispatch
branch
from
947239f
to
8a2e5f1
Compare
|
Updated, not sure how we want to handle protocol version, increase it in every PR that changes stuff or in one final PR after all these PR have been merged? |
|
If you don't increase the protocol version in every PR then thirdparty mods might run into a version/implementation mismatch which can lead to strange problems. So it would be best to either merge all PRs at the same time or to increase it in every PR. |
|
Tbh I still think tools depending on bleed versions is kind of an anti-feature, but our order version is an int which means we can be generous with increasing it. |
teinarss
force-pushed
the
server_dispatch
branch
from
8a2e5f1
to
2e0dbeb
Compare
|
Updated |
There was a problem hiding this comment.
The change from spoofing the disconnect order sender to having it explicitly sent by the server seems like a good idea. I don't see the advantage for wrapping it in a serialized Order though, this seems to add a lot of complexity and redundancy compared to making OrderType.Disconnect carry just the clientID the same way OrderType.SyncHash carries the sync state.
If we do want to keep the Order wrapping can we please change some of my suggestions below to make the code a bit more self explanatory?
|
Updated. |
| @@ -131,8 +131,12 @@ public void TickImmediate() | |||
| return; | |||
|
|
|||
| var frame = BitConverter.ToInt32(packet, 0); | |||
| if (packet.Length == 5 && packet[4] == (byte)OrderType.Disconnect) | |||
| pendingPackets.Remove(clientId); | |||
| if (packet.Length == Order.DisconnectOrderLength + 4 && packet[4] == (byte)OrderType.Disconnect) | |||
There was a problem hiding this comment.
It's weird that the server code treats OrderType as the first byte in the data payload rather than as part of the framing. Fixing this isn't trivial because the OrderType handling is split half between the network code and half inside Order, so best that we leave this to a future PR.
|
Updated |
|
Updated |
|
Updated. |
Changing on how the disconnect order is sent from the server, ie not relying on spoofing the from client. Also did some cleanup around dispatching server orders.