From a24502577fd0a7d53851fc366c037c734d9410e7 Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Wed, 21 Nov 2018 22:00:11 +0100 Subject: [PATCH] DXF-Importer: some small review findings. --- code/DXFHelper.h | 43 +++++++--------- code/DXFLoader.cpp | 121 +++++++++++++++++++++++---------------------- code/DXFLoader.h | 27 +++++----- 3 files changed, 94 insertions(+), 97 deletions(-) diff --git a/code/DXFHelper.h b/code/DXFHelper.h index f3f73d4c2..daf2f97e2 100644 --- a/code/DXFHelper.h +++ b/code/DXFHelper.h @@ -55,21 +55,19 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include namespace Assimp { - namespace DXF { - +namespace DXF { // read pairs of lines, parse group code and value and provide utilities // to convert the data to the target data type. -class LineReader -{ +// do NOT skip empty lines. In DXF files, they count as valid data. +class LineReader { public: LineReader(StreamReaderLE& reader) - // do NOT skip empty lines. In DXF files, they count as valid data. - : splitter(reader,false,true) - , groupcode( 0 ) - , value() - , end() - { + : splitter(reader,false,true) + , groupcode( 0 ) + , value() + , end() { + // empty } // ----------------------------------------- @@ -112,8 +110,6 @@ public: return fast_atof(value.c_str()); } -public: - // ----------------------------------------- /** pseudo-iterator increment to advance to the next (groupcode/value) pair */ LineReader& operator++() { @@ -168,14 +164,12 @@ private: int end; }; - - // represents a POLYLINE or a LWPOLYLINE. or even a 3DFACE The data is converted as needed. -struct PolyLine -{ +struct PolyLine { PolyLine() - : flags() - {} + : flags() { + // empty + } std::vector positions; std::vector colors; @@ -187,14 +181,15 @@ struct PolyLine std::string desc; }; - // reference to a BLOCK. Specifies its own coordinate system. -struct InsertBlock -{ +struct InsertBlock { InsertBlock() - : scale(1.f,1.f,1.f) - , angle() - {} + : pos() + , scale(1.f,1.f,1.f) + , angle() + , name() { + // empty + } aiVector3D pos; aiVector3D scale; diff --git a/code/DXFLoader.cpp b/code/DXFLoader.cpp index 24410da55..6710597df 100644 --- a/code/DXFLoader.cpp +++ b/code/DXFLoader.cpp @@ -71,8 +71,7 @@ const aiColor4D AI_DXF_DEFAULT_COLOR(aiColor4D(0.6f, 0.6f, 0.6f, 0.6f)); // color indices for DXF - 16 are supported, the table is // taken directly from the DXF spec. -static aiColor4D g_aclrDxfIndexColors[] = -{ +static aiColor4D g_aclrDxfIndexColors[] = { aiColor4D (0.6f, 0.6f, 0.6f, 1.0f), aiColor4D (1.0f, 0.0f, 0.0f, 1.0f), // red aiColor4D (0.0f, 1.0f, 0.0f, 1.0f), // green @@ -93,6 +92,11 @@ static aiColor4D g_aclrDxfIndexColors[] = #define AI_DXF_NUM_INDEX_COLORS (sizeof(g_aclrDxfIndexColors)/sizeof(g_aclrDxfIndexColors[0])) #define AI_DXF_ENTITIES_MAGIC_BLOCK "$ASSIMP_ENTITIES_MAGIC" +static const int GroupCode_Name = 2; +static const int GroupCode_XComp = 10; +static const int GroupCode_YComp = 20; +static const int GroupCode_ZComp = 30; + static const aiImporterDesc desc = { "Drawing Interchange Format (DXF) Importer", "", @@ -123,12 +127,12 @@ DXFImporter::~DXFImporter() { // Returns whether the class can handle the format of the given file. bool DXFImporter::CanRead( const std::string& filename, IOSystem* pIOHandler, bool checkSig ) const { const std::string& extension = GetExtension( filename ); - if ( extension == "dxf" ) { + if ( extension == desc.mFileExtensions ) { return true; } if ( extension.empty() || checkSig ) { - static const char *pTokens[] = { "SECTION", "HEADER", "ENDSEC", "BLOCKS" }; + const char *pTokens[] = { "SECTION", "HEADER", "ENDSEC", "BLOCKS" }; return BaseImporter::SearchFileHeaderForToken(pIOHandler, filename, pTokens, 4, 32 ); } @@ -137,29 +141,25 @@ bool DXFImporter::CanRead( const std::string& filename, IOSystem* pIOHandler, bo // ------------------------------------------------------------------------------------------------ // Get a list of all supported file extensions -const aiImporterDesc* DXFImporter::GetInfo () const -{ +const aiImporterDesc* DXFImporter::GetInfo () const { return &desc; } // ------------------------------------------------------------------------------------------------ // Imports the given file into the given scene structure. -void DXFImporter::InternReadFile( const std::string& pFile, - aiScene* pScene, - IOSystem* pIOHandler) -{ - std::shared_ptr file = std::shared_ptr( pIOHandler->Open( pFile) ); +void DXFImporter::InternReadFile( const std::string& filename, aiScene* pScene, IOSystem* pIOHandler) { + std::shared_ptr file = std::shared_ptr( pIOHandler->Open( filename) ); // Check whether we can read the file - if( file.get() == NULL) { - throw DeadlyImportError( "Failed to open DXF file " + pFile + ""); + if( file.get() == nullptr ) { + throw DeadlyImportError( "Failed to open DXF file " + filename + ""); } - // check whether this is a binaray DXF file - we can't read binary DXF files :-( + // Check whether this is a binary DXF file - we can't read binary DXF files :-( char buff[AI_DXF_BINARY_IDENT_LEN+1] = {0}; file->Read(buff,AI_DXF_BINARY_IDENT_LEN,1); - if (!strncmp(AI_DXF_BINARY_IDENT.c_str(),buff,AI_DXF_BINARY_IDENT_LEN)) { + if (0 == strncmp(AI_DXF_BINARY_IDENT.c_str(),buff,AI_DXF_BINARY_IDENT_LEN)) { throw DeadlyImportError("DXF: Binary files are not supported at the moment"); } @@ -228,13 +228,11 @@ void DXFImporter::InternReadFile( const std::string& pFile, } // ------------------------------------------------------------------------------------------------ -void DXFImporter::ConvertMeshes(aiScene* pScene, DXF::FileData& output) -{ +void DXFImporter::ConvertMeshes(aiScene* pScene, DXF::FileData& output) { // the process of resolving all the INSERT statements can grow the // poly-count excessively, so log the original number. // XXX Option to import blocks as separate nodes? if (!DefaultLogger::isNullLogger()) { - unsigned int vcount = 0, icount = 0; for (const DXF::Block& bl : output.blocks) { for (std::shared_ptr pl : bl.lines) { @@ -295,7 +293,7 @@ void DXFImporter::ConvertMeshes(aiScene* pScene, DXF::FileData& output) } } - if (!pScene->mNumMeshes) { + if ( 0 == pScene->mNumMeshes) { throw DeadlyImportError("DXF: this file contains no 3d data"); } @@ -368,8 +366,7 @@ void DXFImporter::ConvertMeshes(aiScene* pScene, DXF::FileData& output) // ------------------------------------------------------------------------------------------------ -void DXFImporter::ExpandBlockReferences(DXF::Block& bl,const DXF::BlockMap& blocks_by_name) -{ +void DXFImporter::ExpandBlockReferences(DXF::Block& bl,const DXF::BlockMap& blocks_by_name) { for (const DXF::InsertBlock& insert : bl.insertions) { // first check if the referenced blocks exists ... @@ -409,8 +406,7 @@ void DXFImporter::ExpandBlockReferences(DXF::Block& bl,const DXF::BlockMap& bloc } // ------------------------------------------------------------------------------------------------ -void DXFImporter::GenerateMaterials(aiScene* pScene, DXF::FileData& /*output*/) -{ +void DXFImporter::GenerateMaterials(aiScene* pScene, DXF::FileData& /*output*/) { // generate an almost-white default material. Reason: // the default vertex color is GREY, so we are // already at Assimp's usual default color. @@ -435,8 +431,7 @@ void DXFImporter::GenerateMaterials(aiScene* pScene, DXF::FileData& /*output*/) } // ------------------------------------------------------------------------------------------------ -void DXFImporter::GenerateHierarchy(aiScene* pScene, DXF::FileData& /*output*/) -{ +void DXFImporter::GenerateHierarchy(aiScene* pScene, DXF::FileData& /*output*/) { // generate the output scene graph, which is just the root node with a single child for each layer. pScene->mRootNode = new aiNode(); pScene->mRootNode->mName.Set(""); @@ -490,17 +485,17 @@ void DXFImporter::ParseBlock(DXF::LineReader& reader, DXF::FileData& output) { while( !reader.End() && !reader.Is(0,"ENDBLK")) { switch(reader.GroupCode()) { - case 2: + case GroupCode_Name: block.name = reader.Value(); break; - case 10: + case GroupCode_XComp: block.base.x = reader.ValueAsFloat(); break; - case 20: + case GroupCode_YComp: block.base.y = reader.ValueAsFloat(); break; - case 30: + case GroupCode_ZComp: block.base.z = reader.ValueAsFloat(); break; } @@ -527,9 +522,8 @@ void DXFImporter::ParseBlock(DXF::LineReader& reader, DXF::FileData& output) { } // ------------------------------------------------------------------------------------------------ -void DXFImporter::ParseEntities(DXF::LineReader& reader, DXF::FileData& output) -{ - // push a new block onto the stack. +void DXFImporter::ParseEntities(DXF::LineReader& reader, DXF::FileData& output) { + // Push a new block onto the stack. output.blocks.push_back( DXF::Block() ); DXF::Block& block = output.blocks.back(); @@ -559,27 +553,25 @@ void DXFImporter::ParseEntities(DXF::LineReader& reader, DXF::FileData& output) " inserted blocks in ENTITIES" ); } -void DXFImporter::ParseInsertion(DXF::LineReader& reader, DXF::FileData& output) -{ +void DXFImporter::ParseInsertion(DXF::LineReader& reader, DXF::FileData& output) { output.blocks.back().insertions.push_back( DXF::InsertBlock() ); DXF::InsertBlock& bl = output.blocks.back().insertions.back(); while( !reader.End() && !reader.Is(0)) { - switch(reader.GroupCode()) - { + switch(reader.GroupCode()) { // name of referenced block - case 2: + case GroupCode_Name: bl.name = reader.Value(); break; // translation - case 10: + case GroupCode_XComp: bl.pos.x = reader.ValueAsFloat(); break; - case 20: + case GroupCode_YComp: bl.pos.y = reader.ValueAsFloat(); break; - case 30: + case GroupCode_ZComp: bl.pos.z = reader.ValueAsFloat(); break; @@ -706,8 +698,7 @@ void DXFImporter::ParsePolyLine(DXF::LineReader& reader, DXF::FileData& output) #define DXF_VERTEX_FLAG_HAS_POSITIONS 0x40 // ------------------------------------------------------------------------------------------------ -void DXFImporter::ParsePolyLineVertex(DXF::LineReader& reader, DXF::PolyLine& line) -{ +void DXFImporter::ParsePolyLineVertex(DXF::LineReader& reader, DXF::PolyLine& line) { unsigned int cnti = 0, flags = 0; unsigned int indices[4]; @@ -720,8 +711,7 @@ void DXFImporter::ParsePolyLineVertex(DXF::LineReader& reader, DXF::PolyLine& li break; } - switch (reader.GroupCode()) - { + switch (reader.GroupCode()) { case 8: // layer to which the vertex belongs to - assume that // this is always the layer the top-level poly-line @@ -736,15 +726,15 @@ void DXFImporter::ParsePolyLineVertex(DXF::LineReader& reader, DXF::PolyLine& li break; // VERTEX COORDINATES - case 10: + case GroupCode_XComp: out.x = reader.ValueAsFloat(); break; - case 20: + case GroupCode_YComp: out.y = reader.ValueAsFloat(); break; - case 30: + case GroupCode_ZComp: out.z = reader.ValueAsFloat(); break; @@ -780,6 +770,7 @@ void DXFImporter::ParsePolyLineVertex(DXF::LineReader& reader, DXF::PolyLine& li if (indices[i] == 0) { ASSIMP_LOG_WARN("DXF: invalid vertex index, indices are one-based."); --line.counts.back(); + // Workaround to fix issue 2229 if (line.counts.back() == 0) { line.counts.pop_back(); } @@ -821,62 +812,74 @@ void DXFImporter::Parse3DFace(DXF::LineReader& reader, DXF::FileData& output) break; // x position of the first corner - case 10: vip[0].x = reader.ValueAsFloat(); + case 10: + vip[0].x = reader.ValueAsFloat(); b[2] = true; break; // y position of the first corner - case 20: vip[0].y = reader.ValueAsFloat(); + case 20: + vip[0].y = reader.ValueAsFloat(); b[2] = true; break; // z position of the first corner - case 30: vip[0].z = reader.ValueAsFloat(); + case 30: + vip[0].z = reader.ValueAsFloat(); b[2] = true; break; // x position of the second corner - case 11: vip[1].x = reader.ValueAsFloat(); + case 11: + vip[1].x = reader.ValueAsFloat(); b[3] = true; break; // y position of the second corner - case 21: vip[1].y = reader.ValueAsFloat(); + case 21: + vip[1].y = reader.ValueAsFloat(); b[3] = true; break; // z position of the second corner - case 31: vip[1].z = reader.ValueAsFloat(); + case 31: + vip[1].z = reader.ValueAsFloat(); b[3] = true; break; // x position of the third corner - case 12: vip[2].x = reader.ValueAsFloat(); + case 12: + vip[2].x = reader.ValueAsFloat(); b[0] = true; break; // y position of the third corner - case 22: vip[2].y = reader.ValueAsFloat(); + case 22: + vip[2].y = reader.ValueAsFloat(); b[0] = true; break; // z position of the third corner - case 32: vip[2].z = reader.ValueAsFloat(); + case 32: + vip[2].z = reader.ValueAsFloat(); b[0] = true; break; // x position of the fourth corner - case 13: vip[3].x = reader.ValueAsFloat(); + case 13: + vip[3].x = reader.ValueAsFloat(); b[1] = true; break; // y position of the fourth corner - case 23: vip[3].y = reader.ValueAsFloat(); + case 23: + vip[3].y = reader.ValueAsFloat(); b[1] = true; break; // z position of the fourth corner - case 33: vip[3].z = reader.ValueAsFloat(); + case 33: + vip[3].z = reader.ValueAsFloat(); b[1] = true; break; diff --git a/code/DXFLoader.h b/code/DXFLoader.h index 3392b8f63..e7f534e98 100644 --- a/code/DXFLoader.h +++ b/code/DXFLoader.h @@ -50,24 +50,23 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include namespace Assimp { - namespace DXF { - class LineReader; - struct FileData; - struct PolyLine; - struct Block; - struct InsertBlock; - - typedef std::map BlockMap; - } +// Forward declarations +namespace DXF { + class LineReader; + struct FileData; + struct PolyLine; + struct Block; + struct InsertBlock; + typedef std::map BlockMap; +} // --------------------------------------------------------------------------- -/** DXF importer implementation. - * -*/ -class DXFImporter : public BaseImporter -{ +/** + * @brief DXF importer implementation. + */ +class DXFImporter : public BaseImporter { public: DXFImporter(); ~DXFImporter();