review comments

src/client/mapblock_mesh.cpp

@@ -57,7 +57,7 @@ void MeshMakeData::fillBlockData(const v3s16 &bp, MapNode *data)
 	VoxelArea data_area(v3s16(0,0,0), data_size - v3s16(1,1,1));
 
 	v3s16 blockpos_nodes = bp * MAP_BLOCKSIZE;
-	m_vmanip.copyFrom(data, MAP_BLOCKSIZE * MAP_BLOCKSIZE * MAP_BLOCKSIZE, data_area, v3s16(0,0,0), blockpos_nodes, data_size);
+	m_vmanip.copyFrom(data, false, data_area, v3s16(0,0,0), blockpos_nodes, data_size);
 }
 
 void MeshMakeData::fillSingleNode(MapNode data, MapNode padding)

src/mapblock.cpp

@@ -214,7 +214,7 @@ void MapBlock::copyTo(VoxelManipulator &dst)
 	VoxelArea data_area(v3s16(0,0,0), data_size - v3s16(1,1,1));
 
 	// Copy from data to VoxelManipulator
-	dst.copyFrom(data, m_is_mono_block ? 1 : nodecount, data_area, v3s16(0,0,0),
+	dst.copyFrom(data, m_is_mono_block, data_area, v3s16(0,0,0),
 			getPosRelative(), data_size);
 }
 
