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

fix: dumped buffer must not be empty #240

Closed
wants to merge 5 commits into from
+30 −6
Closed

fix: dumped buffer must not be empty #240

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3d1532a
fix type!
zardoy Jun 7, 2024
ff0dee5
fix again!
zardoy Jun 7, 2024
becb25b
and fix again!
zardoy Jun 7, 2024
d38c5b3
fix: dumped buffer must not be empty
zardoy Jun 7, 2024
8884ea0
4096->1024
zardoy Jun 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/pc/1.13/ChunkColumn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

}

return smartBuffer.toBuffer()
}

Expand Down
4 changes: 4 additions & 0 deletions src/pc/1.14/ChunkColumn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,10 @@ module.exports = (Block, mcData) => {
smartBuffer.writeInt32BE(biome)
})

if (!smartBuffer.length) {
return Buffer.alloc(4096)
}

return smartBuffer.toBuffer()
}

Expand Down
3 changes: 3 additions & 0 deletions src/pc/1.15/ChunkColumn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,9 @@ module.exports = (Block, mcData) => {
section.write(smartBuffer)
}
})
if (!smartBuffer.length) {
return Buffer.alloc(4096)
}
return smartBuffer.toBuffer()
}

Expand Down
3 changes: 3 additions & 0 deletions src/pc/1.16/ChunkColumn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,9 @@ module.exports = (Block, mcData) => {
section.write(smartBuffer)
}
})
if (!smartBuffer.length) {
return Buffer.alloc(4096)
}
return smartBuffer.toBuffer()
}

Expand Down
3 changes: 3 additions & 0 deletions src/pc/1.17/ChunkColumn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,9 @@ module.exports = (Block, mcData) => {
section.write(smartBuffer)
}
})
if (!smartBuffer.length) {
return Buffer.alloc(4096)
}
return smartBuffer.toBuffer()
}

Expand Down
3 changes: 3 additions & 0 deletions src/pc/1.18/ChunkColumn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,9 @@ module.exports = (Block, mcData) => {
this.sections[i].write(smartBuffer)
this.biomes[i].write(smartBuffer)
}
if (!smartBuffer.length) {
return Buffer.alloc(4096)
}
return smartBuffer.toBuffer()
}

Expand Down
4 changes: 4 additions & 0 deletions src/pc/1.9/ChunkColumn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,10 @@ module.exports = (Block, mcData) => {
smartBuffer.writeUInt8(biome)
})

if (!smartBuffer.length) {
return Buffer.alloc(4096)
}

return smartBuffer.toBuffer()
}

Expand Down
12 changes: 6 additions & 6 deletions types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ declare class PCChunk extends CommonChunk {
getBlockData(pos: Vec3): number
getBlockLight(pos: Vec3): number
getSkyLight(pos: Vec3): number
getBiome(pos: Vec3): number
getBiome(pos: Vec3): number
setBlockStateId(pos: Vec3, stateId: number): number
setBlockType(pos: Vec3, id: number): void
setBlockData(pos: Vec3, data: Buffer): void
Expand All @@ -46,7 +46,7 @@ declare class PCChunk extends CommonChunk {
dumpBiomes(): Array<number>
dumpLight(): Buffer
loadLight(data: Buffer, skyLightMask: number, blockLightMask: number, emptySkyLightMask?: number, emptyBlockLightMask?: number): void
loadLightParse(skyLight: Buffer[], blockLight: Buffer[], skyLightMask: number[][], blockLightMask: number[][], emptySkyLightMask: number[][], emptyBlockLightMask: number[][]): void
loadParsedLight?(skyLight: Buffer[], blockLight: Buffer[], skyLightMask: number[][], blockLightMask: number[][], emptySkyLightMask: number[][], emptyBlockLightMask: number[][]): void
loadBiomes(newBiomesArray: Array<number>): void;
dump(bitMap?: number, skyLightSent?: boolean): Buffer
load(data: Buffer, bitMap?: number, skyLightSent?: boolean, fullChunk?: boolean): void
Expand Down Expand Up @@ -113,7 +113,7 @@ declare class BedrockChunk extends CommonChunk {
maxCY: number
// The version of the chunk column (analog to DataVersion on PCChunk)
chunkVersion: number
// Holds all the block entities in the chunk, the string keys are
// Holds all the block entities in the chunk, the string keys are
// the concatenated chunk column-relative position of the block.
blockEntities: Record<string, NBT>
// Holds entities in the chunk, the string key is the entity ID
Expand Down Expand Up @@ -174,7 +174,7 @@ declare class BedrockChunk extends CommonChunk {
networkEncodeSubChunkNoCache(y: number): Promise<Buffer>

/**
*
*
* @param blobs The blob hashes sent in the SubChunk packet
* @param blobStore The Blob Store holding the chunk data
* @param payload The remaining data sent in the SubChunk packet, border blocks
Expand All @@ -198,7 +198,7 @@ declare class BedrockChunk extends CommonChunk {
loadHeights(map: Uint16Array): void
writeHeightMap(stream): void

//
//
// Section management
getSection(pos): SubChunk
// Returns chunk at a Y index, adjusted for chunks at negative-Y
Expand Down Expand Up @@ -226,4 +226,4 @@ export const enum BlobType {
Biomes = 1,
}

export default function loader(mcVersionOrRegistry: string | Registry): typeof PCChunk | typeof BedrockChunk
export default function loader(mcVersionOrRegistry: string | Registry): typeof PCChunk | typeof BedrockChunk
Loading