Make AreaStore IDs persistent (#8888)

Port of Old MT commit fec30e37ac
From: SmallJoker <SmallJoker@users.noreply.github.com>
Date: Sat, 21 Sep 2019 17:54:52 +0200

builtin/game/features.lua

@@ -11,6 +11,7 @@ core.features = {
     area_store_custom_ids = true,
     add_entity_with_staticdata = true,
     no_chat_message_prediction = true,
+    area_store_persistent_ids = true,
 }
 
 function core.has_feature(arg)

doc/lua_api.txt

@@ -2450,6 +2450,8 @@ Strings that need to be translated can contain several escapes, preceded by `@`.
         -- ^ add_entity supports passing initial staticdata to on_activate
            no_chat_message_prediction = true,
         -- ^ Chat messages are no longer predicted
+           area_store_persistent_ids = true,
+        -- ^ Whether AreaStore IDs are kept on save/load (5.1.0)
         }
 * `minetest.has_feature(arg)`: returns `boolean, missing_features`
     * `arg`: string or table in format `{foo=true, bar=true}`
@@ -3811,45 +3813,52 @@ An `InvRef` is a reference to an inventory.
     * returns `{type="undefined"}` in case location is not known
 
 ### `AreaStore`
-A fast access data structure to store areas, and find areas near a given position or area.
+
+A fast access data structure to store areas, and find areas near a given
+position or area.
 Every area has a `data` string attribute to store additional information.
-You can create an empty `AreaStore` by calling `AreaStore()`, or `AreaStore(type_name)`.
-If you chose the parameter-less constructor, a fast implementation will be automatically
-chosen for you.
+You can create an empty `AreaStore` by calling `AreaStore()`, or
+`AreaStore(type_name)`. The mod decides where to save and load AreaStore.
+If you chose the parameter-less constructor, a fast implementation will be
+automatically chosen for you.
 
 #### Methods
-* `get_area(id, include_borders, include_data)`: returns the area with the id `id`.
-  (optional) Boolean values `include_borders` and `include_data` control what's copied.
-  Returns nil if specified area id does not exist.
-* `get_areas_for_pos(pos, include_borders, include_data)`: returns all areas that contain
-  the position `pos`. (optional) Boolean values `include_borders` and `include_data` control
-  what's copied.
-* `get_areas_in_area(edge1, edge2, accept_overlap, include_borders, include_data)`:
-  returns all areas that contain all nodes inside the area specified by `edge1` and `edge2` (inclusive).
-  If `accept_overlap` is true, also areas are returned that have nodes in common with the specified area.
-  (optional) Boolean values `include_borders` and `include_data` control what's copied.
-* `insert_area(edge1, edge2, data, [id])`: inserts an area into the store. Returns the new area's ID,
-  or nil if the insertion failed. The (inclusive) positions `edge1` and `edge2` describe the area.
-  `data` is a string stored with the area.  If passed, `id` will be used as the internal area ID,
-  it must be a unique number between 0 and 2^32-2. If you use the `id` parameter you must always use it,
-  or insertions are likely to fail due to conflicts.
-* `reserve(count)`: reserves resources for at most `count` many contained areas.
+
+* `get_area(id, include_borders, include_data)`
+    * Returns the area information about the specified ID.
+    * Returned values are either of these:
+
+            nil  -- Area not found
+            true -- Without `include_borders` and `include_data`
+            {
+                min = pos, max = pos -- `include_borders == true`
+                data = string        -- `include_data == true`
+            }
+
+* `get_areas_for_pos(pos, include_borders, include_data)`
+    * Returns all areas as table, indexed by the area ID.
+    * Table values: see `get_area`.
+
+* `get_areas_in_area(edge1, edge2, accept_overlap, include_borders, include_data)`
+    * Returns all areas that contain all nodes inside the area specified by `edge1`
+      and `edge2` (inclusive).
+    * `accept_overlap`: if `true`, areas are returned that have nodes in
+      common (intersect) with the specified area.
+    * Returns the same values as `get_areas_for_pos`.
+
+* `insert_area(edge1, edge2, data, [id])`: inserts an area into the store.
+    * Returns the new area's ID, or nil if the insertion failed.
+    * The (inclusive) positions `edge1` and `edge2` describe the area.
+    * `data` is a string stored with the area.
+    * `id` (optional): will be used as the internal area ID if it is an unique
+      number between 0 and 2^32-2.
+
+* `reserve(count)`: reserves resources for at most `count` many contained
+  areas.
   Only needed for efficiency, and only some implementations profit.
-* `remove_area(id)`: removes the area with the given id from the store, returns success.
-* `set_cache_params(params)`: sets params for the included prefiltering cache.
-  Calling invalidates the cache, so that its elements have to be newly generated.
-    * `params`:
-      {
-        enabled = boolean, -- whether to enable, default true
-        block_radius = number, -- the radius (in nodes) of the areas the cache generates
-                                  prefiltered lists for, minimum 16, default 64
-        limit = number, -- the cache's size, minimum 20, default 1000
-      }
-* `to_string()`: Experimental. Returns area store serialized as a (binary) string.
-* `to_file(filename)`: Experimental. Like `to_string()`, but writes the data to a file.
-* `from_string(str)`: Experimental. Deserializes string and loads it into the AreaStore.
-  Returns success and, optionally, an error message.
-* `from_file(filename)`: Experimental. Like `from_string()`, but reads the data from a file.
+
+* `remove_area(id)`: removes the area with the given id from the store, returns
+  success.
 
 ### `ItemStack`
 An `ItemStack` is a stack of items.

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;
 

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,19 +143,30 @@ 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);
 }

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?
@@ -76,27 +81,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()
@@ -106,6 +125,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;

src/util/areastore.h

@@ -36,22 +36,23 @@ with this program; if not, write to the Free Software Foundation, Inc.,
     #include "util/serialize.h"
 #endif
 
+struct Area
+{
+    Area(u32 area_id) : id(area_id) {}
 
-struct Area {
-    Area() {}
-    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;
 };
 
-
-class AreaStore {
+class AreaStore
+{
 public:
     AreaStore() :
         m_res_cache(1000, &cacheMiss, this)
@@ -109,7 +110,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 +126,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;
 };