Fix AreaStore's IDs persistence (#8888)
authorSmallJoker <SmallJoker@users.noreply.github.com>
Sat, 21 Sep 2019 15:54:52 +0000 (17:54 +0200)
committerGitHub <noreply@github.com>
Sat, 21 Sep 2019 15:54:52 +0000 (17:54 +0200)
Improve documentation
Read old formats
Fix free ID function. Return first gap in map

builtin/game/features.lua
doc/lua_api.txt
src/script/lua_api/l_areastore.cpp
src/unittest/test_areastore.cpp
src/util/areastore.cpp
src/util/areastore.h

index 51d21e86c8a2825fc01115ec14c4c8a74b824efe..0af0dc1dafaf153870e783a487830bafad4fae15 100644 (file)
@@ -14,6 +14,7 @@ core.features = {
        object_independent_selectionbox = true,
        httpfetch_binary_data = true,
        formspec_version_element = true,
+       area_store_persistent_ids = true,
 }
 
 function core.has_feature(arg)
index 04882ad59d73d234b60767418d79314be8ca0e69..8a5ff78e8ffc57386b4fa45d6c0e1566c550d393 100644 (file)
@@ -3804,6 +3804,8 @@ Utilities
           httpfetch_binary_data = true,
           -- Whether formspec_version[<version>] may be used (5.1.0)
           formspec_version_element = true,
+          -- Whether AreaStore's IDs are kept on save/load (5.1.0)
+          area_store_persistent_ids = true,
       }
 
 * `minetest.has_feature(arg)`: returns `boolean, missing_features`
@@ -5197,35 +5199,38 @@ 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)`.
+`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.
+* `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.  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.
+    * 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.
index d53d74aa829f798083f6444456fc7f795422014d..908c766b065ca20a7dda1d354ce39926790c2662 100644 (file)
@@ -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;
 
index a6d4706e44d55bd7cb649bbbd414bbe0c96128ea..691cd69d28a871c7235e0e6d66850924af2ccca3 100644 (file)
@@ -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);
 }
 
index 50d237bbacd4a14ecdf7f7e3ef348f433935591d..cea526336b66b8d5ec1e16e9baf2833c8bbbc2bd 100644 (file)
@@ -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;
index 24840210e4cb57815f0cd59a1dee53efa3f39853..150a043dbf591a089a57890c87ea713f89ba776e 100644 (file)
@@ -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;
 };