world chunk streaming - [merged] #129

Closed jdonkervliet closed 4 years ago

In GitLab by @larsdetombe on May 9, 2020, 13:19

Merges feature/world-chunk-streaming -> development

This merge request is for the issue #8

changes:

Move chunk streaming in the streamBlocks method in GlowPlayer to GlowWorld
Keep track of previous locations of players in a map
Add simple executor to stream the blocks

In GitLab by @swabbur on May 9, 2020, 13:22

Commented on src/main/java/net/glowstone/net/GlowSession.java line 75

We no longer need these changes in GlowSession, do we?

In GitLab by @swabbur on May 9, 2020, 13:22

Commented on src/main/java/net/glowstone/scheduler/GlowScheduler.java line 44

The whitelines should be after instead of before the MAX_THREADS variable.

In GitLab by @larsdetombe on May 9, 2020, 13:29

Commented on src/main/java/net/glowstone/scheduler/GlowScheduler.java line 44

shouldn't there be a whiteline before as well? Or do you want the MAX_THREADS directly after PULSE_EVERY? I understand the whiteline after though.

In GitLab by @larsdetombe on May 9, 2020, 13:33

Commented on src/main/java/net/glowstone/net/GlowSession.java line 75

Yeah we don't need this anymore, it is from implementations we tried before so I will remove this along with other unnecessary changes which I forgot to remove.

In GitLab by @julian9499 on May 9, 2020, 13:38

Commented on src/main/java/net/glowstone/entity/GlowPlayer.java line 758

Remove the () around world.getWorldBorder()

In GitLab by @larsdetombe on May 9, 2020, 13:47

Commented on src/main/java/net/glowstone/net/GlowSession.java line 75

changed this line in version 2 of the diff

In GitLab by @larsdetombe on May 9, 2020, 13:47

Commented on src/main/java/net/glowstone/entity/GlowPlayer.java line 758

changed this line in version 2 of the diff

In GitLab by @larsdetombe on May 9, 2020, 13:47

added 1 commit

e8d2ca03 - Fix unnecessary changes

Compare with previous version

In GitLab by @larsdetombe on May 9, 2020, 13:54

resolved all threads

In GitLab by @JimVliet on May 9, 2020, 13:55

Commented on src/main/java/net/glowstone/GlowWorld.java line 425

Why are the player previous locations now stored in this hashmap? Is there a particular reason for this?

In GitLab by @larsdetombe on May 9, 2020, 13:55

changed the description

In GitLab by @JimVliet on May 9, 2020, 13:57

Commented on src/main/java/net/glowstone/GlowWorld.java line 572

Since this boolean is used throughout the code, can you put down a comment explaining the purpose of it?

In GitLab by @larsdetombe on May 9, 2020, 13:57

Commented on src/main/java/net/glowstone/GlowWorld.java line 425

because we moved the functionality from glowPlayer to GlowWorld, we need to keep track of the previous location of all the players. The reason we track the previous location is to check if the player moved to another chunk and if we need to load new chunks (and subscribe) for this player

In GitLab by @larsdetombe on May 9, 2020, 14:00

Commented on src/main/java/net/glowstone/GlowWorld.java line 572

This code is actually only used in 2 if statements, but we can add a comment to explain it, A better name for this boolean would be force, as it forces an update when the previous location is unknown (for example when the player just joined)

In GitLab by @swabbur on May 9, 2020, 14:20

Commented on src/main/java/net/glowstone/GlowWorld.java line 425

We may later move this elsewhere, but as the refactoring of GlowPlayer isn't done yet, it hard to say where it should go.

In GitLab by @larsdetombe on May 9, 2020, 14:33

Commented on src/main/java/net/glowstone/GlowWorld.java line 572

changed this line in version 3 of the diff

In GitLab by @larsdetombe on May 9, 2020, 14:33

added 1 commit

5f9b31dd - Remove viewdistance bugfix

Compare with previous version

In GitLab by @larsdetombe on May 9, 2020, 14:33

Commented on src/main/java/net/glowstone/GlowWorld.java line 572

I changed the name and added a comment

In GitLab by @larsdetombe on May 9, 2020, 14:34

resolved all threads

In GitLab by @JimVliet on May 9, 2020, 14:51

merged

In GitLab by @JimVliet on May 9, 2020, 14:51

mentioned in commit d212708dab3f876fb3482c3903114a5841c3eb2c

In GitLab by @jdonkervliet on May 11, 2020, 13:06

Commented on src/main/java/net/glowstone/GlowWorld.java line 568

Could we move this to a separate class?

An important "rule" in building systems is to separate policy and mechanism*.

The mechanism (pub/sub) should be separated from the policy/policies (which topics get created? who publishes to which topic? who subscribes to which topic?).

A simple and effective way to implement this, is to build a separate scheduler that decides these things. This allows you to significantly change the behavior of the system by merely swapping in another scheduler.

In the case of our messaging system, setting up a topic per chunk, and having players subscribe to nearby chunks, is a reasonable starting point. But it's important that we can easily change the policy to experiment with different system behavior.

*Computer systems, much like algorithms or software engineering, is a field of research within computer science.

In GitLab by @swabbur on May 11, 2020, 15:14

Commented on src/main/java/net/glowstone/GlowWorld.java line 568

We would love to move this code to a separate class. However, we are in the process of moving a number of other methods over (from GlowPlayer) to GlowWorld that are all tightly connected to each other. Once the moving of those methods is complete, we will look at extracting the functionality to a separate class (or even multiple classes). Doing that now would require us to perform all those movements of methods in a single step. Which we think would be unnecessarily complicated. Moving it piece-by-piece makes it easier to divide the workload and keeps things organized.

In GitLab by @swabbur on May 11, 2020, 15:25

resolved all threads

In GitLab by @JimVliet on May 11, 2020, 15:37

resolved all threads

In GitLab by @JimVliet on May 11, 2020, 15:37

Commented on src/main/java/net/glowstone/GlowWorld.java line 568

created #45 to continue this discussion

atlarge-research / opencraft

world chunk streaming - [merged] #129