Read the Value, not the Attribute

Correct some mistakes made when moving to pugixml from IrrXML
Fixes #4179

code/AssetLib/Collada/ColladaParser.cpp

@@ -924,6 +924,8 @@ void ColladaParser::ReadMaterial(XmlNode &node, Collada::Material &pMaterial) {
 void ColladaParser::ReadLight(XmlNode &node, Collada::Light &pLight) {
     XmlNodeIterator xmlIt(node, XmlNodeIterator::PreOrderMode);
     XmlNode currentNode;
+    // TODO: Check the current technique and skip over unsupported extra techniques
+
     while (xmlIt.getNext(currentNode)) {
         const std::string &currentName = currentNode.name();
         if (currentName == "spot") {
@@ -949,33 +951,34 @@ void ColladaParser::ReadLight(XmlNode &node, Collada::Light &pLight) {
             content = fast_atoreal_move<ai_real>(content, (ai_real &)pLight.mColor.b);
             SkipSpacesAndLineEnd(&content);
         } else if (currentName == "constant_attenuation") {
-            XmlParser::getRealAttribute(currentNode, "constant_attenuation", pLight.mAttConstant);
+            XmlParser::getValueAsFloat(currentNode, pLight.mAttConstant);
         } else if (currentName == "linear_attenuation") {
-            XmlParser::getRealAttribute(currentNode, "linear_attenuation", pLight.mAttLinear);
+            XmlParser::getValueAsFloat(currentNode, pLight.mAttLinear);
         } else if (currentName == "quadratic_attenuation") {
-            XmlParser::getRealAttribute(currentNode, "quadratic_attenuation", pLight.mAttQuadratic);
+            XmlParser::getValueAsFloat(currentNode, pLight.mAttQuadratic);
         } else if (currentName == "falloff_angle") {
-            XmlParser::getRealAttribute(currentNode, "falloff_angle", pLight.mFalloffAngle);
+            XmlParser::getValueAsFloat(currentNode, pLight.mFalloffAngle);
         } else if (currentName == "falloff_exponent") {
-            XmlParser::getRealAttribute(currentNode, "falloff_exponent", pLight.mFalloffExponent);
+            XmlParser::getValueAsFloat(currentNode, pLight.mFalloffExponent);
         }
         // FCOLLADA extensions
         // -------------------------------------------------------
         else if (currentName == "outer_cone") {
-            XmlParser::getRealAttribute(currentNode, "outer_cone", pLight.mOuterAngle);
-        } else if (currentName == "penumbra_angle") { // ... and this one is even deprecated
-            XmlParser::getRealAttribute(currentNode, "penumbra_angle", pLight.mPenumbraAngle);
+            XmlParser::getValueAsFloat(currentNode, pLight.mOuterAngle);
+        } else if (currentName == "penumbra_angle") { // this one is deprecated, now calculated using outer_cone
+            XmlParser::getValueAsFloat(currentNode, pLight.mPenumbraAngle);
         } else if (currentName == "intensity") {
-            XmlParser::getRealAttribute(currentNode, "intensity", pLight.mIntensity);
-        } else if (currentName == "falloff") {
-            XmlParser::getRealAttribute(currentNode, "falloff", pLight.mOuterAngle);
+            XmlParser::getValueAsFloat(currentNode, pLight.mIntensity);
+        }
+        else if (currentName == "falloff") {
+            XmlParser::getValueAsFloat(currentNode, pLight.mOuterAngle);
         } else if (currentName == "hotspot_beam") {
-            XmlParser::getRealAttribute(currentNode, "hotspot_beam", pLight.mFalloffAngle);
+            XmlParser::getValueAsFloat(currentNode, pLight.mFalloffAngle);
         }
         // OpenCOLLADA extensions
         // -------------------------------------------------------
         else if (currentName == "decay_falloff") {
-            XmlParser::getRealAttribute(currentNode, "decay_falloff", pLight.mOuterAngle);
+            XmlParser::getValueAsFloat(currentNode, pLight.mOuterAngle);
         }
     }
 }
@@ -1109,7 +1112,7 @@ void ColladaParser::ReadEffectProfileCommon(XmlNode &node, Collada::Effect &pEff
         // GOOGLEEARTH/OKINO extensions
         // -------------------------------------------------------
         else if (currentName == "double_sided")
-            XmlParser::getBoolAttribute(currentNode, currentName.c_str(), pEffect.mDoubleSided);
+            XmlParser::getValueAsBool(currentNode, pEffect.mDoubleSided);
 
         // FCOLLADA extensions
         // -------------------------------------------------------
@@ -1121,9 +1124,9 @@ void ColladaParser::ReadEffectProfileCommon(XmlNode &node, Collada::Effect &pEff
         // MAX3D extensions
         // -------------------------------------------------------
         else if (currentName == "wireframe") {
-            XmlParser::getBoolAttribute(currentNode, currentName.c_str(), pEffect.mWireframe);
+            XmlParser::getValueAsBool(currentNode, pEffect.mWireframe);
         } else if (currentName == "faceted") {
-            XmlParser::getBoolAttribute(currentNode, currentName.c_str(), pEffect.mFaceted);
+            XmlParser::getValueAsBool(currentNode, pEffect.mFaceted);
         }
     }
 }
@@ -1142,23 +1145,23 @@ void ColladaParser::ReadSamplerProperties(XmlNode &node, Sampler &out) {
         // MAYA extensions
         // -------------------------------------------------------
         if (currentName == "wrapU") {
-            XmlParser::getBoolAttribute(currentNode, currentName.c_str(), out.mWrapU);
+            XmlParser::getValueAsBool(currentNode, out.mWrapU);
         } else if (currentName == "wrapV") {
-            XmlParser::getBoolAttribute(currentNode, currentName.c_str(), out.mWrapV);
+            XmlParser::getValueAsBool(currentNode, out.mWrapV);
         } else if (currentName == "mirrorU") {
-            XmlParser::getBoolAttribute(currentNode, currentName.c_str(), out.mMirrorU);
+            XmlParser::getValueAsBool(currentNode, out.mMirrorU);
         } else if (currentName == "mirrorV") {
-            XmlParser::getBoolAttribute(currentNode, currentName.c_str(), out.mMirrorV);
+            XmlParser::getValueAsBool(currentNode, out.mMirrorV);
         } else if (currentName == "repeatU") {
-            XmlParser::getRealAttribute(currentNode, currentName.c_str(), out.mTransform.mScaling.x);
+            XmlParser::getValueAsFloat(currentNode, out.mTransform.mScaling.x);
         } else if (currentName == "repeatV") {
-            XmlParser::getRealAttribute(currentNode, currentName.c_str(), out.mTransform.mScaling.y);
+            XmlParser::getValueAsFloat(currentNode, out.mTransform.mScaling.y);
         } else if (currentName == "offsetU") {
-            XmlParser::getRealAttribute(currentNode, currentName.c_str(), out.mTransform.mTranslation.x);
+            XmlParser::getValueAsFloat(currentNode, out.mTransform.mTranslation.x);
         } else if (currentName == "offsetV") {
-            XmlParser::getRealAttribute(currentNode, currentName.c_str(), out.mTransform.mTranslation.y);
+            XmlParser::getValueAsFloat(currentNode, out.mTransform.mTranslation.y);
         } else if (currentName == "rotateUV") {
-            XmlParser::getRealAttribute(currentNode, currentName.c_str(), out.mTransform.mRotation);
+            XmlParser::getValueAsFloat(currentNode, out.mTransform.mRotation);
         } else if (currentName == "blend_mode") {
             std::string v;
             XmlParser::getValueAsString(currentNode, v);
@@ -1178,14 +1181,14 @@ void ColladaParser::ReadSamplerProperties(XmlNode &node, Sampler &out) {
         // OKINO extensions
         // -------------------------------------------------------
         else if (currentName == "weighting") {
-            XmlParser::getRealAttribute(currentNode, currentName.c_str(), out.mWeighting);
+            XmlParser::getValueAsFloat(currentNode, out.mWeighting);
         } else if (currentName == "mix_with_previous_layer") {
-            XmlParser::getRealAttribute(currentNode, currentName.c_str(), out.mMixWithPrevious);
+            XmlParser::getValueAsFloat(currentNode, out.mMixWithPrevious);
         }
         // MAX3D extensions
         // -------------------------------------------------------
         else if (currentName == "amount") {
-            XmlParser::getRealAttribute(currentNode, currentName.c_str(), out.mWeighting);
+            XmlParser::getValueAsFloat(currentNode, out.mWeighting);
         }
     }
 }

include/assimp/XmlParser.h

@@ -42,8 +42,8 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #ifndef INCLUDED_AI_IRRXML_WRAPPER
 #define INCLUDED_AI_IRRXML_WRAPPER
 
-#include <assimp/DefaultLogger.hpp>
 #include <assimp/ai_assert.h>
+#include <assimp/DefaultLogger.hpp>
 
 #include "BaseImporter.h"
 #include "IOStream.hpp"
@@ -112,7 +112,7 @@ public:
 
     ///	@brief  Will clear the parsed xml-file.
     void clear() {
-        if(mData.empty()) {
+        if (mData.empty()) {
             mDoc = nullptr;
             return;
         }
@@ -243,7 +243,7 @@ public:
     /// @param name     [in] The attribute name to look for.
     /// @param val      [out] The int value from the attribute.
     /// @return true, if the node contains an attribute with the given name and if the value is an int.
-    static inline bool getIntAttribute(XmlNode &xmlNode, const char *name, int &val ) {
+    static inline bool getIntAttribute(XmlNode &xmlNode, const char *name, int &val) {
         pugi::xml_attribute attr = xmlNode.attribute(name);
         if (attr.empty()) {
             return false;
@@ -258,7 +258,7 @@ public:
     /// @param name     [in] The attribute name to look for.
     /// @param val      [out] The real value from the attribute.
     /// @return true, if the node contains an attribute with the given name and if the value is a real.
-    static inline bool getRealAttribute( XmlNode &xmlNode, const char *name, ai_real &val ) {
+    static inline bool getRealAttribute(XmlNode &xmlNode, const char *name, ai_real &val) {
         pugi::xml_attribute attr = xmlNode.attribute(name);
         if (attr.empty()) {
             return false;
@@ -284,7 +284,6 @@ public:
 
         val = attr.as_float();
         return true;
-
     }
 
     /// @brief Will try to get a double attribute value.
@@ -322,7 +321,7 @@ public:
     /// @param name     [in] The attribute name to look for.
     /// @param val      [out] The bool value from the attribute.
     /// @return true, if the node contains an attribute with the given name and if the value is a bool.
-    static inline bool getBoolAttribute( XmlNode &xmlNode, const char *name, bool &val ) {
+    static inline bool getBoolAttribute(XmlNode &xmlNode, const char *name, bool &val) {
         pugi::xml_attribute attr = xmlNode.attribute(name);
         if (attr.empty()) {
             return false;
@@ -330,14 +329,13 @@ public:
 
         val = attr.as_bool();
         return true;
-
     }
 
     /// @brief Will try to get the value of the node as a string.
     /// @param node     [in] The node to search in.
     /// @param text     [out] The value as a text.
     /// @return true, if the value can be read out.
-    static inline bool getValueAsString( XmlNode &node, std::string &text ) {
+    static inline bool getValueAsString(XmlNode &node, std::string &text) {
         text = std::string();
         if (node.empty()) {
             return false;
@@ -352,7 +350,7 @@ public:
     /// @param node     [in] The node to search in.
     /// @param text     [out] The value as a float.
     /// @return true, if the value can be read out.
-    static inline bool getValueAsFloat( XmlNode &node, ai_real &v ) {
+    static inline bool getValueAsFloat(XmlNode &node, ai_real &v) {
         if (node.empty()) {
             return false;
         }
@@ -360,10 +358,38 @@ public:
         v = node.text().as_float();
 
         return true;
+    }
+
+    /// @brief Will try to get the value of the node as an integer.
+    /// @param node     [in] The node to search in.
+    /// @param text     [out] The value as a int.
+    /// @return true, if the value can be read out.
+    static inline bool getValueAsInt(XmlNode &node, int &v) {
+        if (node.empty()) {
+            return false;
+        }
+
+        v = node.text().as_int();
+
+        return true;
+    }
+
+    /// @brief Will try to get the value of the node as an bool.
+    /// @param node     [in] The node to search in.
+    /// @param text     [out] The value as a bool.
+    /// @return true, if the value can be read out.
+    static inline bool getValueAsBool(XmlNode& node, bool& v)
+    {
+        if (node.empty()) {
+            return false;
+        }
 
+        v = node.text().as_bool();
+
+        return true;
     }
 
- private:
+private:
     pugi::xml_document *mDoc;
     TNodeType mCurrent;
     std::vector<char> mData;
@@ -376,8 +402,8 @@ class XmlNodeIterator {
 public:
     /// @brief The iteration mode.
     enum IterationMode {
-        PreOrderMode,   ///< Pre-ordering, get the values, continue the iteration.
-        PostOrderMode   ///< Post-ordering, continue the iteration, get the values.
+        PreOrderMode, ///< Pre-ordering, get the values, continue the iteration.
+        PostOrderMode ///< Post-ordering, continue the iteration, get the values.
     };
     ///	@brief  The class constructor
     /// @param  parent      [in] The xml parent to to iterate through.
@@ -400,7 +426,7 @@ public:
 
     ///	@brief  Will iterate through all children in pre-order iteration.
     /// @param  node    [in] The nod to iterate through.
-    void collectChildrenPreOrder( XmlNode &node ) {
+    void collectChildrenPreOrder(XmlNode &node) {
         if (node != mParent && node.type() == pugi::node_element) {
             mNodes.push_back(node);
         }

test/unit/utColladaExport.cpp

@@ -125,13 +125,21 @@ TEST_F(utColladaExport, testExportLight) {
     ASSERT_TRUE(pTest->HasLights());
 
     const unsigned int origNumLights(pTest->mNumLights);
-    std::unique_ptr<aiLight[]> origLights(new aiLight[origNumLights]);
-    std::vector<std::string> origNames;
+    // There are FIVE!!! LIGHTS!!!
+    EXPECT_EQ(5, origNumLights) << "lights.dae should contain five lights";
+
+    std::vector<aiLight> origLights(5);
     for (size_t i = 0; i < origNumLights; i++) {
-        origNames.push_back(pTest->mLights[i]->mName.C_Str());
         origLights[i] = *(pTest->mLights[i]);
     }
 
+    // Check loaded first light properly
+    EXPECT_STREQ("Lamp", origLights[0].mName.C_Str());
+    EXPECT_EQ(aiLightSource_POINT, origLights[0].mType);
+    EXPECT_FLOAT_EQ(1.0f, origLights[0].mAttenuationConstant);
+    EXPECT_FLOAT_EQ(0.0f, origLights[0].mAttenuationLinear);
+    EXPECT_FLOAT_EQ(0.00111109f, origLights[0].mAttenuationQuadratic);
+
     // Common metadata
     // Confirm was loaded by the Collada importer
     aiString origImporter;
@@ -191,7 +199,7 @@ TEST_F(utColladaExport, testExportLight) {
     for (size_t i = 0; i < origNumLights; i++) {
         const aiLight *orig = &origLights[i];
         const aiLight *read = imported->mLights[i];
-        EXPECT_EQ(0, strncmp(origNames[i].c_str(), read->mName.C_Str(), origNames[i].size()));
+        EXPECT_EQ(0, strcmp(orig->mName.C_Str(), read->mName.C_Str()));
         EXPECT_EQ(orig->mType, read->mType);
         EXPECT_FLOAT_EQ(orig->mAttenuationConstant, read->mAttenuationConstant);
         EXPECT_FLOAT_EQ(orig->mAttenuationLinear, read->mAttenuationLinear);