From 2d09449c47e25856b2ca599c7b315f1f8012f8cb Mon Sep 17 00:00:00 2001 From: John Turner <7strbass@gmail.com> Date: Mon, 30 Sep 2024 17:18:56 -0400 Subject: [PATCH] --[Bugfix] - Compare Configuration scalar doubles properly (#2478) * --remove unnecessary magnum decorator * --add and test support for scalar ConfigValues requiring fuzzy compare This uses the same mechanism that Magnum constructs use, so results are consistent * --separate Configuration fuzzy compare tests; minor updates * --make sure comparisons only involve the number of non-hidden values There is a possibility, albeit remote, that 2 otherwise identical configurations might have a different number of internal-use/hidden values and would appear to be different when they were, in fact, the same. Not anymore. Note, these are only accessible internally (in c++ source) and so the likelihood of this happening is vanishingly small. --- src/esp/core/Configuration.cpp | 60 +++++++++++++++--------- src/esp/core/Configuration.h | 83 +++++++++++++++++++++++++++++---- src/tests/ConfigurationTest.cpp | 33 +++++++++++++ 3 files changed, 147 insertions(+), 29 deletions(-) diff --git a/src/esp/core/Configuration.cpp b/src/esp/core/Configuration.cpp index a6d8030d67..d5992aafba 100644 --- a/src/esp/core/Configuration.cpp +++ b/src/esp/core/Configuration.cpp @@ -207,18 +207,35 @@ bool operator==(const ConfigValue& a, const ConfigValue& b) { if (a._typeAndFlags != b._typeAndFlags) { return false; } + const auto dataType = a.getType(); // Pointer-backed data types need to have _data dereffed - if (isConfigValTypePointerBased(a.getType())) { - return pointerBasedConfigTypeHandlerFor(a.getType()) - .comparator(a._data, b._data); + if (isConfigValTypePointerBased(dataType)) { + return pointerBasedConfigTypeHandlerFor(dataType).comparator(a._data, + b._data); + } + + // By here we know the type is a trivial type and that the types for both + // values are equal + if (a.reqsFuzzyCompare()) { + // Type is specified to require fuzzy comparison + switch (dataType) { + case ConfigValType::Double: { + return Mn::Math::equal(a.get(), b.get()); + } + default: { + CORRADE_ASSERT_UNREACHABLE( + "Unknown/unsupported Type in ConfigValue::operator==()", ""); + } + } } - // Trivial type : a._data holds the actual value - // _data array will always hold only legal data, since a ConfigValue should - // never change type. + // Trivial non-fuzzy-comparison-requiring type : a._data holds the actual + // value _data array will always hold only legal data, since a ConfigValue + // should never change type. return std::equal(std::begin(a._data), std::end(a._data), std::begin(b._data)); -} + +} // ConfigValue::operator== bool operator!=(const ConfigValue& a, const ConfigValue& b) { return !(a == b); @@ -236,7 +253,7 @@ std::string ConfigValue::getAsString() const { return std::to_string(get()); } case ConfigValType::Double: { - return std::to_string(get()); + return Cr::Utility::formatString("{}", get()); } case ConfigValType::String: { return get(); @@ -295,7 +312,8 @@ std::string ConfigValue::getAsString() const { io::JsonGenericValue ConfigValue::writeToJsonObject( io::JsonAllocator& allocator) const { - // unknown is checked before this function is called, so does not need support + // unknown is checked before this function is called, so does not need + // support switch (getType()) { case ConfigValType::Boolean: { return io::toJsonValue(get(), allocator); @@ -487,8 +505,8 @@ int Configuration::loadOneConfigFromJson(int numConfigSettings, } else { // The array does not match any currently supported magnum // objects, so place in indexed subconfig of values. - // decrement count by 1 - the recursive subgroup load will count all the - // values. + // decrement count by 1 - the recursive subgroup load will count all + // the values. --numConfigSettings; // create a new subgroup std::shared_ptr subGroupPtr = @@ -496,8 +514,8 @@ int Configuration::loadOneConfigFromJson(int numConfigSettings, // load array into subconfig numConfigSettings += subGroupPtr->loadFromJsonArray(jsonObj); } - // value in array is a number of specified length, else it is a string, an - // object or a nested array + // value in array is a number of specified length, else it is a string, + // an object or a nested array } else { // decrement count by 1 - the recursive subgroup load will count all the // values. @@ -584,8 +602,8 @@ void Configuration::writeValuesToJson(io::JsonGenericValue& jsonObj, << "`, so nothing will be written to JSON for this key."; } else if (valIter->second.shouldWriteToFile()) { - // Create Generic value for key, using allocator, to make sure its a copy - // and lives long enough + // Create Generic value for key, using allocator, to make sure its a + // copy and lives long enough writeValueToJsonInternal(valIter->second, valIter->first.c_str(), jsonObj, allocator); } else { @@ -602,8 +620,8 @@ void Configuration::writeSubconfigsToJson(io::JsonGenericValue& jsonObj, ++cfgIter) { // only save if subconfig tree has value entries if (cfgIter->second->getConfigTreeNumValues() > 0) { - // Create Generic value for key, using allocator, to make sure its a copy - // and lives long enough + // Create Generic value for key, using allocator, to make sure its a + // copy and lives long enough io::JsonGenericValue name{cfgIter->first.c_str(), allocator}; io::JsonGenericValue subObj = cfgIter->second->writeToJsonObject(allocator); @@ -663,9 +681,9 @@ void Configuration::setSubconfigValsOfTypeInVector( /** * @brief Retrieves a shared pointer to a copy of the subConfig @ref * esp::core::config::Configuration that has the passed @p name . This will - * create a pointer to a new sub-configuration if none exists already with that - * name, but will not add this configuration to this Configuration's internal - * storage. + * create a pointer to a new sub-configuration if none exists already with + * that name, but will not add this configuration to this Configuration's + * internal storage. * * @param name The name of the configuration to retrieve. * @return A pointer to a copy of the configuration having the requested @@ -866,7 +884,7 @@ Mn::Debug& operator<<(Mn::Debug& debug, const Configuration& cfg) { bool operator==(const Configuration& a, const Configuration& b) { if ((a.getNumSubconfigs() != b.getNumSubconfigs()) || - (a.getNumValues() != b.getNumValues())) { + (a.getNumVisibleValues() != b.getNumVisibleValues())) { return false; } for (const auto& entry : a.configMap_) { diff --git a/src/esp/core/Configuration.h b/src/esp/core/Configuration.h index 88a8dc4ecb..6844144b62 100644 --- a/src/esp/core/Configuration.h +++ b/src/esp/core/Configuration.h @@ -133,6 +133,13 @@ enum ConfigValStatus : uint64_t { * properly read from or written to file otherwise. */ isTranslated = 1ULL << 34, + + /** + * @brief This @ref ConfigValue requires manual fuzzy comparison (i.e. floating + * point scalar type) using the Magnum::Math::equal method. Magnum types + * already perform fuzzy comparison. + */ + reqsFuzzyComparison = 1ULL << 35, }; // enum class ConfigValStatus /** @@ -157,6 +164,24 @@ constexpr bool isConfigValTypeNonTrivial(ConfigValType type) { static_cast(ConfigValType::_nonTrivialTypes); } +/** + * @brief Function template to return whether the value requires fuzzy + * comparison or not. + */ +template +constexpr bool useFuzzyComparisonFor() { + // Default for all types is no. + return false; +} + +/** + * @brief Specify that @ref ConfigValType::Double scalar floating point values require fuzzy comparison + */ +template <> +constexpr bool useFuzzyComparisonFor() { + return true; +} + /** * @brief Function template to return type enum for specified type. All * supported types should have a specialization of this function handling their @@ -272,8 +297,7 @@ constexpr ConfigValType configValTypeFor() { /** * @brief Stream operator to support display of @ref ConfigValType enum tags */ -MAGNUM_EXPORT Mn::Debug& operator<<(Mn::Debug& debug, - const ConfigValType& value); +Mn::Debug& operator<<(Mn::Debug& debug, const ConfigValType& value); /** * @brief This class uses an anonymous tagged union to store values of different @@ -355,7 +379,8 @@ class ConfigValue { } /** - * @brief Get this ConfigVal's value. Type is stored as a Pointer. + * @brief Get this ConfigVal's value. For Types that are stored in _data as a + * Pointer. */ template EnableIf()), const T&> @@ -367,7 +392,8 @@ class ConfigValue { } /** - * @brief Get this ConfigVal's value. Type is stored directly in buffer. + * @brief Get this ConfigVal's value. For Types that are stored directly in + * buffer. */ template EnableIf()), const T&> @@ -413,6 +439,9 @@ class ConfigValue { //_data should be destructed at this point, construct a new value setInternalTyped(value); + // set whether this type requires fuzzy comparison or not + setReqsFuzzyCompare(useFuzzyComparisonFor()); + } // ConfigValue::setInternal /** @@ -621,6 +650,29 @@ class ConfigValue { setState(ConfigValStatus::isTranslated, isTranslated); } + /** + * @brief Check whether this ConfigVal requires a fuzzy comparison for + * equality (i.e. for a scalar double). + * + * The comparisons for such a type + * should use Magnum::Math::equal to be consistent with similarly configured + * magnum types. + */ + inline bool reqsFuzzyCompare() const { + return getState(ConfigValStatus::reqsFuzzyComparison); + } + /** + * @brief Check whether this ConfigVal requires a fuzzy comparison for + * equality (i.e. for a scalar double). + * + * The comparisons for such a type + * should use Magnum::Math::equal to be consistent with similarly configured + * magnum types. + */ + inline void setReqsFuzzyCompare(bool fuzzyCompare) { + setState(ConfigValStatus::reqsFuzzyComparison, fuzzyCompare); + } + /** * @brief Whether or not this @ref ConfigValue should be written to file during * common execution. The reason we may not want to do this might be that the @@ -657,7 +709,7 @@ class ConfigValue { /** * @brief provide debug stream support for @ref ConfigValue */ -MAGNUM_EXPORT Mn::Debug& operator<<(Mn::Debug& debug, const ConfigValue& value); +Mn::Debug& operator<<(Mn::Debug& debug, const ConfigValue& value); /** * @brief This class holds Configuration data in a map of ConfigValues, and @@ -1175,10 +1227,26 @@ class Configuration { } /** - * @brief returns number of values in this Configuration. + * @brief Returns number of values in this Configuration. */ int getNumValues() const { return valueMap_.size(); } + /** + * @brief Returns number of non-hidden values in this Configuration. This is + * necessary for determining whether or not configurations are "effectively" + * equal, where they contain the same data but may vary in number + * internal-use-only fields. + */ + int getNumVisibleValues() const { + int numVals = 0; + for (const auto& val : valueMap_) { + if (!val.second.isHiddenVal()) { + numVals += 1; + } + } + return numVals; + } + /** * @brief Return total number of values held by this Configuration and all * its subconfigs. @@ -1734,8 +1802,7 @@ class Configuration { /** * @brief provide debug stream support for a @ref Configuration */ -MAGNUM_EXPORT Mn::Debug& operator<<(Mn::Debug& debug, - const Configuration& value); +Mn::Debug& operator<<(Mn::Debug& debug, const Configuration& value); template <> std::vector Configuration::getSubconfigValsOfTypeInVector( diff --git a/src/tests/ConfigurationTest.cpp b/src/tests/ConfigurationTest.cpp index 35b66d7f6b..29ca1ca979 100644 --- a/src/tests/ConfigurationTest.cpp +++ b/src/tests/ConfigurationTest.cpp @@ -2,6 +2,7 @@ // This source code is licensed under the MIT license found in the // LICENSE file in the root directory of this source tree. +#include #include #include #include "esp/core/Configuration.h" @@ -76,6 +77,14 @@ struct ConfigurationTest : Cr::TestSuite::Tester { */ void TestConfiguration(); + /** + * @brief Test Configuration comparisons between value types requiring fuzzy + * logic (i.e. doubles). All additional numeric types added in the future that + * require fuzzy comparison should have their fuzzy equality bounds tested + * here. + */ + void TestConfigurationFuzzyVals(); + /** * @brief Test Configuration find capability. Find returns a list of * subconfiguration keys required to find a particular key @@ -103,6 +112,7 @@ struct ConfigurationTest : Cr::TestSuite::Tester { ConfigurationTest::ConfigurationTest() { addTests({ &ConfigurationTest::TestConfiguration, + &ConfigurationTest::TestConfigurationFuzzyVals, &ConfigurationTest::TestSubconfigFind, &ConfigurationTest::TestSubconfigFindAndMerge, &ConfigurationTest::TestSubconfigFilter, @@ -269,6 +279,29 @@ void ConfigurationTest::TestConfiguration() { CORRADE_COMPARE(cfg.get("myString"), "test"); } // ConfigurationTest::TestConfiguration test +void ConfigurationTest::TestConfigurationFuzzyVals() { + Configuration cfg; + + // Specify values to test + cfg.set("fuzzyTestVal0", 1.0); + cfg.set("fuzzyTestVal1", 1.0 + Mn::Math::TypeTraits::epsilon() / 2); + // Scale the epsilon to be too big to be seen as the same. + cfg.set("fuzzyTestVal2", 1.0 + Mn::Math::TypeTraits::epsilon() * 4); + + CORRADE_VERIFY(cfg.hasValue("fuzzyTestVal0")); + CORRADE_VERIFY(cfg.hasValue("fuzzyTestVal1")); + CORRADE_VERIFY(cfg.hasValue("fuzzyTestVal2")); + // Verify very close doubles are considered sufficiently close by fuzzy + // compare + CORRADE_COMPARE(cfg.get("fuzzyTestVal0"), cfg.get("fuzzyTestVal1")); + + // verify very close but not-quite-close enough doubles are considered + // different by magnum's fuzzy compare + CORRADE_COMPARE_AS(cfg.get("fuzzyTestVal0"), cfg.get("fuzzyTestVal2"), + Cr::TestSuite::Compare::NotEqual); + +} // ConfigurationTest::TestConfigurationFuzzyVals + // Test configuration find functionality void ConfigurationTest::TestSubconfigFind() { Configuration::ptr cfg = Configuration::create();