Refactor MMVManip to get rid of m_loaded_blocks

src/map.cpp

@@ -772,6 +772,11 @@ void MMVManip::initialEmerge(v3s16 p_min, v3s16 p_max, bool load_if_inexistent)
 		infostream<<std::endl;
 	}
 
+	std::map<v3s16, bool> had_blocks;
+	// we can skip this calculation if the areas are disjoint
+	if (!m_area.intersect(block_area_nodes).hasEmptyExtent())
+		had_blocks = getCoveredBlocks();
+
 	const bool all_new = m_area.hasEmptyExtent();
 	addArea(block_area_nodes);
 
@@ -779,12 +784,12 @@ void MMVManip::initialEmerge(v3s16 p_min, v3s16 p_max, bool load_if_inexistent)
 	for(s32 y=p_min.Y; y<=p_max.Y; y++)
 	for(s32 x=p_min.X; x<=p_max.X; x++)
 	{
-		u8 flags = 0;
-		MapBlock *block;
 		v3s16 p(x,y,z);
-		if (m_loaded_blocks.count(p) > 0)
+		// if this block was already in the vmanip and it has data, skip
+		if (auto it = had_blocks.find(p); it != had_blocks.end() && it->second)
 			continue;
 
+		MapBlock *block;
 		bool block_data_inexistent = false;
 		{
 			TimeTaker timer2("emerge load", &emerge_load_time);
@@ -803,21 +808,53 @@ void MMVManip::initialEmerge(v3s16 p_min, v3s16 p_max, bool load_if_inexistent)
 				assert(block);
 				block->copyTo(*this);
 			} else {
-				flags |= VMANIP_BLOCK_DATA_INEXIST;
-
 				// Mark area inexistent
 				VoxelArea a(p*MAP_BLOCKSIZE, (p+1)*MAP_BLOCKSIZE-v3s16(1,1,1));
 				setFlags(a, VOXELFLAG_NO_DATA);
 			}
 		}
-
-		m_loaded_blocks[p] = flags;
 	}
 
 	if (all_new)
 		m_is_dirty = false;
 }
 
