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

Activity

1. nickelpro commented on Jan 2, 2023

   

   nickelpro

   on Jan 2, 2023

   Last edited by nickelpro

   Member

   More actions

   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().
2. nickelpro

   changed the title [-]getMask for 1.15.2 returns wrong value in some cases[/-] [+]chunk `.dump()` doesn't match section mask for versions all versions <=1.17[/+] on Jan 2, 2023
3. nickelpro commented on Jan 2, 2023

   

   nickelpro

   on Jan 2, 2023

   Member

   More actions

   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
4. kf106 commented on Jan 2, 2023

   

   kf106

   on Jan 2, 2023

   Last edited by kf106

   Author

   More actions

   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.
5. nickelpro

   changed the title [-]chunk `.dump()` doesn't match section mask for versions all versions <=1.17[/-] [+]chunk `.dump()` doesn't match section mask for all versions <=1.17[/+] on Jan 2, 2023
6. kf106 commented on Jan 2, 2023

   

   kf106

   on Jan 2, 2023

   Author

   More actions

   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.
7. extremeheat commented on Jan 2, 2023

   

   extremeheat

   on Jan 2, 2023

   Member

   More actions

   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.