-
Notifications
You must be signed in to change notification settings - Fork 60
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
fix: dumped buffer must not be empty #240
base: master
Are you sure you want to change the base?
Conversation
src/pc/1.13/ChunkColumn.js
Outdated
@@ -199,6 +199,10 @@ module.exports = (Block, mcData) => { | |||
smartBuffer.writeInt32BE(biome) | |||
}) | |||
|
|||
if (!smartBuffer.length) { | |||
return Buffer.alloc(4096) |
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.
What are these zeros representing? Do you have a link to minecraft code to explain?
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.
It's just a workoaround so the load method (which expects the buffer to be not empty) doesn't crash. I'm not sure why I used the size of 4096. I see the native sever uses the length of 1024.
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.
There should not be any "workarounds" in stable API methods. The libs should adhere to whatever the vanilla behavior is. In this case "load" deserializes a vanilla chunk from the network and "dump" serializes something to be sent over the network.
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.
Yes, in this case dump works incorrectly then. Afaik even minecraft-protocol doesn't allow serialization of empty buffers. I understand there should be no workarounds, can you elaborate on the correct way to fix 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.
Allocating a buffer of the right size
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.
Ok! Will also try to add a test
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.
Allocating a buffer of the right size
@rom1504 ok, got it fixed. should be good now
@@ -199,6 +199,10 @@ module.exports = (Block, mcData) => { | |||
smartBuffer.writeInt32BE(biome) | |||
}) | |||
|
|||
if (!smartBuffer.length) { | |||
return Buffer.alloc(1024) |
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 do we want to create empty buffers? if there is a reason, add a comment
I already made this test in prismarine-anvil-provider:
which runs against this file: https://github.com/zardoy/prismarine-provider-anvil/blob/everything/test/fixtures/1.13.2/r.-1.-1.mca which appears to be the world of type the void
and checked that native server always sends the buffer filled with 0 in these cases