Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/AnimationMerger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
#include <QRegularExpression>
#include <cctype>
#include <unordered_map>
#include <vector>
#include <cmath>

// Registry: skeleton name → up-axis (1=Y-up, 2=Z-up).
// Populated by AnimationMerger::registerSkeletonUpAxis() at import time.
Expand Down Expand Up @@ -586,6 +588,8 @@ int AnimationMerger::simplifyAnimation(Ogre::Skeleton* skel,
newAnim->setRotationInterpolationMode(rotInterpMode);

for (const auto& td : tracks) {
if (td.keys.empty())
continue;
auto* newTrack = newAnim->createNodeTrack(td.handle);
if (td.associatedNode)
newTrack->setAssociatedNode(td.associatedNode);
Expand Down
42 changes: 41 additions & 1 deletion src/Assimp/Importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "BoneProcessor.h"
#include "MeshProcessor.h"
#include <algorithm>
#include <string_view>

Ogre::MeshPtr AssimpToOgreImporter::loadModel(const std::string& path, bool convertToLeftHanded, unsigned int additionalFlags) {
skeleton.reset(); // Clear any skeleton from a previous import
Expand All @@ -59,15 +60,54 @@
flags |= aiProcess_ConvertToLeftHanded;
flags |= additionalFlags;

auto pathEndsWithInsensitive = [](const std::string& p, std::string_view suf) -> bool {

Check warning on line 63 in src/Assimp/Importer.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the redundant return type of this lambda.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S0YCRG3_GkEIKTQXa&open=AZ3S0YCRG3_GkEIKTQXa&pullRequest=325
const size_t n = suf.size();
if (p.size() < n)
return false;
for (size_t i = 0; i < n; ++i) {
char a = p[p.size() - n + i];
char b = suf[i];
if (a >= 'A' && a <= 'Z')
a = static_cast<char>(a - 'A' + 'a');
if (b >= 'A' && b <= 'Z')
b = static_cast<char>(b - 'A' + 'a');
if (a != b)
return false;
}
return true;
};

const aiScene* scene = importer.ReadFile(path, flags);
// Do this immediately after ReadFile while the scene is still valid.
m_sceneUpAxis = 1; // default: Y-up
if (scene && scene->mMetaData)
scene->mMetaData->Get("UpAxis", m_sceneUpAxis);

// Some FBX animation takes fail the full post-process stack (null scene or no root)
// but load with a lighter flag set. Retry once before giving up.
if ((!scene || !scene->mRootNode) &&
(pathEndsWithInsensitive(path, ".fbx") || pathEndsWithInsensitive(path, ".fbxa"))) {
unsigned int lightFlags = aiProcess_Triangulate |
aiProcess_ValidateDataStructure |
aiProcess_LimitBoneWeights |
aiProcess_PopulateArmatureData |
aiProcess_GlobalScale;
if (convertToLeftHanded)
lightFlags |= aiProcess_ConvertToLeftHanded;
lightFlags |= additionalFlags;
importer.SetPropertyBool(AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS, false);
scene = importer.ReadFile(path, lightFlags);
m_sceneUpAxis = 1;
if (scene && scene->mMetaData)
scene->mMetaData->Get("UpAxis", m_sceneUpAxis);
}
Comment on lines +88 to +103

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep the retry path genuinely “light.”

lightFlags |= additionalFlags can immediately re-enable the same post-process bits that caused the first import to fail, so the fallback stops being a real fallback as soon as a caller supplies extra Assimp flags. This should carry through only a known-safe subset, not the whole caller mask.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Assimp/Importer.cpp` around lines 88 - 103, The retry path currently ORs
in additionalFlags (lightFlags |= additionalFlags) which can re-enable the
post-process bits that caused the initial failure; change this to only merge a
curated safe subset: define a SAFE_FALLBACK_FLAGS mask containing only
known-safe aiProcess_* bits (e.g., aiProcess_Triangulate,
aiProcess_ValidateDataStructure, aiProcess_LimitBoneWeights,
aiProcess_PopulateArmatureData, aiProcess_GlobalScale and optionally
aiProcess_ConvertToLeftHanded) and replace the OR with lightFlags |=
(additionalFlags & SAFE_FALLBACK_FLAGS) so importer.ReadFile(path, lightFlags)
uses only the permitted fallback flags (referencing lightFlags, additionalFlags,
SAFE_FALLBACK_FLAGS, convertToLeftHanded, and importer.ReadFile).


// A null scene or missing root node is always fatal.
if(!scene || !scene->mRootNode) {
Ogre::LogManager::getSingleton().logError("ERROR::ASSIMP::" + std::string(importer.GetErrorString()));
const char* errStr = importer.GetErrorString();
const std::string errMsg = (errStr && *errStr) ? std::string(errStr)
: std::string("ReadFile failed (no scene / no root node)");
Ogre::LogManager::getSingleton().logError("ERROR::ASSIMP::" + errMsg);
return {};
}
// animationOnly: the scene has no geometry (e.g. Unreal Engine retarget FBX).
Expand Down
37 changes: 31 additions & 6 deletions src/Assimp/MaterialProcessor.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,25 @@
#include "MaterialProcessor.h"
#include "RTShaderHelper.h"

namespace {
static Ogre::Pass* ensureFirstPass(const Ogre::MaterialPtr& mat)

Check warning on line 5 in src/Assimp/MaterialProcessor.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove the redundant "static" specifier.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ3S0YE4G3_GkEIKTQXc&open=AZ3S0YE4G3_GkEIKTQXc&pullRequest=325
{
if (!mat)
return nullptr;
Ogre::Technique* tech = nullptr;
if (mat->getNumTechniques() == 0)
tech = mat->createTechnique();
else
tech = mat->getTechnique(0);

if (!tech)
return nullptr;
if (tech->getNumPasses() == 0)
return tech->createPass();
return tech->getPass(0);
}
} // namespace

void MaterialProcessor::loadScene(const aiScene* scene)
{
for(auto i = 0u; i < scene->mNumMaterials; i++) {
Expand Down Expand Up @@ -42,37 +61,43 @@
}
if(normalTexPtr) {
Ogre::LogManager::getSingleton().logMessage("MaterialProcessor: Applying RTSS normal map '" + normalFilename + "' to existing material '" + materialName + "'");
// Some materials can exist without any techniques/passes (e.g. partially loaded
// script materials). Ensure a valid pass exists before RTSS touches it.
(void)ensureFirstPass(existingMaterial);
applyRTSSNormalMap(existingMaterial, normalTexPtr->getName());
Comment on lines +64 to 67

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard RTSS application when pass creation fails.

At Line 66 the return from ensureFirstPass(existingMaterial) is ignored, but Line 67 still calls applyRTSSNormalMap. If pass creation/retrieval fails, this can reintroduce the crash path you’re trying to eliminate.

Suggested fix
-                (void)ensureFirstPass(existingMaterial);
-                applyRTSSNormalMap(existingMaterial, normalTexPtr->getName());
+                Ogre::Pass* existingPass = ensureFirstPass(existingMaterial);
+                if (!existingPass) {
+                    Ogre::LogManager::getSingleton().logMessage(
+                        "MaterialProcessor: Skipping RTSS normal map for existing material '" + materialName +
+                        "' because no valid pass could be ensured");
+                } else {
+                    applyRTSSNormalMap(existingMaterial, normalTexPtr->getName());
+                }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Assimp/MaterialProcessor.cpp` around lines 64 - 67, ensure the code
checks the result of ensureFirstPass(existingMaterial) before calling
applyRTSSNormalMap: capture the return value from ensureFirstPass (or the pass
pointer it returns) and only call applyRTSSNormalMap(existingMaterial,
normalTexPtr->getName()) when ensureFirstPass indicates success; if
ensureFirstPass fails, skip the RTSS normal-map application (and optionally log
or handle the failure) to avoid dereferencing a missing pass.

}
}
return existingMaterial;
}