@@ -232,6 +232,8 @@ void MapBlock::copyFrom(const VoxelManipulator &src)
 
 void MapBlock::reallocate(u32 count, MapNode n)
 {
+	assert(count == 1 || count == nodecount);
+
 	delete[] data;
 	if (!m_is_mono_block && count == 1)
 		porting::TrackFreedMemory(sizeof(MapNode) * nodecount);
@@ -307,8 +309,8 @@ void MapBlock::expireIsAirCache()
 // Renumbers the content IDs (starting at 0 and incrementing)
 // Note that there's no technical reason why we *have to* renumber the IDs,
 // but we do it anyway as it also helps compressability.
-static void getBlockNodeIdMapping(NameIdMapping *nimap, MapNode *nodes,
+static void getBlockNodeIdMapping(NameIdMapping *nimap, MapNode *nodes, u32 nodecount,
-	const NodeDefManager *nodedef, u32 nodecount)
+	const NodeDefManager *nodedef)
 {
 	IdIdMapping &mapping = IdIdMapping::giveClearedThreadLocalInstance();
 
@@ -427,9 +429,9 @@ void MapBlock::serialize(std::ostream &os_compressed, u8 version, bool disk, int
 	if(disk)
 	{
 		const size_t size = m_is_mono_block ? 1 : nodecount;
-		auto tmp_nodes = std::make_unique<MapNode[]>(size);
+		std::unique_ptr<MapNode[]> tmp_nodes(new MapNode[size]);
 		std::copy_n(data, size, tmp_nodes.get());
-		getBlockNodeIdMapping(&nimap, tmp_nodes.get(), m_gamedef->ndef(), size);
+		getBlockNodeIdMapping(&nimap, tmp_nodes.get(), size, m_gamedef->ndef());
 
 		buf = MapNode::serializeBulk(version, tmp_nodes.get(), nodecount,
 				content_width, params_width, m_is_mono_block);

src/mapblock.h

@@ -30,7 +30,7 @@ class TestMapBlock;
 ////
 
 enum ModReason : u32 {
-	MOD_REASON_REALLOCATE                 = 1 << 0,
+//	UNUSED                                = 1 << 0,
 	MOD_REASON_SET_IS_UNDERGROUND         = 1 << 1,
 	MOD_REASON_SET_LIGHTING_COMPLETE      = 1 << 2,
 	MOD_REASON_SET_GENERATED              = 1 << 3,
@@ -440,9 +440,9 @@ private:
 	*/
 
 	void deSerialize_pre22(std::istream &is, u8 version, bool disk);
-	// check if all nodes are identical, if so store them as a single node
+	// check if all nodes are identical, if so convert to monoblock
 	void tryShrinkNodes();
-	// if only a single node is stored, expand storage back to the full array
+	// if a monoblock, expand storage back to the full array
 	void expandNodesIfNeeded();
 	void reallocate(u32 count, MapNode n);
 
@@ -487,7 +487,7 @@ private:
 	 * fragmentation (the array is exactly 16K, or exactly 4 bytes for a "monoblock"),
 	 * CPU caches and/or optimizability of algorithms working on this array.
 	 */
-	MapNode * data = nullptr;
+	MapNode *data = nullptr;
 
 	// provides the item and node definitions
 	IGameDef *m_gamedef;
@@ -498,6 +498,10 @@ private:
 	*/
 	float m_usage_timer = 0;
 
+	/*
+	 * For "monoblocks", the whole block is filled with the same node, only this node is stored.
+	 * (For reduced memory usage)
+	 */
 	bool m_is_mono_block;
 public:
 	//// ABM optimizations ////

src/mapnode.cpp

@@ -602,12 +602,13 @@ Buffer<u8> MapNode::serializeBulk(int version,
 	// Writing to the buffer linearly is faster
 	u8 *p = &databuf[0];
 	if (is_mono_block) {
+		MapNode n = nodes[0];
 		for (u32 i = 0; i < nodecount; i++, p += 2)
-			writeU16(p, nodes[0].param0);
+			writeU16(p, n.param0);
 		for (u32 i = 0; i < nodecount; i++, p++)
-			writeU8(p, nodes[0].param1);
+			writeU8(p, n.param1);
 		for (u32 i = 0; i < nodecount; i++, p++)
-			writeU8(p, nodes[0].param2);
+			writeU8(p, n.param2);
 	} else {
 		for (u32 i = 0; i < nodecount; i++, p += 2)
 			writeU16(p, nodes[i].param0);

src/unittest/test_mapblock.cpp

@@ -58,52 +58,40 @@ void TestMapBlock::testMonoblock(IGameDef *gamedef)
 {
 	MapBlock block({}, gamedef);
 	UASSERT(block.m_is_mono_block);
-	MapNode *t = block.data;
 	block.data[0] = MapNode(CONTENT_AIR);
 
+	// make the array is expanded
 	block.expandNodesIfNeeded();
-	UASSERT(block.data != t);
 	UASSERT(std::all_of(block.data, block.data + MapBlock::nodecount, [](MapNode &n) { return n == MapNode(CONTENT_AIR); }));
-	t = block.data;
 
 	// covert to monoblock
 	block.tryShrinkNodes();
 	UASSERT(block.m_is_mono_block);
 	UASSERT(block.data[0].param0 == CONTENT_AIR);
-	UASSERT(block.data != t);
-	t = block.data;
 
 	// get the data(), should deconvert the block
-	MapNode *d1 = block.getData();
+	block.getData();
 	UASSERT(!block.m_is_mono_block);
-	UASSERT(block.data != t);
-	UASSERT(block.data == d1);
 
 	// covert back to mono block
 	block.tryShrinkNodes();
 	UASSERT(block.m_is_mono_block);
-	t = block.data;
 
 	// deconvert explicitly
 	block.expandNodesIfNeeded();
 	UASSERT(!block.m_is_mono_block);
-	UASSERT(block.data != t);
 
 	// covert back to mono block
 	block.tryShrinkNodes();
 	UASSERT(block.m_is_mono_block);
-	t = block.data;
 
 	// set a node, should deconvert the block
 	block.setNode(5,5,5, MapNode(42));
 	UASSERT(!block.m_is_mono_block);
-	UASSERT(block.data != t);
-	t = block.data;
 
 	// cannot covert to mono block
 	block.tryShrinkNodes();
 	UASSERT(!block.m_is_mono_block);
-	UASSERT(block.data == t);
 
 	// set all nodes to 42
 	for (size_t i = 0; i < MapBlock::nodecount; ++i) {
@@ -114,8 +102,6 @@ void TestMapBlock::testMonoblock(IGameDef *gamedef)
 	block.tryShrinkNodes();
 	UASSERT(block.m_is_mono_block);
 	UASSERT(block.data[0].param0 == 42);
-	UASSERT(block.data != t);
-	t = block.data;
 
 	VoxelManipulator vmm;
 	v3s16 data_size(MAP_BLOCKSIZE, MAP_BLOCKSIZE, MAP_BLOCKSIZE);

src/voxel.cpp

@@ -177,7 +177,7 @@ void VoxelManipulator::addArea(const VoxelArea &area)
 	delete[] old_flags;
 }
 
-void VoxelManipulator::copyFrom(MapNode *src, size_t n_nodes, const VoxelArea& src_area,
+void VoxelManipulator::copyFrom(MapNode *src, bool is_mono_block, const VoxelArea& src_area,
 		v3s16 from_pos, v3s16 to_pos, const v3s16 &size)
 {
 	/* The reason for this optimised code is that we're a member function
@@ -216,7 +216,7 @@ void VoxelManipulator::copyFrom(MapNode *src, size_t n_nodes, const VoxelArea& s
 
 	for (s16 z = 0; z < size.Z; z++) {
 		for (s16 y = 0; y < size.Y; y++) {
-			if (n_nodes == 1) {
+			if (is_mono_block) {
 				std::fill_n(m_data + i_local, size.X, src[0]);
 			} else {
 				std::copy_n(src + i_src, size.X, m_data + i_local);

src/voxel.h

@@ -482,7 +482,7 @@ public:
 		Copy data and set flags to 0
 		dst_area.getExtent() <= src_area.getExtent()
 	*/
-	void copyFrom(MapNode *src, size_t n_nodes, const VoxelArea& src_area,
+	void copyFrom(MapNode *src, bool is_mono_block, const VoxelArea& src_area,
 			v3s16 from_pos, v3s16 to_pos, const v3s16 &size);
 
 	// Copy data