From d96f5e1c76a8df4b6983853905d88b78eb95bedd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lars=20M=C3=BCller?= <34514239+appgurueu@users.noreply.github.com> Date: Fri, 2 May 2025 21:27:00 +0200 Subject: [PATCH] MetaDataRef: Make `set_float` preserve numbers exactly (#16090) --- doc/lua_api.md | 4 +-- games/devtest/mods/unittests/metadata.lua | 11 ++++++- src/script/lua_api/l_metadata.cpp | 21 +++++++------ src/unittest/test_utilities.cpp | 37 +++++++++++++++++++++++ src/util/string.cpp | 27 +++++++++++++++++ src/util/string.h | 4 +++ 6 files changed, 92 insertions(+), 12 deletions(-) diff --git a/doc/lua_api.md b/doc/lua_api.md index ab2a4229c..b492f5be5 100644 --- a/doc/lua_api.md +++ b/doc/lua_api.md @@ -8168,8 +8168,8 @@ of the `${k}` syntax in formspecs is not deprecated. The value will be converted into a string when stored. * `get_int(key)`: Returns `0` if key not present. * `set_float(key, value)` - * The range for the value is system-dependent (usually 32 bits). - The value will be converted into a string when stored. + * Store a number (a 64-bit float) exactly. + * The value will be converted into a string when stored. * `get_float(key)`: Returns `0` if key not present. * `get_keys()`: returns a list of all keys in the metadata. * `to_table()`: diff --git a/games/devtest/mods/unittests/metadata.lua b/games/devtest/mods/unittests/metadata.lua index bdd51c3f3..5c518d627 100644 --- a/games/devtest/mods/unittests/metadata.lua +++ b/games/devtest/mods/unittests/metadata.lua @@ -8,6 +8,8 @@ compare_meta:from_table({ c = "3", d = "4", e = "e", + ["0.3"] = "0.29999999999999999", + ["0.1+0.2"] = "0.30000000000000004", }, }) @@ -21,6 +23,9 @@ local function test_metadata(meta) meta:set_string("", "!") meta:set_string("", "") + meta:set_float("0.3", 0.3) + meta:set_float("0.1+0.2", 0.1 + 0.2) + assert(meta:equals(compare_meta)) local tab = meta:to_table() @@ -29,6 +34,8 @@ local function test_metadata(meta) assert(tab.fields.c == "3") assert(tab.fields.d == "4") assert(tab.fields.e == "e") + assert(tab.fields["0.3"] == "0.29999999999999999") + assert(tab.fields["0.1+0.2"] == "0.30000000000000004") local keys = meta:get_keys() assert(table.indexof(keys, "a") > 0) @@ -36,7 +43,7 @@ local function test_metadata(meta) assert(table.indexof(keys, "c") > 0) assert(table.indexof(keys, "d") > 0) assert(table.indexof(keys, "e") > 0) - assert(#keys == 5) + assert(#keys == 7) assert(not meta:contains("")) assert(meta:contains("a")) @@ -55,6 +62,8 @@ local function test_metadata(meta) assert(meta:get_float("a") == 1.0) assert(meta:get_int("e") == 0) assert(meta:get_float("e") == 0.0) + assert(meta:get_float("0.3") == 0.3) + assert(meta:get_float("0.1+0.2") == 0.1 + 0.2) meta:set_float("f", 1.1) meta:set_string("g", "${f}") diff --git a/src/script/lua_api/l_metadata.cpp b/src/script/lua_api/l_metadata.cpp index 1eaf1cfba..53a87c874 100644 --- a/src/script/lua_api/l_metadata.cpp +++ b/src/script/lua_api/l_metadata.cpp @@ -10,6 +10,7 @@ #include "map.h" #include "server.h" #include "util/basic_macros.h" +#include "util/string.h" MetaDataRef *MetaDataRef::checkAnyMetadata(lua_State *L, int narg) { @@ -166,9 +167,9 @@ int MetaDataRef::l_get_float(lua_State *L) std::string str_; const std::string &str = meta->getString(name, &str_); - // Convert with Lua, as is done in set_float. - lua_pushlstring(L, str.data(), str.size()); - lua_pushnumber(L, lua_tonumber(L, -1)); + // TODO this silently produces 0.0 if conversion fails, which is a footgun + f64 number = my_string_to_double(str).value_or(0.0); + lua_pushnumber(L, number); return 1; } @@ -179,12 +180,11 @@ int MetaDataRef::l_set_float(lua_State *L) MetaDataRef *ref = checkAnyMetadata(L, 1); std::string name = luaL_checkstring(L, 2); - luaL_checknumber(L, 3); - // Convert number to string with Lua as it gives good precision. - std::string str = readParam(L, 3); + f64 number = luaL_checknumber(L, 3); IMetadata *meta = ref->getmeta(true); - if (meta != NULL && meta->setString(name, str)) + // Note: Do not use Lua's tostring for the conversion - it rounds. + if (meta != NULL && meta->setString(name, my_double_to_string(number))) ref->reportMetadataChange(&name); return 0; } @@ -289,8 +289,11 @@ bool MetaDataRef::handleFromTable(lua_State *L, int table, IMetadata *meta) while (lua_next(L, fieldstable) != 0) { // key at index -2 and value at index -1 std::string name = readParam(L, -2); - auto value = readParam(L, -1); - meta->setString(name, value); + if (lua_type(L, -1) == LUA_TNUMBER) { + log_deprecated(L, "Passing `fields` with number values " + "is deprecated and may result in loss of precision."); + } + meta->setString(name, readParam(L, -1)); lua_pop(L, 1); // Remove value, keep key for next iteration } lua_pop(L, 1); diff --git a/src/unittest/test_utilities.cpp b/src/unittest/test_utilities.cpp index 7a07c37fe..093dcf1ef 100644 --- a/src/unittest/test_utilities.cpp +++ b/src/unittest/test_utilities.cpp @@ -5,6 +5,7 @@ #include "test.h" #include +#include #include "util/enriched_string.h" #include "util/numeric.h" #include "util/string.h" @@ -48,6 +49,7 @@ public: void testColorizeURL(); void testSanitizeUntrusted(); void testReadSeed(); + void testMyDoubleStringConversions(); }; static TestUtilities g_test_instance; @@ -84,6 +86,7 @@ void TestUtilities::runTests(IGameDef *gamedef) TEST(testColorizeURL); TEST(testSanitizeUntrusted); TEST(testReadSeed); + TEST(testMyDoubleStringConversions); } //////////////////////////////////////////////////////////////////////////////// @@ -763,3 +766,37 @@ void TestUtilities::testReadSeed() // hashing should produce some non-zero number UASSERT(read_seed("hello") != 0); } + +void TestUtilities::testMyDoubleStringConversions() +{ + const auto expect_parse_failure = [](const std::string &s) { + UASSERT(!my_string_to_double(s).has_value()); + }; + expect_parse_failure(""); + expect_parse_failure("helloworld"); + expect_parse_failure("42x"); + + const auto expect_double = [](const std::string &s, double expected) { + auto got = my_string_to_double(s); + UASSERT(got.has_value()); + UASSERTEQ(double, *got, expected); + }; + expect_double("1", 1.0); + expect_double("42", 42.0); + expect_double("42.25", 42.25); + expect_double("3e3", 3000.0); + expect_double("0xff", 255.0); + expect_double("0x1.0p+1", 2.0); + + UASSERT(std::isnan(my_string_to_double(my_double_to_string( + std::numeric_limits::quiet_NaN())).value())); + const auto test_round_trip = [](double number) { + auto got = my_string_to_double(my_double_to_string(number)); + UASSERT(got.has_value()); + UASSERTEQ(double, *got, number); + }; + test_round_trip(std::numeric_limits::infinity()); + test_round_trip(-std::numeric_limits::infinity()); + test_round_trip(0.3); + test_round_trip(0.1 + 0.2); +} diff --git a/src/util/string.cpp b/src/util/string.cpp index 03f9d5cf1..ea0f0c29a 100644 --- a/src/util/string.cpp +++ b/src/util/string.cpp @@ -1065,3 +1065,30 @@ std::optional str_to_v3f(std::string_view str) return value; } + +std::string my_double_to_string(double number) +{ + if (std::isfinite(number)) { + char buf[64]; + snprintf(buf, sizeof(buf), "%.17g", number); + return buf; + } + if (number < 0) + return "-inf"; + if (number > 0) + return "inf"; + return "nan"; +} + +std::optional my_string_to_double(const std::string &s) +{ + if (s.empty()) + return std::nullopt; + char *end = nullptr; + errno = 0; + // Note: this also supports hexadecimal notation like "0x1.0p+1" + double number = std::strtod(s.data(), &end); + if (end != &*s.end()) + return std::nullopt; + return number; +} diff --git a/src/util/string.h b/src/util/string.h index 3dcbfe85f..de9c318a3 100644 --- a/src/util/string.h +++ b/src/util/string.h @@ -465,6 +465,10 @@ inline std::string ftos(float f) return oss.str(); } +/// @brief Converts double to string. Handles high precision and inf/nan. +std::string my_double_to_string(double number); +/// @brief Converts string to double. Handles high precision and inf/nan. +std::optional my_string_to_double(const std::string &s); /** * Replace all occurrences of \p pattern in \p str with \p replacement.