Skip to content

feat(scan): Ogre-backed ACMR + per-file cleanup (Phase 6 slice C2) #501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
fernandotonon merged 5 commits into from
May 13, 2026
Merged

feat(scan): Ogre-backed ACMR + per-file cleanup (Phase 6 slice C2) #501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0fb3cde
feat(scan): Ogre-backed ACMR + per-file resource cleanup (Phase 6 sli…
fernandotonon May 13, 2026
0742429
fix(scan): address SonarCloud findings on slice C2
fernandotonon May 13, 2026
8e5065d
fix(scan): log Ogre unload exceptions instead of silently swallowing
fernandotonon May 13, 2026
13f306c
feat(scan): load .mesh files via Ogre instead of Assimp
fernandotonon May 13, 2026
b26b8dd
fix(ci): keep robot.mesh excluded until next release rebuilds action …
fernandotonon May 13, 2026
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
2 changes: 1 addition & 1 deletion CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ Three singletons manage core state. All run on the main thread. Access via `Clas
- Activated via `qtmesh` symlink (created at build time), `--cli` flag, or recognized subcommand as first arg.
- Redirects stdout to stderr (Ogre/Qt noise) and writes CLI output to the original stdout fd. Uses `_exit()` to avoid Ogre static destructor crashes on macOS.
- **AnimationMerger** (`src/AnimationMerger.h/cpp`): Public `renameAnimation()` static method used by both CLI and GUI for animation renaming.
- **ScanEngine** (`src/ScanEngine.h/cpp`): Directory scanner for 3D asset linting. Uses Assimp directly (no Ogre needed) for lightweight metadata extraction. Enumerates files via glob patterns, evaluates configurable rules (format restrictions, size/complexity limits, naming conventions, texture existence), produces text/JSON/SARIF reports. Supports auto-fix for naming convention violations.
- **ScanEngine** (`src/ScanEngine.h/cpp`): Directory scanner for 3D asset linting. Uses Assimp for scene-graph metadata (vertex/face counts, materials, animation timelines, redundant-keyframe analysis) and Ogre for the heavier checks (ACMR via `MeshImporterExporter`, PlayStation TMD/PLY/RSD via the editor's importers, the `--fix` re-import path). Enumerates files via glob patterns, evaluates configurable rules (format restrictions, size/complexity limits, naming conventions, texture existence, `max_acmr`), produces text/JSON/SARIF reports. Per-file cleanup happens in `clearOgreSceneForScanImport` which destroys scene nodes and flushes MeshManager / SkeletonManager so a 1000-asset scan doesn't accumulate state. Supports auto-fix for naming convention violations and redundant-keyframe simplification.
- **ScanConfig** (`src/ScanConfig.h/cpp`): Config loader for `qtmesh.yml`/`.json`. Includes a minimal YAML parser for the specific config schema (scalars, inline/block lists, one level of section nesting). Supports scan paths, rule configuration, fix behavior, and report output settings.

### Mesh Import/Export
Expand Down
6 changes: 5 additions & 1 deletion qtmesh.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ scan:
- "**/*.rsd"
- "**/*.ply"
exclude:
# robot.mesh uses Ogre MeshSerializer v1.40 which Assimp cannot read
# robot.mesh is MeshSerializer v1.40. The local in-tree build reads it
# fine via Ogre 14.5.x; the published 'fernandotonon/qtmesh' Docker image
# used by the scan-assets-qtmesh job ships an older Ogre that only
# supports up to v1.8. Drop this exclude once the next release rebuilds
# the action image against current Ogre.
- "**/robot.mesh"
- "**/build/**"
- "**/.git/**"
Expand Down
254 changes: 213 additions & 41 deletions src/ScanEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include <OgreMaterialManager.h>
#include <OgreMesh.h>
#include <OgreMeshManager.h>
#include <OgreSkeletonManager.h>
#include <OgreSubMesh.h>

#include "PS1/PS1PLY.h"
Expand All @@ -50,9 +51,10 @@

bool ensureOgreHeadlessQuiet()
{
// ScanEngine is normally Assimp-only and shouldn't spam Ogre logs when a fix path
// needs Ogre. Mirror CLIPipeline's default behavior: suppress debug output unless
// the user explicitly asked for verbose logs (ScanEngine has no --verbose flag).
// ScanEngine uses Ogre for fix paths and (slice C2) for ACMR, but doesn't
// need the Ogre log spam in the CLI output. Mirror CLIPipeline's default
// behavior: suppress debug output unless the user explicitly asked for
// verbose logs (ScanEngine has no --verbose flag).
if (!Ogre::LogManager::getSingletonPtr()) {
auto* logMgr = new Ogre::LogManager();
logMgr->createLog("ogre.log", true, false, true); // default, debugOut=false, suppressFile=true
Expand Down Expand Up @@ -103,9 +105,14 @@
}
}

// MeshImporterExporter::importer appends to the scene without clearing prior imports.
// Scan --fix runs multiple FBX imports in one process; stale entities/skeleton
// handles would make entity/skeleton selection wrong and can corrupt Ogre state.
// MeshImporterExporter::importer appends to the scene without clearing prior
// imports. The scan loops over many files in one process — both inspect and
// --fix import each one — so without per-file cleanup stale entities,
// meshes, and skeletons pile up in Ogre's resource managers across a scan
// of a large directory. Clear the scene graph and ask each resource manager
// to drop anything no longer referenced. Wrapped in try/catch because
// unloadUnreferencedResources can throw if some asset is still pinned by
// a resource group's load order.
static void clearOgreSceneForScanImport()
{
if (!Manager::getSingletonPtr())
Expand All @@ -117,6 +124,29 @@
if (sn)
manager->destroySceneNode(sn);
}

// Free per-file resources so a 1000-asset scan doesn't accumulate
// ~N meshes / materials / skeletons in the manager pools. Exceptions
// from the unloads are non-fatal — they typically mean the manager
// still has a reference somewhere (a Skeleton pinned by a Mesh, etc.).
// Log and move on so the next file can still be scanned.
auto* log = Ogre::LogManager::getSingletonPtr()
? Ogre::LogManager::getSingleton().getDefaultLog()
: nullptr;
try {
Ogre::MeshManager::getSingleton().unloadUnreferencedResources(true);
} catch (const Ogre::Exception& e) {
if (log) log->logMessage(
std::string("ScanEngine: MeshManager unload skipped — ") + e.what(),
Ogre::LML_NORMAL);
}
try {
Ogre::SkeletonManager::getSingleton().unloadUnreferencedResources(true);
} catch (const Ogre::Exception& e) {
if (log) log->logMessage(
std::string("ScanEngine: SkeletonManager unload skipped — ") + e.what(),
Ogre::LML_NORMAL);
}
}

} // namespace
Expand Down Expand Up @@ -209,6 +239,38 @@
}
}

// Walk an Ogre skeleton populating boneCount + animation list on AssetInfo.
// Mirrors what the Assimp pass extracts so .mesh + .skeleton assets show the
// same metadata as Assimp-loaded formats. Keyframe count per animation is the
// maximum across node tracks, matching the per-anim "maxKeys" the Assimp path
// computes. Redundant-keyframe analysis is intentionally skipped here — that
// pass is Assimp-channel-specific and not load-bearing for .mesh files.
static void fillAssetInfoFromOgreSkeleton(AssetInfo& info, const Ogre::SkeletonPtr& skel)
{
if (!skel)
return;
info.hasSkeleton = true;
info.boneCount = skel->getNumBones();
for (unsigned short i = 0; i < skel->getNumBones(); ++i)
if (auto* b = skel->getBone(i))

Check warning on line 255 in src/ScanEngine.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 "b" is "class Ogre::Bone *". 


See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ4fV_HmDEwl2Xf2NQjl&open=AZ4fV_HmDEwl2Xf2NQjl&pullRequest=501
info.boneNames.append(QString::fromStdString(b->getName()));

info.animationCount = skel->getNumAnimations();
for (unsigned short a = 0; a < skel->getNumAnimations(); ++a) {
const Ogre::Animation* anim = skel->getAnimation(a);
if (!anim)
continue;
info.animationNames.append(QString::fromStdString(anim->getName()));
info.animationDurations.append(static_cast<double>(anim->getLength()));
unsigned maxKeys = 0;
for (const auto& [handle, track] : anim->_getNodeTrackList()) {
if (track)
maxKeys = std::max(maxKeys, static_cast<unsigned>(track->getNumKeyFrames()));
}
info.animationKeyframeCounts.append(static_cast<int>(maxKeys));
}
}

#ifdef QTMESH_UNIT_TESTS
void ScanEngine::testApplyOgreMeshInspectCounts(AssetInfo& info, const Ogre::MeshPtr& mesh)
{
Expand All @@ -221,6 +283,94 @@
}
#endif

// Phase 6 slice C2: Ogre-backed ACMR. The Assimp face-array walk that used
// to populate AssetInfo::weightedAcmr produced numbers that did not match
// the editor's in-app validator (Assimp re-orders triangles during import,
// so its flattened indices have different cache locality than what Ogre's
// MeshSerializer ultimately ships to the GPU). This helper imports the
// file through MeshImporterExporter — the same path the editor uses — and
// computes ACMR over the actual Ogre index buffers, so scan-side thresholds
// (max_acmr) and in-editor metrics agree on every asset.

// Read an Ogre SubMesh's index buffer into a uint32 vector, transparently
// handling the 16/32-bit variants. Returns empty on non-triangulated or
// degenerate input.
static std::vector<uint32_t> readSubmeshIndexBuffer(const Ogre::SubMesh* sub)
{
std::vector<uint32_t> idxFlat;
if (!sub || !sub->indexData || !sub->indexData->indexBuffer) return idxFlat;
const Ogre::IndexData* id = sub->indexData;
if (id->indexCount < 3 || id->indexCount % 3 != 0) return idxFlat;

const bool use16 = id->indexBuffer->getType() == Ogre::HardwareIndexBuffer::IT_16BIT;
idxFlat.resize(id->indexCount);
const void* src = id->indexBuffer->lock(Ogre::HardwareBuffer::HBL_READ_ONLY);
if (use16) {
const auto* in = static_cast<const uint16_t*>(src);
for (size_t i = 0; i < idxFlat.size(); ++i)
idxFlat[i] = in[id->indexStart + i];
} else {
const auto* in = static_cast<const uint32_t*>(src);
for (size_t i = 0; i < idxFlat.size(); ++i)
idxFlat[i] = in[id->indexStart + i];
}
id->indexBuffer->unlock();
return idxFlat;
}

// Walk one entity's submeshes, accumulating ACMR weighted by triangle count.
static void accumulateEntityAcmr(const Ogre::Entity* entity,
double& weightedSum, unsigned int& totalTris)
{
if (!entity) return;
const Ogre::MeshPtr mesh = entity->getMesh();
if (!mesh) return;
for (unsigned int s = 0; s < mesh->getNumSubMeshes(); ++s) {
const std::vector<uint32_t> idxFlat = readSubmeshIndexBuffer(mesh->getSubMesh(s));
if (idxFlat.empty()) continue;
const double acmr = VertexCacheOptimizer::computeAcmr(idxFlat);
const auto tris = static_cast<unsigned int>(idxFlat.size() / 3);
weightedSum += acmr * tris;
totalTris += tris;
}
}

// Returns true when an ACMR was produced; false on import failure or no
// triangulated submeshes. Clears the scene + unloads Ogre resources on the
// way out so the caller's loop doesn't accumulate state.
static bool computeWeightedAcmrViaOgre(const QString& filePath, AssetInfo& info)
{
if (!ensureOgreHeadlessQuiet())
return false;

clearOgreSceneForScanImport();
// Import via the editor's loader; default flags (0) match what the GUI
// uses for File > Open, so the index order we measure is the order the
// user actually ships.
MeshImporterExporter::importer({QFileInfo(filePath).absoluteFilePath()}, 0);

auto* mgr = Manager::getSingleton();
if (!mgr) return false;
const auto& entities = mgr->getEntities();
if (entities.isEmpty()) {
// Some formats (animation-only FBX) produce no entity; not an error
// for the scan, just nothing to measure ACMR on.
clearOgreSceneForScanImport();
return false;
}

double weightedSum = 0.0;
unsigned int totalTris = 0;
for (const Ogre::Entity* entity : entities)
accumulateEntityAcmr(entity, weightedSum, totalTris);

if (totalTris > 0)
info.weightedAcmr = weightedSum / totalTris;

clearOgreSceneForScanImport();
return totalTris > 0;
}
Comment on lines +341 to +372

@coderabbitai coderabbitai Bot May 13, 2026

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

Make the Ogre ACMR path truly non-fatal.

This helper is documented as best-effort, but any exception after MeshImporterExporter::importer(...) or indexBuffer->lock(...) currently escapes inspectAsset(), skips clearOgreSceneForScanImport(), and can leave the hardware buffer locked. Wrap the import/walk in a try/catch and put both scene cleanup and unlock() behind scope-exit guards so one bad asset only suppresses weightedAcmr.

🤖 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/ScanEngine.cpp` around lines 252 - 315, computeWeightedAcmrViaOgre
currently lets exceptions from MeshImporterExporter::importer or the indexed
buffer lock escape, skipping clearOgreSceneForScanImport and leaving buffers
locked; wrap the import/mesh traversal in a try/catch so exceptions are
swallowed (return false or leave weightedAcmr unset) and always run cleanup, and
add scope-exit guards around each indexBuffer->lock to ensure
indexBuffer->unlock is called even on error; specifically, protect the block
that calls MeshImporterExporter::importer(...) and the loop over
entities/submeshes (where indexBuffer->lock() is used) and ensure
clearOgreSceneForScanImport() is invoked in all paths, while keeping
computeAcmr/weighted sum computation (VertexCacheOptimizer::computeAcmr,
weightedSum/totalTris updates) unchanged.


template<typename ImportFn>
static bool loadAndFillOgreInspect(AssetInfo& info, ImportFn&& importFn, QString* detailErr)
{
Expand Down Expand Up @@ -566,8 +716,12 @@
} // namespace

// ---------------------------------------------------------------------------
// Asset inspection: Assimp for most formats; PlayStation TMD / Psy-Q PLY / RSD
// use the same Ogre importers as the editor (headless Ogre context).
// Asset inspection: Assimp drives scene-graph metadata extraction (vertex /
// face counts, materials, animations, redundant-keyframe analysis); Ogre
// drives ACMR (slice C2) and the PlayStation TMD / Psy-Q PLY / RSD paths.
// Both run through a single headless Ogre context shared across files; per-
// file cleanup happens in clearOgreSceneForScanImport (which also flushes
// MeshManager / SkeletonManager so a 1000-asset scan doesn't pile state).
// ---------------------------------------------------------------------------

bool ScanEngine::isAssimpResultLoadFailure(const aiScene* scene, const char* assimpErrorString,
Expand Down Expand Up @@ -684,6 +838,44 @@
return info;
}

// Ogre native .mesh: load via Ogre's MeshSerializer (the same path the
// editor uses) instead of routing through Assimp. Assimp's .mesh reader
// is limited to v1.41+ and fails on older serializer versions (e.g.
// robot.mesh / v1.40). ACMR is computed below via the shared Ogre path.
if (extLower == QLatin1String("mesh")) {
QString detailErr;
Ogre::MeshPtr loaded;
const bool ok = loadAndFillOgreInspect(
info,
[filePath, &loaded](const std::string& /*mn*/) -> Ogre::MeshPtr {

Check warning on line 850 in src/ScanEngine.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=AZ4fV_HmDEwl2Xf2NQjn&open=AZ4fV_HmDEwl2Xf2NQjn&pullRequest=501
const QFileInfo fi(filePath);
const Ogre::String meshResName = fi.fileName().toStdString();
const Ogre::String meshGroup = fi.absolutePath().toStdString();
Ogre::ResourceGroupManager& rgm = Ogre::ResourceGroupManager::getSingleton();
if (!rgm.resourceGroupExists(meshGroup)) {

Check warning on line 855 in src/ScanEngine.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use the init-statement to declare "rgm" inside the if statement. 


See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ4fV_HmDEwl2Xf2NQjo&open=AZ4fV_HmDEwl2Xf2NQjo&pullRequest=501
rgm.createResourceGroup(meshGroup);
rgm.addResourceLocation(meshGroup, "FileSystem", meshGroup);
rgm.initialiseResourceGroup(meshGroup);
}
if (auto existing = Ogre::MeshManager::getSingleton().getByName(meshResName, meshGroup))
Ogre::MeshManager::getSingleton().remove(existing);
loaded = Ogre::MeshManager::getSingleton().load(meshResName, meshGroup);
return loaded;
},
&detailErr);
if (ok && loaded && loaded->hasSkeleton())

Check warning on line 866 in src/ScanEngine.cpp

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use the init-statement to declare "ok" inside the if statement. 


See more on https://sonarcloud.io/project/issues?id=fernandotonon_QtMeshEditor&issues=AZ4fV_HmDEwl2Xf2NQjm&open=AZ4fV_HmDEwl2Xf2NQjm&pullRequest=501
fillAssetInfoFromOgreSkeleton(info, loaded->getSkeleton());
// Run the standard ACMR path (loads through MeshImporterExporter into
// its own scene). Safe to call after the inspect helper above because
// computeWeightedAcmrViaOgre uses a separate per-scan clear cycle.
computeWeightedAcmrViaOgre(filePath, info);
info.filePath = filePath;
info.relativePath = QDir(scanRoot).relativeFilePath(filePath);
info.format = extLower;
info.fileSize = QFileInfo(filePath).size();
return info;
}

Assimp::Importer importer;
importer.SetPropertyBool(AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS, false);

Expand Down Expand Up @@ -713,10 +905,10 @@
info.materialCount = scene->mNumMaterials;
info.animationCount = scene->mNumAnimations;

// Vertex & face counts + skeleton detection + weighted ACMR
// Vertex / face / skeleton metadata from Assimp's scene graph. ACMR is
// computed via Ogre below (slice C2) so the numbers line up with what
// the editor's in-app validator reports.
std::set<std::string> uniqueBones;
double acmrTriWeightedSum = 0.0;
unsigned int acmrTotalTris = 0;
for (unsigned i = 0; i < scene->mNumMeshes; ++i) {
const aiMesh* mesh = scene->mMeshes[i];
if (!mesh)
Expand All @@ -728,29 +920,7 @@
continue;
uniqueBones.insert(mesh->mBones[b]->mName.C_Str());
}

// Phase 6 slice C: ACMR for cache friendliness. Flatten Assimp's
// per-face indices into a flat uint32 vector and run the same
// pure-data primitive the editor / CLI use. Skip non-triangle
// primitives (point clouds, lines, strips).
std::vector<uint32_t> idxFlat;
idxFlat.reserve(static_cast<size_t>(mesh->mNumFaces) * 3);
for (unsigned f = 0; f < mesh->mNumFaces; ++f) {
const aiFace& face = mesh->mFaces[f];
if (face.mNumIndices != 3) continue;
idxFlat.push_back(face.mIndices[0]);
idxFlat.push_back(face.mIndices[1]);
idxFlat.push_back(face.mIndices[2]);
}
if (!idxFlat.empty()) {
const double acmr = VertexCacheOptimizer::computeAcmr(idxFlat);
const auto tris = static_cast<unsigned int>(idxFlat.size() / 3);
acmrTriWeightedSum += acmr * tris;
acmrTotalTris += tris;
}
}
if (acmrTotalTris > 0)
info.weightedAcmr = acmrTriWeightedSum / acmrTotalTris;
info.boneCount = static_cast<unsigned int>(uniqueBones.size());
info.hasSkeleton = !uniqueBones.empty();
for (const auto& boneName : uniqueBones)
Expand Down Expand Up @@ -823,6 +993,11 @@
if (scene->mNumTextures > 0)
info.hasEmbeddedTextures = true;

