Fix nether portal and travel to nether - [merged] #155

Closed jdonkervliet closed 4 years ago

In GitLab by @larsdetombe on May 25, 2020, 20:29

Merges bugfix/nether-portal -> development

This resolves issue #72 and #83

Nether portals of sizes 2x3 can be created with flint and steel
Portal blocks are visibly shown in the portal
Travel between nether and overworld is possible and chunk streaming keeps working
A delay is added to players trying to teleport to the nether (in creative this is still instant)
AreaOfInterest class is created to store previous location and view distance of player

In GitLab by @larsdetombe on May 25, 2020, 20:32

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1044

Oops this should be nether

In GitLab by @swabbur on May 25, 2020, 20:40

Commented on src/main/java/net/glowstone/block/itemtype/ItemFlintAndSteel.java line 62

Missing a . at the end of the sentence.

In GitLab by @swabbur on May 25, 2020, 20:41

Commented on src/main/java/net/glowstone/block/itemtype/ItemFlintAndSteel.java line 75

Could you add a whiteline at the start of this method?

In GitLab by @swabbur on May 25, 2020, 20:42

Commented on src/main/java/net/glowstone/block/itemtype/ItemFlintAndSteel.java line 68

The use of a local variable to store a constant seems a bit odd. If you really want it to be shorter, either store it in a local constant (class member variable), or import it statically.

In GitLab by @swabbur on May 25, 2020, 20:43

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

It might be better to create getters for the previousAreaOfInterest and location variables, as they can than be used in Java streams and can be automatically copied (which is especially important with location).

In GitLab by @swabbur on May 25, 2020, 20:44

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

Now that we have access to the previous view distance, should that be compared as well?

In GitLab by @swabbur on May 25, 2020, 20:45

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

The old view distance is not taken into consideration when determining which chunks should be loaded and/or unloaded.

In GitLab by @swabbur on May 25, 2020, 20:47

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

Might it be better to store the radius instead of the view distance? As the radius is what we actually use to determine which chunks are interesting.

We could also store the view distance, keeping the AreaOfInterest class decoupled from GlowServer, and provide a method for computing the (previous) radius int computeRadius(int maxViewDistance);.

In GitLab by @swabbur on May 25, 2020, 20:48

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

As the area-of-interest value itself is never replaced, it could be made final.

In GitLab by @swabbur on May 25, 2020, 20:49

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

I think it would be better to create a separate file for this class.

In GitLab by @swabbur on May 25, 2020, 20:50

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

Could you add an equals method, such that we can simplify the check whether the AreaOfInterest has changed in streamChunks/TopicPolicy?

In GitLab by @JimVliet on May 26, 2020, 24:41

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

There is an open issue for this: #43. So do we want to fix this in this MR or another one? I'd argue fix this separately.

In GitLab by @JimVliet on May 26, 2020, 24:42

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

The radius is normally just 1 + viewdistance. So I'd say keep it as is.

In GitLab by @JimVliet on May 26, 2020, 24:45

Commented on src/main/java/net/glowstone/block/itemtype/ItemFlintAndSteel.java line 49

I am not sure if I understand this code correctly, but wouldn't placing extra obsidian around the block result in the portal having the wrong orientation.

In GitLab by @wubero on May 26, 2020, 09:42

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

I agree, we should fix this in a separate MR

In GitLab by @wubero on May 26, 2020, 09:44

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

You're missing javadoc before this constructor and before the class definition