atlarge-research / opencraft

Other
5 stars 2 forks source link

WIP: Entities via Broker - [closed] #153

Closed jdonkervliet closed 4 years ago

jdonkervliet commented 4 years ago

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

Merges feature/entities -> development

Resolves issues #5 and #6.

Moves entity updates and removals from GlowPlayer to GlowWorld. Entity spawns and chunk management are still in progress.

jdonkervliet commented 4 years ago

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

added 30 commits

Compare with previous version

jdonkervliet commented 4 years ago

In GitLab by @wubero on May 25, 2020, 19:57

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

I assume a comment will be placed here?

jdonkervliet commented 4 years ago

In GitLab by @wubero on May 25, 2020, 19:57

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

Missing javadoc

jdonkervliet commented 4 years ago

In GitLab by @wubero on May 25, 2020, 19:58

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

Might be better to cut this line a little earlier? It looks kind of weird this way.

jdonkervliet commented 4 years ago

In GitLab by @wubero on May 25, 2020, 19:58

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

Missing javadoc

jdonkervliet commented 4 years ago

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

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

The method is private and does therefore, in my opinion, not require additional documentation.

jdonkervliet commented 4 years ago

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

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

Correct, although most of these inline comments will be removed once all the refactoring is complete.

jdonkervliet commented 4 years ago

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

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

I could cut it by placing it in curly brackets:

messages.forEach((chunk, chunkMessages) -> {
    chunkMessages.forEach(message -> messagingSystem.broadcast(chunk, message));
});

Or even:

messages.forEach((chunk, chunkMessages) -> {
    chunkMessages.forEach(message -> {
        messagingSystem.broadcast(chunk, message);
    });
});

But splitting it at a . feels a bit weird. Either way, the line is within the length limit.

jdonkervliet commented 4 years ago

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

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

I personally prefer javadoc on these type of methods

jdonkervliet commented 4 years ago

In GitLab by @wubero on May 25, 2020, 22:38

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

I would also prefer javadoc for these methods

jdonkervliet commented 4 years ago

In GitLab by @wubero on May 25, 2020, 22:38

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

I feel like the second one would be the cleanest looking

jdonkervliet commented 4 years ago

In GitLab by @swabbur on May 27, 2020, 01:07

closed