Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pc1.20.2 #1265

Merged
merged 25 commits into from
Dec 27, 2023
Merged

Pc1.20.2 #1265

merged 25 commits into from
Dec 27, 2023

Conversation

extremeheat
Copy link
Member

@extremeheat extremeheat commented Dec 18, 2023

Fix #1258

The configuration state introduces lots of small problems in server/client expectations so there remain some errors in the test due to state desync. Will need to be investigated further

Needs audit of protocol.json for 1.20.2 against vanilla source code diff. Also, node-protodef-validator is not working.

After PrismarineJS/prismarine-nbt#81 and PrismarineJS/minecraft-data#810

@@ -45,6 +45,7 @@ function createServer (options = {}) {
server.onlineModeExceptions = Object.create(null)
server.favicon = favicon
server.options = options
options.registryCodec = options.registryCodec || mcData.registryCodec || mcData.loginPacket?.dimensionCodec
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dimensionCodec is no longer in loginPacket. So it it needs to be dumped and put into mcdata seperately. It's required before clients are able to join (to exit configuration state)

@@ -43,35 +43,36 @@ function writeUUID (value, buffer, offset) {
return offset + 16
}

function readNbt (buffer, offset) {
return nbt.proto.read(buffer, offset, 'nbt')
function readNbt (buffer, offset, { tagType } = { tagType: 'nbt' }) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we confident this is only useful for the network serialization?

Would there be any drawback to put this in prismarine-nbt ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already just directly calling prismarine-nbt so it should be possible to just import the prismarine-nbt types directly into the protocol. But the optionalNBT type may actually require a custom type as there is a special case with zero.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that would be much cleaner as all nbt stuff would then live in prismarine-nbt

Also important if other stuff than nmp use the same format, for example anvil

Comment on lines 216 to 217
client.state = states.PLAY
server.emit('playerJoin', client)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, problem seems due to not waiting for client to send back a finish_config

@extremeheat
Copy link
Member Author

extremeheat commented Dec 26, 2023

ok, tests are now passing, but looks like there may be an issue with packet_chunk_batch_start

[05:40:13] [Server thread/INFO]: [Not Secure] <Player> hello everyone; I have logged in.
PartialReadError: Read error for undefined : undefined
    at new ExtendableError (/workspace/node-minecraft-protocol/node_modules/protodef/src/utils.js:63:13)
    at new PartialReadError (/workspace/node-minecraft-protocol/node_modules/protodef/src/utils.js:70:5)
    at Object.readVarInt [as varint] (/workspace/node-minecraft-protocol/node_modules/protodef/src/datatypes/utils.js:69:45)
    at Object.packet_chunk_batch_start (/workspace/node-minecraft-protocol/node_modules/protodef/src/g-ReadCompiler104533269d.js:956:67)
    at /workspace/node-minecraft-protocol/node_modules/protodef/src/g-ReadCompiler104533269d.js:3002:74
    at packet (/workspace/node-minecraft-protocol/node_modules/protodef/src/g-ReadCompiler104533269d.js:3104:9)
    at CompiledProtodef.read (/workspace/node-minecraft-protocol/node_modules/protodef/src/compiler.js:70:12)
    at e.message (/workspace/node-minecraft-protocol/node_modules/protodef/src/compiler.js:111:49)
    at tryCatch (/workspace/node-minecraft-protocol/node_modules/protodef/src/utils.js:50:16)
    at CompiledProtodef.parsePacketBuffer (/workspace/node-minecraft-protocol/node_modules/protodef/src/compiler.js:111:29)
    at FullPacketParser.parsePacketBuffer (/workspace/node-minecraft-protocol/node_modules/protodef/src/serializer.js:68:23)
    at FullPacketParser._transform (/workspace/node-minecraft-protocol/node_modules/protodef/src/serializer.js:74:21)
  ...
    at Socket.emit (node:events:514:28)
    ...
      ✔ does not crash for 10000ms (10489ms)

Currently chunk batch start looks like

        "packet_chunk_batch_start": [
          "container",
          [
            {
              "name": "batchSize",
              "type": "varint"
            }
          ]
        ],

But it seems chunk batch start in 1.20.2 has no fields https://github.com/extremeheat/extracted_minecraft_data/blob/client1.20.2/client/net/minecraft/network/protocol/game/ClientboundChunkBatchStartPacket.java

but finish retains the varint
https://github.com/extremeheat/extracted_minecraft_data/blob/client1.20.2/client/net/minecraft/network/protocol/game/ClientboundChunkBatchFinishedPacket.java

@extremeheat
Copy link
Member Author

Actually, packet_chunk_batch_start is a new packet in 1.20.2, it's incorrect in mc-data. Would be a good idea to check the other types also against the source code, there are probably more unreviewed errors/missing things

@wgaylord
Copy link
Contributor

wgaylord commented Dec 26, 2023

Actually, packet_chunk_batch_start is a new packet in 1.20.2, it's incorrect in mc-data. Would be a good idea to check the other types also against the source code, there are probably more unreviewed errors/missing things

Looks like that may have been a dumb copy paste error on my part when doing all the packet changes in 1.20.2. As all my work was based on info from wiki.vg (Probably should have sanity checked it from source at some point)

@extremeheat
Copy link
Member Author

Ok, tests are passing now. Validator issue is fixed, just need to check over the protocol.json now. But anyway I think this can be merged before that, would just need to be checking before working on mineflayer

@extremeheat extremeheat marked this pull request as ready for review December 27, 2023 02:00
@@ -16,8 +16,6 @@ module.exports = {
size: buffer.length - offset
}
}],
nbt: ['native', minecraft.nbt[0]],
optionalNbt: ['native', minecraft.optionalNbt[0]],
compressedNbt: ['native', minecraft.compressedNbt[0]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could also move compressedNbt but not a big deal

client.once('login_acknowledged', onClientLoginAck)
} else {
client.state = states.PLAY
server.emit('playerJoin', client)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this new event should be documented.

It would be breaking for flying-squid (and other users), why do we change from 'login' ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server sets the mode to from LOGIN to CONFIG after receiving a login_ack from the client. The server emits login right after the ack, but the state is still in CONFIG so sending PLAY packets (like previous login event would let you do) would fail, so in 1.20.2+ now need to either listen for state event for it to turn to PLAY or use the added playerJoin event before sending game packets

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok makes sense

@rom1504
Copy link
Member

rom1504 commented Dec 27, 2023

looks good, I'm glad we could solve the nbt change with no hack and actually even improving the code, nice work @extremeheat

I think it would be good to fix PartialReadError: Read error for undefined : undefined before merging as this is definitely going to break usage of the lib for this version

@wgaylord do you want to check the packets you added against the source? that way you can learn to do it for 1.20.2 and then apply the same method for 1.20.3

@wgaylord
Copy link
Contributor

Yeah, I am going to go thru all the packets I touched and double check them some time tomorrow.

This was referenced Dec 27, 2023
@rom1504
Copy link
Member

rom1504 commented Dec 27, 2023

No value for type soundSource , packedChunkedPos in packet tests

@rom1504
Copy link
Member

rom1504 commented Dec 27, 2023

does seem to work fully except for this one, no error anymore for 1.20.2

@rom1504
Copy link
Member

rom1504 commented Dec 27, 2023

I think it's good now, merging

@rom1504 rom1504 merged commit 112926d into PrismarineJS:master Dec 27, 2023
23 checks passed
@rom1504
Copy link
Member

rom1504 commented Dec 27, 2023

/makerelease

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support 1.20.2
3 participants