Review Updates

- Do not remove trailing 0s for scientific notation. This just checks for the 'e' character after stripping leading and trailing spaces and leading zeros. - Skip empty tokens, though this is invalid for MaterialX.
kwokcb · Aug 25, 2024 · 43ea222 · 43ea222
1 parent 57752b9
commit 43ea222
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 22 deletions.
diff --git a/source/MaterialXCore/Util.cpp b/source/MaterialXCore/Util.cpp
@@ -181,6 +181,7 @@ string parentNamePath(const string& namePath)
     return EMPTY_STRING;
 }
 
+
 std::string normalizeValueString(const std::string& str, const std::string valueType)
 {
     // Currently supported value types.
@@ -190,6 +191,8 @@ std::string normalizeValueString(const std::string& str, const std::string value
         return str;
     }
 
+    const string TOKEN_SEPARATOR = ", ";
+
     std::string result;
     std::string token;
 
@@ -210,37 +213,52 @@ std::string normalizeValueString(const std::string& str, const std::string value
             token.erase(0, token.find_first_not_of(' '));
             token.erase(token.find_last_not_of(' ') + 1);
 
-            // Remove leading zeros
-            token.erase(0, token.find_first_not_of('0'));
-
-            // Preserve 0 values
-            if (token.empty() || token[0] == '.')
+            // Skip if the token is empty. Note that this is
+            // invalid in MaterialX but can still be stored this way. 
+            if (!token.empty())
             {
-                token = "0" + token;
-            }
+                // Remove leading zeros
+                token.erase(0, token.find_first_not_of('0'));
 
-            // If the token contains a decimal point, remove trailing zeros
-            size_t decimalPos = token.find('.');
-            if (decimalPos != std::string::npos)
-            {
-                token.erase(token.find_last_not_of('0') + 1);
+                // Preserve 0 values
+                if (token.empty() || token[0] == '.')
+                {
+                    token = "0" + token;
+                }
 
-                // If the token ends with a decimal point after removing trailing zeros, remove it
-                if (token.back() == '.')
+                // Check if string token has a 'e' character
+                // implying it is a floating point number in scientific notation.
+                // If it does not have an 'e' character, remove trailing zeros.
+                //
+                size_t ePos = token.find('e');
+                if (ePos == std::string::npos)
                 {
-                    token.pop_back();
+                    // If the token contains a decimal point, remove trailing zeros
+                    size_t decimalPos = token.find('.');
+                    if (decimalPos != std::string::npos)
+                    {
+                        token.erase(token.find_last_not_of('0') + 1);
+
+                        // If the token ends with a decimal point after removing trailing zeros, remove it
+                        if (token.back() == '.')
+                        {
+                            token.pop_back();
+                        }
+                    }
                 }
             }
 
             // Append the formatted token to the result with a comma
-            result += token + ", ";
+            result += token + TOKEN_SEPARATOR;
         }
 
-        // Remove the last comma
+        // Remove the last separator
         if (!result.empty())
         {
-            result.pop_back();
-            result.pop_back();
+            for (size_t i = 0; i < TOKEN_SEPARATOR.size(); i++)
+            {
+                result.pop_back();
+            }
         }
     }
 

diff --git a/source/MaterialXTest/MaterialXCore/CoreUtil.cpp b/source/MaterialXTest/MaterialXCore/CoreUtil.cpp
@@ -45,6 +45,9 @@ TEST_CASE("Value normalization", "[coreutil]")
     std::string resultInteger = mx::normalizeValueString(inputInteger, "integer");
     REQUIRE(resultInteger == "12");
 
+    std::string inputScalar = "  1.2e-10  ";
+    std::string resultScalar = mx::normalizeValueString(inputScalar, "float");
+    REQUIRE(resultScalar == "1.2e-10");
     std::string inputScalar1 = "  00.1000  ";
     std::string resultScalar1 = mx::normalizeValueString(inputScalar1, "float");
     REQUIRE(resultScalar1 == "0.1");
@@ -68,11 +71,15 @@ TEST_CASE("Value normalization", "[coreutil]")
     std::string resultVector2 = mx::normalizeValueString(inputVector2, "vector2");
     REQUIRE(resultVector2 == "1.2, 0.0001");
 
-    std::string inputVector3 = "01.0,         2.0,  0000.2310, 0.1";
+    std::string inputVector3 = "01.0,         1.2e-10 ,  0000.2310, 0.1";
     std::string resultVector3 = mx::normalizeValueString(inputVector3, "vector4");
-    REQUIRE(resultVector3 == "1, 2, 0.231, 0.1");
+    REQUIRE(resultVector3 == "1, 1.2e-10, 0.231, 0.1");
     resultVector3 = mx::normalizeValueString(inputVector3, "color4");
-    REQUIRE(resultVector3 == "1, 2, 0.231, 0.1");
+    REQUIRE(resultVector3 == "1, 1.2e-10, 0.231, 0.1");
+
+    std::string inputVector4 = "01.0,  ,  0000.2310, 0.1";
+    std::string resultVector4 = mx::normalizeValueString(inputVector4, "vector4");
+    REQUIRE(resultVector4 == "1, , 0.231, 0.1");
 
     std::string inputMatrix3 = "01.0,         2.0,  0000.2310, "
         "   01.0,         2.0,  0000.2310, "

diff --git a/source/MaterialXTest/MaterialXCore/Document.cpp b/source/MaterialXTest/MaterialXCore/Document.cpp
@@ -128,6 +128,7 @@ TEST_CASE("Document equivalence", "[document]")
 
     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", 
@@ -158,6 +159,7 @@ TEST_CASE("Document equivalence", "[document]")
     std::multimap<std::string, std::string> 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" });
@@ -194,5 +196,10 @@ TEST_CASE("Document equivalence", "[document]")
 
     // Note: do not check doc2 == doc as that is a pointer comparison
     bool equivalent = (*doc2 == *doc);
+    if (!equivalent)
+    {
+        std::cout << "doc 1: " << mx::prettyPrint(doc) << std::endl;
+        std::cout << "doc 2: " << mx::prettyPrint(doc2) << std::endl;
+    }
     REQUIRE(equivalent);
 }