Fix unloading chunks - [merged] #176

Closed jdonkervliet closed 4 years ago

In GitLab by @larsdetombe on Jun 8, 2020, 19:08

Merges bugfix/fix-unloading-chunks -> development

This merge request resolves issues #80 and #104

Fixes a problem with not unloading chunks from the previous world when switching to another
Fixes a problem with not unloading chunks when disconnecting(thus not releasing some chunk locks)
Fixes a problem regarding unsubscribing when changing worlds or disconnecting.
Fixes a problem with entity spawning when changing worlds, no entities should be spawning when a player changes worlds.

In GitLab by @swabbur on Jun 8, 2020, 20:07

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

What does playerCheck mean here? The name is a little confusing.

In GitLab by @swabbur on Jun 8, 2020, 20:08

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

Is there a non-destructive way of merging these collections?

In GitLab by @swabbur on Jun 8, 2020, 20:09

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

We may want to add an additional check to the next for-loop that ensures no new chunks are loaded whenever a player is removed.

In GitLab by @swabbur on Jun 8, 2020, 20:09

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

Missing whiteline at the start and end of this code-block.

In GitLab by @swabbur on Jun 8, 2020, 20:11

It might be better to refactor the code such that added/removed players handled separately from players that continue to exist. That would further simplify both the messaging system and chunk streaming code.

For example, chunk streaming could be simplified since no force variable or null-checks would be required anymore. And, the for-loops in the chunk streaming method would also be easier to execute as their inner if-statement would require less checks.

Similarly, adding an add and remove to the messaging system could simplify that process as well.

In GitLab by @JimVliet on Jun 8, 2020, 20:13

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

Is there a particular reason for reformatting this line? Since it doesn't seem like it exceeds the max line length.

In GitLab by @larsdetombe on Jun 8, 2020, 20:24

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

I didn't really know how to name this variable. I was thinking along the lines of the players to check, however it is basically the players + removed players

In GitLab by @larsdetombe on Jun 8, 2020, 20:28

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

It does exceed the max line length, the && !player.isRemoved() caused it too exceed the max line length of 120.

In GitLab by @larsdetombe on Jun 8, 2020, 20:30

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

It is not necessary, because a player that is removed in the previous pulse has no ability to move in the next pulse. I can add it just as an extra safety barrier however.

In GitLab by @larsdetombe on Jun 8, 2020, 20:57

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

I could create another variable removedPlayers with the contents of previousPlayers and use the removeAll operation on that. I could also use a stream, but that is relatively slow. The best option would be to use sets, thus making previousPlayers a set and using addAll to create the new players that have to be checked.

In GitLab by @swabbur on Jun 8, 2020, 21:03

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

If the player object is kept alive as it exists within another dimensions, its location may change. It might even be better to keep a list of previous locations/radii as well. I think that would be the only way to ensure that the data can safely be used.

Removing the dependency on the previous players by storing a copy of their (id + location + radius) would also remove the requirement to lock the world while updating a player's known chunks.

In GitLab by @swabbur on Jun 8, 2020, 21:04

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

I don't think using a set would be appropriate here, as we will need to iterate it. Or is there an array-based set?

In GitLab by @JimVliet on Jun 8, 2020, 21:32

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

Oh sorry, I did not notice you added an exta condition...

In GitLab by @JimVliet on Jun 8, 2020, 21:36

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

Do you think it would be appropriate to abstract this for loop into a function, such that we can unload chunks specifically for the removed players? This would remove any need for the removeAll call on the previousPlayers.

In GitLab by @larsdetombe on Jun 9, 2020, 12:02

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

The player.isRemoved() only returns true if the player is not online on the server anymore