Fix AreaStore's IDs persistence (#8888)

Improve documentation
Read old formats
Fix free ID function. Return first gap in map

4 files changed, 61 insertions, 19 deletions

diff --git a/src/script/lua_api/l_areastore.cpp b/src/script/lua_api/l_areastore.cpp
index d53d74aa8..908c766b0 100644
--- a/src/script/lua_api/l_areastore.cpp
+++ b/src/script/lua_api/l_areastore.cpp
@@ -185,6 +185,7 @@ int LuaAreaStore::l_insert_area(lua_State *L)
 	if (lua_isnumber(L, 5))
 		a.id = lua_tonumber(L, 5);
 
+	// Insert & assign a new ID if necessary
 	if (!ast->insertArea(&a))
 		return 0;
 
diff --git a/src/unittest/test_areastore.cpp b/src/unittest/test_areastore.cpp
index a6d4706e4..691cd69d2 100644
--- a/src/unittest/test_areastore.cpp
+++ b/src/unittest/test_areastore.cpp
@@ -128,11 +128,11 @@ void TestAreaStore::testSerialization()
 	VectorAreaStore store;
 
 	Area a(v3s16(-1, 0, 1), v3s16(0, 1, 2));
-	a.data = "Area A";
+	a.data = "Area AA";
 	store.insertArea(&a);
 
 	Area b(v3s16(123, 456, 789), v3s16(32000, 100, 10));
-	b.data = "Area B";
+	b.data = "Area BB";
 	store.insertArea(&b);
 
 	std::ostringstream os;
@@ -143,20 +143,31 @@ void TestAreaStore::testSerialization()
 			"\x00\x02"  // Count
 			"\xFF\xFF\x00\x00\x00\x01"  // Area A min edge
 			"\x00\x00\x00\x01\x00\x02"  // Area A max edge
-			"\x00\x06"  // Area A data length
-			"Area A"  // Area A data
+			"\x00\x07"  // Area A data length
+			"Area AA"   // Area A data
 			"\x00\x7B\x00\x64\x00\x0A"  // Area B min edge (last two swapped with max edge for sorting)
 			"\x7D\x00\x01\xC8\x03\x15"  // Area B max edge (^)
-			"\x00\x06"  // Area B data length
-			"Area B",  // Area B data
+			"\x00\x07"  // Area B data length
+			"Area BB"   // Area B data
+			"\x00\x00\x00\x00"  // ID A = 0
+			"\x00\x00\x00\x01", // ID B = 1
 			1 + 2 +
-			6 + 6 + 2 + 6 +
-			6 + 6 + 2 + 6);
+			(6 + 6 + 2 + 7) * 2 + // min/max edge, length, data
+			2 * 4); // Area IDs
+
 	UASSERTEQ(const std::string &, str, str_wanted);
 
 	std::istringstream is(str);
 	store.deserialize(is);
 
-	UASSERTEQ(size_t, store.size(), 4);  // deserialize() doesn't clear the store
+	// deserialize() doesn't clear the store
+	// But existing IDs are overridden
+	UASSERTEQ(size_t, store.size(), 2);
+
+	Area c(v3s16(33, -2, -6), v3s16(4, 77, -76));
+	c.data = "Area CC";
+	store.insertArea(&c);
+
+	UASSERTEQ(u32, c.id, 2);
 }
 
diff --git a/src/util/areastore.cpp b/src/util/areastore.cpp
index 50d237bba..cea526336 100644
--- a/src/util/areastore.cpp
+++ b/src/util/areastore.cpp
@@ -64,6 +64,11 @@ const Area *AreaStore::getArea(u32 id) const
 
 void AreaStore::serialize(std::ostream &os) const
 {
+	// WARNING:
+	// Before 5.1.0-dev: version != 0 throws SerializationError
+	// After 5.1.0-dev:  version >= 5 throws SerializationError
+	// Forwards-compatibility is assumed before version 5.
+
 	writeU8(os, 0); // Serialisation version
 
 	// TODO: Compression?
@@ -75,27 +80,41 @@ void AreaStore::serialize(std::ostream &os) const
 		writeU16(os, a.data.size());
 		os.write(a.data.data(), a.data.size());
 	}
+
+	// Serialize IDs
+	for (const auto &it : areas_map)
+		writeU32(os, it.second.id);
 }
 
 void AreaStore::deserialize(std::istream &is)
 {
 	u8 ver = readU8(is);
-	if (ver != 0)
+	// Assume forwards-compatibility before version 5
+	if (ver >= 5)
 		throw SerializationError("Unknown AreaStore "
 				"serialization version!");
 
 	u16 num_areas = readU16(is);
+	std::vector<Area> areas;
 	for (u32 i = 0; i < num_areas; ++i) {
-		Area a;
+		Area a(U32_MAX);
 		a.minedge = readV3S16(is);
 		a.maxedge = readV3S16(is);
 		u16 data_len = readU16(is);
 		char *data = new char[data_len];
 		is.read(data, data_len);
 		a.data = std::string(data, data_len);
-		insertArea(&a);
+		areas.emplace_back(a);
 		delete [] data;
 	}
+
+	bool read_ids = is.good(); // EOF for old formats
+
+	for (auto &area : areas) {
+		if (read_ids)
+			area.id = readU32(is);
+		insertArea(&area);
+	}
 }
 
 void AreaStore::invalidateCache()
@@ -105,6 +124,19 @@ void AreaStore::invalidateCache()
 	}
 }
 
+u32 AreaStore::getNextId() const
+{
+	u32 free_id = 0;
+	for (const auto &area : areas_map) {
+		if (area.first > free_id)
+			return free_id; // Found gap
+
+		free_id = area.first + 1;
+	}
+	// End of map
+	return free_id;
+}
+
 void AreaStore::setCacheParams(bool enabled, u8 block_radius, size_t limit)
 {
 	m_cache_enabled = enabled;
diff --git a/src/util/areastore.h b/src/util/areastore.h
index 24840210e..150a043db 100644
--- a/src/util/areastore.h
+++ b/src/util/areastore.h
@@ -37,15 +37,15 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 
 
 struct Area {
-	Area() = default;
+	Area(u32 area_id) : id(area_id) {}
 
-	Area(const v3s16 &mine, const v3s16 &maxe) :
-		minedge(mine), maxedge(maxe)
+	Area(const v3s16 &mine, const v3s16 &maxe, u32 area_id = U32_MAX) :
+		id(area_id), minedge(mine), maxedge(maxe)
 	{
 		sortBoxVerticies(minedge, maxedge);
 	}
 
-	u32 id = U32_MAX;
+	u32 id;
 	v3s16 minedge, maxedge;
 	std::string data;
 };
@@ -109,7 +109,7 @@ protected:
 	virtual void getAreasForPosImpl(std::vector<Area *> *result, v3s16 pos) = 0;
 
 	/// Returns the next area ID and increments it.
-	u32 getNextId() { return m_next_id++; }
+	u32 getNextId() const;
 
 	// Note: This can't be an unordered_map, since all
 	// references would be invalidated on rehash.
@@ -125,8 +125,6 @@ private:
 	/// If you modify this, call invalidateCache()
 	u8 m_cacheblock_radius = 64;
 	LRUCache<v3s16, std::vector<Area *> > m_res_cache;
-
-	u32 m_next_id = 0;
 };