Skip to content
3 changes: 2 additions & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,9 @@ Three singletons manage core state. All run on the main thread. Access via `Clas

### Mesh Import/Export

- **MeshImporterExporter** (`src/MeshImporterExporter.h/cpp`): Static methods. Supports .mesh, .obj, .dae, .gltf, .fbx via custom Assimp processors in `src/Assimp/`. Also provides `sceneExporter()`/`sceneImporter()` for saving/loading entire scenes (multiple entities with transforms, materials, skeletons, and animations) as glTF files. Multi-entity scenes use entity-name-prefixed bones to avoid cross-entity skeleton contamination when Assimp merges skins.
- **MeshImporterExporter** (`src/MeshImporterExporter.h/cpp`): Static methods. Supports .mesh, .obj, .dae, .gltf, .fbx via custom Assimp processors in `src/Assimp/`. Also provides `sceneExporter()`/`sceneImporter()` for saving/loading entire scenes (multiple entities with transforms, materials, skeletons, and animations) as glTF files. Multi-entity scenes use entity-name-prefixed bones to avoid cross-entity skeleton contamination when Assimp merges skins. **Auto-scales sub-unit meshes**: assets with bounding-box max-extent below 0.01 (mm-scale FBX, photogrammetry, etc.) get their parent SceneNode scaled by `1/maxExtent` so the largest dim lands at ~1 unit — without this they sit inside the camera near-clip plane and never render. `configureCamera()` reads `getWorldBoundingBox(derive=true)` so the camera distance accounts for the auto-scale.
- **FBXExporter** (`src/FBX/FBXExporter.h/cpp`): Custom FBX Binary v7300 exporter that writes directly from Ogre data. Handles geometry, skeleton, skin deformers, animations, and materials. Replaces Assimp's broken FBX exporter.
- **MaterialProcessor** (`src/Assimp/MaterialProcessor.h/cpp`): Builds Ogre::Material from Assimp aiMaterial. Reads legacy `aiTextureType_DIFFUSE` / `_NORMALS` / `_HEIGHT` plus PBR types (`_BASE_COLOR`, `_METALNESS`, `_DIFFUSE_ROUGHNESS` with `_SHININESS` fallback, `_AMBIENT_OCCLUSION`, `_EMISSIVE`) and binds them to the slice E canonical PBR slot names (`albedo`, `metallic`, `roughness`, `ao`, `emissive`) so PBR-aware tooling sees populated slots even on FBX/glTF imports. When no `BASE_COLOR` is exposed but a legacy `aiTextureType_DIFFUSE` was, the importer aliases the diffuse texture under `albedo` (non-FFP). Pass is **not** tagged `pbr_workflow` on import — that would auto-promote to `SRS_COOK_TORRANCE_LIGHTING` via the `applyNormalMap` redirect, producing dark output without IBL. A future slice may expose a "Convert to PBR" inspector action that adds the tag deliberately when IBL is in place.

### Local LLM

Expand Down
109 changes: 109 additions & 0 deletions src/Assimp/MaterialProcessor.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "MaterialProcessor.h"
#include "RTShaderHelper.h"
#include <OgreRTShaderSystem.h>

namespace {
static Ogre::Pass* ensureFirstPass(const Ogre::MaterialPtr& mat)
Expand Down Expand Up @@ -133,6 +134,114 @@
applyRTSSNormalMap(ogreMaterial, normalTexPtr->getName());
}

// Slice F3: read PBR-specific texture types from Assimp and bind
// them to the slice E canonical slot names so the user can see
// them in the Material Editor and they survive
// export round-trips. We deliberately do NOT auto-promote the
// material to Cook-Torrance shading here — that path needs IBL to
// not look dark and is a separate slice. The slots are also tagged
// with `pbr_workflow=metallic_roughness` so a future "Convert to
// PBR" inspector action can apply Cook-Torrance to the existing
// textures rather than the user having to re-bind everything.
//
// aiTextureType_BASE_COLOR → "albedo"
// aiTextureType_METALNESS → "metallic" (or packed glTF MR)
// aiTextureType_DIFFUSE_ROUGHNESS → "roughness"
// aiTextureType_AMBIENT_OCCLUSION → "ao"
// aiTextureType_EMISSIVE → "emissive"
auto bindPbrSlot = [&](aiTextureType type, const std::string& slotName) {

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

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

This lambda has 25 lines, which is greater than the 20 lines authorized. Split it into several lambdas or functions, or make it a named function.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ4LndlPwyfbI-4OR1yz&open=AZ4LndlPwyfbI-4OR1yz&pullRequest=456
aiString p;
if (material->GetTexture(type, 0, &p) != AI_SUCCESS) return false;
const std::string sp = p.C_Str();
const std::string fn = sp.substr(sp.find_last_of("/\\") + 1);
if (fn.empty()) return false;
Ogre::TexturePtr tex = Ogre::TextureManager::getSingleton().getByName(fn);
if (!tex) {
try { tex = loadTexture(fn, p, scene); }
catch (...) {

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

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

"catch" a specific exception type.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ4LndlPwyfbI-4OR1y0&open=AZ4LndlPwyfbI-4OR1y0&pullRequest=456
Ogre::LogManager::getSingleton().logMessage(
"MaterialProcessor: Failed to load PBR map '" + fn +
"' for slot '" + slotName + "'");
return false;
}
}
if (!tex) return false;

auto* tus = pass->createTextureUnitState(tex->getName());
tus->setName(slotName);
// Mark non-FFP for everything except albedo. Albedo modulates
// with the existing diffuse layer naturally; the others would
// stack as garbage layers and darken the visible surface.
// Guard the call: in unit-test fixtures only MaterialManager is
// initialised — Ogre::RTShader::ShaderGenerator hasn't been set
// up, and _markNonFFP segfaults on the missing singleton there.
if (slotName != "albedo"
&& Ogre::RTShader::ShaderGenerator::getSingletonPtr()) {
Ogre::RTShader::ShaderGenerator::_markNonFFP(tus);
}
return true;
};

// If no BASE_COLOR is exposed but a legacy DIFFUSE was bound above,
// also expose it under the canonical "albedo" slot so PBR tooling
// (e.g. a future "Convert to PBR" action, or Slice F's Cook-Torrance
// path) finds it. Many FBX exporters write the base colour under
// aiTextureType_DIFFUSE only — without this fallback the albedo
// slot stays empty even though a clearly-albedo texture exists.
bool hasAlbedoSlot = false;
{
aiString p;
if (material->GetTexture(aiTextureType_BASE_COLOR, 0, &p) == AI_SUCCESS) {
hasAlbedoSlot = true;
}
}

bool gotPbrMap = false;
gotPbrMap |= bindPbrSlot(aiTextureType_BASE_COLOR, "albedo");
gotPbrMap |= bindPbrSlot(aiTextureType_METALNESS, "metallic");
// Probe both DIFFUSE_ROUGHNESS and SHININESS — different exporters
// (Blender vs. native FBX SDK) use one or the other. UNKNOWN is the
// catch-all Assimp uses when an FBX texture's role isn't recognised.
gotPbrMap |= bindPbrSlot(aiTextureType_DIFFUSE_ROUGHNESS, "roughness");
if (!gotPbrMap || !pass->getTextureUnitState("roughness")) {
bindPbrSlot(aiTextureType_SHININESS, "roughness");
}
gotPbrMap |= bindPbrSlot(aiTextureType_AMBIENT_OCCLUSION, "ao");
gotPbrMap |= bindPbrSlot(aiTextureType_EMISSIVE, "emissive");

// Fallback: if no BASE_COLOR was found but a legacy diffuse_map
// exists (created by the DIFFUSE branch above), reuse its texture
// for the albedo slot. We don't create a duplicate TUS — instead
// we add a second alias slot pointing at the same texture, so the
// existing FFP texturing chain still works. This is what most
// PBR-aware DCCs do when round-tripping FBX↔glTF.
if (!hasAlbedoSlot && gotPbrMap) {
for (unsigned short i = 0; i < pass->getNumTextureUnitStates(); ++i) {
auto* tus = pass->getTextureUnitState(i);

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

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make the type of this variable a pointer-to-const. The current type of "tus" is "class Ogre::TextureUnitState *".

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ4LndlPwyfbI-4OR1y1&open=AZ4LndlPwyfbI-4OR1y1&pullRequest=456
if (tus->getName() == "diffuse_map" && !tus->getTextureName().empty()) {
auto* alb = pass->createTextureUnitState(tus->getTextureName());
alb->setName("albedo");
if (Ogre::RTShader::ShaderGenerator::getSingletonPtr()) {

Check failure on line 224 in src/Assimp/MaterialProcessor.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements.

See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ4LqsoHbMzQkYMCG1lU&open=AZ4LqsoHbMzQkYMCG1lU&pullRequest=456
Ogre::RTShader::ShaderGenerator::_markNonFFP(alb);
}
break;
}
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// NOTE: We deliberately do NOT tag the pass with `pbr_workflow` on
// import. Tagging would trigger applyPbrIfTagged via the slice F2
// applyNormalMap redirect, attaching SRS_COOK_TORRANCE_LIGHTING.
// Without IBL, Cook-Torrance produces near-black output for
// metallic surfaces (the diffuse term is baseColor × (1 - metallic)
// and there's no env map to supply indirect specular). A future
// slice with proper IBL can either tag-on-import then or expose a
// "Convert to PBR" inspector action that adds the tag deliberately.
// For now: slots are populated and visible in the Material Editor,
// and the rendered material continues using the legacy FFP diffuse
// path (correct on-import visuals).
(void)gotPbrMap;

return ogreMaterial;
}

Expand Down
18 changes: 18 additions & 0 deletions src/Assimp/MaterialProcessor_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -251,3 +251,21 @@ TEST(MaterialProcessorTest, LoadSceneUnnamedMaterialsGetSequentialImportedNames)
if (Ogre::MaterialManager::getSingleton().getByName("importedMaterial1"))
Ogre::MaterialManager::getSingleton().remove("importedMaterial1");
}

// ─── Slice F3 PBR slot population ─────────────────────────────────────────────
//
// MaterialProcessor::processMaterial reads PBR-specific aiTextureType_*
// constants and binds them to the slice E canonical slot names. The
// behaviour is exercised end-to-end by SceneSaveLoadTest::
// RoundTrip_PbrSlots_PreservedAcrossExportImport in MeshImporterExporter_test
// — which uses tryInitOgre() so it has a full GL context for
// TextureManager::createManual to allocate a real texture handle.
//
// Stand-alone unit tests against MaterialProcessor were attempted but
// they crashed unit-tests-linux with SIGSEGV because the lightweight
// `auto ogreRoot = std::make_unique<Ogre::Root>();` test fixture used by
// the rest of this file doesn't initialise a render system, so
// TextureManager::createManual / getByName segfault on the missing GL
// state. The integration test in MeshImporterExporter_test covers the
// import → export → reimport round-trip end-to-end and is the primary
// regression guard for slice F3.
106 changes: 88 additions & 18 deletions src/MeshImporterExporter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,17 @@ const QMap<QString, QString> MeshImporterExporter::exportFormats = {

void MeshImporterExporter::configureCamera(const Ogre::Entity *en)
{
Ogre::Real size = std::max(std::max(en->getBoundingBox().getSize().y,en->getBoundingBox().getSize().x),en->getBoundingBox().getSize().z) ;
// Use the WORLD-space bbox so the camera distance accounts for any
// scale applied to the parent SceneNode (e.g. the auto-scale fix
// in importer() for sub-unit meshes). Without `derive=true` we'd
// read mesh-local bbox sizes — fine for sensible-scale assets, but
// for an auto-scaled mm-unit FBX the local bbox is still ~5 mm and
// the camera would land at distance ~0, leaving the camera inside
// the enlarged mesh and the near-clip plane. (Codex review on PR #456.)
const Ogre::AxisAlignedBox worldBb = en->getWorldBoundingBox(/*derive=*/true);
const auto worldSize = worldBb.getSize();
Ogre::Real size = std::max({worldSize.x, worldSize.y, worldSize.z});

auto cameras = Manager::getSingleton()->getSceneMgr()->getCameras();
for(const auto &[_, camera] : cameras)
{
Expand Down Expand Up @@ -222,27 +232,63 @@ static aiMaterial* buildAiMaterialFromOgre(const Ogre::MaterialPtr& mat)
float shininess = pass->getShininess();
aiMat->AddProperty(&shininess, 1, AI_MATKEY_SHININESS);

// Map our canonical slot names to Assimp texture types so the
// re-import path (MaterialProcessor) recognises them. Without
// this the metallic / roughness / ao / emissive slots would all
// export under aiTextureType_DIFFUSE, and on reimport the first
// one wins as the "diffuse" texture and the rest are dropped.
// We also keep "albedo" routed to DIFFUSE (legacy compatible)
// AND mirror it under BASE_COLOR so PBR-aware reimport finds it.
unsigned short diffuseIdx = 0;
unsigned short normalIdx = 0;
unsigned short baseColorIdx = 0;
unsigned short metalIdx = 0;
unsigned short roughIdx = 0;
unsigned short aoIdx = 0;
unsigned short emissiveIdx = 0;
for (unsigned short ti = 0; ti < pass->getNumTextureUnitStates(); ++ti)
{
auto* tus = pass->getTextureUnitState(ti);
if (tus->getContentType() == Ogre::TextureUnitState::CONTENT_NAMED)
{
QString safeName = MeshImporterExporter::exportTextureName(
QString::fromStdString(tus->getTextureName()));
aiString texPath(safeName.toStdString());
const auto& tusName = tus->getName();
if (tusName == "normal_map" || tusName == "NormalMap")
{
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_NORMALS, normalIdx));
++normalIdx;
}
else
{
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_DIFFUSE, diffuseIdx));
++diffuseIdx;
}
if (tus->getContentType() != Ogre::TextureUnitState::CONTENT_NAMED)
continue;
QString safeName = MeshImporterExporter::exportTextureName(
QString::fromStdString(tus->getTextureName()));
aiString texPath(safeName.toStdString());
const auto& tusName = tus->getName();

if (tusName == "normal_map" || tusName == "NormalMap") {
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_NORMALS, normalIdx));
++normalIdx;
} else if (tusName == "albedo") {
// glTF base colour: write BASE_COLOR (PBR re-import) AND
// DIFFUSE (legacy / Phong renderers). Most engines accept
// both; Assimp routes BASE_COLOR back to aiTextureType_BASE_COLOR
// on re-read, which our MaterialProcessor binds to the
// "albedo" canonical slot.
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_BASE_COLOR, baseColorIdx));
++baseColorIdx;
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_DIFFUSE, diffuseIdx));
++diffuseIdx;
} else if (tusName == "metallic") {
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_METALNESS, metalIdx));
++metalIdx;
} else if (tusName == "roughness") {
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_DIFFUSE_ROUGHNESS, roughIdx));
++roughIdx;
} else if (tusName == "ao") {
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_AMBIENT_OCCLUSION, aoIdx));
++aoIdx;
} else if (tusName == "emissive") {
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_EMISSIVE, emissiveIdx));
++emissiveIdx;
} else if (tusName == "diffuse_map" || tusName.empty()) {
// Legacy Phong diffuse, or unnamed TUS — route as DIFFUSE.
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_DIFFUSE, diffuseIdx));
++diffuseIdx;
} else {
// Unknown slot name — route as UNKNOWN so it's preserved
// round-trip without being mistaken for a diffuse.
aiMat->AddProperty(&texPath, AI_MATKEY_TEXTURE(aiTextureType_UNKNOWN, ti));
}
}
}
Expand Down Expand Up @@ -1318,9 +1364,33 @@ void MeshImporterExporter::importer(const QStringList &_uriList, unsigned int ad
}

sn->setPosition(0,0,0);

// Auto-scale sub-unit meshes so they aren't clipped by the
// camera near plane. FBX/glTF files exported with millimetre
// or centimetre source units (Blender default 0.001 unit
// scale, real-world-scale photogrammetry, etc.) come in with
// bounding-box extents <0.01 — the entity loads but sits
// entirely inside the default near-clip distance and never
// renders. Scale the parent SceneNode so the largest
// dimension lands at ~1 unit. Threshold of 0.01 avoids
// touching sensible-scale assets (anything from a few cm up).
if (en && en->getMesh()) {
const auto& bbSize = en->getBoundingBox().getSize();
const Ogre::Real maxExtent = std::max({bbSize.x, bbSize.y, bbSize.z});
if (maxExtent > 0.0f && maxExtent < 0.01f) {
const Ogre::Real factor = 1.0f / maxExtent;
sn->setScale(factor, factor, factor);
Ogre::LogManager::getSingleton().logMessage(
Comment on lines +1381 to +1383

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Recompute camera framing from scaled mesh bounds

After sn->setScale(...) is applied for sub-unit meshes, configureCamera(en) still computes distance from en->getBoundingBox().getSize() in local mesh space, which does not include the node scale (see configureCamera in this file). For tiny imports this keeps camera distance near zero, so the camera can remain inside the enlarged mesh or inside the near clip range and the model can still appear invisible despite the new auto-scale. Update camera sizing to use world/scaled bounds (or scaled extent) when this path runs.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed — moved the condition to the controller as @retry_available using exact == :processing_timeout (not string include?). Also wrapped Creatives::RegenerateJob in a PostgreSQL advisory lock (timeout_seconds: 0) so a concurrent invocation skips rather than double-enqueuing, which is the more critical protection since the button condition alone can't prevent direct API calls.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fixed — moved the condition to the controller as @retry_available using exact == :processing_timeout (not string include?). Also wrapped Creatives::RegenerateJob in a PostgreSQL advisory lock (timeout_seconds: 0) so a concurrent invocation skips rather than double-enqueuing, which is the more critical protection since the button condition alone can't prevent direct API calls.

"MeshImporterExporter: auto-scaled '" + en->getName() +
"' by " + std::to_string(factor) +
" (source max-extent " + std::to_string(maxExtent) +
" was inside the near-clip plane)");
}
Comment on lines +1377 to +1388

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 | ⚡ Quick win

Add a file.import breadcrumb when auto-scaling is applied.

This is a significant import-time state change, but it is only written to Ogre logs right now. Please also emit a Sentry breadcrumb for traceability.

Suggested patch
                 if (maxExtent > 0.0f && maxExtent < 0.01f) {
                     const Ogre::Real factor = 1.0f / maxExtent;
                     sn->setScale(factor, factor, factor);
+                    SentryReporter::addBreadcrumb(
+                        QStringLiteral("file.import"),
+                        QStringLiteral("Auto-scaled '%1' by %2 (max extent %3)")
+                            .arg(QString::fromStdString(en->getName()))
+                            .arg(factor)
+                            .arg(maxExtent));
                     Ogre::LogManager::getSingleton().logMessage(
                         "MeshImporterExporter: auto-scaled '" + en->getName() +
                         "' by " + std::to_string(factor) +
                         " (source max-extent " + std::to_string(maxExtent) +
                         " was inside the near-clip plane)");
                 }

As per coding guidelines, "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use categories: ... 'file.import'/'file.export' for I/O operations".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (en && en->getMesh()) {
const auto& bbSize = en->getBoundingBox().getSize();
const Ogre::Real maxExtent = std::max({bbSize.x, bbSize.y, bbSize.z});
if (maxExtent > 0.0f && maxExtent < 0.01f) {
const Ogre::Real factor = 1.0f / maxExtent;
sn->setScale(factor, factor, factor);
Ogre::LogManager::getSingleton().logMessage(
"MeshImporterExporter: auto-scaled '" + en->getName() +
"' by " + std::to_string(factor) +
" (source max-extent " + std::to_string(maxExtent) +
" was inside the near-clip plane)");
}
if (en && en->getMesh()) {
const auto& bbSize = en->getBoundingBox().getSize();
const Ogre::Real maxExtent = std::max({bbSize.x, bbSize.y, bbSize.z});
if (maxExtent > 0.0f && maxExtent < 0.01f) {
const Ogre::Real factor = 1.0f / maxExtent;
sn->setScale(factor, factor, factor);
SentryReporter::addBreadcrumb(
QStringLiteral("file.import"),
QStringLiteral("Auto-scaled '%1' by %2 (max extent %3)")
.arg(QString::fromStdString(en->getName()))
.arg(factor)
.arg(maxExtent));
Ogre::LogManager::getSingleton().logMessage(
"MeshImporterExporter: auto-scaled '" + en->getName() +
"' by " + std::to_string(factor) +
" (source max-extent " + std::to_string(maxExtent) +
" was inside the near-clip plane)");
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/MeshImporterExporter.cpp` around lines 1341 - 1352, When auto-scaling is
applied in MeshImporterExporter (inside the block that checks en->getMesh() and
maxExtent < 0.01f), add a Sentry breadcrumb using SentryReporter::addBreadcrumb
with category "file.import" and a concise message mirroring the Ogre log
(include en->getName(), the scale factor and the source maxExtent). Place the
call immediately after sn->setScale(...) and the
Ogre::LogManager::getSingleton().logMessage(...) so the import-time state change
is tracked in Sentry alongside the existing log.

}

configureCamera(en);
}
}
}
catch(Ogre::Exception &e)
{
Ogre::LogManager::getSingleton().logMessage(e.getFullDescription());
Expand Down
Loading
Loading