// Slice C2: compute ACMR via Ogre so the scan numbers line up with the
// editor's in-app validator. Failures are non-fatal — the rest of the
// AssetInfo stays valid and we just leave weightedAcmr at 0.
computeWeightedAcmrViaOgre(filePath, info);

@chatgpt-codex-connector chatgpt-codex-connector Bot May 13, 2026

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 Handle Ogre ACMR failures instead of silently passing

The new Ogre ACMR path returns false on headless-init/import failures, but inspectAsset ignores that result and leaves weightedAcmr at 0, which makes max_acmr checks silently pass for affected assets. This creates false negatives whenever Ogre can't initialize (e.g., some CI/headless setups) or the Ogre import path fails while Assimp metadata still loads, so scans can report compliant assets even though ACMR was never measured.

Useful? React with 👍 / 👎.


return info;
}

Expand Down Expand Up @@ -982,15 +1157,12 @@
QString("%1 vertices is below minimum of %2")
.arg(asset.vertexCount).arg(config.minVertexCount)});

// ---- max_acmr ---- (Phase 6 slice C)
// ---- max_acmr ---- (Phase 6 slice C / C2)
//
// ACMR is measured on Assimp's flattened triangle list, which does not
// perfectly match the order Ogre's MeshSerializer produces — Assimp's
// numbers tend to run higher than the editor's report. The rule still
// catches the meshes that need a reorder; the exact threshold needs
// calibration to the Assimp pipeline (1.5 is a reasonable starting
// point for "this asset should be reordered"). The Ogre-backed scan
// backend (slice C2) will produce matching numbers when available.
// ACMR is measured on the same Ogre index buffer the editor's in-app
// validator sees, so scan-side numbers match in-editor numbers
// one-for-one. A typical reorder-friendly ceiling is around 1.0; assets
// above that benefit from `qtmesh vertex-cache -o <out>`.
if (config.maxAcmr > 0.0 && asset.weightedAcmr > config.maxAcmr) {
findings.append({asset.relativePath, "max_acmr", Severity::Warning,
QString("ACMR %1 exceeds limit of %2 — reorder index buffer for "
Expand Down
Loading
Loading