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

taking minecraft: and no start with minecraft: in loadDimensionCodec #35

Closed
wants to merge 5 commits into from

Conversation

qwqtoday
Copy link

#34 fix

@extremeheat
Copy link
Member

Can you update the accompanying write method?

@qwqtoday
Copy link
Author

do you mean you want me to make "if it's 1.20.5+ then call other method if it is 1.20.4 or lower then call this method"

@extremeheat
Copy link
Member

No, there is a writeDimensionCodec method and it is still writing it with the minecraft: prefix for 1.20.5

@qwqtoday
Copy link
Author

qwqtoday commented May 22, 2024

oh ok, I didn't understand what is writeDimensionCodec, I know now

@extremeheat
Copy link
Member

extremeheat commented May 22, 2024

One way is to check on every assignment, or to simplify a function can be used to resolve the name based on a version branch:

  const id = str => version[>=](1.20.5) ? str : ('minecraft:' + str) 
  codec[id('dimension_type')] = nbt.comp({

@qwqtoday
Copy link
Author

cool, now we just have to update minecraft-data in order to use this new feature (the login packet also needed to change)

@qwqtoday qwqtoday changed the title taking minecraft: and no start with minecraft: in loadDimensionCodec 🚨🚨🚨 really important pr, hope you will merge asap 🚨 taking minecraft: and no start with minecraft: in loadDimensionCodec May 31, 2024
lib/pc/index.js Outdated
@@ -5,13 +5,15 @@ nbt.float = value => ({ type: 'float', value })
const { networkBiomesToMcDataSchema, mcDataSchemaToNetworkBiomes } = require('./transforms')

module.exports = (data, staticData) => {
const id = str => data.version['>=']('1.20.5') ? str : ('minecraft:' + str)
Copy link
Member

Choose a reason for hiding this comment

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

Please use a mc data feature instead

Copy link
Author

@qwqtoday qwqtoday Jun 3, 2024

Choose a reason for hiding this comment

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

do you mean using data.supportFeature()?

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Author

Choose a reason for hiding this comment

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

Oh ok

@qwqtoday qwqtoday changed the title 🚨🚨🚨 really important pr, hope you will merge asap 🚨 taking minecraft: and no start with minecraft: in loadDimensionCodec taking minecraft: and no start with minecraft: in loadDimensionCodec Jun 3, 2024
@qwqtoday
Copy link
Author

now we need someone to make this in mcdata

@extremeheat
Copy link
Member

This seems unrelated to 1.20.5. 1.20.5 per dumped packet data still uses minecraft: prefix in the codec

https://github.com/extremeheat/minecraft-data/blob/1.20.5protocol/data/pc/1.20.5/loginPacket.json

@qwqtoday
Copy link
Author

qwqtoday commented Jul 6, 2024

This seems unrelated to 1.20.5. 1.20.5 per dumped packet data still uses minecraft: prefix in the codec

https://github.com/extremeheat/minecraft-data/blob/1.20.5protocol/data/pc/1.20.5/loginPacket.json

no man, the dimensionCodec field shouldn't exist in the loginPacket anymore

@rom1504
Copy link
Member

rom1504 commented Jul 6, 2024

You can ask complains to Mojang if you think they should stop sending that field then.

@qwqtoday
Copy link
Author

qwqtoday commented Jul 6, 2024

You can ask complains to Mojang if you think they should stop sending that field then.

I am saying it isn't there anymore

@extremeheat
Copy link
Member

The loginPacket data is accurate, the dimensionCodec data was moved to its own registry_data packet but we still merge it into the loginPacket in minecraft-data for compatibility with existing code. I don't know about 1.20.6 but for 1.20.5 it is working with #37 now.

@qwqtoday
Copy link
Author

qwqtoday commented Jul 8, 2024

The server I play on sent packet like that, so I assumed that the 1.20.5 have some behaviour like that.

@Gjum Gjum mentioned this pull request Aug 18, 2024
2 tasks
@@ -5,13 +5,15 @@ nbt.float = value => ({ type: 'float', value })
const { networkBiomesToMcDataSchema, mcDataSchemaToNetworkBiomes } = require('./transforms')

module.exports = (data, staticData) => {
const id = str => data.supportFeature('registryKeysStartWithMinecraft') ? str : ('minecraft:' + str)
Copy link
Member

Choose a reason for hiding this comment

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

this feature is not defined in minecraft data

Copy link
Author

Choose a reason for hiding this comment

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

Can someone define it for me

Copy link
Member

Choose a reason for hiding this comment

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

Make a PR to minecraft-data to add it

But I don't really get what you want to do here since Minecraft prefixes are there for all versions

@rom1504
Copy link
Member

rom1504 commented Sep 8, 2024

in what case is the minecraft prefix missing?

@qwqtoday
Copy link
Author

qwqtoday commented Sep 9, 2024

in what case is the minecraft prefix missing?

Some cases, I don’t know what are all the cases but one of them is joining a server with higher version with the via version plugin

Not sure if write is affected, we can revert that if it isn’t.

@qwqtoday
Copy link
Author

qwqtoday commented Sep 9, 2024

Packets like this are valid packets and can be handled by the Minecraft client, as an identifier namespace defaults to minecraft: if it’s not provided

@GenerelSchwerz
Copy link

bump, this project fixes connection issues with ViaProxy to 1.21.1.

@GenerelSchwerz
Copy link

Update: this merge needs to be done in order for https://github.com/GenerelSchwerz/mineflayer-viaproxy to be stable. I can patch the code into prismarine-registry manually right now, but I'd rather not.

let hasDynamicDimensionData = false

return {
loadDimensionCodec (codec) {
const dimensionCodec = nbt.simplify(codec)

const chat = dimensionCodec['minecraft:chat_type']?.value
const chat = dimensionCodec['minecraft:chat_type']?.value ?? dimensionCodec.chat_type?.value
Copy link
Member

Choose a reason for hiding this comment

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

Use function id here

@@ -30,7 +32,7 @@ module.exports = (data, staticData) => {
}
}

const dimensions = dimensionCodec['minecraft:dimension_type']?.value
const dimensions = dimensionCodec['minecraft:dimension_type']?.value ?? dimensionCodec.dimension_type?.value
Copy link
Member

Choose a reason for hiding this comment

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

Use function id

@@ -46,7 +48,7 @@ module.exports = (data, staticData) => {
}
}

const biomes = dimensionCodec['minecraft:worldgen/biome']?.value
const biomes = dimensionCodec['minecraft:worldgen/biome']?.value ?? dimensionCodec['worldgen/biome']?.value
Copy link
Member

Choose a reason for hiding this comment

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

Use function id

@qwqtoday
Copy link
Author

@rom1504 the read function is used by mineflayer to handle that packet, according to wiki.vg, that part is a identifier and a identifier defaults to the minecraft namespace if not specified, but it doesn’t mean that servers should always send that identifier starts with minecraft or not.
both identifier is valid and i want to handle both

@rom1504
Copy link
Member

rom1504 commented Oct 12, 2024

done in #37

@rom1504 rom1504 closed this Oct 12, 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.

4 participants