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.

Sign up for GitHub

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

Jump to bottom

fix: dumped buffer must not be empty #240

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: dumped buffer must not be empty #240

wants to merge 5 commits into from

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Jun 7, 2024

I already made this test in prismarine-anvil-provider:

const dumped = oldChunk.dump()
const lights = oldChunk.dumpLight()
const biomes = oldChunk.dumpBiomes()

const chunk = new Chunk()
chunk.load(dumped, chunk.getMask(), true, true) // always expects buffer data to read
chunk.loadBiomes(biomes)
chunk.loadParsedLight?.(lights.skyLight, lights.blockLight, chunk.skyLightMask, chunk.blockLightMask, chunk.emptySkyLightMask, chunk.emptyBlockLightMask)

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

extremeheat
extremeheat reviewed Jun 7, 2024
View reviewed changes
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)
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Member

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

which load method do you mean? if you mean vanilla one; then ok we should do that and leave a comment to explain this is to address a bug in the vanilla implementation
if it's our implementation of load, then we should fix the bug

Copy link
Member

Choose a reason for hiding this comment

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

and checked that native server always sends the buffer filled with 0 in these cases

this seems to suggest the mojang server; is that indeed what you mean? Do you know why they have this bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's a bug, most probably the vanilla server just tried to normalize the data so the client marks received chunk as empty (otherwise it would print waiting for chunks)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I was about our load method. Not sure what should be fixed in that method instead

@zardoy zardoy mentioned this pull request Jun 13, 2024
Open
rom1504
rom1504 reviewed Jun 16, 2024
View reviewed changes
src/pc/1.13/ChunkColumn.js
@@ -199,6 +199,10 @@ module.exports = (Block, mcData) => {
smartBuffer.writeInt32BE(biome)
})

if (!smartBuffer.length) {
return Buffer.alloc(1024)
Copy link
Member

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

@rom1504
Copy link
Member

rom1504 commented Oct 29, 2024

PrismarineJS/prismarine-provider-anvil#81 looks like this is working without this change

is this prismarine chunk pr truly necessary?

@zardoy
Copy link
Contributor Author

zardoy commented Oct 30, 2024

PrismarineJS/prismarine-provider-anvil#81 looks like this is working without this change

is this prismarine chunk pr truly necessary?

I never said it is required for basic functionality for new versions, it is not related. Can you look at OP please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@extremeheat extremeheat extremeheat left review comments

@rom1504 rom1504 rom1504 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
PR Triage
Status: Almost too old
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zardoy @rom1504 @extremeheat