fix client-side zone request crashes - #13
Conversation
+ changed error message for consistency
+ organized imports
useful for high latency clients
LastExceed
left a comment
There was a problem hiding this comment.
I'm impressed by how quickly your git skills have improved. I was admittedly a bit scared by the size of the PR at first, but in the end I had no trouble reviewing the changes.
Unfortunately I'm not really sold on the delayed zone loading stuff yet (which is the bulk of this PR). Relying on arbitrary delays just feels very error prone, both implementation wise and simply due to the fact that bandwidth/latency/perfomance varies heavily from player to player. Keeping track of zone loading states also seems like a big burden that I would very much like to avoid.
I'm also still not fully convinced that we absolutely need this. Does waiting for an AreaRequest of the given zone really not suffice to prevent crashes?
If the crashes only happen when the client receives block updates during loading screens, then I might have an idea: IIRC the client completely stops all network traffic during loading screens, and the timers in the creature data heavily fall behind. This could be used to detect when a client has "catched up", and is something that the anti-cheat needs for speedhack detection anyway. Do you see any potential there?
There was a problem hiding this comment.
If even an empty/unchanged loot update really accomplishes everything that P48 does, then this is an elegant solution. I just remembered that there is some mechanism in the game that brightens the zone tiles on the world map, which I think had something to do with AreaRequests and/or some of the WorldUpdate sub-structures. It seems I have never written it down, and since forgotten the details +.+ Would you mind testing what the exact conditions were for a map tile to become A) revealed, and B) bright?
There was a problem hiding this comment.
I am afraid this is a classical case of premature optimization. I understand that this is trying to prevent the server from doing the same data serialization over and over, but I have a few concerns with this:
- What is the rationale behind using
Arc<[u8]>instead ofVec<u8>? - By pre-serializing the blocks into a raw packet, they can no longer be included in a
WorldUpdatethat also contains other data (e.g. loot), which means this change is reducing computational load on the server at the cost of increased bandwidth, which is likely a much tighter bottle neck than CPU time - While there is currently no use case for retaining the information of which blocks make up a given model yet, the emphasis here is on "yet". I think there is a good chance that there will be a need for it in the future
p48 is unsafe & block updates race with client-side terrain data when sent immediately.
this fix attempts to make the server hold the client's hand and "guesstimate" when the client is ready and in a safe state to accept the zone update
an additional feature this introduces is force feeding adjacent zones to the client (client only requests its current zone):
this allows players to see blocks and loot in adjacent zones