--[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.

Author: jturner65

Diff for: 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<double>(), b.get<double>());
+      }
+      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<int>());
     }
     case ConfigValType::Double: {
-      return std::to_string(get<double>());
+      return Cr::Utility::formatString("{}", get<double>());
     }
     case ConfigValType::String: {
       return get<std::string>();
@@ -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<bool>(), allocator);
@@ -487,17 +505,17 @@ 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<core::config::Configuration> subGroupPtr =
             editSubconfig<core::config::Configuration>(key);
         // 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 for: 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<int>(ConfigValType::_nonTrivialTypes);
 }
 
+/**
+ * @brief Function template to return whether the value requires fuzzy
+ * comparison or not.
+ */
+template <typename T>
+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<double>() {
+  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<Mn::Rad>() {
 /**
  * @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 <typename T>
   EnableIf<isConfigValTypePointerBased(configValTypeFor<T>()), 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 <typename T>
   EnableIf<!isConfigValTypePointerBased(configValTypeFor<T>()), 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<T>());
+
   }  // 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<float> Configuration::getSubconfigValsOfTypeInVector(

Diff for: 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 <Corrade/TestSuite/Compare/Numeric.h>
 #include <Corrade/TestSuite/Tester.h>
 #include <Corrade/Utility/FormatStl.h>
 #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<std::string>("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<double>::epsilon() / 2);
+  // Scale the epsilon to be too big to be seen as the same.
+  cfg.set("fuzzyTestVal2", 1.0 + Mn::Math::TypeTraits<double>::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();