JMS message serialization - [merged] #173

Closed jdonkervliet closed 4 years ago

In GitLab by @swabbur on Jun 6, 2020, 16:32

Merges bugfix/jms-message-serialization -> development

Resolves issue #103.

The prior implementation of the JMS broker wasn't actually distributing any messages due to them being send as transactions and those transactions never being commited. Fixing this issue resulted in a number of bugs due to how serialization of messages was performed. A number of messages could not be decoded and the opcodes of inbound and outbound messages did not match, causing these to be incorrectly returned to the server for forwarding to users. Altering these opcodes directly was not an option, since the original Minecraft client actually makes use of these mismatching opcodes. Therefore, a new JmsCodec was created to combine the required message codecs into a single codec, prepending each encoded message with a new, symmetric, opcode.

Also, the construction order of GlowWorld was altered for future support of concurrent world generation. If not ordered in this way, it would have been possible for the world decorators to post block changes before the list of block changes were initialized.

In GitLab by @swabbur on Jun 6, 2020, 16:34

changed the description

In GitLab by @swabbur on Jun 6, 2020, 16:37

added 1 commit

c6575d0c - Revert ActiveMQ broker factory method to its original

Compare with previous version

In GitLab by @larsdetombe on Jun 6, 2020, 18:31

Commented on src/main/java/net/glowstone/messaging/brokers/JmsBroker.java line 52

Do we want to keep this listener, for the exceptions or can it be removed?

In GitLab by @larsdetombe on Jun 6, 2020, 21:10

I tested it with ActiveMQ, placing sand blocks did work, water flowed really nice, entity spawning and interacting is working nice, portal creation and travelling between worlds works good and a few other things I tested all looked good. ActiveMQ seems to be working on windows for me.

In GitLab by @swabbur on Jun 6, 2020, 21:33

Commented on src/main/java/net/glowstone/messaging/brokers/JmsBroker.java line 52

We want to keep it, as any exception thrown within the JMS message callback would be muted and would close the underlying connection without warning.

In GitLab by @larsdetombe on Jun 7, 2020, 13:55

resolved all threads

In GitLab by @swabbur on Jun 7, 2020, 16:10

marked as a Work In Progress

In GitLab by @swabbur on Jun 7, 2020, 16:10

changed the description

In GitLab by @swabbur on Jun 7, 2020, 17:18

added 1 commit

ca92a0db - Implement missing decode methods

Compare with previous version

In GitLab by @swabbur on Jun 7, 2020, 17:42

added 1 commit

b62f92ad - Implement minimal codec tests for non-play message codecs

Compare with previous version

In GitLab by @swabbur on Jun 7, 2020, 20:26

added 1 commit

029b7a2b - Implement missing non-gameplay message codecs

Compare with previous version

In GitLab by @swabbur on Jun 7, 2020, 20:46

added 1 commit

c850fdcd - Remove non-gameplay message codecs

Compare with previous version

In GitLab by @larsdetombe on Jun 8, 2020, 11:01

Commented on src/main/java/net/glowstone/net/codec/play/game/MultiBlockChangeCodec.java line 24

Can we simply read the short here? Because in the encode there is some heavy bit-shifting and bit operations going on.

In GitLab by @larsdetombe on Jun 8, 2020, 11:03

Commented on src/main/java/net/glowstone/net/codec/play/scoreboard/ScoreboardTeamCodec.java line 16

You forgot the decode method here :)

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

Commented on src/main/java/net/glowstone/net/codec/play/game/MultiBlockChangeCodec.java line 24

Yes, we can use a short here. The bit-operations that follow will, however, convert it to an integer anyways. As we will be only actually using the lower 16-bits, the output values would be the same. Therefore, I will change it to a short as it better matches the value we read from the buffer.

In GitLab by @swabbur on Jun 10, 2020, 11:37

Commented on src/main/java/net/glowstone/net/codec/play/game/MultiBlockChangeCodec.java line 24

