Merge pull request #2250 from assimp/OFF_Importer_Redesign_js_francal

closes https://github.com/assimp/assimp/issues/2228: prepare pull-req…

code/OFFLoader.cpp

@@ -106,15 +106,23 @@ const aiImporterDesc* OFFImporter::GetInfo () const
     return &desc;
 }
 
+
+// skip blank space, lines and comments
+static void NextToken(const char **car, const char* end) {
+  SkipSpacesAndLineEnd(car);
+  while (*car < end && (**car == '#' || **car == '\n' || **car == '\r')) {
+    SkipLine(car);
+    SkipSpacesAndLineEnd(car);
+  }
+}
+
 // ------------------------------------------------------------------------------------------------
 // Imports the given file into the given scene structure.
-void OFFImporter::InternReadFile( const std::string& pFile,
-    aiScene* pScene, IOSystem* pIOHandler)
-{
+void OFFImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOSystem* pIOHandler) {
     std::unique_ptr<IOStream> file( pIOHandler->Open( pFile, "rb"));
 
     // Check whether we can read from the file
-    if( file.get() == NULL) {
+    if( file.get() == nullptr) {
         throw DeadlyImportError( "Failed to open OFF file " + pFile + ".");
     }
 
@@ -123,15 +131,61 @@ void OFFImporter::InternReadFile( const std::string& pFile,
     TextFileToBuffer(file.get(),mBuffer2);
     const char* buffer = &mBuffer2[0];
 
-    char line[4096];
-    GetNextLine(buffer,line);
-    if ('O' == line[0]) {
-        GetNextLine(buffer,line); // skip the 'OFF' line
+    // Proper OFF header parser. We only implement normal loading for now.
+    bool hasTexCoord = false, hasNormals = false, hasColors = false;
+    bool hasHomogenous = false, hasDimension = false;
+    unsigned int dimensions = 3;
+    const char* car = buffer;
+    const char* end = buffer + mBuffer2.size();
+    NextToken(&car, end);
+    
+    if (car < end - 2 && car[0] == 'S' && car[1] == 'T') {
+      hasTexCoord = true; car += 2;
+    }
+    if (car < end - 1 && car[0] == 'C') {
+      hasColors = true; car++;
+    }
+    if (car < end- 1 && car[0] == 'N') {
+      hasNormals = true; car++;
+    }
+    if (car < end - 1 && car[0] == '4') {
+      hasHomogenous = true; car++;
+    }
+    if (car < end - 1 && car[0] == 'n') {
+      hasDimension = true; car++;
+    }
+    if (car < end - 3 && car[0] == 'O' && car[1] == 'F' && car[2] == 'F') {
+        car += 3;
+	NextToken(&car, end);
+    } else {
+      // in case there is no OFF header (which is allowed by the
+      // specification...), then we might have unintentionally read an
+      // additional dimension from the primitive count fields
+      dimensions = 3;
+      hasHomogenous = false;
+      NextToken(&car, end);
+      
+      // at this point the next token should be an integer number
+      if (car >= end - 1 || *car < '0' || *car > '9') {
+	throw DeadlyImportError("OFF: Header is invalid");
+      }
+    }
+    if (hasDimension) {
+        dimensions = strtoul10(car, &car);
+	NextToken(&car, end);
+    }
+    if (dimensions > 3) {
+        throw DeadlyImportError
+	  ("OFF: Number of vertex coordinates higher than 3 unsupported");
     }
 
-    const char* sz = line; SkipSpaces(&sz);
-    const unsigned int numVertices = strtoul10(sz,&sz);SkipSpaces(&sz);
-    const unsigned int numFaces = strtoul10(sz,&sz);
+    NextToken(&car, end);
+    const unsigned int numVertices = strtoul10(car, &car);
+    NextToken(&car, end);
+    const unsigned int numFaces = strtoul10(car, &car);
+    NextToken(&car, end);
+    strtoul10(car, &car);  // skip edge count
+    NextToken(&car, end);
 
     if (!numVertices) {
         throw DeadlyImportError("OFF: There are no valid vertices");
@@ -147,91 +201,127 @@ void OFFImporter::InternReadFile( const std::string& pFile,
     pScene->mMeshes[0] = mesh;
 
     mesh->mNumFaces = numFaces;
-    aiFace* faces = new aiFace [mesh->mNumFaces];
+    aiFace* faces = new aiFace[mesh->mNumFaces];
     mesh->mFaces = faces;
 
-    std::vector<aiVector3D> tempPositions(numVertices);
+    mesh->mNumVertices = numVertices;
+    mesh->mVertices = new aiVector3D[numVertices];
+    mesh->mNormals = hasNormals ? new aiVector3D[numVertices] : nullptr;
+    mesh->mColors[0] = hasColors ? new aiColor4D[numVertices] : nullptr;
+
+    if (hasTexCoord) {
+        mesh->mNumUVComponents[0] = 2;
+        mesh->mTextureCoords[0] = new aiVector3D[numVertices];
+    }
+    char line[4096];
+    buffer = car;
+    const char *sz = car;
 
     // now read all vertex lines
-    for (unsigned int i = 0; i< numVertices;++i)
-    {
-        if(!GetNextLine(buffer,line))
-        {
+    for (unsigned int i = 0; i < numVertices; ++i) {
+        if(!GetNextLine(buffer, line)) {
             ASSIMP_LOG_ERROR("OFF: The number of verts in the header is incorrect");
             break;
         }
-        aiVector3D& v = tempPositions[i];
-
-        sz = line; SkipSpaces(&sz);
-        sz = fast_atoreal_move<ai_real>(sz,(ai_real&)v.x); SkipSpaces(&sz);
-        sz = fast_atoreal_move<ai_real>(sz,(ai_real&)v.y); SkipSpaces(&sz);
-        fast_atoreal_move<ai_real>(sz,(ai_real&)v.z);
+        aiVector3D& v = mesh->mVertices[i];	
+        sz = line;
+
+	// helper array to write a for loop over possible dimension values
+	ai_real* vec[3] = {&v.x, &v.y, &v.z};
+
+	// stop at dimensions: this allows loading 1D or 2D coordinate vertices
+        for (unsigned int dim = 0; dim < dimensions; ++dim ) {
+	    SkipSpaces(&sz);
+	    sz = fast_atoreal_move<ai_real>(sz, *vec[dim]);
+	}
+
+	// if has homogenous coordinate, divide others by this one
+	if (hasHomogenous) {
+	    SkipSpaces(&sz);
+	    ai_real w = 1.;
+	    sz = fast_atoreal_move<ai_real>(sz, w);
+            for (unsigned int dim = 0; dim < dimensions; ++dim ) {
+	        *(vec[dim]) /= w;
+	    }
+	}
+
+	// read optional normals
+	if (hasNormals) {
+	    aiVector3D& n = mesh->mNormals[i];
+	    SkipSpaces(&sz);
+	    sz = fast_atoreal_move<ai_real>(sz,(ai_real&)n.x);
+	    SkipSpaces(&sz);
+	    sz = fast_atoreal_move<ai_real>(sz,(ai_real&)n.y);
+	    SkipSpaces(&sz);
+	    fast_atoreal_move<ai_real>(sz,(ai_real&)n.z);
+	}
+	
+	// reading colors is a pain because the specification says it can be
+	// integers or floats, and any number of them between 1 and 4 included,
+	// until the next comment or end of line
+	// in theory should be testing type !
+	if (hasColors) {
+	    aiColor4D& c = mesh->mColors[0][i];
+	    SkipSpaces(&sz);
+	    sz = fast_atoreal_move<ai_real>(sz,(ai_real&)c.r);
+            if (*sz != '#' && *sz != '\n' && *sz != '\r') {
+	        SkipSpaces(&sz);
+	        sz = fast_atoreal_move<ai_real>(sz,(ai_real&)c.g);
+            } else {
+	        c.g = 0.;
+	    }
+            if (*sz != '#' && *sz != '\n' && *sz != '\r') {
+	        SkipSpaces(&sz);
+	        sz = fast_atoreal_move<ai_real>(sz,(ai_real&)c.b);
+            } else {
+	        c.b = 0.;
+	    }
+            if (*sz != '#' && *sz != '\n' && *sz != '\r') {
+	        SkipSpaces(&sz);
+	        sz = fast_atoreal_move<ai_real>(sz,(ai_real&)c.a);
+            } else {
+	        c.a = 1.;
+	    }
+	}
+        if (hasTexCoord) {
+	    aiVector3D& t = mesh->mTextureCoords[0][i];
+	    SkipSpaces(&sz);
+	    sz = fast_atoreal_move<ai_real>(sz,(ai_real&)t.x);
+	    SkipSpaces(&sz);
+	    fast_atoreal_move<ai_real>(sz,(ai_real&)t.y);
+	}
     }
 
-
-    // First find out how many vertices we'll need
-    const char* old = buffer;
-    for (unsigned int i = 0; i< mesh->mNumFaces;++i)
-    {
-        if(!GetNextLine(buffer,line))
-        {
+    // load faces with their indices
+    faces = mesh->mFaces;
+    for (unsigned int i = 0; i < numFaces; ) {
+        if(!GetNextLine(buffer,line)) {
             ASSIMP_LOG_ERROR("OFF: The number of faces in the header is incorrect");
             break;
         }
-        sz = line;SkipSpaces(&sz);
-        faces->mNumIndices = strtoul10(sz,&sz);
-        if(!(faces->mNumIndices) || faces->mNumIndices > 9)
-        {
-            ASSIMP_LOG_ERROR("OFF: Faces with zero indices aren't allowed");
-            --mesh->mNumFaces;
-            continue;
-        }
-        mesh->mNumVertices += faces->mNumIndices;
-        ++faces;
-    }
-
-    if (!mesh->mNumVertices)
-        throw DeadlyImportError("OFF: There are no valid faces");
-
-    // allocate storage for the output vertices
-    std::vector<aiVector3D> verts;
-    verts.reserve(mesh->mNumVertices);
-
-    // second: now parse all face indices
-    buffer = old;
-    faces = mesh->mFaces;
-    for (unsigned int i = 0, p = 0; i< mesh->mNumFaces;)
-    {
-        if(!GetNextLine(buffer,line))break;
-
         unsigned int idx;
-        sz = line;SkipSpaces(&sz);
+        sz = line; SkipSpaces(&sz);
         idx = strtoul10(sz,&sz);
-        if(!(idx) || idx > 9)
+        if(!idx || idx > 9) {
+	    ASSIMP_LOG_ERROR("OFF: Faces with zero indices aren't allowed");
+            --mesh->mNumFaces;
             continue;
-
-        faces->mIndices = new unsigned int [faces->mNumIndices];
-        for (unsigned int m = 0; m < faces->mNumIndices;++m)
-        {
+	}
+	faces->mNumIndices = idx;
+        faces->mIndices = new unsigned int[faces->mNumIndices];
+        for (unsigned int m = 0; m < faces->mNumIndices;++m) {
             SkipSpaces(&sz);
             idx = strtoul10(sz,&sz);
-            if ((idx) >= numVertices)
-            {
+            if (idx >= numVertices) {
                 ASSIMP_LOG_ERROR("OFF: Vertex index is out of range");
-                idx = numVertices-1;
+                idx = numVertices - 1;
             }
-            faces->mIndices[m] = p++;
-            verts.push_back(tempPositions[idx]);
+            faces->mIndices[m] = idx;
         }
         ++i;
         ++faces;
     }
-
-    if (mesh->mNumVertices != verts.size()) {
-        throw DeadlyImportError("OFF: Vertex count mismatch");
-    }
-    mesh->mVertices = new aiVector3D[verts.size()];
-    memcpy(mesh->mVertices, &verts[0], verts.size() * sizeof(aiVector3D));
+    
     // generate the output node graph
     pScene->mRootNode = new aiNode();
     pScene->mRootNode->mName.Set("<OFFRoot>");
@@ -248,8 +338,8 @@ void OFFImporter::InternReadFile( const std::string& pFile,
     pcMat->AddProperty(&clr,1,AI_MATKEY_COLOR_DIFFUSE);
     pScene->mMaterials[0] = pcMat;
 
-    const int twosided =1;
-    pcMat->AddProperty(&twosided,1,AI_MATKEY_TWOSIDED);
+    const int twosided = 1;
+    pcMat->AddProperty(&twosided, 1, AI_MATKEY_TWOSIDED);
 }
 
 #endif // !! ASSIMP_BUILD_NO_OFF_IMPORTER

code/glTF2Importer.cpp

@@ -1024,20 +1024,26 @@ void glTF2Importer::ImportAnimations(glTF2::Asset& r)
         }
         
         // Use the latest keyframe for the duration of the animation
-        unsigned int maxDuration = 0;
+        double maxDuration = 0;
         for (unsigned int j = 0; j < ai_anim->mNumChannels; ++j) {
             auto chan = ai_anim->mChannels[j];
             if (chan->mNumPositionKeys) {
                 auto lastPosKey = chan->mPositionKeys[chan->mNumPositionKeys - 1];
-                if (lastPosKey.mTime > maxDuration) maxDuration = lastPosKey.mTime;
+                if (lastPosKey.mTime > maxDuration) {
+                    maxDuration = lastPosKey.mTime;
+                }
             }
             if (chan->mNumRotationKeys) {
                 auto lastRotKey = chan->mRotationKeys[chan->mNumRotationKeys - 1];
-                if (lastRotKey.mTime > maxDuration) maxDuration = lastRotKey.mTime;
+                if (lastRotKey.mTime > maxDuration) {
+                    maxDuration = lastRotKey.mTime;
+                }
             }
             if (chan->mNumScalingKeys) {
                 auto lastScaleKey = chan->mScalingKeys[chan->mNumScalingKeys - 1];
-                if (lastScaleKey.mTime > maxDuration) maxDuration = lastScaleKey.mTime;
+                if (lastScaleKey.mTime > maxDuration) {
+                    maxDuration = lastScaleKey.mTime;
+                }
             }
         }
         ai_anim->mDuration = maxDuration;

test/CMakeLists.txt

@@ -49,6 +49,7 @@ INCLUDE_DIRECTORIES(
 if (MSVC)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING")
 endif()
+
 # Add the temporary output directories to the library path to make sure the
 # Assimp library can be found, even if it is not installed system-wide yet.
 LINK_DIRECTORIES( ${Assimp_BINARY_DIR} ${AssetImporter_BINARY_DIR}/lib )
@@ -121,6 +122,7 @@ SET( IMPORTERS
   unit/ImportExport/utCOBImportExport.cpp
   unit/ImportExport/utOgreImportExport.cpp
   unit/ImportExport/utQ3BSPFileImportExport.cpp
+  unit/ImportExport/utOFFImportExport.cpp
 )
 
 SET( MATERIAL

test/unit/ImportExport/utOFFImportExport.cpp

@@ -0,0 +1,63 @@
+/*
+---------------------------------------------------------------------------
+Open Asset Import Library (assimp)
+---------------------------------------------------------------------------
+
+Copyright (c) 2006-2018, assimp team
+
+
+
+All rights reserved.
+
+Redistribution and use of this software in source and binary forms,
+with or without modification, are permitted provided that the following
+conditions are met:
+
+* Redistributions of source code must retain the above
+copyright notice, this list of conditions and the
+following disclaimer.
+
+* Redistributions in binary form must reproduce the above
+copyright notice, this list of conditions and the
+following disclaimer in the documentation and/or other
+materials provided with the distribution.
+
+* Neither the name of the assimp team, nor the names of its
+contributors may be used to endorse or promote products
+derived from this software without specific prior
+written permission of the assimp team.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+---------------------------------------------------------------------------
+*/
+
+#include "UnitTestPCH.h"
+#include "SceneDiffer.h"
+#include "AbstractImportExportBase.h"
+#include <assimp/scene.h>
+#include <assimp/Importer.hpp>
+#include <assimp/Exporter.hpp>
+#include <assimp/postprocess.h>
+
+class utOFFImportExport : public AbstractImportExportBase {
+protected:
+    virtual bool importerTest() {
+        ::Assimp::Importer importer;
+        const aiScene *scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/OFF/Cube.off", aiProcess_ValidateDataStructure);
+        return nullptr != scene;
+    }
+};
+
+TEST_F(utOFFImportExport, importOFFFromFileTest) {
+    EXPECT_TRUE(importerTest());
+}

test/unit/utObjTools.cpp

@@ -115,4 +115,3 @@ TEST_F( utObjTools, countComponents_TwoLines_Success ) {
     size_t numComps = test_parser.testGetNumComponentsInDataDefinition();
     EXPECT_EQ( 3U, numComps );
 }
-