chunk .dump() doesn't match section mask for all versions <=1.17

[kf106]

In 1.15.2 PC version if you build up a chunk, then delete the blocks (replacing them with air), the chunk.getMask value returned is that for the previous tall chunk, i.e. it doesn't recompute the mask to only count upward to the last non-air block.

This means that if you build a tall tower, delete it with mining, save the chunk using chunk.dump, save the bitmask value using chunk.getMask, and then try to load the saved chunk using chunk.load with the stored getMask value, you get an error that you're reading past the memory bound.

This is using flying-squid and prismarine-web-client

[nickelpro]

So two things:

• We never re-compress a chunk or chunk column (which is something we can argue about the correctness of)
• Therefore, once a block has been set inside a chunk, that chunk is always part of the sectionMask. Once a chunk is non-empty, it is forever non-empty from the point of view of pris-chunk. We never zero any bits in the sectionMask

So .getMask() at runtime is correct, no bug there, because internally that's how the chunk is calculating. Ie, a section object with a palette exists for that part of the chunk column.

The problem is how we serialize in .dump(), we ask the wrong question, we ask:

if (section !== null && !section.isEmpty()) {
  section.write(smartBuffer)
}

Ostensibly as a space-saving measure. And this is wrong, or at least a tricky corner. I think we need to rethink this. It might be we need do a naive compression pass (eliminating empty chunks and recalculating the section mask) whenever dump() is called, but we need some way to communicate that getMask() calls from prior to the dump() are invalid after the dump().

[nickelpro]

I'm not looking at any docs here, is it invalid to serialize a chunk with zero solid blocks? I don't think so.

The bug fix might be that we remove the !section.isEmpty() check from affected versions. And add a separate compress() method to their chunk column objects that does this removal of empty sections and recalcs the section mask.

Ping @extremeheat

[kf106]

I was thinking of adding a return value to .dump() that returns the actual mask for the dumped chunk. But removing isEmpty() and having a compress function which removes empty sections and recalculates the section mask makes more sense to me.

[kf106]

I tried implementing your suggestion, and indeed there are other areas which subsequently fail if you update the section mask.

So I took the quick and easy route and added a new function to get the actual mask for the dumped buffer:

    dumpMask () {
      let newMask = this.sectionMask
      this.sections.forEach((section, i) => {
        if (section !== null && section.isEmpty()) {
          newMask &= ~(1 << i)
        }
      })
      return newMask
    }

When I'm using dump and load in my fs plugin, instead of calling getMask I call dumpMask and get the correct value, and the rest of the code works as before.

[extremeheat]

Ostensibly as a space-saving measure. And this is wrong, or at least a tricky corner. I think we need to rethink this. It might be we need do a naive compression pass (eliminating empty chunks and recalculating the section mask) whenever dump() is called, but we need some way to communicate that getMask() calls from prior to the dump() are invalid after the dump().

Yeah, on the bedrock implementation we have some handling for this:

prismarine-chunk/src/bedrock/1.3/SubChunk.js

Lines 282 to 297 in 23ae199

	// These compation functions reduces the size of the chunk by removing unused blocks
	// and reordering the palette to reduce the number of bits per block
	isCompactable (layer) {
	let newPaletteLength = 0
	for (const block of this.palette[layer]) {
	if (block.count > 0) {
	newPaletteLength++
	}
	}
	return newPaletteLength < this.palette[layer].length
	}
	compact (layer) {
	const newPalette = []
	const map = []
	for (const block of this.palette[layer]) {

And compact() on serializatized export:

prismarine-chunk/src/bedrock/1.3/SubChunk.js

Line 166 in 23ae199

if (compact) this.compact(l) // Compact before encoding

Works by tracking the count of the items in the palette and rebuilding the storage at the end (if needed) when something like dump() would be called.