From fec30e37ac1d160a942777b05a7717b5395c4d99 Mon Sep 17 00:00:00 2001 From: SmallJoker Date: Sat, 21 Sep 2019 17:54:52 +0200 Subject: [PATCH] Fix AreaStore's IDs persistence (#8888) Improve documentation Read old formats Fix free ID function. Return first gap in map --- builtin/game/features.lua | 1 + doc/lua_api.txt | 51 ++++++++++++++++-------------- src/script/lua_api/l_areastore.cpp | 1 + src/unittest/test_areastore.cpp | 29 +++++++++++------ src/util/areastore.cpp | 38 ++++++++++++++++++++-- src/util/areastore.h | 12 +++---- 6 files changed, 90 insertions(+), 42 deletions(-) diff --git a/builtin/game/features.lua b/builtin/game/features.lua index 51d21e86c..0af0dc1da 100644 --- a/builtin/game/features.lua +++ b/builtin/game/features.lua @@ -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) diff --git a/doc/lua_api.txt b/doc/lua_api.txt index 04882ad59..8a5ff78e8 100644 --- a/doc/lua_api.txt +++ b/doc/lua_api.txt @@ -3804,6 +3804,8 @@ Utilities httpfetch_binary_data = true, -- Whether formspec_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. 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 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 *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 > m_res_cache; - - u32 m_next_id = 0; };