From bb66af544a6e177cdf5a8d6e27dd105c947375fd Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Sun, 2 Dec 2018 13:08:47 +0100 Subject: [PATCH 1/2] closes https://github.com/assimp/assimp/issues/2228: prepare pull-request. --- code/OFFLoader.cpp | 240 +++++++++++++++++++++++++++++++-------------- 1 file changed, 165 insertions(+), 75 deletions(-) diff --git a/code/OFFLoader.cpp b/code/OFFLoader.cpp index 81f9c9916..977b1b36c 100644 --- a/code/OFFLoader.cpp +++ b/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 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 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]; + aiVector3D& v = mesh->mVertices[i]; + sz = line; - sz = line; SkipSpaces(&sz); - sz = fast_atoreal_move(sz,(ai_real&)v.x); SkipSpaces(&sz); - sz = fast_atoreal_move(sz,(ai_real&)v.y); SkipSpaces(&sz); - fast_atoreal_move(sz,(ai_real&)v.z); + // 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(sz, *vec[dim]); + } + + // if has homogenous coordinate, divide others by this one + if (hasHomogenous) { + SkipSpaces(&sz); + ai_real w = 1.; + sz = fast_atoreal_move(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(sz,(ai_real&)n.x); + SkipSpaces(&sz); + sz = fast_atoreal_move(sz,(ai_real&)n.y); + SkipSpaces(&sz); + fast_atoreal_move(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(sz,(ai_real&)c.r); + if (*sz != '#' && *sz != '\n' && *sz != '\r') { + SkipSpaces(&sz); + sz = fast_atoreal_move(sz,(ai_real&)c.g); + } else { + c.g = 0.; + } + if (*sz != '#' && *sz != '\n' && *sz != '\r') { + SkipSpaces(&sz); + sz = fast_atoreal_move(sz,(ai_real&)c.b); + } else { + c.b = 0.; + } + if (*sz != '#' && *sz != '\n' && *sz != '\r') { + SkipSpaces(&sz); + sz = fast_atoreal_move(sz,(ai_real&)c.a); + } else { + c.a = 1.; + } + } + if (hasTexCoord) { + aiVector3D& t = mesh->mTextureCoords[0][i]; + SkipSpaces(&sz); + sz = fast_atoreal_move(sz,(ai_real&)t.x); + SkipSpaces(&sz); + fast_atoreal_move(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; ++i ) { + 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"); + unsigned int idx; + sz = line; SkipSpaces(&sz); + idx = strtoul10(sz,&sz); + if(!idx || idx > 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 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); - idx = strtoul10(sz,&sz); - if(!(idx) || idx > 9) - 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(""); @@ -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 From ce91f5c88890cc635986ccc52afb56d375efed5d Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Mon, 3 Dec 2018 21:24:06 +0100 Subject: [PATCH 2/2] Off-Importer: introduce unittest. --- code/OFFLoader.cpp | 2 +- code/glTF2Importer.cpp | 14 +++-- test/CMakeLists.txt | 2 + test/unit/ImportExport/utOFFImportExport.cpp | 63 ++++++++++++++++++++ test/unit/utObjTools.cpp | 1 - 5 files changed, 76 insertions(+), 6 deletions(-) create mode 100644 test/unit/ImportExport/utOFFImportExport.cpp diff --git a/code/OFFLoader.cpp b/code/OFFLoader.cpp index 977b1b36c..a72e6d9d4 100644 --- a/code/OFFLoader.cpp +++ b/code/OFFLoader.cpp @@ -294,7 +294,7 @@ void OFFImporter::InternReadFile( const std::string& pFile, aiScene* pScene, IOS // load faces with their indices faces = mesh->mFaces; - for (unsigned int i = 0; i < numFaces; ++i ) { + 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; diff --git a/code/glTF2Importer.cpp b/code/glTF2Importer.cpp index ff9fd4269..f6a664e5e 100755 --- a/code/glTF2Importer.cpp +++ b/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; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 01c8daa09..7e2aec270 100644 --- a/test/CMakeLists.txt +++ b/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 diff --git a/test/unit/ImportExport/utOFFImportExport.cpp b/test/unit/ImportExport/utOFFImportExport.cpp new file mode 100644 index 000000000..a355beab6 --- /dev/null +++ b/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 +#include +#include +#include + +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()); +} diff --git a/test/unit/utObjTools.cpp b/test/unit/utObjTools.cpp index 093244eb1..6604dfd59 100644 --- a/test/unit/utObjTools.cpp +++ b/test/unit/utObjTools.cpp @@ -115,4 +115,3 @@ TEST_F( utObjTools, countComponents_TwoLines_Success ) { size_t numComps = test_parser.testGetNumComponentsInDataDefinition(); EXPECT_EQ( 3U, numComps ); } -