+std::map<v3s16, bool> MMVManip::getCoveredBlocks() const
+{
+	std::map<v3s16, bool> ret;
+	if (m_area.hasEmptyExtent())
+		return ret;
+
+	// Figure out if *any* node in this block has data according to m_flags
+	const auto &check_block = [this] (v3s16 bp) -> bool {
+		v3s16 pmin = bp * MAP_BLOCKSIZE;
+		v3s16 pmax = pmin + v3s16(MAP_BLOCKSIZE-1);
+		for(s16 z=pmin.Z; z<=pmax.Z; z++)
+		for(s16 y=pmin.Y; y<=pmax.Y; y++)
+		for(s16 x=pmin.X; x<=pmax.X; x++) {
+			if (!(m_flags[m_area.index(x,y,z)] & VOXELFLAG_NO_DATA))
+				return true;
+		}
+		return false;
+	};
+
+	v3s16 bpmin = getNodeBlockPos(m_area.MinEdge);
+	v3s16 bpmax = getNodeBlockPos(m_area.MaxEdge);
+
+	if (bpmin * MAP_BLOCKSIZE != m_area.MinEdge)
+		throw BaseException("MMVManip not block-aligned");
+	if ((bpmax+1) * MAP_BLOCKSIZE - v3s16(1) != m_area.MaxEdge)
+		throw BaseException("MMVManip not block-aligned");
+
+	for(s16 z=bpmin.Z; z<=bpmax.Z; z++)
+	for(s16 y=bpmin.Y; y<=bpmax.Y; y++)
+	for(s16 x=bpmin.X; x<=bpmax.X; x++) {
+		v3s16 bp(x,y,z);
+		ret[bp] = check_block(bp);
+	}
+	return ret;
+}
+
 void MMVManip::blitBackAll(std::map<v3s16, MapBlock*> *modified_blocks,
 	bool overwrite_generated) const
 {
@@ -825,16 +862,14 @@ void MMVManip::blitBackAll(std::map<v3s16, MapBlock*> *modified_blocks,
 		return;
 	assert(m_map);
 
-	/*
-		Copy data of all blocks
-	*/
-	assert(!m_loaded_blocks.empty());
-	for (auto &loaded_block : m_loaded_blocks) {
-		v3s16 p = loaded_block.first;
+	// Copy all the blocks with data back to the map
+	const auto loaded_blocks = getCoveredBlocks();
+	for (auto &it : loaded_blocks) {
+		if (!it.second)
+			continue;
+		v3s16 p = it.first;
 		MapBlock *block = m_map->getBlockNoCreateNoEx(p);
-		bool existed = !(loaded_block.second & VMANIP_BLOCK_DATA_INEXIST);
-		if (!existed || (block == NULL) ||
-			(!overwrite_generated && block->isGenerated()))
+		if (!block || (!overwrite_generated && block->isGenerated()))
 			continue;
 
 		block->copyFrom(*this);
@@ -860,11 +895,7 @@ MMVManip *MMVManip::clone() const
 		ret->m_flags = new u8[size];
 		memcpy(ret->m_flags, m_flags, size * sizeof(u8));
 	}
-
 	ret->m_is_dirty = m_is_dirty;
-	// Even if the copy is disconnected from a map object keep the information
-	// needed to write it back to one
-	ret->m_loaded_blocks = m_loaded_blocks;
 
 	return ret;
 }

src/map.h

@@ -307,16 +307,29 @@ public:
 	MMVManip(Map *map);
 	virtual ~MMVManip() = default;
 
-	virtual void clear()
-	{
-		VoxelManipulator::clear();
-		m_loaded_blocks.clear();
-	}
-
+	/*
+		Loads specified area from map and *adds* it to the area already
+		contained in the VManip.
+	*/
 	void initialEmerge(v3s16 blockpos_min, v3s16 blockpos_max,
 		bool load_if_inexistent = true);
 
-	// This is much faster with big chunks of generated data
+	/**
+		Uses the flags array to determine which blocks the VManip covers,
+		and for which of them we have any data.
+		@warning requires VManip area to be block-aligned
+		@return map of blockpos -> any data?
+	*/
+	std::map<v3s16, bool> getCoveredBlocks() const;
+
+	/**
+		Writes data in VManip back to the map. Blocks without any data in the VManip
+		are skipped.
+		@note VOXELFLAG_NO_DATA is checked per-block, not per-node. So you need
+		to ensure that the relevant parts of m_data are initialized.
+		@param modified_blocks output array of touched blocks (optional)
+		@param overwrite_generated if false, blocks marked as generate in the map are not changed
+	*/
 	void blitBackAll(std::map<v3s16, MapBlock*> * modified_blocks,
 		bool overwrite_generated = true) const;
 
@@ -339,13 +352,4 @@ protected:
 
 	// may be null
 	Map *m_map = nullptr;
-	/*
-		key = blockpos
-		value = flags describing the block
-	*/
-	std::map<v3s16, u8> m_loaded_blocks;
-
-	enum : u8 {
-		VMANIP_BLOCK_DATA_INEXIST = 1 << 0,
-	};
 };

src/unittest/test_voxelarea.cpp

@@ -264,6 +264,10 @@ void TestVoxelArea::test_intersect()
 	UASSERT(v3.intersect(v1) == v1.intersect(v3));
 	UASSERT(v1.intersect(v4) ==
 			VoxelArea({-10, -2, -10}, {10, 2, 10}));
+
+	// edge cases
+	UASSERT(VoxelArea().intersect(v1).hasEmptyExtent());
+	UASSERT(v1.intersect(VoxelArea()).hasEmptyExtent());
 }
 
 void TestVoxelArea::test_index_xyz_all_pos()

src/unittest/test_voxelmanipulator.cpp

@@ -22,6 +22,7 @@ public:
 	void testBasic(const NodeDefManager *nodedef);
 	void testEmerge(IGameDef *gamedef);
 	void testBlitBack(IGameDef *gamedef);
