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

make 1.16.2 the tested version instead of 1.16.1 #757

Closed
wants to merge 2 commits into from
Closed

Conversation

rom1504
Copy link
Member

@rom1504 rom1504 commented Aug 16, 2020

No description provided.

@rom1504
Copy link
Member Author

rom1504 commented Aug 16, 2020

I'm betting on something wrong in https://github.com/ProtoDef-io/node-protodef/blob/master/src/datatypes/compiler-utils.js#L37
maybe you could have a quick look @Karang to see if you see anything obvious ?
Problematic bitfield is there https://github.com/PrismarineJS/minecraft-data/blob/master/data/pc/1.16.2/protocol.json#L1587

@rom1504
Copy link
Member Author

rom1504 commented Aug 16, 2020

#718 could help simplify this procedure and confidence in it

@Karang
Copy link
Contributor

Karang commented Aug 16, 2020

I'm betting on something wrong in https://github.com/ProtoDef-io/node-protodef/blob/master/src/datatypes/compiler-utils.js#L37
maybe you could have a quick look @Karang to see if you see anything obvious ?
Problematic bitfield is there https://github.com/PrismarineJS/minecraft-data/blob/master/data/pc/1.16.2/protocol.json#L1587

Nothing obvious, I'll run the test manually later.

@Karang
Copy link
Contributor

Karang commented Aug 16, 2020

Looks like the problem is in the generated test packet (p1), it doesn't match the schema (x, y, z gets generated inside and outside the bitfield):

p1: {
  "x": 1,
  "z": 1,
  "y": 1,
  "chunkCoordinates": {
    "x": 1,
    "z": 1,
    "y": 1
  },
  "notTrustEdges": true,
  "records": [
    1
  ]
}
p2: {
  "chunkCoordinates": {
    "x": 1,
    "z": 1,
    "y": 1
  },
  "notTrustEdges": true,
  "records": [
    1
  ]
}

1) packets 1.16.2
       play,ClientBound,packet_multi_block_change:

      Uncaught AssertionError [ERR_ASSERTION]: field x missing in p2, in p1 it has value 1

Edit: indeed it gets added twice https://github.com/PrismarineJS/node-minecraft-protocol/blob/master/test/packetTest.js#L170 (context and result) @rom1504 do you remember what was the intent behind this code ?

@rom1504
Copy link
Member Author

rom1504 commented Aug 16, 2020

I don't remember, I just removed this line.
These packets tests should be generalized and cleaned up I think ProtoDef-io/node-protodef#83

@rom1504
Copy link
Member Author

rom1504 commented Aug 16, 2020

mc-server 1.16.2 "clients can log in and chat" failing now apparently

@rom1504
Copy link
Member Author

rom1504 commented Aug 16, 2020

most likely related to packet changes, I'll list them here so it's easier to update mineflayer and flying squid too

@rom1504
Copy link
Member Author

rom1504 commented Aug 16, 2020

reference PrismarineJS/minecraft-data#326

@rom1504
Copy link
Member Author

rom1504 commented Aug 21, 2020

  • packet_multi_block_change : different fields
  • packet_map_chunk : ignoreOldData field is gone + biomes is a variable size array now (instead of 1024)
  • packet_login : new isHardcore bool, dimension now nbt, maxPlayers now varint
  • packet_unlock_recipes : new recipe kinds
  • new packet packet_displayed_recipe
  • new packet packet_recipe_book
  • packet_crafting_book_data was removed

Important changes for nmp, mineflayer and flying-squid :

  • packet_multi_block_change : change needed in mineflayer
  • packet_map_chunk : change needed in nmp, mineflayer and flying-squid
  • packet_login : change needed in nmp, mineflayer and flying-squid

@rom1504
Copy link
Member Author

rom1504 commented Sep 5, 2020

worked a bit on this. Main thing that is annoying is that login packet. The server is now expected to provide a big list of stuff each biome should be doing.
In 1.16.1 they introduced having to send a dimension codec which was already big, but now it's really way too big to put in the example.
I think we should save the default value in a json in minecraft-data. and then it would be used here and in flying-squid in one line.
Will do that another time

@GroobleDierne
Copy link
Contributor

worked a bit on this. Main thing that is annoying is that login packet. The server is now expected to provide a big list of stuff each biome should be doing.
In 1.16.1 they introduced having to send a dimension codec which was already big, but now it's really way too big to put in the example.
I think we should save the default value in a json in minecraft-data. and then it would be used here and in flying-squid in one line.
Will do that another time

When you say I think we should save the default value in a json in minecraft-data, are you talking about the whole login packet or just the dimension codec field ?

@TheDudeFromCI
Copy link
Member

worked a bit on this. Main thing that is annoying is that login packet. The server is now expected to provide a big list of stuff each biome should be doing.
In 1.16.1 they introduced having to send a dimension codec which was already big, but now it's really way too big to put in the example.
I think we should save the default value in a json in minecraft-data. and then it would be used here and in flying-squid in one line.
Will do that another time

When you say I think we should save the default value in a json in minecraft-data, are you talking about the whole login packet or just the dimension codec field ?

Just the dimension part, since the rest of the elements are just sent one at a time. The dimension part is quite a large chunk of data on it's own, however. Plus it's likely to change from version to version as new biomes are added or modified.

@TheDudeFromCI
Copy link
Member

Might as well note this now.

As mentioned in the Discord server, the approach to generating this data would be to log into a 1.16.2 server using mc-protocol and basically save all data from the login packet. Then simply extract the biome data elements into MC-Data.

@rom1504
Copy link
Member Author

rom1504 commented Oct 3, 2020

yes
in the previous versions it was reasonable to store this directly in the example as it was only tens of lines, now it's thousands of lines, it's not reasonable anymore, which is why I'm suggesting to put it in mc data

@GroobleDierne
Copy link
Contributor

I am interested to do it, is there an existing file for these data?

@rom1504
Copy link
Member Author

rom1504 commented Oct 3, 2020

no but

As mentioned in the Discord server, the approach to generating this data would be to log into a 1.16.2 server using mc-protocol and basically save all data from the login packet. Then simply extract the biome data elements into MC-Data.

@GroobleDierne
Copy link
Contributor

I meant in MCData

@rom1504
Copy link
Member Author

rom1504 commented Oct 3, 2020

same answer

@GroobleDierne
Copy link
Contributor

ok

@TheDudeFromCI
Copy link
Member

Not yet. You can create a new called biomeLogin.json or something.

@rom1504
Copy link
Member Author

rom1504 commented Oct 3, 2020

@GroobleDierne released your file as mc data 2.69.0, you can now use it as .loginPacket here

@GroobleDierne
Copy link
Contributor

Thanks. I will do a PR soon for NMP

@rom1504
Copy link
Member Author

rom1504 commented Oct 4, 2020

#765

@rom1504
Copy link
Member Author

rom1504 commented Oct 6, 2020

closing in favor of #765

@rom1504 rom1504 closed this Oct 6, 2020
@rom1504 rom1504 deleted the 1.16.2 branch October 6, 2020 20:44
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.

4 participants