Ogre::MaterialPtr ogreMaterial = Ogre::MaterialManager::getSingleton().create(materialName, Ogre::ResourceGroupManager::DEFAULT_RESOURCE_GROUP_NAME);
Ogre::Pass* pass = ensureFirstPass(ogreMaterial);
if (!pass)
return ogreMaterial;

aiColor3D color(0.f, 0.f, 0.f);
if(AI_SUCCESS == material->Get(AI_MATKEY_COLOR_DIFFUSE, color)) {
ogreMaterial->getTechnique(0)->getPass(0)->setDiffuse(color.r, color.g, color.b, 1.0f);
pass->setDiffuse(color.r, color.g, color.b, 1.0f);
}

if(AI_SUCCESS == material->Get(AI_MATKEY_COLOR_AMBIENT, color)) {
// PBR-workflow exporters often set ambient to (0,0,0) which kills ambient
// lighting in Ogre's Phong model. Keep Ogre's default (white) in that case.
if(color.r > 0.001f || color.g > 0.001f || color.b > 0.001f)
ogreMaterial->getTechnique(0)->getPass(0)->setAmbient(color.r, color.g, color.b);
pass->setAmbient(color.r, color.g, color.b);
}

if(AI_SUCCESS == material->Get(AI_MATKEY_COLOR_SPECULAR, color)) {
ogreMaterial->getTechnique(0)->getPass(0)->setSpecular(color.r, color.g, color.b, 1.0f);
pass->setSpecular(color.r, color.g, color.b, 1.0f);
}

if(AI_SUCCESS == material->Get(AI_MATKEY_COLOR_EMISSIVE, color)) {
ogreMaterial->getTechnique(0)->getPass(0)->setSelfIllumination(color.r, color.g, color.b);
pass->setSelfIllumination(color.r, color.g, color.b);
}

float shininess = 0.0f;
if(AI_SUCCESS == material->Get(AI_MATKEY_SHININESS, shininess)) {
ogreMaterial->getTechnique(0)->getPass(0)->setShininess(shininess);
pass->setShininess(shininess);
}

// Handle textures
Expand All @@ -85,7 +110,7 @@
if(!texturePtr){
texturePtr = loadTexture(textureFilename, path, scene);
}
auto* tus = ogreMaterial->getTechnique(0)->getPass(0)->createTextureUnitState(texturePtr->getName());
auto* tus = pass->createTextureUnitState(texturePtr->getName());
tus->setName("diffuse_map");
}

Expand Down
Loading
Loading