+	void testBlitBack2(IGameDef *gamedef);
 };
 
 static TestVoxelManipulator g_test_instance;
@@ -31,6 +32,7 @@ void TestVoxelManipulator::runTests(IGameDef *gamedef)
 	TEST(testBasic, gamedef->ndef());
 	TEST(testEmerge, gamedef);
 	TEST(testBlitBack, gamedef);
+	TEST(testBlitBack2, gamedef);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -90,7 +92,7 @@ void TestVoxelManipulator::testEmerge(IGameDef *gamedef)
 	UASSERTEQ(auto, vm.getNodeNoExNoEmerge({0,bs+1,0}).getContent(), t_CONTENT_BRICK);
 
 	// emerge out of bounds: should produce empty data
-	vm.initialEmerge({0,0,0}, {0,2,0}, false);
+	vm.initialEmerge({0,2,0}, {0,2,0}, false);
 	UASSERTEQ(auto, vm.m_area.getExtent(), v3s32(bs,3*bs,bs));
 
 	UASSERTEQ(auto, vm.getNodeNoExNoEmerge({0,2*bs,0}).getContent(), CONTENT_IGNORE);
@@ -133,3 +135,47 @@ void TestVoxelManipulator::testBlitBack(IGameDef *gamedef)
 	// ignore nodes are not written (is this an intentional feature?)
 	UASSERTEQ(auto, map.getNode({2,2,2}).getContent(), CONTENT_AIR);
 }
+
+void TestVoxelManipulator::testBlitBack2(IGameDef *gamedef)
+{
+	constexpr int bs = MAP_BLOCKSIZE;
+
+	DummyMap map(gamedef, {0,0,0}, {1,1,1});
+	map.fill({0,0,0}, {1,1,1}, CONTENT_AIR);
+
+	// Create a vmanip "manually" without using initialEmerge
+	MMVManip vm(&map);
+	vm.addArea(VoxelArea({0,0,0}, v3s16(1,2,1) * bs - v3s16(1)));
+
+	// Lower block is initialized with ignore, upper with lava
+	for(s16 z=0; z<bs; z++)
+	for(s16 y=0; y<2*bs; y++)
+	for(s16 x=0; x<bs; x++) {
+		auto c = y >= bs ? t_CONTENT_LAVA : CONTENT_IGNORE;
+		vm.setNodeNoEmerge({x,y,z}, c);
+	}
+	// But pretend the upper block was not actually initialized
+	vm.setFlags(VoxelArea({0,bs,0}, v3s16(1,2,1) * bs - v3s16(1)), VOXELFLAG_NO_DATA);
+	// Add a node to the lower one
+	vm.setNodeNoEmerge({0,1,0}, t_CONTENT_TORCH);
+
+	// Verify covered blocks
+	{
+		auto cov = vm.getCoveredBlocks();
+		UASSERTEQ(size_t, cov.size(), 2);
+		auto it = cov.find({0,0,0});
+		UASSERT(it != cov.end() && it->second);
+		it = cov.find({0,1,0});
+		UASSERT(it != cov.end() && !it->second);
+	}
+
+	// Now blit it back
+	std::map<v3s16, MapBlock*> modified;
+	vm.blitBackAll(&modified);
+	// The lower block data should have been written
+	UASSERTEQ(size_t, modified.size(), 1);
+	UASSERTEQ(auto, modified.begin()->first, v3s16(0,0,0));
+	UASSERTEQ(auto, map.getNode({0,1,0}).getContent(), t_CONTENT_TORCH);
+	// The upper one should not!
+	UASSERTEQ(auto, map.getNode({0,bs,0}).getContent(), CONTENT_AIR);
+}

src/voxel.h

@@ -469,7 +469,7 @@ public:
 		Control
 	*/
 
-	virtual void clear();
+	void clear();
 
 	void print(std::ostream &o, const NodeDefManager *nodemgr,
 			VoxelPrintMode mode=VOXELPRINT_MATERIAL) const;