From f9563519428797a3a34b4a84cd8305cf479d7dda Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Tue, 30 Jan 2024 22:14:18 +0100 Subject: [PATCH 1/2] Fix crash in viewer (#5446) --- tools/assimp_view/Display.cpp | 73 ++++++++++++++--------------------- 1 file changed, 30 insertions(+), 43 deletions(-) diff --git a/tools/assimp_view/Display.cpp b/tools/assimp_view/Display.cpp index 95ed41615..5c2ee8ff5 100644 --- a/tools/assimp_view/Display.cpp +++ b/tools/assimp_view/Display.cpp @@ -518,20 +518,19 @@ int CDisplay::AddTextureToDisplayList(unsigned int iType, return 1; } //------------------------------------------------------------------------------- -int CDisplay::AddMaterialToDisplayList(HTREEITEM hRoot, - unsigned int iIndex) -{ +int CDisplay::AddMaterialToDisplayList(HTREEITEM hRoot, unsigned int iIndex) { ai_assert(nullptr != hRoot); aiMaterial* pcMat = g_pcAsset->pcScene->mMaterials[iIndex]; + if (g_pcAsset->pcScene->mNumMeshes == 0) { + return -1; + } // find the first mesh using this material index unsigned int iMesh = 0; - for (unsigned int i = 0; i < g_pcAsset->pcScene->mNumMeshes;++i) - { - if (iIndex == g_pcAsset->pcScene->mMeshes[i]->mMaterialIndex) - { + for (unsigned int i = 0; i < g_pcAsset->pcScene->mNumMeshes;++i) { + if (iIndex == g_pcAsset->pcScene->mMeshes[i]->mMaterialIndex) { iMesh = i; break; } @@ -540,12 +539,9 @@ int CDisplay::AddMaterialToDisplayList(HTREEITEM hRoot, // use the name of the material, if possible char chTemp[512]; aiString szOut; - if (AI_SUCCESS != aiGetMaterialString(pcMat,AI_MATKEY_NAME,&szOut)) - { + if (AI_SUCCESS != aiGetMaterialString(pcMat,AI_MATKEY_NAME,&szOut)) { ai_snprintf(chTemp,512,"Material %i",iIndex+1); - } - else - { + } else { ai_snprintf(chTemp,512,"%s (%i)",szOut.data,iIndex+1); } TVITEMEXW tvi; @@ -577,17 +573,15 @@ int CDisplay::AddMaterialToDisplayList(HTREEITEM hRoot, aiTextureOp eOp; aiString szPath; bool bNoOpacity = true; - for (unsigned int i = 0; i <= AI_TEXTURE_TYPE_MAX;++i) - { + for (unsigned int i = 0; i <= AI_TEXTURE_TYPE_MAX;++i) { unsigned int iNum = 0; - while (true) - { - if (AI_SUCCESS != aiGetMaterialTexture(pcMat,(aiTextureType)i,iNum, - &szPath,nullptr, &iUV,&fBlend,&eOp)) - { + while (true) { + if (AI_SUCCESS != aiGetMaterialTexture(pcMat,(aiTextureType)i,iNum, &szPath,nullptr, &iUV,&fBlend,&eOp)) { break; } - if (aiTextureType_OPACITY == i)bNoOpacity = false; + if (aiTextureType_OPACITY == i) { + bNoOpacity = false; + } AddTextureToDisplayList(i,iNum,&szPath,hTexture,iUV,fBlend,eOp,iMesh); ++iNum; } @@ -595,8 +589,7 @@ int CDisplay::AddMaterialToDisplayList(HTREEITEM hRoot, AssetHelper::MeshHelper* pcMesh = g_pcAsset->apcMeshes[iMesh]; - if (pcMesh->piDiffuseTexture && pcMesh->piDiffuseTexture == pcMesh->piOpacityTexture && bNoOpacity) - { + if (pcMesh->piDiffuseTexture && pcMesh->piDiffuseTexture == pcMesh->piOpacityTexture && bNoOpacity) { // check whether the diffuse texture is not a default texture // {9785DA94-1D96-426b-B3CB-BADC36347F5E} @@ -606,9 +599,7 @@ int CDisplay::AddMaterialToDisplayList(HTREEITEM hRoot, uint32_t iData = 0; DWORD dwSize = 4; - if(FAILED( pcMesh->piDiffuseTexture->GetPrivateData(guidPrivateData,&iData,&dwSize) || - 0xffffffff == iData)) - { + if(FAILED( pcMesh->piDiffuseTexture->GetPrivateData(guidPrivateData,&iData,&dwSize) || 0xffffffff == iData)) { // seems the diffuse texture contains alpha, therefore it has been // added to the opacity channel, too. Add a special value ... AddTextureToDisplayList(aiTextureType_OPACITY | 0x40000000, @@ -625,33 +616,26 @@ int CDisplay::AddMaterialToDisplayList(HTREEITEM hRoot, this->AddMaterial(info); return 1; } + //------------------------------------------------------------------------------- // Expand all elements in the tree-view -int CDisplay::ExpandTree() -{ +int CDisplay::ExpandTree() { // expand all materials - for (std::vector< MaterialInfo >::iterator - i = m_asMaterials.begin(); - i != m_asMaterials.end();++i) - { + for (std::vector< MaterialInfo >::iterator i = m_asMaterials.begin(); i != m_asMaterials.end();++i) { TreeView_Expand(GetDlgItem(g_hDlg,IDC_TREE1),(*i).hTreeItem,TVE_EXPAND); } // expand all nodes - for (std::vector< NodeInfo >::iterator - i = m_asNodes.begin(); - i != m_asNodes.end();++i) - { + for (std::vector< NodeInfo >::iterator i = m_asNodes.begin(); i != m_asNodes.end();++i) { TreeView_Expand(GetDlgItem(g_hDlg,IDC_TREE1),(*i).hTreeItem,TVE_EXPAND); } TreeView_Expand(GetDlgItem(g_hDlg,IDC_TREE1),m_hRoot,TVE_EXPAND); return 1; } + //------------------------------------------------------------------------------- // Get image list for tree view -int CDisplay::LoadImageList(void) -{ - if (!m_hImageList) - { +int CDisplay::LoadImageList() { + if (!m_hImageList) { // First, create the image list we will need. // FIX: Need RGB888 color space to display all colors correctly HIMAGELIST hIml = ImageList_Create( 16,16,ILC_COLOR24, 5, 0 ); @@ -682,12 +666,13 @@ int CDisplay::LoadImageList(void) m_hImageList = hIml; } + return 1; } + //------------------------------------------------------------------------------- // Fill tree view -int CDisplay::FillDisplayList(void) -{ +int CDisplay::FillDisplayList(void) { LoadImageList(); // Initialize the tree view window. @@ -713,11 +698,11 @@ int CDisplay::FillDisplayList(void) (LPARAM)(LPTVINSERTSTRUCT)&sNew); // add each loaded material to the tree - for (unsigned int i = 0; i < g_pcAsset->pcScene->mNumMaterials;++i) + for (unsigned int i = 0; i < g_pcAsset->pcScene->mNumMaterials; ++i) AddMaterialToDisplayList(m_hRoot,i); // add each mesh to the tree - for (unsigned int i = 0; i < g_pcAsset->pcScene->mNumMeshes;++i) + for (unsigned int i = 0; i < g_pcAsset->pcScene->mNumMeshes; ++i) AddMeshToDisplayList(i,m_hRoot); // now add all loaded nodes recursively @@ -729,8 +714,10 @@ int CDisplay::FillDisplayList(void) // everything reacts a little bit slowly if D3D is rendering, // so give GDI a small hint to leave the couch and work ;-) UpdateWindow(g_hDlg); + return 1; } + //------------------------------------------------------------------------------- // Main render loop int CDisplay::OnRender() From 3476c798016d6db6dce6682ea05f87f4683651aa Mon Sep 17 00:00:00 2001 From: Kim Kulling Date: Wed, 31 Jan 2024 09:30:54 +0100 Subject: [PATCH 2/2] Static code analysis fixes (#5447) * Static code analysis fixes - Fix warning in LOW * Fix possible out of bound access. * Add default to class declaration --- code/AssetLib/Irr/IRRMeshLoader.cpp | 35 +++++++++++------------------ code/AssetLib/Irr/IRRMeshLoader.h | 9 +++++--- code/AssetLib/LWS/LWSLoader.cpp | 14 ++++++------ 3 files changed, 26 insertions(+), 32 deletions(-) diff --git a/code/AssetLib/Irr/IRRMeshLoader.cpp b/code/AssetLib/Irr/IRRMeshLoader.cpp index 639b4fff9..4a2f70882 100644 --- a/code/AssetLib/Irr/IRRMeshLoader.cpp +++ b/code/AssetLib/Irr/IRRMeshLoader.cpp @@ -3,7 +3,7 @@ Open Asset Import Library (assimp) --------------------------------------------------------------------------- -Copyright (c) 2006-2022, assimp team +Copyright (c) 2006-2024, assimp team All rights reserved. @@ -69,14 +69,6 @@ static constexpr aiImporterDesc desc = { "xml irrmesh" }; -// ------------------------------------------------------------------------------------------------ -// Constructor to be privately used by Importer -IRRMeshImporter::IRRMeshImporter() = default; - -// ------------------------------------------------------------------------------------------------ -// Destructor, private as well -IRRMeshImporter::~IRRMeshImporter() = default; - // ------------------------------------------------------------------------------------------------ // Returns whether the class can handle the format of the given file. bool IRRMeshImporter::CanRead(const std::string &pFile, IOSystem *pIOHandler, bool /*checkSig*/) const { @@ -116,8 +108,9 @@ void IRRMeshImporter::InternReadFile(const std::string &pFile, std::unique_ptr file(pIOHandler->Open(pFile)); // Check whether we can read from the file - if (file == nullptr) + if (file == nullptr) { throw DeadlyImportError("Failed to open IRRMESH file ", pFile); + } // Construct the irrXML parser XmlParser parser; @@ -148,13 +141,11 @@ void IRRMeshImporter::InternReadFile(const std::string &pFile, // int vertexFormat = 0; // 0 = normal; 1 = 2 tcoords, 2 = tangents bool useColors = false; - /* - ** irrmesh files have a top level owning multiple nodes. - ** Each contains , , and - ** tags here directly owns the material data specs - ** are a vertex per line, contains position, UV1 coords, maybe UV2, normal, tangent, bitangent - ** is ignored, I think assimp recalculates those? - */ + // irrmesh files have a top level owning multiple nodes. + // Each contains , , and + // tags here directly owns the material data specs + // are a vertex per line, contains position, UV1 coords, maybe UV2, normal, tangent, bitangent + // is ignored, I think assimp recalculates those? // Parse the XML file pugi::xml_node const &meshNode = root.child("mesh"); @@ -201,7 +192,6 @@ void IRRMeshImporter::InternReadFile(const std::string &pFile, // This is possible ... remove the mesh from the list and skip further reading ASSIMP_LOG_WARN("IRRMESH: Found mesh with zero vertices"); releaseMaterial(&curMat); - // releaseMesh(&curMesh); continue; // Bail out early }; @@ -331,7 +321,8 @@ void IRRMeshImporter::InternReadFile(const std::string &pFile, // NOTE this might explode for UTF-16 and wchars const char *sz = indicesNode.text().get(); - const char *end = sz + std::strlen(sz) + 1; + const char *end = sz + std::strlen(sz); + // For each index loop over aiMesh faces while (SkipSpacesAndLineEnd(&sz, end)) { if (curFace >= faceEnd) { @@ -377,8 +368,9 @@ void IRRMeshImporter::InternReadFile(const std::string &pFile, } } // We should be at the end of mFaces - if (curFace != faceEnd) + if (curFace != faceEnd) { ASSIMP_LOG_ERROR("IRRMESH: Not enough indices"); + } } // Finish processing the mesh - do some small material workarounds @@ -388,8 +380,7 @@ void IRRMeshImporter::InternReadFile(const std::string &pFile, aiMaterial *mat = (aiMaterial *)curMat; mat->AddProperty(&curColors[0].a, 1, AI_MATKEY_OPACITY); } - // textMeaning = 2; - + // end of previous buffer. A material and a mesh should be there if (!curMat || !curMesh) { ASSIMP_LOG_ERROR("IRRMESH: A buffer must contain a mesh and a material"); diff --git a/code/AssetLib/Irr/IRRMeshLoader.h b/code/AssetLib/Irr/IRRMeshLoader.h index 9ec5c983d..4ab3615ee 100644 --- a/code/AssetLib/Irr/IRRMeshLoader.h +++ b/code/AssetLib/Irr/IRRMeshLoader.h @@ -2,7 +2,7 @@ Open Asset Import Library (assimp) ---------------------------------------------------------------------- -Copyright (c) 2006-2022, assimp team +Copyright (c) 2006-2024, assimp team All rights reserved. @@ -62,8 +62,11 @@ namespace Assimp { */ class IRRMeshImporter : public BaseImporter, public IrrlichtBase { public: - IRRMeshImporter(); - ~IRRMeshImporter() override; + /// @brief The class constructor. + IRRMeshImporter() = default; + + /// @brief The class destructor. + ~IRRMeshImporter() override = default; // ------------------------------------------------------------------- /** Returns whether the class can handle the format of the given file. diff --git a/code/AssetLib/LWS/LWSLoader.cpp b/code/AssetLib/LWS/LWSLoader.cpp index bf6e9ee31..4c3a44785 100644 --- a/code/AssetLib/LWS/LWSLoader.cpp +++ b/code/AssetLib/LWS/LWSLoader.cpp @@ -3,7 +3,7 @@ Open Asset Import Library (assimp) --------------------------------------------------------------------------- -Copyright (c) 2006-2022, assimp team +Copyright (c) 2006-2024, assimp team All rights reserved. @@ -155,7 +155,8 @@ const aiImporterDesc *LWSImporter::GetInfo() const { } static constexpr int MagicHackNo = 150392; - // ------------------------------------------------------------------------------------------------ + +// ------------------------------------------------------------------------------------------------ // Setup configuration properties void LWSImporter::SetupProperties(const Importer *pImp) { // AI_CONFIG_FAVOUR_SPEED @@ -248,13 +249,12 @@ void LWSImporter::ReadEnvelope(const LWS::Element &dad, LWO::Envelope &fill) { // Read animation channels in the old LightWave animation format void LWSImporter::ReadEnvelope_Old(std::list::const_iterator &it,const std::list::const_iterator &endIt, LWS::NodeDesc &nodes, unsigned int) { - unsigned int num=0, sub_num=0; if (++it == endIt) { ASSIMP_LOG_ERROR("LWS: Encountered unexpected end of file while parsing object motion"); return; } - num = strtoul10((*it).tokens[0].c_str()); + const unsigned int num = strtoul10((*it).tokens[0].c_str()); for (unsigned int i = 0; i < num; ++i) { nodes.channels.emplace_back(); LWO::Envelope &envl = nodes.channels.back(); @@ -266,8 +266,8 @@ void LWSImporter::ReadEnvelope_Old(std::list::const_iterator &it,c ASSIMP_LOG_ERROR("LWS: Encountered unexpected end of file while parsing object motion"); return; } - sub_num = strtoul10((*it).tokens[0].c_str()); - + + const unsigned int sub_num = strtoul10((*it).tokens[0].c_str()); for (unsigned int n = 0; n < sub_num; ++n) { if (++it == endIt) { ASSIMP_LOG_ERROR("LWS: Encountered unexpected end of file while parsing object motion"); @@ -283,7 +283,7 @@ void LWSImporter::ReadEnvelope_Old(std::list::const_iterator &it,c fast_atoreal_move((*it).tokens[0].c_str(), f); key.time = f; - envl.keys.push_back(key); + envl.keys.emplace_back(key); } } }