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

1.20.3 / 1.20.4 support #1275

Merged
merged 19 commits into from
Feb 26, 2024
Merged

1.20.3 / 1.20.4 support #1275

merged 19 commits into from
Feb 26, 2024

Conversation

wgaylord
Copy link
Contributor

@wgaylord wgaylord commented Dec 30, 2023

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.

@wgaylord
Copy link
Contributor Author

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',
Copy link
Member

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 ?

Copy link
Contributor Author

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.

@extremeheat
Copy link
Member

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.

@wgaylord
Copy link
Contributor Author

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.

// 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) {
Copy link
Member

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

@rom1504
Copy link
Member

rom1504 commented Dec 30, 2023

please update the list of supported versions in the readme

@wgaylord
Copy link
Contributor Author

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.

@@ -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) {
Copy link
Member

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

@wgaylord
Copy link
Contributor Author

wgaylord commented Jan 1, 2024

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.

@wgaylord wgaylord closed this Jan 1, 2024
@rom1504
Copy link
Member

rom1504 commented Jan 3, 2024

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

@rom1504 rom1504 reopened this Jan 3, 2024
@rom1504
Copy link
Member

rom1504 commented Jan 7, 2024

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 ?

@wgaylord
Copy link
Contributor Author

wgaylord commented Jan 8, 2024

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.

@rom1504
Copy link
Member

rom1504 commented Jan 14, 2024

would be nice to finish this
we now have 1.20.2 support in minecraft ; so if this gets finished then we can have 1.20.4 in mineflayer and be up to date

module.exports = { nameToMcOfflineUUID }
function fromIntArray (arr) {
const buf = Buffer.alloc(16)
arr.forEach((num, index) => { buf.writeInt32LE(num, index * 4) })
Copy link
Member

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

@@ -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) {
Copy link
Member

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

Copy link
Member

@extremeheat extremeheat Jan 14, 2024

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

Copy link
Member

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

  1. we duplicate handleNbtComponent in nmp and mineflayer and flying-squid (and any other nmp users)
  2. we expose handleNbtComponent in nmp and use them in users
  3. we expose handleNbtComponent in pchat and use it in nmp, mineflayer, flying-squid and other places
  4. 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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@extremeheat extremeheat Jan 14, 2024

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()

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 should expose an independent method in pchat

Copy link
Member

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

@rom1504 rom1504 changed the title WIP 1.20.3 / 1.20.4 support 1.20.3 / 1.20.4 support Jan 14, 2024
// already plaintext JSON or empty
return nbtDataOrString
}
client._handleNbtComponent = handleNbtComponent
Copy link
Member

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)

@boredcoder411
Copy link

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?

@rom1504
Copy link
Member

rom1504 commented Feb 15, 2024

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

@boredcoder411
Copy link

I would love to, but what do I actually need to get to work?

@rom1504
Copy link
Member

rom1504 commented Feb 16, 2024

@boredcoder411 PrismarineJS/mineflayer#3310 check the failing tests, read our comments above, then fix it

@TerminalCalamitas
Copy link

Is there a reason that this PR isn't getting merged even though it is passing all the checks?

@rom1504
Copy link
Member

rom1504 commented Feb 19, 2024

Yes, please read above comments

@rom1504
Copy link
Member

rom1504 commented Feb 24, 2024

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) {
Copy link
Member

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

@rom1504
Copy link
Member

rom1504 commented Feb 24, 2024

ok last thing to do here is updating the chat handling in the server example
we could also decide to push that post this PR; but I think it'd be better to do it here

@rom1504
Copy link
Member

rom1504 commented Feb 24, 2024

/fixlint

@rom1504bot
Copy link
Contributor

I ran npm run fix, but there are errors still left that must be manually resolved:

> [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)

@rom1504
Copy link
Member

rom1504 commented Feb 26, 2024

ok updated examples

@rom1504 rom1504 merged commit 1d9a382 into PrismarineJS:master Feb 26, 2024
24 checks passed
@rom1504
Copy link
Member

rom1504 commented Feb 26, 2024

/makerelease

@rom1504bot rom1504bot mentioned this pull request Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

6 participants