-
Notifications
You must be signed in to change notification settings - Fork 0
Introduce Chunk Transfer and Processing Limits #67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/client/terrain/systems.rs
Outdated
| let positions: Vec<IVec3> = chunks.into_iter().map(|chunk| chunk.position).collect(); | ||
| let mut positions: Vec<IVec3> = chunks.into_iter().map(|chunk| chunk.position).collect(); | ||
| positions.sort_by(|a,b | { | ||
| (a - origin).length_squared().cmp(&(b - origin).length_squared()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do square length because we don't care about exact values and we can skip the calculation of the square root, which can be pricey
| positions, client_id | ||
| ); | ||
|
|
||
| let chunks: Vec<Chunk> = positions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YEET
cb341
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there..
| server.send_message( | ||
| *client_id, | ||
| DefaultChannel::ReliableUnordered, | ||
| message.unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎁
| server.send_message( | ||
| *client_id, | ||
| DefaultChannel::ReliableUnordered, | ||
| message.unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should never fail and if it does I want the entire thing to die.
although.. network outage is a thing, huh?
| let mut positions: Vec<IVec3> = chunks.into_iter().map(|chunk| chunk.position).collect(); | ||
| positions.sort_by(|a, b| { | ||
| (a - origin) | ||
| .length_squared() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested by Josua
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to go for quadratic distances because i don't rly care about the exact value but rather the ordering. also i get to not compute a square root in each comparsion which is nice.
| }); | ||
|
|
||
| let batched_positions = positions.chunks(16); | ||
| let batched_positions = positions.chunks(32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manual testing showed that the server can handle larger requests easily now that we don't processs them at once.
|
|
||
| #[cfg(all(not(feature = "skip_terrain"), not(feature = "lock_player")))] | ||
| const SPAWN_POINT: Vec3 = Vec3::new(0.0, 64.0, 0.0); | ||
| const SPAWN_POINT: Vec3 = Vec3::new(0.0, 43.0, 0.0); // TODO: determine terrain height at 0,0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future me problems
| self.queues.remove(&client_id); | ||
| } | ||
|
|
||
| pub fn retain<F>(&mut self, f: F) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy paste from Dequeue docs.
| ) { | ||
| const MAX_REQUESTS_PER_CYCLE_PER_PLAYER: usize = 5; | ||
|
|
||
| requests.retain(|client_id, positions| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some very ugly code snippets before I learned about retain
Problem
Before this PR chunk batches were processed right after being received at the server.
When booting up the client, a large initial batch for the spawn area was requested, which made the server unable to respond to messages while it was busy serializing huge portions of the pre-generated world. After that the server responds with multiple consequtive serialized batches to the client at once which overwhelmed it.
Result
Lag spikes for everyone when someone else joined the server + Long waiting time when starting the simulation.
Solution
This PR aims to limit the amount of chunk processing that gets done by leveraging HashMaps and Dequeues.
I won't dig into details here because most likely nobody is going to see this PR anyways.
Now the simulation supports larger render distances and more simulations players 🥳
I put this task on high prio as it will make developing future features more comfy.
Comparison
Before
https://github.com/user-attachments/assets/583b883c-9bd8-431a-9057-76955cdafb1f
After
https://github.com/user-attachments/assets/516fd9c7-6ba1-415b-b08a-5f374466b048
Note
Frame times seem bad bud that's mostly because this is footage of a testing build with extensive logging and without all the fancy optimization.
Other
The system is still expensive but at least it isn't as blocking now.
When more performance improvements are needed, I'll get back to it.