changed this line in version 7 of the diff

In GitLab by @swabbur on Jun 10, 2020, 11:37

Commented on src/main/java/net/glowstone/net/codec/play/scoreboard/ScoreboardTeamCodec.java line 16

changed this line in version 7 of the diff

In GitLab by @swabbur on Jun 10, 2020, 11:37

added 5 commits

5c2d1a36 - Implement remaining codec tests
892ef0ea - Fix MultiBlockChange codec
25a44ce5 - Clean up codecs
b414f685 - Mock Bukkit.getItemFactory to prevent NPEs
668016bc - Implement remaining codecs and fix tests

Compare with previous version

In GitLab by @swabbur on Jun 10, 2020, 11:37

unmarked as a Work In Progress

In GitLab by @julian9499 on Jun 10, 2020, 11:38

Please update your branch to the current development version

In GitLab by @swabbur on Jun 10, 2020, 11:39

changed the description

In GitLab by @swabbur on Jun 10, 2020, 12:45

added 1 commit

4a39b161 - Fix HeldItemCodec

Compare with previous version

In GitLab by @wubero on Jun 10, 2020, 13:36

Commented on src/main/java/net/glowstone/net/codec/play/game/UserListItemCodec.java line 35

We should probably consider removing this

In GitLab by @larsdetombe on Jun 10, 2020, 13:47

Commented on src/main/java/net/glowstone/net/codec/play/inv/OpenWindowCodec.java line 20

Should this not simply check if there are any bytes left and then just try to read the integer? The only way there are bytes left is if the integer has been written. This could simply be if (buffer.readableBytes() >= 0)

In GitLab by @wubero on Jun 10, 2020, 13:52

Commented on src/main/java/net/glowstone/net/codec/play/player/AdvancementsCodec.java line 78

Might be handy to javadoc this since it's not that obvious what it does if you're not familiar with codecs

In GitLab by @larsdetombe on Jun 10, 2020, 14:22

Commented on src/main/java/net/glowstone/net/codec/play/scoreboard/ScoreboardTeamCodec.java line 45

should this be toUpperCase? because in the encode they do toLowerCase, but the opposite is not per se toLowerCase. It does seem to work, but is it required? the same holds for the code below

In GitLab by @swabbur on Jun 10, 2020, 14:26

Commented on src/main/java/net/glowstone/net/codec/play/inv/OpenWindowCodec.java line 20

Sure, that would work too.

In GitLab by @swabbur on Jun 10, 2020, 14:30

Commented on src/main/java/net/glowstone/net/codec/play/player/AdvancementsCodec.java line 78

I've added some JavaDoc.

In GitLab by @swabbur on Jun 10, 2020, 14:32

Commented on src/main/java/net/glowstone/net/codec/play/scoreboard/ScoreboardTeamCodec.java line 45

Yes, the toUpperCase is here required as enum value names are all upper case. I'm not sure why it was originally converted to lower-case, but is seems to be part of the network protocol of Minecraft itself.

In GitLab by @larsdetombe on Jun 10, 2020, 14:34

Commented on src/test/java/net/glowstone/net/codec/CodecTest.java line 17

Could you maybe add javadoc to this class, the other test classes don't need it as they extend this class, but it might be nice to add javadoc here.

In GitLab by @swabbur on Jun 10, 2020, 14:35

Commented on src/main/java/net/glowstone/net/codec/play/game/UserListItemCodec.java line 35

Sure!

In GitLab by @larsdetombe on Jun 10, 2020, 14:35

Commented on src/test/java/net/glowstone/net/codec/MockedCodecTest.java line 28

Javadoc in this class would also be nice to explain the difference between codecTest and why this class is needed.

In GitLab by @swabbur on Jun 10, 2020, 14:49

Commented on src/main/java/net/glowstone/net/codec/play/game/UserListItemCodec.java line 35

changed this line in version 9 of the diff