From 97c9ff2b0a0636dc4231df37acc138887f712277 Mon Sep 17 00:00:00 2001 From: Bernard Kwok Date: Wed, 4 Sep 2024 15:01:45 -0400 Subject: [PATCH 1/6] Working version of isEquivalent(). --- source/MaterialXCore/Element.cpp | 156 ++++++++++++++++++ source/MaterialXCore/Element.h | 57 +++++++ .../MaterialXTest/MaterialXCore/Document.cpp | 130 +++++++++++++++ 3 files changed, 343 insertions(+) diff --git a/source/MaterialXCore/Element.cpp b/source/MaterialXCore/Element.cpp index 94836245a7..48ec9dfe6d 100644 --- a/source/MaterialXCore/Element.cpp +++ b/source/MaterialXCore/Element.cpp @@ -81,6 +81,69 @@ bool Element::operator!=(const Element& rhs) const return !(*this == rhs); } +bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options) const +{ + if (getCategory() != rhs->getCategory() || + getName() != rhs->getName()) + { + options.status = "Name or category does not match for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + + // Compare attribute names. + StringVec attributeNames = getAttributeNames(); + StringVec rhsAttributeNames = rhs->getAttributeNames(); + + // Filter out any attributes specified in the options. + const StringSet& skipAttributes = options.skipAttributes; + if (!skipAttributes.empty()) + { + attributeNames.erase(std::remove_if(attributeNames.begin(), attributeNames.end(), + [&skipAttributes](const string& attr) { return skipAttributes.find(attr) != skipAttributes.end(); }), + attributeNames.end()); + rhsAttributeNames.erase(std::remove_if(rhsAttributeNames.begin(), rhsAttributeNames.end(), + [&skipAttributes](const string& attr) { return skipAttributes.find(attr) != skipAttributes.end(); }), + rhsAttributeNames.end()); + } + + // Ignore ordering if option specified. + if (options.ignoreAttributeOrder) + { + // Sort the string arrays + std::sort(attributeNames.begin(), attributeNames.end()); + std::sort(rhsAttributeNames.begin(), rhsAttributeNames.end()); + } + if (attributeNames != rhsAttributeNames) + { + options.status = "Attribute names differ for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + + for (const string& attr : rhsAttributeNames) + { + if (getAttribute(attr) != rhs->getAttribute(attr)) + { + options.status = "Attribute " + attr + " differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + } + + // Compare children. + const vector& c1 = getChildren(); + const vector& c2 = rhs->getChildren(); + if (c1.size() != c2.size()) + { + options.status = "Child count differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + for (size_t i = 0; i < c1.size(); i++) + { + if (!c1[i]->isEquivalent(c2[i], options)) + return false; + } + return true; +} + void Element::setName(const string& name) { ElementPtr parent = getParent(); @@ -482,6 +545,99 @@ TypeDefPtr TypedElement::getTypeDef() const // ValueElement methods // +bool ValueElement::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options) const +{ + if (getCategory() != rhs->getCategory() || + getName() != rhs->getName()) + { + options.status = "Name or category does not match for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + + // Compare attribute names. + StringVec attributeNames = getAttributeNames(); + StringVec rhsAttributeNames = rhs->getAttributeNames(); + + // Filter out any attributes specified in the options. + const StringSet& skipAttributes = options.skipAttributes; + if (!skipAttributes.empty()) + { + attributeNames.erase(std::remove_if(attributeNames.begin(), attributeNames.end(), + [&skipAttributes](const string& attr) { return skipAttributes.find(attr) != skipAttributes.end(); }), + attributeNames.end()); + rhsAttributeNames.erase(std::remove_if(rhsAttributeNames.begin(), rhsAttributeNames.end(), + [&skipAttributes](const string& attr) { return skipAttributes.find(attr) != skipAttributes.end(); }), + rhsAttributeNames.end()); + } + + // Ignore ordering if option specified. + if (options.ignoreAttributeOrder) + { + // Sort the string arrays + std::sort(attributeNames.begin(), attributeNames.end()); + std::sort(rhsAttributeNames.begin(), rhsAttributeNames.end()); + } + if (attributeNames != rhsAttributeNames) + { + options.status = "Attribute names differ for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + + const StringSet uiAttributes = { + ValueElement::UI_MIN_ATTRIBUTE, ValueElement::UI_MAX_ATTRIBUTE, + ValueElement::UI_SOFT_MIN_ATTRIBUTE, ValueElement::UI_SOFT_MAX_ATTRIBUTE, + ValueElement::UI_STEP_ATTRIBUTE + }; + + // Get precision and format options + ScopedFloatFormatting fmt(options.format, options.precision); + + ConstValueElementPtr rhsValueElement = rhs->asA(); + for (const string& attr : rhsAttributeNames) + { + // Check value equality + if (attr == ValueElement::VALUE_ATTRIBUTE) + { + ValuePtr thisValue = getValue(); + ValuePtr rhsValue = rhsValueElement->getValue(); + if (thisValue && rhsValue) + { + if (thisValue->getValueString() != rhsValue->getValueString()) + { + options.status = "Attribute 'value' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + } + } + + // Check ui attribute value equality + else if (uiAttributes.find(attr) != uiAttributes.end()) + { + const string& uiAttribute = getAttribute(attr); + const string& rhsUiAttribute = getAttribute(attr); + ValuePtr uiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(uiAttribute, getType()) : nullptr; + ValuePtr rhsUiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(rhsUiAttribute, getType()) : nullptr; + if (uiValue && rhsUiValue) + { + if (uiValue->getValueString() != rhsUiValue->getValueString()) + { + options.status = "Attribute '" + attr + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + } + } + + // Perform string comparison + else if (getAttribute(attr) != rhsValueElement->getAttribute(attr)) + { + options.status = "Attribute '" + attr + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } + } + + return true; +} + string ValueElement::getResolvedValueString(StringResolverPtr resolver) const { if (!StringResolver::isResolvedType(getType())) diff --git a/source/MaterialXCore/Element.h b/source/MaterialXCore/Element.h index d1abcdfdca..d22f42863a 100644 --- a/source/MaterialXCore/Element.h +++ b/source/MaterialXCore/Element.h @@ -71,6 +71,46 @@ using ElementMap = std::unordered_map; /// A standard function taking an ElementPtr and returning a boolean. using ElementPredicate = std::function; +/// @class ElemenEquivalenceOptions +/// A set of options for controlling for equivalence comparison. +class MX_CORE_API ElementEquivalenceOptions +{ + public: + ElementEquivalenceOptions() + { + format = Value::getFloatFormat(); + precision = Value::getFloatPrecision(); + skipAttributes = {}; + ignoreAttributeOrder = true; + status = EMPTY_STRING; + }; + ~ElementEquivalenceOptions() { } + + /// Floating point format option for floating point value comparisons + Value::FloatFormat format; + + /// Floating point precision option for floating point value comparisons + int precision; + + /// Attribute filtering options. By default all attributes are considered. + /// Name, category attributes cannot be skipped. + /// + /// For example UI attribute comparision be skipped by setting: + /// skipAttributes = { + /// ValueElement::UI_MIN_ATTRIBUTE, ValueElement::UI_MAX_ATTRIBUTE, + /// ValueElement::UI_SOFT_MIN_ATTRIBUTE, ValueElement::UI_SOFT_MAX_ATTRIBUTE, + /// ValueElement::UI_STEP_ATTRIBUTE, Element::XPOS_ATTRIBUTE, + /// Element::YPOS_ATTRIBUTE }; + StringSet skipAttributes; + + /// Option to indicate whether to ignore the order that attributes + /// are specified on an element. Default is to ignore order. + bool ignoreAttributeOrder; + + /// Status string for first difference found. + string status; +}; + /// @class Element /// The base class for MaterialX elements. /// @@ -99,6 +139,9 @@ class MX_CORE_API Element : public std::enable_shared_from_this template friend class ElementRegistry; public: + /// @name Comparison interfaces + /// @{ + /// Return true if the given element tree, including all descendants, /// is identical to this one. bool operator==(const Element& rhs) const; @@ -107,6 +150,12 @@ class MX_CORE_API Element : public std::enable_shared_from_this /// differs from this one. bool operator!=(const Element& rhs) const; + /// Return tue if the given element treee, including all descendents, + /// is considered to be equivalent to this one based on the equivalence + /// criteria provided. + virtual bool isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options) const; + + /// @} /// @name Category /// @{ @@ -925,6 +974,14 @@ class MX_CORE_API ValueElement : public TypedElement public: virtual ~ValueElement() { } + /// @name Comparison interfaces + /// @{ + + /// Return tue if the given value element is considered to be equivalent to this + /// one based on the equivalence criteria provided. + bool isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options) const override; + + /// @} /// @name Value String /// @{ diff --git a/source/MaterialXTest/MaterialXCore/Document.cpp b/source/MaterialXTest/MaterialXCore/Document.cpp index fb35e58ccb..479c5cad21 100644 --- a/source/MaterialXTest/MaterialXCore/Document.cpp +++ b/source/MaterialXTest/MaterialXCore/Document.cpp @@ -10,6 +10,9 @@ #include #include +#include +#include + namespace mx = MaterialX; TEST_CASE("Document", "[document]") @@ -116,3 +119,130 @@ TEST_CASE("Document", "[document]") // Validate the combined document. REQUIRE(doc->validate()); } + +TEST_CASE("Document equivalence", "[document]") +{ + mx::DocumentPtr doc = mx::createDocument(); + std::multimap inputMap; + + inputMap.insert({ "color3", " 1.0, +2.0, 3.0 " }); + inputMap.insert({ "color4", "1.0, 2.00, 0.3000, -4" }); + inputMap.insert({ "float", " 1.2e-10 " }); + inputMap.insert({ "float", " 00.1000 " }); + inputMap.insert({ "integer", " 12 " }); + inputMap.insert({ "matrix33", + "01.0, 2.0, 0000.2310, " + " 01.0, 2.0, 0000.2310, " + "01.0, 2.0, 0000.2310 " }); + inputMap.insert({ "matrix44", + "01.0, 2.0, 0000.2310, 0.100, " + "01.0, 2.0, 0000.2310, 0.100, " + "01.0, 2.0, 0000.2310, 0.100, " + "01.0, 2.0, 0000.2310, 0.100" }); + inputMap.insert({ "vector2", "1.0, 0.012345608" }); // For precision check + inputMap.insert({ "vector3", " 1.0, +2.0, 3.0 " }); + inputMap.insert({ "vector4", "1.0, 2.00, 0.3000, -4" }); + inputMap.insert({ "string", "mystring" }); + inputMap.insert({ "boolean", "false" }); + inputMap.insert({ "filename", "filename1" }); + + unsigned int index = 0; + for (auto it = inputMap.begin(); it != inputMap.end(); ++it) + { + const std::string inputType = (*it).first; + mx::InputPtr input = doc->addInput("input" + std::to_string(index), inputType); + if (inputType == "float") + { + input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.0100 "); + input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 01.0100 "); + } + input->setValueString((*it).second); + index++; + } + + mx::DocumentPtr doc2 = mx::createDocument(); + std::multimap inputMap2; + inputMap2.insert({ "color3", "1, 2, 3" }); + inputMap2.insert({ "color4", "1, 2, 0.3, -4" }); + inputMap2.insert({ "float", "1.2e-10" }); + inputMap2.insert({ "float", "0.1" }); + inputMap2.insert({ "integer", "12" }); + inputMap2.insert({ "matrix33", "1, 2, 0.231, 1, 2, 0.231, 1, 2, 0.231, 1, 2, 0.231" }); + inputMap2.insert({ "matrix44", "1, 2, 0.231, 0.1, 1, 2, 0.231, 0.1, 1, 2, 0.231, 0.1, 1, 2, 0.231, 0.1" }); + inputMap2.insert({ "vector2", "1, 0.012345611" }); // For precision check + inputMap2.insert({ "vector3", "1, 2, 3" }); + inputMap2.insert({ "vector4", "1, 2, 0.3, -4" }); + inputMap2.insert({ "string", "mystring" }); + inputMap2.insert({ "boolean", "false" }); + inputMap2.insert({ "filename", "filename1" }); + + index = 0; + for (auto it = inputMap2.begin(); it != inputMap2.end(); ++it) + { + const std::string inputType = (*it).first; + mx::InputPtr input = doc2->addInput("input" + std::to_string(index), inputType); + input->setValueString((*it).second); + if (inputType == "float") + { + input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.0100 "); + input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 01.0100 "); + } + index++; + } + + // Check attibute values + mx::ElementEquivalenceOptions options; + bool equivalent = doc->isEquivalent(doc2, options); + if (!equivalent) + { + std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; + std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; + } + REQUIRE(equivalent); + + unsigned int currentPrecision = mx::Value::getFloatPrecision(); + // This will compare 0.012345608 versus: 1, 0.012345611 for input10 + options.precision = 8; + options.status = mx::EMPTY_STRING; + equivalent = doc->isEquivalent(doc2, options); + if (equivalent) + { + std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; + std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; + } + else + { + std::cout << "Expected difference: " << options.status << std::endl; + } + REQUIRE(!equivalent); + options.precision = currentPrecision; + + // Check attribute order ignore. Some inputs have differing attribute order + // which will be caught. + options.ignoreAttributeOrder = false; + options.status = mx::EMPTY_STRING; + equivalent = doc->isEquivalent(doc2, options); + if (equivalent) + { + std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; + std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; + } + else + { + std::cout << "Expected difference: " << options.status << std::endl; + } + REQUIRE(!equivalent); + + // Check attribute filtering of inputs with differing attribute ordering. + options.ignoreAttributeOrder = false; + options.status = mx::EMPTY_STRING; + options.skipAttributes = { mx::ValueElement::UI_MIN_ATTRIBUTE, mx::ValueElement::UI_MAX_ATTRIBUTE }; + equivalent = doc->isEquivalent(doc2, options); + if (!equivalent) + { + std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; + std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; + } + REQUIRE(equivalent); + +} From 59bacd83a5ca9f65f55a2aea593b85b38070de6a Mon Sep 17 00:00:00 2001 From: Bernard Kwok Date: Wed, 4 Sep 2024 15:22:36 -0400 Subject: [PATCH 2/6] Add option to perform literal string vs value comparisons. --- source/MaterialXCore/Element.cpp | 56 +++++++++++-------- source/MaterialXCore/Element.h | 6 ++ .../MaterialXTest/MaterialXCore/Document.cpp | 22 +++++++- 3 files changed, 57 insertions(+), 27 deletions(-) diff --git a/source/MaterialXCore/Element.cpp b/source/MaterialXCore/Element.cpp index 48ec9dfe6d..dc51942389 100644 --- a/source/MaterialXCore/Element.cpp +++ b/source/MaterialXCore/Element.cpp @@ -595,43 +595,51 @@ bool ValueElement::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& ConstValueElementPtr rhsValueElement = rhs->asA(); for (const string& attr : rhsAttributeNames) { - // Check value equality - if (attr == ValueElement::VALUE_ATTRIBUTE) + bool performStringCompare = true; + if (!options.skipValueComparisons) { - ValuePtr thisValue = getValue(); - ValuePtr rhsValue = rhsValueElement->getValue(); - if (thisValue && rhsValue) + // Check value equality + if (attr == ValueElement::VALUE_ATTRIBUTE) { - if (thisValue->getValueString() != rhsValue->getValueString()) + ValuePtr thisValue = getValue(); + ValuePtr rhsValue = rhsValueElement->getValue(); + if (thisValue && rhsValue) { - options.status = "Attribute 'value' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; - return false; + if (thisValue->getValueString() != rhsValue->getValueString()) + { + options.status = "Attribute 'value' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } } + performStringCompare = false; } - } - // Check ui attribute value equality - else if (uiAttributes.find(attr) != uiAttributes.end()) - { - const string& uiAttribute = getAttribute(attr); - const string& rhsUiAttribute = getAttribute(attr); - ValuePtr uiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(uiAttribute, getType()) : nullptr; - ValuePtr rhsUiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(rhsUiAttribute, getType()) : nullptr; - if (uiValue && rhsUiValue) + // Check ui attribute value equality + else if (uiAttributes.find(attr) != uiAttributes.end()) { - if (uiValue->getValueString() != rhsUiValue->getValueString()) + const string& uiAttribute = getAttribute(attr); + const string& rhsUiAttribute = getAttribute(attr); + ValuePtr uiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(uiAttribute, getType()) : nullptr; + ValuePtr rhsUiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(rhsUiAttribute, getType()) : nullptr; + if (uiValue && rhsUiValue) { - options.status = "Attribute '" + attr + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; - return false; + if (uiValue->getValueString() != rhsUiValue->getValueString()) + { + options.status = "Attribute '" + attr + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } } + performStringCompare = false; } } - // Perform string comparison - else if (getAttribute(attr) != rhsValueElement->getAttribute(attr)) + if (performStringCompare) { - options.status = "Attribute '" + attr + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; - return false; + if (getAttribute(attr) != rhsValueElement->getAttribute(attr)) + { + options.status = "Attribute '" + attr + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + return false; + } } } diff --git a/source/MaterialXCore/Element.h b/source/MaterialXCore/Element.h index d22f42863a..4858fc908b 100644 --- a/source/MaterialXCore/Element.h +++ b/source/MaterialXCore/Element.h @@ -82,6 +82,7 @@ class MX_CORE_API ElementEquivalenceOptions precision = Value::getFloatPrecision(); skipAttributes = {}; ignoreAttributeOrder = true; + skipValueComparisons = false; status = EMPTY_STRING; }; ~ElementEquivalenceOptions() { } @@ -107,6 +108,11 @@ class MX_CORE_API ElementEquivalenceOptions /// are specified on an element. Default is to ignore order. bool ignoreAttributeOrder; + /// Do not perform any value comparisions. Instead perform exact string comparisons for attributes + /// Default is false. The operator==() method can be used instead as it always performs + /// a strict comparison. Default is false. + bool skipValueComparisons; + /// Status string for first difference found. string status; }; diff --git a/source/MaterialXTest/MaterialXCore/Document.cpp b/source/MaterialXTest/MaterialXCore/Document.cpp index 479c5cad21..5de9c0c843 100644 --- a/source/MaterialXTest/MaterialXCore/Document.cpp +++ b/source/MaterialXTest/MaterialXCore/Document.cpp @@ -184,15 +184,31 @@ TEST_CASE("Document equivalence", "[document]") input->setValueString((*it).second); if (inputType == "float") { - input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.0100 "); - input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 01.0100 "); + input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.01"); + input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 1.01"); } index++; } - // Check attibute values mx::ElementEquivalenceOptions options; + + // Check skipping all value compares + options.skipValueComparisons = true; bool equivalent = doc->isEquivalent(doc2, options); + if (equivalent) + { + std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; + std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; + } + else + { + std::cout << "Expected difference: " << options.status << std::endl; + } + REQUIRE(!equivalent); + + // Check attibute values + options.skipValueComparisons = false; + equivalent = doc->isEquivalent(doc2, options); if (!equivalent) { std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; From a5a36278a65bb3f4a13c9c813a472bad9a3668e1 Mon Sep 17 00:00:00 2001 From: Bernard Kwok Date: Thu, 5 Sep 2024 11:10:58 -0400 Subject: [PATCH 3/6] Review updates: - Separate out results into option results class argument. - Add isAttributeEquivalent() to avoid duplication --- source/MaterialXCore/Element.cpp | 152 ++++++++---------- source/MaterialXCore/Element.h | 68 ++++++-- .../MaterialXTest/MaterialXCore/Document.cpp | 39 +++-- 3 files changed, 153 insertions(+), 106 deletions(-) diff --git a/source/MaterialXCore/Element.cpp b/source/MaterialXCore/Element.cpp index dc51942389..d7d63e6815 100644 --- a/source/MaterialXCore/Element.cpp +++ b/source/MaterialXCore/Element.cpp @@ -81,12 +81,14 @@ bool Element::operator!=(const Element& rhs) const return !(*this == rhs); } -bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options) const +bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options, + ElementEquivalenceResult* result) const { if (getCategory() != rhs->getCategory() || getName() != rhs->getName()) { - options.status = "Name or category does not match for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + if (result) + result->addMessage("Name or category does not match for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); return false; } @@ -115,15 +117,15 @@ bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& optio } if (attributeNames != rhsAttributeNames) { - options.status = "Attribute names differ for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + if (result) + result->addMessage("Attribute names differ for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); return false; } for (const string& attr : rhsAttributeNames) { - if (getAttribute(attr) != rhs->getAttribute(attr)) + if (!isAttributeEquivalent(rhs, attr, options, result)) { - options.status = "Attribute " + attr + " differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; return false; } } @@ -133,17 +135,30 @@ bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& optio const vector& c2 = rhs->getChildren(); if (c1.size() != c2.size()) { - options.status = "Child count differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; + if (result) + result->addMessage("Child count differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); return false; } for (size_t i = 0; i < c1.size(); i++) { - if (!c1[i]->isEquivalent(c2[i], options)) + if (!c1[i]->isEquivalent(c2[i], options, result)) return false; } return true; } +bool Element::isAttributeEquivalent(ConstElementPtr rhs, const string& attributeName, + ElementEquivalenceOptions& /*options*/, ElementEquivalenceResult* result) const +{ + if (getAttribute(attributeName) != rhs->getAttribute(attributeName)) + { + if (result) + result->addMessage("Attribute " + attributeName + " differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + return false; + } + return true; +} + void Element::setName(const string& name) { ElementPtr parent = getParent(); @@ -545,102 +560,65 @@ TypeDefPtr TypedElement::getTypeDef() const // ValueElement methods // -bool ValueElement::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options) const -{ - if (getCategory() != rhs->getCategory() || - getName() != rhs->getName()) - { - options.status = "Name or category does not match for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; - return false; - } - - // Compare attribute names. - StringVec attributeNames = getAttributeNames(); - StringVec rhsAttributeNames = rhs->getAttributeNames(); - - // Filter out any attributes specified in the options. - const StringSet& skipAttributes = options.skipAttributes; - if (!skipAttributes.empty()) +bool ValueElement::isAttributeEquivalent(ConstElementPtr rhs, const string& attributeName, + ElementEquivalenceOptions& options, ElementEquivalenceResult* result) const +{ + bool perforDefaultCompare = true; + + if (!options.skipValueComparisons) { - attributeNames.erase(std::remove_if(attributeNames.begin(), attributeNames.end(), - [&skipAttributes](const string& attr) { return skipAttributes.find(attr) != skipAttributes.end(); }), - attributeNames.end()); - rhsAttributeNames.erase(std::remove_if(rhsAttributeNames.begin(), rhsAttributeNames.end(), - [&skipAttributes](const string& attr) { return skipAttributes.find(attr) != skipAttributes.end(); }), - rhsAttributeNames.end()); - } - - // Ignore ordering if option specified. - if (options.ignoreAttributeOrder) - { - // Sort the string arrays - std::sort(attributeNames.begin(), attributeNames.end()); - std::sort(rhsAttributeNames.begin(), rhsAttributeNames.end()); - } - if (attributeNames != rhsAttributeNames) - { - options.status = "Attribute names differ for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; - return false; - } + const StringSet uiAttributes = + { + ValueElement::UI_MIN_ATTRIBUTE, ValueElement::UI_MAX_ATTRIBUTE, + ValueElement::UI_SOFT_MIN_ATTRIBUTE, ValueElement::UI_SOFT_MAX_ATTRIBUTE, + ValueElement::UI_STEP_ATTRIBUTE + }; - const StringSet uiAttributes = { - ValueElement::UI_MIN_ATTRIBUTE, ValueElement::UI_MAX_ATTRIBUTE, - ValueElement::UI_SOFT_MIN_ATTRIBUTE, ValueElement::UI_SOFT_MAX_ATTRIBUTE, - ValueElement::UI_STEP_ATTRIBUTE - }; + // Get precision and format options + ScopedFloatFormatting fmt(options.format, options.precision); - // Get precision and format options - ScopedFloatFormatting fmt(options.format, options.precision); + ConstValueElementPtr rhsValueElement = rhs->asA(); - ConstValueElementPtr rhsValueElement = rhs->asA(); - for (const string& attr : rhsAttributeNames) - { - bool performStringCompare = true; - if (!options.skipValueComparisons) + // Check value equality + if (attributeName == ValueElement::VALUE_ATTRIBUTE) { - // Check value equality - if (attr == ValueElement::VALUE_ATTRIBUTE) + ValuePtr thisValue = getValue(); + ValuePtr rhsValue = rhsValueElement->getValue(); + if (thisValue && rhsValue) { - ValuePtr thisValue = getValue(); - ValuePtr rhsValue = rhsValueElement->getValue(); - if (thisValue && rhsValue) + if (thisValue->getValueString() != rhsValue->getValueString()) { - if (thisValue->getValueString() != rhsValue->getValueString()) - { - options.status = "Attribute 'value' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; - return false; - } + if (result) + result->addMessage("Attribute 'value' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + return false; } - performStringCompare = false; } + perforDefaultCompare = false; + } - // Check ui attribute value equality - else if (uiAttributes.find(attr) != uiAttributes.end()) + // Check ui attribute value equality + else if (uiAttributes.find(attributeName) != uiAttributes.end()) + { + const string& uiAttribute = getAttribute(attributeName); + const string& rhsUiAttribute = getAttribute(attributeName); + ValuePtr uiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(uiAttribute, getType()) : nullptr; + ValuePtr rhsUiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(rhsUiAttribute, getType()) : nullptr; + if (uiValue && rhsUiValue) { - const string& uiAttribute = getAttribute(attr); - const string& rhsUiAttribute = getAttribute(attr); - ValuePtr uiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(uiAttribute, getType()) : nullptr; - ValuePtr rhsUiValue = !rhsUiAttribute.empty() ? Value::createValueFromStrings(rhsUiAttribute, getType()) : nullptr; - if (uiValue && rhsUiValue) + if (uiValue->getValueString() != rhsUiValue->getValueString()) { - if (uiValue->getValueString() != rhsUiValue->getValueString()) - { - options.status = "Attribute '" + attr + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; - return false; - } + if (result) + result->addMessage("Attribute '" + attributeName + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + return false; } - performStringCompare = false; } + perforDefaultCompare = false; } + } - if (performStringCompare) - { - if (getAttribute(attr) != rhsValueElement->getAttribute(attr)) - { - options.status = "Attribute '" + attr + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"; - return false; - } - } + if (perforDefaultCompare) + { + return Element::isAttributeEquivalent(rhs, attributeName, options, result); } return true; diff --git a/source/MaterialXCore/Element.h b/source/MaterialXCore/Element.h index 4858fc908b..9881d74a0a 100644 --- a/source/MaterialXCore/Element.h +++ b/source/MaterialXCore/Element.h @@ -71,6 +71,37 @@ using ElementMap = std::unordered_map; /// A standard function taking an ElementPtr and returning a boolean. using ElementPredicate = std::function; +/// @class ElemenEquivalenceResult +/// The results of comparing for equivalence. +class MX_CORE_API ElementEquivalenceResult +{ + public: + ElementEquivalenceResult() = default; + ~ElementEquivalenceResult() = default; + + /// Append to list of equivalence messages + void addMessage(const string& message) + { + messages.push_back(message); + } + + /// Clear result information + void clear() + { + messages.clear(); + } + + /// Get a list of equivalence messages + const StringVec& getMessages() const + { + return messages; + } + + private: + /// A list of feedback messages + StringVec messages; +}; + /// @class ElemenEquivalenceOptions /// A set of options for controlling for equivalence comparison. class MX_CORE_API ElementEquivalenceOptions @@ -83,7 +114,6 @@ class MX_CORE_API ElementEquivalenceOptions skipAttributes = {}; ignoreAttributeOrder = true; skipValueComparisons = false; - status = EMPTY_STRING; }; ~ElementEquivalenceOptions() { } @@ -112,9 +142,6 @@ class MX_CORE_API ElementEquivalenceOptions /// Default is false. The operator==() method can be used instead as it always performs /// a strict comparison. Default is false. bool skipValueComparisons; - - /// Status string for first difference found. - string status; }; /// @class Element @@ -156,10 +183,26 @@ class MX_CORE_API Element : public std::enable_shared_from_this /// differs from this one. bool operator!=(const Element& rhs) const; - /// Return tue if the given element treee, including all descendents, + /// Return true if the given element treee, including all descendents, /// is considered to be equivalent to this one based on the equivalence /// criteria provided. - virtual bool isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options) const; + /// @param rhs Element to compare against + /// @param options Equivalence criteria + /// @param result Results of comparison if argument is specified. + /// @return True if the elements are equivalent. False otherwise. + bool isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options, + ElementEquivalenceResult* result = nullptr) const; + + /// Return true if the attribute on a given element is equivalent + /// based on the equivalence criteria provided. + /// @param rhs Element to compare against + /// @param attributeName Name of attribute to compare + /// @param options Equivalence criteria + /// @param result Results of comparison if argument is specified. + /// @return True if the attribute on the elements are equivalent. False otherwise. + virtual bool isAttributeEquivalent(ConstElementPtr rhs, const string& attributeName, + ElementEquivalenceOptions& options, + ElementEquivalenceResult* result = nullptr) const; /// @} /// @name Category @@ -983,9 +1026,16 @@ class MX_CORE_API ValueElement : public TypedElement /// @name Comparison interfaces /// @{ - /// Return tue if the given value element is considered to be equivalent to this - /// one based on the equivalence criteria provided. - bool isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options) const override; + /// Return true if the attribute on a given element is equivalent + /// based on the equivalence criteria provided. + /// @param rhs Element to compare against + /// @param attributeName Name of attribute to compare + /// @param options Equivalence criteria + /// @param result Results of comparison if argument is specified. + /// @return True if the attribute on the elements are equivalent. False otherwise. + bool isAttributeEquivalent(ConstElementPtr rhs, const string& attributeName, + ElementEquivalenceOptions& options, + ElementEquivalenceResult* result = nullptr) const override; /// @} /// @name Value String diff --git a/source/MaterialXTest/MaterialXCore/Document.cpp b/source/MaterialXTest/MaterialXCore/Document.cpp index 5de9c0c843..a342b3e407 100644 --- a/source/MaterialXTest/MaterialXCore/Document.cpp +++ b/source/MaterialXTest/MaterialXCore/Document.cpp @@ -181,6 +181,7 @@ TEST_CASE("Document equivalence", "[document]") { const std::string inputType = (*it).first; mx::InputPtr input = doc2->addInput("input" + std::to_string(index), inputType); + // Note: order of value and ui attributes is different for ordering comparison input->setValueString((*it).second); if (inputType == "float") { @@ -191,10 +192,11 @@ TEST_CASE("Document equivalence", "[document]") } mx::ElementEquivalenceOptions options; + mx::ElementEquivalenceResult result; // Check skipping all value compares options.skipValueComparisons = true; - bool equivalent = doc->isEquivalent(doc2, options); + bool equivalent = doc->isEquivalent(doc2, options, &result); if (equivalent) { std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; @@ -202,15 +204,23 @@ TEST_CASE("Document equivalence", "[document]") } else { - std::cout << "Expected difference: " << options.status << std::endl; + for (const std::string& message : result.getMessages()) + { + std::cout << "Expected difference: " << message << std::endl; + } + result.clear(); } REQUIRE(!equivalent); // Check attibute values options.skipValueComparisons = false; - equivalent = doc->isEquivalent(doc2, options); + equivalent = doc->isEquivalent(doc2, options, &result); if (!equivalent) { + for (const std::string& message : result.getMessages()) + { + std::cout << "Unexpected difference: " << message << std::endl; + } std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } @@ -219,7 +229,6 @@ TEST_CASE("Document equivalence", "[document]") unsigned int currentPrecision = mx::Value::getFloatPrecision(); // This will compare 0.012345608 versus: 1, 0.012345611 for input10 options.precision = 8; - options.status = mx::EMPTY_STRING; equivalent = doc->isEquivalent(doc2, options); if (equivalent) { @@ -228,7 +237,10 @@ TEST_CASE("Document equivalence", "[document]") } else { - std::cout << "Expected difference: " << options.status << std::endl; + for (const std::string& message : result.getMessages()) + { + std::cout << "Expected difference: " << message << std::endl; + } } REQUIRE(!equivalent); options.precision = currentPrecision; @@ -236,8 +248,8 @@ TEST_CASE("Document equivalence", "[document]") // Check attribute order ignore. Some inputs have differing attribute order // which will be caught. options.ignoreAttributeOrder = false; - options.status = mx::EMPTY_STRING; - equivalent = doc->isEquivalent(doc2, options); + result.clear(); + equivalent = doc->isEquivalent(doc2, options, &result); if (equivalent) { std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; @@ -245,17 +257,24 @@ TEST_CASE("Document equivalence", "[document]") } else { - std::cout << "Expected difference: " << options.status << std::endl; + for (const std::string& message : result.getMessages()) + { + std::cout << "Expected difference: " << message << std::endl; + } } REQUIRE(!equivalent); // Check attribute filtering of inputs with differing attribute ordering. options.ignoreAttributeOrder = false; - options.status = mx::EMPTY_STRING; + result.clear(); options.skipAttributes = { mx::ValueElement::UI_MIN_ATTRIBUTE, mx::ValueElement::UI_MAX_ATTRIBUTE }; - equivalent = doc->isEquivalent(doc2, options); + equivalent = doc->isEquivalent(doc2, options, &result); if (!equivalent) { + for (const std::string& message : result.getMessages()) + { + std::cout << "Unexpected difference: " << message << std::endl; + } std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } From 7791d188148da19f2cd58923698b93435059b657 Mon Sep 17 00:00:00 2001 From: Bernard Kwok Date: Fri, 6 Sep 2024 11:46:08 -0400 Subject: [PATCH 4/6] Add in underered child testing. Refine result struct. --- source/MaterialXCore/Element.cpp | 45 +++++++-- source/MaterialXCore/Element.h | 36 +++++-- .../MaterialXTest/MaterialXCore/Document.cpp | 96 ++++++++++++------- 3 files changed, 125 insertions(+), 52 deletions(-) diff --git a/source/MaterialXCore/Element.cpp b/source/MaterialXCore/Element.cpp index d7d63e6815..f9d3aedc8e 100644 --- a/source/MaterialXCore/Element.cpp +++ b/source/MaterialXCore/Element.cpp @@ -42,6 +42,14 @@ const string ValueElement::UNIFORM_ATTRIBUTE = "uniform"; Element::CreatorMap Element::_creatorMap; +const string ElementEquivalenceResult::ATTRIBUTE = "attribute"; +const string ElementEquivalenceResult::ATTRIBUTE_NAMES = "attribute names"; +const string ElementEquivalenceResult::CHILD_COUNT = "child count"; +const string ElementEquivalenceResult::CHILD_NAME = "child name"; +const string ElementEquivalenceResult::NAME = "name"; +const string ElementEquivalenceResult::CATEGORY = "category"; + + // // Element methods // @@ -84,11 +92,16 @@ bool Element::operator!=(const Element& rhs) const bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& options, ElementEquivalenceResult* result) const { - if (getCategory() != rhs->getCategory() || - getName() != rhs->getName()) + if (getName() != rhs->getName()) + { + if (result) + result->addDifference(getNamePath(), rhs->getNamePath(), ElementEquivalenceResult::NAME); + return false; + } + if (getCategory() != rhs->getCategory()) { if (result) - result->addMessage("Name or category does not match for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + result->addDifference(getNamePath(), rhs->getNamePath(), ElementEquivalenceResult::CATEGORY); return false; } @@ -118,7 +131,7 @@ bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& optio if (attributeNames != rhsAttributeNames) { if (result) - result->addMessage("Attribute names differ for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + result->addDifference(getNamePath(), rhs->getNamePath(), ElementEquivalenceResult::ATTRIBUTE_NAMES); return false; } @@ -136,12 +149,26 @@ bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& optio if (c1.size() != c2.size()) { if (result) - result->addMessage("Child count differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + result->addDifference(getNamePath(), rhs->getNamePath(), ElementEquivalenceResult::CHILD_COUNT); return false; } for (size_t i = 0; i < c1.size(); i++) { - if (!c1[i]->isEquivalent(c2[i], options, result)) + ElementPtr c2Element = c2[i]; + // Handle unordered children if parent is a graph. + if (this->isA()) + { + const string& childName = c1[i]->getName(); + c2Element = rhs->getChild(childName); + if (!c2Element) + { + if (result) + result->addDifference(c1[i]->getNamePath(), "", ElementEquivalenceResult::CHILD_NAME, + childName); + return false; + } + } + if (!c1[i]->isEquivalent(c2Element, options, result)) return false; } return true; @@ -153,7 +180,7 @@ bool Element::isAttributeEquivalent(ConstElementPtr rhs, const string& attribute if (getAttribute(attributeName) != rhs->getAttribute(attributeName)) { if (result) - result->addMessage("Attribute " + attributeName + " differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + result->addDifference(getNamePath(), rhs->getNamePath(), ElementEquivalenceResult::ATTRIBUTE, attributeName); return false; } return true; @@ -589,7 +616,7 @@ bool ValueElement::isAttributeEquivalent(ConstElementPtr rhs, const string& attr if (thisValue->getValueString() != rhsValue->getValueString()) { if (result) - result->addMessage("Attribute 'value' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + result->addDifference(getNamePath(), rhs->getNamePath(), ElementEquivalenceResult::ATTRIBUTE, attributeName); return false; } } @@ -608,7 +635,7 @@ bool ValueElement::isAttributeEquivalent(ConstElementPtr rhs, const string& attr if (uiValue->getValueString() != rhsUiValue->getValueString()) { if (result) - result->addMessage("Attribute '" + attributeName + "' differs for elements: '" + getNamePath() + "', '" + rhs->getNamePath() + "'"); + result->addDifference(getNamePath(), rhs->getNamePath(), ElementEquivalenceResult::ATTRIBUTE, attributeName); return false; } } diff --git a/source/MaterialXCore/Element.h b/source/MaterialXCore/Element.h index 9881d74a0a..45e8be8113 100644 --- a/source/MaterialXCore/Element.h +++ b/source/MaterialXCore/Element.h @@ -79,27 +79,45 @@ class MX_CORE_API ElementEquivalenceResult ElementEquivalenceResult() = default; ~ElementEquivalenceResult() = default; - /// Append to list of equivalence messages - void addMessage(const string& message) + /// Append to list of equivalence differences + void addDifference(const string& path1, const string& path2, const string& differenceType, + const string& name=EMPTY_STRING) { - messages.push_back(message); + StringVec difference = { path1, path2, differenceType, name}; + differences.push_back(difference); } /// Clear result information void clear() { - messages.clear(); + differences.clear(); } - /// Get a list of equivalence messages - const StringVec& getMessages() const + /// Get a list of equivalence differences + /// Difference is of the form: + /// { path to 1st element, path to 2nd element, difference type, [attribute if is attribute difference] } + StringVec getDifference(size_t index) const { - return messages; + if (index < differenceCount()) + return differences[index]; + return StringVec(); } + const size_t differenceCount() const + { + return differences.size(); + } + + static const string ATTRIBUTE; + static const string ATTRIBUTE_NAMES; + static const string CHILD_COUNT; + static const string CHILD_NAME; + static const string NAME; + static const string CATEGORY; + private: - /// A list of feedback messages - StringVec messages; + /// A list of differences + vector differences; }; /// @class ElemenEquivalenceOptions diff --git a/source/MaterialXTest/MaterialXCore/Document.cpp b/source/MaterialXTest/MaterialXCore/Document.cpp index a342b3e407..8e791bfaa0 100644 --- a/source/MaterialXTest/MaterialXCore/Document.cpp +++ b/source/MaterialXTest/MaterialXCore/Document.cpp @@ -10,7 +10,7 @@ #include #include -#include +#include #include namespace mx = MaterialX; @@ -120,15 +120,25 @@ TEST_CASE("Document", "[document]") REQUIRE(doc->validate()); } +void printDifferences(mx::ElementEquivalenceResult& result, const std::string& label) +{ + size_t differenceCount = result.differenceCount(); + for (size_t i=0; i inputMap; + std::unordered_multimap inputMap; inputMap.insert({ "color3", " 1.0, +2.0, 3.0 " }); inputMap.insert({ "color4", "1.0, 2.00, 0.3000, -4" }); - inputMap.insert({ "float", " 1.2e-10 " }); - inputMap.insert({ "float", " 00.1000 " }); inputMap.insert({ "integer", " 12 " }); inputMap.insert({ "matrix33", "01.0, 2.0, 0000.2310, " @@ -145,42 +155,52 @@ TEST_CASE("Document equivalence", "[document]") inputMap.insert({ "string", "mystring" }); inputMap.insert({ "boolean", "false" }); inputMap.insert({ "filename", "filename1" }); + inputMap.insert({ "float", " 1.2e-10 " }); + inputMap.insert({ "float", " 00.1000 " }); unsigned int index = 0; + mx::ElementPtr child = doc->addNodeGraph("mygraph"); + mx::NodeGraphPtr graph = child->asA(); for (auto it = inputMap.begin(); it != inputMap.end(); ++it) { const std::string inputType = (*it).first; - mx::InputPtr input = doc->addInput("input" + std::to_string(index), inputType); + mx::InputPtr input = graph->addInput("input" + std::to_string(index), inputType); if (inputType == "float") { input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.0100 "); input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 01.0100 "); } + else + { + input->setName("input_" + inputType); // Set by name for difference in order test + } input->setValueString((*it).second); index++; } mx::DocumentPtr doc2 = mx::createDocument(); - std::multimap inputMap2; - inputMap2.insert({ "color3", "1, 2, 3" }); - inputMap2.insert({ "color4", "1, 2, 0.3, -4" }); - inputMap2.insert({ "float", "1.2e-10" }); - inputMap2.insert({ "float", "0.1" }); + std::unordered_multimap inputMap2; + inputMap2.insert({ "color4", "1, 2, 0.3, -4" }); inputMap2.insert({ "integer", "12" }); inputMap2.insert({ "matrix33", "1, 2, 0.231, 1, 2, 0.231, 1, 2, 0.231, 1, 2, 0.231" }); inputMap2.insert({ "matrix44", "1, 2, 0.231, 0.1, 1, 2, 0.231, 0.1, 1, 2, 0.231, 0.1, 1, 2, 0.231, 0.1" }); inputMap2.insert({ "vector2", "1, 0.012345611" }); // For precision check - inputMap2.insert({ "vector3", "1, 2, 3" }); - inputMap2.insert({ "vector4", "1, 2, 0.3, -4" }); inputMap2.insert({ "string", "mystring" }); inputMap2.insert({ "boolean", "false" }); + inputMap2.insert({ "color3", "1, 2, 3" }); + inputMap2.insert({ "vector3", "1, 2, 3" }); + inputMap2.insert({ "vector4", "1, 2, 0.3, -4" }); inputMap2.insert({ "filename", "filename1" }); + inputMap2.insert({ "float", "1.2e-10" }); + inputMap2.insert({ "float", "0.1" }); index = 0; + child = doc2->addNodeGraph("mygraph"); + graph = child->asA(); for (auto it = inputMap2.begin(); it != inputMap2.end(); ++it) { const std::string inputType = (*it).first; - mx::InputPtr input = doc2->addInput("input" + std::to_string(index), inputType); + mx::InputPtr input = graph->addInput("input" + std::to_string(index), inputType); // Note: order of value and ui attributes is different for ordering comparison input->setValueString((*it).second); if (inputType == "float") @@ -188,6 +208,10 @@ TEST_CASE("Document equivalence", "[document]") input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.01"); input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 1.01"); } + else + { + input->setName("input_" + inputType); + } index++; } @@ -199,28 +223,23 @@ TEST_CASE("Document equivalence", "[document]") bool equivalent = doc->isEquivalent(doc2, options, &result); if (equivalent) { + std::cout << "Unexpected equivalence:" << std::endl; std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } else { - for (const std::string& message : result.getMessages()) - { - std::cout << "Expected difference: " << message << std::endl; - } - result.clear(); + printDifferences(result, "Expected differences"); } REQUIRE(!equivalent); // Check attibute values options.skipValueComparisons = false; + result.clear(); equivalent = doc->isEquivalent(doc2, options, &result); if (!equivalent) { - for (const std::string& message : result.getMessages()) - { - std::cout << "Unexpected difference: " << message << std::endl; - } + printDifferences(result, "Unexpected difference"); std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } @@ -232,15 +251,13 @@ TEST_CASE("Document equivalence", "[document]") equivalent = doc->isEquivalent(doc2, options); if (equivalent) { + std::cout << "Unexpected equivalence:" << std::endl; std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } else { - for (const std::string& message : result.getMessages()) - { - std::cout << "Expected difference: " << message << std::endl; - } + printDifferences(result, "Expected difference"); } REQUIRE(!equivalent); options.precision = currentPrecision; @@ -252,15 +269,13 @@ TEST_CASE("Document equivalence", "[document]") equivalent = doc->isEquivalent(doc2, options, &result); if (equivalent) { + std::cout << "Unexpected equivalence:" << std::endl; std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } else { - for (const std::string& message : result.getMessages()) - { - std::cout << "Expected difference: " << message << std::endl; - } + printDifferences(result, "Expected differences"); } REQUIRE(!equivalent); @@ -271,13 +286,26 @@ TEST_CASE("Document equivalence", "[document]") equivalent = doc->isEquivalent(doc2, options, &result); if (!equivalent) { - for (const std::string& message : result.getMessages()) - { - std::cout << "Unexpected difference: " << message << std::endl; - } + printDifferences(result, "Unexpected differences"); std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } REQUIRE(equivalent); + // Check for child name miss-match + mx::ElementPtr mismatchElement = doc->getDescendant("mygraph/input_color4"); + mismatchElement->setName("mismatch_color4"); + result.clear(); + equivalent = doc->isEquivalent(doc2, options, &result); + if (!equivalent) + { + printDifferences(result, "Expected differences"); + } + else + { + std::cout << "Unexpected equivalence:" << std::endl; + std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; + std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; + } + REQUIRE(!equivalent); } From 126600e10ba29efc486c550a547d6686535cdd90 Mon Sep 17 00:00:00 2001 From: kwokcb Date: Sat, 7 Sep 2024 19:14:02 -0400 Subject: [PATCH 5/6] Fix test --- source/MaterialXTest/MaterialXCore/Document.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/source/MaterialXTest/MaterialXCore/Document.cpp b/source/MaterialXTest/MaterialXCore/Document.cpp index 8e791bfaa0..c5b069f21b 100644 --- a/source/MaterialXTest/MaterialXCore/Document.cpp +++ b/source/MaterialXTest/MaterialXCore/Document.cpp @@ -169,13 +169,13 @@ TEST_CASE("Document equivalence", "[document]") { input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.0100 "); input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 01.0100 "); + index++; } else { input->setName("input_" + inputType); // Set by name for difference in order test } input->setValueString((*it).second); - index++; } mx::DocumentPtr doc2 = mx::createDocument(); @@ -207,14 +207,17 @@ TEST_CASE("Document equivalence", "[document]") { input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.01"); input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 1.01"); + index++; } else { input->setName("input_" + inputType); } - index++; } + std::cout << "Original Document 1: " << mx::prettyPrint(doc) << std::endl; + std::cout << "Original Document 2: " << mx::prettyPrint(doc2) << std::endl; + mx::ElementEquivalenceOptions options; mx::ElementEquivalenceResult result; From ba70b52372cc81d7cbb9e3a0602eb89a0903cac3 Mon Sep 17 00:00:00 2001 From: Bernard Kwok Date: Sun, 8 Sep 2024 15:26:57 -0400 Subject: [PATCH 6/6] Remove attribute order option. Add in functional graph ordering check. --- source/MaterialXCore/Element.cpp | 37 ++++++---- source/MaterialXCore/Element.h | 5 -- .../MaterialXTest/MaterialXCore/Document.cpp | 73 ++++++++++++------- 3 files changed, 67 insertions(+), 48 deletions(-) diff --git a/source/MaterialXCore/Element.cpp b/source/MaterialXCore/Element.cpp index f9d3aedc8e..cc0b7da2ff 100644 --- a/source/MaterialXCore/Element.cpp +++ b/source/MaterialXCore/Element.cpp @@ -121,13 +121,10 @@ bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& optio rhsAttributeNames.end()); } - // Ignore ordering if option specified. - if (options.ignoreAttributeOrder) - { - // Sort the string arrays - std::sort(attributeNames.begin(), attributeNames.end()); - std::sort(rhsAttributeNames.begin(), rhsAttributeNames.end()); - } + // Ignore attribute ordering by sorting names + std::sort(attributeNames.begin(), attributeNames.end()); + std::sort(rhsAttributeNames.begin(), rhsAttributeNames.end()); + if (attributeNames != rhsAttributeNames) { if (result) @@ -155,17 +152,25 @@ bool Element::isEquivalent(ConstElementPtr rhs, ElementEquivalenceOptions& optio for (size_t i = 0; i < c1.size(); i++) { ElementPtr c2Element = c2[i]; - // Handle unordered children if parent is a graph. - if (this->isA()) + // Handle unordered children if parent is a compound graph (NodeGraph, Document). + // (Functional graphs have a "nodedef" reference and define node interfaces + // so require strict interface ordering.) + GraphElementPtr graph = this->getSelfNonConst()->asA(); + if (graph) { - const string& childName = c1[i]->getName(); - c2Element = rhs->getChild(childName); - if (!c2Element) + NodeGraphPtr nodeGraph = graph->asA(); + DocumentPtr document = graph->asA(); + if (document || (nodeGraph && !nodeGraph->getNodeDef())) { - if (result) - result->addDifference(c1[i]->getNamePath(), "", ElementEquivalenceResult::CHILD_NAME, - childName); - return false; + const string& childName = c1[i]->getName(); + c2Element = rhs->getChild(childName); + if (!c2Element) + { + if (result) + result->addDifference(c1[i]->getNamePath(), "", ElementEquivalenceResult::CHILD_NAME, + childName); + return false; + } } } if (!c1[i]->isEquivalent(c2Element, options, result)) diff --git a/source/MaterialXCore/Element.h b/source/MaterialXCore/Element.h index 45e8be8113..047911f281 100644 --- a/source/MaterialXCore/Element.h +++ b/source/MaterialXCore/Element.h @@ -130,7 +130,6 @@ class MX_CORE_API ElementEquivalenceOptions format = Value::getFloatFormat(); precision = Value::getFloatPrecision(); skipAttributes = {}; - ignoreAttributeOrder = true; skipValueComparisons = false; }; ~ElementEquivalenceOptions() { } @@ -152,10 +151,6 @@ class MX_CORE_API ElementEquivalenceOptions /// Element::YPOS_ATTRIBUTE }; StringSet skipAttributes; - /// Option to indicate whether to ignore the order that attributes - /// are specified on an element. Default is to ignore order. - bool ignoreAttributeOrder; - /// Do not perform any value comparisions. Instead perform exact string comparisons for attributes /// Default is false. The operator==() method can be used instead as it always performs /// a strict comparison. Default is false. diff --git a/source/MaterialXTest/MaterialXCore/Document.cpp b/source/MaterialXTest/MaterialXCore/Document.cpp index c5b069f21b..be0119dcde 100644 --- a/source/MaterialXTest/MaterialXCore/Document.cpp +++ b/source/MaterialXTest/MaterialXCore/Document.cpp @@ -164,7 +164,7 @@ TEST_CASE("Document equivalence", "[document]") for (auto it = inputMap.begin(); it != inputMap.end(); ++it) { const std::string inputType = (*it).first; - mx::InputPtr input = graph->addInput("input" + std::to_string(index), inputType); + mx::InputPtr input = graph->addInput("input_" + std::to_string(index), inputType); if (inputType == "float") { input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.0100 "); @@ -197,16 +197,18 @@ TEST_CASE("Document equivalence", "[document]") index = 0; child = doc2->addNodeGraph("mygraph"); graph = child->asA(); + std::vector floatInputs; for (auto it = inputMap2.begin(); it != inputMap2.end(); ++it) { const std::string inputType = (*it).first; - mx::InputPtr input = graph->addInput("input" + std::to_string(index), inputType); + mx::InputPtr input = graph->addInput("input_" + std::to_string(index), inputType); // Note: order of value and ui attributes is different for ordering comparison input->setValueString((*it).second); if (inputType == "float") { input->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.01"); input->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 1.01"); + floatInputs.push_back(input); index++; } else @@ -215,9 +217,6 @@ TEST_CASE("Document equivalence", "[document]") } } - std::cout << "Original Document 1: " << mx::prettyPrint(doc) << std::endl; - std::cout << "Original Document 2: " << mx::prettyPrint(doc2) << std::endl; - mx::ElementEquivalenceOptions options; mx::ElementEquivalenceResult result; @@ -226,13 +225,13 @@ TEST_CASE("Document equivalence", "[document]") bool equivalent = doc->isEquivalent(doc2, options, &result); if (equivalent) { - std::cout << "Unexpected equivalence:" << std::endl; + std::cout << "Unexpected skip value equivalence:" << std::endl; std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } else { - printDifferences(result, "Expected differences"); + printDifferences(result, "Expected value differences"); } REQUIRE(!equivalent); @@ -242,7 +241,7 @@ TEST_CASE("Document equivalence", "[document]") equivalent = doc->isEquivalent(doc2, options, &result); if (!equivalent) { - printDifferences(result, "Unexpected difference"); + printDifferences(result, "Unexpected value difference"); std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } @@ -254,59 +253,79 @@ TEST_CASE("Document equivalence", "[document]") equivalent = doc->isEquivalent(doc2, options); if (equivalent) { - std::cout << "Unexpected equivalence:" << std::endl; + std::cout << "Unexpected precision equivalence:" << std::endl; std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } else { - printDifferences(result, "Expected difference"); + printDifferences(result, "Expected precision difference"); } REQUIRE(!equivalent); options.precision = currentPrecision; - // Check attribute order ignore. Some inputs have differing attribute order - // which will be caught. - options.ignoreAttributeOrder = false; + // Check attribute filtering of inputs result.clear(); + options.skipAttributes = { mx::ValueElement::UI_MIN_ATTRIBUTE, mx::ValueElement::UI_MAX_ATTRIBUTE }; + for (mx::InputPtr floatInput : floatInputs) + { + floatInput->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, "0.9"); + floatInput->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, "100.0"); + } equivalent = doc->isEquivalent(doc2, options, &result); - if (equivalent) + if (!equivalent) { - std::cout << "Unexpected equivalence:" << std::endl; + printDifferences(result, "Unexpected filtering differences"); std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } - else + REQUIRE(equivalent); + for (mx::InputPtr floatInput : floatInputs) { - printDifferences(result, "Expected differences"); + floatInput->setAttribute(mx::ValueElement::UI_MIN_ATTRIBUTE, " 0.01"); + floatInput->setAttribute(mx::ValueElement::UI_MAX_ATTRIBUTE, " 1.01"); } - REQUIRE(!equivalent); - // Check attribute filtering of inputs with differing attribute ordering. - options.ignoreAttributeOrder = false; + // Check for child name mismatch + mx::ElementPtr mismatchElement = doc->getDescendant("mygraph/input_color4"); + std::string previousName = mismatchElement->getName(); + mismatchElement->setName("mismatch_color4"); result.clear(); - options.skipAttributes = { mx::ValueElement::UI_MIN_ATTRIBUTE, mx::ValueElement::UI_MAX_ATTRIBUTE }; equivalent = doc->isEquivalent(doc2, options, &result); if (!equivalent) { - printDifferences(result, "Unexpected differences"); + printDifferences(result, "Expected name mismatch differences"); + } + else + { + std::cout << "Unexpected name match equivalence:" << std::endl; std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; } + REQUIRE(!equivalent); + mismatchElement->setName(previousName); + result.clear(); + equivalent = doc->isEquivalent(doc2, options, &result); REQUIRE(equivalent); - // Check for child name miss-match - mx::ElementPtr mismatchElement = doc->getDescendant("mygraph/input_color4"); - mismatchElement->setName("mismatch_color4"); + // Check for functional nodegraphs + mx::NodeGraphPtr nodeGraph = doc->getNodeGraph("mygraph"); + REQUIRE(nodeGraph); + doc->addNodeDef("ND_mygraph"); + nodeGraph->setNodeDefString("ND_mygraph"); + mx::NodeGraphPtr nodeGraph2 = doc2->getNodeGraph("mygraph"); + REQUIRE(nodeGraph2); + doc2->addNodeDef("ND_mygraph"); + nodeGraph2->setNodeDefString("ND_mygraph"); result.clear(); equivalent = doc->isEquivalent(doc2, options, &result); if (!equivalent) { - printDifferences(result, "Expected differences"); + printDifferences(result, "Expected functional graph differences"); } else { - std::cout << "Unexpected equivalence:" << std::endl; + std::cout << "Unexpected functional graph equivalence:" << std::endl; std::cout << "Document 1: " << mx::prettyPrint(doc) << std::endl; std::cout << "Document 2: " << mx::prettyPrint(doc2) << std::endl; }