Add a bit of debug code around MapBlock refcounting

src/client/clientmap.cpp

@@ -202,8 +202,14 @@ void ClientMap::onSettingChanged(std::string_view name, bool all)
 
 ClientMap::~ClientMap()
 {
+	verbosestream << FUNCTION_NAME << std::endl;
+
 	g_settings->deregisterAllChangedCallbacks(this);
 
+	// avoid refcount warning from ~Map()
+	clearDrawList();
+	clearDrawListShadow();
+
 	for (auto &it : m_dynamic_buffers)
 		it.second.drop();
 }
@@ -351,23 +357,29 @@ private:
 	v3s16 volume;
 };
 
-void ClientMap::updateDrawList()
+void ClientMap::clearDrawList()
 {
-	ScopeProfiler sp(g_profiler, "CM::updateDrawList()", SPT_AVG);
-
-	m_needs_update_drawlist = false;
-
 	for (auto &i : m_drawlist) {
 		MapBlock *block = i.second;
 		block->refDrop();
 	}
 	m_drawlist.clear();
 
-	for (auto &block : m_keeplist) {
+	for (auto &block : m_keeplist)
 		block->refDrop();
-	}
 	m_keeplist.clear();
 
+	m_needs_update_drawlist = true;
+}
+
+void ClientMap::updateDrawList()
+{
+	ScopeProfiler sp(g_profiler, "CM::updateDrawList()", SPT_AVG);
+
+	clearDrawList();
+
+	m_needs_update_drawlist = false;
+
 	const v3s16 cam_pos_nodes = floatToInt(m_camera_position, BS);
 
 	v3s16 p_blocks_min;
@@ -1519,6 +1531,15 @@ void ClientMap::renderMapShadows(video::IVideoDriver *driver,
 	g_profiler->avg(prefix + "material swaps [#]", material_swaps);
 }
 
+void ClientMap::clearDrawListShadow()
+{
+	for (auto &i : m_drawlist_shadow) {
+		MapBlock *block = i.second;
+		block->refDrop();
+	}
+	m_drawlist_shadow.clear();
+}
+
 /*
 	Custom update draw list for the pov of shadow light.
 */
@@ -1526,11 +1547,7 @@ void ClientMap::updateDrawListShadow(v3f shadow_light_pos, v3f shadow_light_dir,
 {
 	ScopeProfiler sp(g_profiler, "CM::updateDrawListShadow()", SPT_AVG);
 
-	for (auto &i : m_drawlist_shadow) {
-		MapBlock *block = i.second;
-		block->refDrop();
-	}
-	m_drawlist_shadow.clear();
+	clearDrawListShadow();
 
 	// Number of blocks currently loaded by the client
 	u32 blocks_loaded = 0;

src/client/clientmap.h

@@ -89,12 +89,20 @@ public:
 
 	void getBlocksInViewRange(v3s16 cam_pos_nodes,
 		v3s16 *p_blocks_min, v3s16 *p_blocks_max, float range=-1.0f);
+
 	void updateDrawList();
-	// @brief Calculate statistics about the map and keep the blocks alive
+	/// @brief clears m_drawlist and m_keeplist
+	void clearDrawList();
+
+	/// @brief Calculate statistics about the map and keep the blocks alive
 	void touchMapBlocks();
+
 	void updateDrawListShadow(v3f shadow_light_pos, v3f shadow_light_dir, float radius, float length);
+	void clearDrawListShadow();
+
 	// Returns true if draw list needs updating before drawing the next frame.
 	bool needsUpdateDrawList() { return m_needs_update_drawlist; }
+
 	void renderMap(video::IVideoDriver* driver, s32 pass);
 
 	void renderMapShadows(video::IVideoDriver *driver,

src/map.cpp

@@ -30,12 +30,23 @@ Map::Map(IGameDef *gamedef):
 
 Map::~Map()
 {
-	/*
-		Free all MapSectors
-	*/
+	// Free all sectors
+	size_t used = 0;
 	for (auto &sector : m_sectors) {
+		sector.second->deleteBlocks(&used);
 		delete sector.second;
 	}
+	m_sectors.clear();
+
+	if (used > 0) {
+#ifdef NDEBUG
+		std::ostream &to = infostream;
+#else
+		std::ostream &to = warningstream;
+#endif
+		PrintInfo(to);
+		to << used << " blocks deleted despite reference count > 0. Potential bug." << std::endl;
+	}
 }
 
 void Map::addEventReceiver(MapEventReceiver *event_receiver)

src/mapblock.h

@@ -187,26 +187,28 @@ public:
 	//// Position stuff
 	////
 
+	/// @return map position of block
 	inline v3s16 getPos()
 	{
 		return m_pos;
 	}
 
+	/// @return in-world position of the block (== pos * MAP_BLOCKSIZE)
 	inline v3s16 getPosRelative()
 	{
 		return m_pos_relative;
 	}
 
-	inline core::aabbox3d<s16> getBox() {
+	/// @return in-world box of the block
+	inline core::aabbox3d<s16> getBox()
+	{
 		return getBox(getPosRelative());
 	}
 
-	static inline core::aabbox3d<s16> getBox(const v3s16 &pos_relative)
+	static inline core::aabbox3d<s16> getBox(v3s16 pos_relative)
 	{
 		return core::aabbox3d<s16>(pos_relative,
-				pos_relative
-				+ v3s16(MAP_BLOCKSIZE, MAP_BLOCKSIZE, MAP_BLOCKSIZE)
-				- v3s16(1,1,1));
+				pos_relative + v3s16(MAP_BLOCKSIZE - 1));
 	}
 
 	////
@@ -360,7 +362,7 @@ public:
 	}
 
 	////
-	//// Reference counting (see m_refcount)
+	//// Reference counting (different purposes on client vs. server)
 	////
 
 	inline void refGrab()
@@ -470,10 +472,6 @@ private:
 	 */
 	v3s16 m_pos_relative;
 
-	/*
-		Reference count; currently used for determining if this block is in
-		the list of blocks to be drawn.
-	*/
 	short m_refcount = 0;
 
 	/*

src/mapsector.cpp

@@ -19,12 +19,18 @@ MapSector::~MapSector()
 	deleteBlocks();
 }
 
-void MapSector::deleteBlocks()
+void MapSector::deleteBlocks(size_t *used_count)
 {
-	// Clear cache
 	m_block_cache = nullptr;
 
-	// Delete all blocks
+	size_t u = 0;
+	for (auto &it : m_blocks) {
+		if (it.second->refGet() > 0)
+			u++;
+		it.second.reset();
+	}
+	if (used_count)
+		*used_count += u;
 	m_blocks.clear();
 }
 

src/mapsector.h

@@ -29,7 +29,9 @@ public:
 	MapSector(Map *parent, v2s16 pos, IGameDef *gamedef);
 	virtual ~MapSector();
 
-	void deleteBlocks();
+	/// @brief Deletes all blocks (regardless of reference count).
+	/// @param used_count output: number of blocks which were still ref'd
+	void deleteBlocks(size_t *used_count = nullptr);
 
 	v2s16 getPos() const
 	{
@@ -60,7 +62,8 @@ public:
 
 	bool empty() const { return m_blocks.empty(); }
 
-	int size() const { return m_blocks.size(); }
+	size_t size() const { return m_blocks.size(); }
+
 protected:
 
 	// The pile of MapBlocks