-
-
Notifications
You must be signed in to change notification settings - Fork 239
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
1.20.3 / 1.20.4 support #1275
1.20.3 / 1.20.4 support #1275
Conversation
Honestly not very happy with what I came up with for fixing the chat for the client but it does work. Maybe we need to expand this out to pchat and have it handle it since it the server side of nmp will need to handle this nbt stuff as well and its not as simple. |
src/version.js
Outdated
@@ -1,6 +1,6 @@ | |||
'use strict' | |||
|
|||
module.exports = { | |||
defaultVersion: '1.20.2', | |||
supportedVersions: ['1.7', '1.8.8', '1.9.4', '1.10.2', '1.11.2', '1.12.2', '1.13.2', '1.14.4', '1.15.2', '1.16.5', '1.17.1', '1.18.2', '1.19', '1.19.2', '1.19.3', '1.19.4', '1.20', '1.20.1', '1.20.2'] | |||
defaultVersion: '1.20.3', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it's useful to have 1.20.3 there ? What about testing only 1.20.4 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah missed changing the defaultVersion to 1.20.4 when I added the 1.20.4 to the supportedVersions list.
This is not handling all the chat packets. We don't have tests for chat which is not good, so this should be manually tested against the examples in online mode also. |
Which ones is it missing? It covers all 3 chat packets. |
src/client/chat.js
Outdated
// Used for 1.20.3+ to convert NBT back into the JSON string expected of a chat message. | ||
function handleNBTStrings (nbtDataOrString) { | ||
// Check if nbtDataOrString is actually defined | ||
if (nbtDataOrString) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be behind a feature flag, not doing typeof checks
please update the list of supported versions in the readme |
When running the chat example it seems like we have an issue in the declare_commands packet somewhere. Also I do agree, should good behind a featureFlag. Since that prevents Mojang changing something in the future and breaking that type check hack. |
src/client/chat.js
Outdated
@@ -16,6 +17,25 @@ function isFormatted (message) { | |||
} | |||
} | |||
|
|||
// Used for 1.20.3+ to convert NBT back into the JSON string expected of a chat message. | |||
function handleNBTStrings (nbtDataOrString) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this function be exposed? I think there are now dozen of packets using this nbt thing
Personally I am not happy with were this PR is going, the way I handle the NBT chat needs to be re-considered totally, with new info I learned from digging thru the change logs for the snapshots and we also have to dig thru the protocol.json to find what is breaking the declare command packet. |
Let's maybe let it open for now and close if a new PR gets open? it does provide pieces of info even if not everything is good yet |
so I looked at this PR again and actually I don't see what's wrong with it? we just need to expose handleNBTStrings and do some manual testing and it should be fine? what did you have in mind @wgaylord ? |
It should be fine just doing that, but I am not that happy with how the handleNBTString works at all, but it could be good to get support for 1.20.3/4 then write a small library to properly handle the NBT String stuff. Plus we still need a function to go the other way for the server part of nmp. |
would be nice to finish this |
src/datatypes/uuid.js
Outdated
module.exports = { nameToMcOfflineUUID } | ||
function fromIntArray (arr) { | ||
const buf = Buffer.alloc(16) | ||
arr.forEach((num, index) => { buf.writeInt32LE(num, index * 4) }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this may be big endian
src/client/chat.js
Outdated
@@ -22,6 +24,21 @@ module.exports = function (client, options) { | |||
client._lastChatSignature = null | |||
client._lastRejectedMessage = null | |||
|
|||
// 1.20.3+ serializes chat components in chat packets with NBT. Non-chat packets that send messages (like disconnect) still use JSON chat. | |||
// NMP API expects a JSON string, so since the schema is mostly the same we can convert the NBT to JSON with a transform to UUID encoding | |||
function handleNbtComponent (nbtDataOrString) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should expose that function here or in pchat so that mineflayer and flying-squid can use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added prismarine-chat to the test code so it's cleaner (as opposed to handling chat internally), but otherwise there isn't much to add to prismarine-chat. The handleNbtComponent
basically just calls simplify and does some mapping for UUIDs.
But I guess there could be a ChatMessage.fromNbtComponent()
inside prismarine-chat to get a ChatMessage directly. But would it would require a change in nmp API as playerChat
and systemChat
current expect JSON strings, not ChatMessage's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my concern is how do we handle the "chat nbt" in the 10 other non-chat packets where that "chat nbt" is used in mineflayer?
I can see these solutions
- we duplicate handleNbtComponent in nmp and mineflayer and flying-squid (and any other nmp users)
- we expose handleNbtComponent in nmp and use them in users
- we expose handleNbtComponent in pchat and use it in nmp, mineflayer, flying-squid and other places
- we expose handleNbtComponent in pnbt and use it in all users
I think 3 or 4 are better.
2 is okay
1 I don't think it's a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/PrismarineJS/minecraft-data/blob/master/data/pc/1.20.3/protocol.json look for all occurences of anonymousNbt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I see, looks like will need some changes to all node-minecraft-protocol, prismarine-chat, mineflayer to swap the event from JSON string to a ChatMessage for #4. Maybe updating fromNotch() to and then emitting the ChatMessage:
static fromNotch (msg) {
if (registry.supportFeature('chatPacketsUseNbtComponents') && msg.type) {
const simplified = nbt.simplify(msg)
const json = JSON.stringify(simplified, (key, val) => {
if (key === 'id' && Array.isArray(val)) return uuidFromIntArray(val)
return val
})
return new ChatMessage(JSON.parse(json))
} else try {
return new ChatMessage(JSON.parse(msg))
} catch (e) {
return new ChatMessage(msg)
}
}
However doing this would break the fromNetwork in prismarine-chat as it expects a JSON object when formatting the chat string, so that too would need to be updated.
Would also need to figure out a good way to do toNotch()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should expose an independent method in pchat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but yeah hmm let's figure out how to unblock this
src/client/chat.js
Outdated
// already plaintext JSON or empty | ||
return nbtDataOrString | ||
} | ||
client._handleNbtComponent = handleNbtComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this exposed privately and then not used?
if we want to actually expose it, it should be exposed publicly (without _ and in API.md)
I don't get what needs to be done for 1.20.4 compatibility. I have a week off starting Tuesday, so what can I do to help? |
Maybe the best way to understand would be to open a PR on mineflayer pointing to this branch for nmp and get it to run |
I would love to, but what do I actually need to get to work? |
@boredcoder411 PrismarineJS/mineflayer#3310 check the failing tests, read our comments above, then fix it |
Is there a reason that this PR isn't getting merged even though it is passing all the checks? |
Yes, please read above comments |
code lgtm but I wonder if we need to update any example / doc |
@@ -10,6 +10,11 @@ const options = { | |||
const server = mc.createServer(options) | |||
const mcData = require('minecraft-data')(server.version) | |||
const loginPacket = mcData.loginPacket | |||
function chatText (text) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this server example since it's the only one that seems to have chat handling been updated for 1.20
ok last thing to do here is updating the chat handling in the server example |
/fixlint |
I ran > [email protected] fix > standard --fix /home/runner/work/node-minecraft-protocol/node-minecraft-protocol/examples/server/server.js:15:7: 'nbt' is not defined. (no-undef) /home/runner/work/node-minecraft-protocol/node-minecraft-protocol/examples/server/server.js:15:24: 'nbt' is not defined. (no-undef) standard: Use JavaScript Standard Style (https://standardjs.com) |
add missing import
ok updated examples |
/makerelease |
Updated minecraft-data, added 1.20.3 and 1.20.4 to the versions lists.
Added particle type into the packetTest.js.
Current Tests for 1.20.3/4 fail due to chat issues.
Next step its to handle everywhere string is now an NBT object.