Skip to content

fix client-side zone request crashes - #13

Open
fayeforpay wants to merge 11 commits into
LastExceed:masterfrom
fayeforpay:zone-request
Open

fix client-side zone request crashes#13
fayeforpay wants to merge 11 commits into
LastExceed:masterfrom
fayeforpay:zone-request

Conversation

@fayeforpay

Copy link
Copy Markdown
Contributor

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

@LastExceed LastExceed left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. What is the rationale behind using Arc<[u8]> instead of Vec<u8>?
  2. By pre-serializing the blocks into a raw packet, they can no longer be included in a WorldUpdate that 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
  3. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants