feat(scan): Ogre-backed ACMR + per-file cleanup (Phase 6 slice C2) - #501
Merged
Conversation
…ce C2)
`qtmesh scan` previously reported ACMR from Assimp's flattened triangle
list, which has different cache locality than what Ogre's MeshSerializer
ships to the GPU. The numbers didn't match the in-editor validator on
the same asset (e.g. ninja.mesh read 3.0 in scan vs 0.93 in the editor),
forcing operators to calibrate two separate ceilings.
This commit pipes ACMR through Ogre via the editor's MeshImporterExporter
so the scan sees the same index order the user actually ships. On
ninja.mesh both `qtmesh scan` and `qtmesh vertex-cache` now print
ACMR 0.932 — one ceiling, one truth.
ScanEngine changes:
- New `computeWeightedAcmrViaOgre()` helper imports each asset through
MeshImporterExporter, walks every entity's submesh index buffer (16/
32-bit aware), runs `VertexCacheOptimizer::computeAcmr` on the flat
uint32 list, weight-averages by triangle count, and writes the result
to `AssetInfo::weightedAcmr`. Failures are non-fatal — the rest of the
AssetInfo stays valid and we just leave weightedAcmr at 0.
- `inspectAsset`'s Assimp face-array ACMR loop is gone. Vertex / face /
skeleton metadata still come from Assimp's scene graph; ACMR is
exclusively Ogre's job now.
- `clearOgreSceneForScanImport()` now also flushes
`MeshManager::unloadUnreferencedResources(true)` and
`SkeletonManager::unloadUnreferencedResources(true)` so scanning 1000
assets in one process doesn't pile state. Wrapped in try/catch
because unload can throw if some resource is still pinned.
Documentation cleanup (per user direction — Assimp-only was historical,
not deliberate):
- CLAUDE.md: dropped "lightweight Assimp-only metadata extraction"
framing. Replaced with the actual architecture: Assimp for metadata,
Ogre for ACMR + PS1 + --fix, shared headless context with per-file
cleanup.
- DocsApp performance-concepts paragraph: removed the "scan numbers run
higher than the in-app validator" caveat. They match now. Calibration
hint updated from 1.5 → 1.0 (same ceiling the editor uses).
- DocsApp max_acmr RuleCard: same update.
- ScanEngine source comments: removed the "future Ogre-backed scan
backend will reconcile the numbers" placeholder language since this
IS that backend.
Manual smoke (numbers match exactly):
qtmesh scan media/models --max-acmr 0.8 --include "**/*.mesh"
-> [warn] max_acmr: ACMR 0.932 exceeds limit of 0.800
qtmesh vertex-cache media/models/ninja.mesh
-> Weighted ACMR: 0.932 → 0.841
What's NOT in this PR (intentional scope limit, captured for a future
C3 slice): full conversion of vertex/face/material/animation extraction
to walk Ogre's scene graph instead of Assimp's aiScene. The current
path is hybrid by design — Assimp's scene-graph traversal is well-tuned
and the redundant-keyframe analyzer (which lives in ScanEngine, not
AnimationMerger) operates on Assimp's animation channels. Migrating
those to Ogre's Skeleton::getAnimation() is a separate ~500-LoC
refactor with its own test coverage; this slice focuses on the
operator-facing problem (ACMR numbers don't agree).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughACMR measurement moved from Assimp to an Ogre-backed path (via MeshImporterExporter); per-file Ogre resource unloads and skeleton metadata extraction were added; inspectAsset now calls computeWeightedAcmrViaOgre and native ChangesOgre ACMR migration + docs/config
Sequence DiagramsequenceDiagram
participant inspectAsset
participant AssimpPass
participant computeWeightedAcmrViaOgre
participant MeshImporterExporter
participant OgreScene
participant MeshManager
participant SkeletonManager
inspectAsset->>AssimpPass: extract metadata (bones, animations)
inspectAsset->>computeWeightedAcmrViaOgre: filePath, info
computeWeightedAcmrViaOgre->>MeshImporterExporter: re-import asset
MeshImporterExporter->>OgreScene: load meshes/entities
computeWeightedAcmrViaOgre->>OgreScene: lock/read index buffers, compute ACMR
computeWeightedAcmrViaOgre->>MeshManager: unloadUnreferencedResources(true)
computeWeightedAcmrViaOgre->>SkeletonManager: unloadUnreferencedResources(true)
computeWeightedAcmrViaOgre->>inspectAsset: set info.weightedAcmr (if available)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fb3cde2bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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); |
There was a problem hiding this comment.
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 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/ScanEngine.cpp`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4eaa4ae8-84eb-4d5b-ba83-533268b066cc
📒 Files selected for processing (3)
CLAUDE.mdsrc/ScanEngine.cppwebsite/src/DocsApp.jsx
Comment on lines
+252
to
+315
| 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 (Ogre::Entity* entity : entities) { | ||
| if (!entity) continue; | ||
| const Ogre::MeshPtr mesh = entity->getMesh(); | ||
| if (!mesh) continue; | ||
| for (unsigned int s = 0; s < mesh->getNumSubMeshes(); ++s) { | ||
| const Ogre::SubMesh* sub = mesh->getSubMesh(s); | ||
| if (!sub || !sub->indexData || !sub->indexData->indexBuffer) continue; | ||
| if (sub->indexData->indexCount < 3) continue; | ||
| if (sub->indexData->indexCount % 3 != 0) continue; | ||
|
|
||
| const bool use16 = sub->indexData->indexBuffer->getType() | ||
| == Ogre::HardwareIndexBuffer::IT_16BIT; | ||
| std::vector<uint32_t> idxFlat; | ||
| idxFlat.resize(sub->indexData->indexCount); | ||
| const void* src = sub->indexData->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[sub->indexData->indexStart + i]; | ||
| } else { | ||
| const auto* in = static_cast<const uint32_t*>(src); | ||
| for (size_t i = 0; i < idxFlat.size(); ++i) | ||
| idxFlat[i] = in[sub->indexData->indexStart + i]; | ||
| } | ||
| sub->indexData->indexBuffer->unlock(); | ||
|
|
||
| const double acmr = VertexCacheOptimizer::computeAcmr(idxFlat); | ||
| const auto tris = static_cast<unsigned int>(idxFlat.size() / 3); | ||
| weightedSum += acmr * tris; | ||
| totalTris += tris; | ||
| } | ||
| } | ||
|
|
||
| if (totalTris > 0) | ||
| info.weightedAcmr = weightedSum / totalTris; | ||
|
|
||
| clearOgreSceneForScanImport(); | ||
| return totalTris > 0; | ||
| } |
There was a problem hiding this comment.
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.
- S108/S2486/S2738: empty catch blocks now scope to Ogre::Exception with an explanatory comment instead of swallowing every exception type unannotated. - S3776/S134: extract readSubmeshIndexBuffer() and accumulateEntityAcmr() helpers so computeWeightedAcmrViaOgre stays shallow and under the cognitive complexity threshold. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SonarCloud S2486: a caught exception must be either handled or rethrown. The previous fix tightened the catch to Ogre::Exception but the empty body still tripped the rule. These catches exist because unloadUnreferencedResources can throw when a resource is still referenced (e.g. a Skeleton pinned by a Mesh) and we don't want one stuck reference to abort a 1000-asset scan. Log via Ogre::LogManager and continue — log lines surface in --verbose so operators can spot a pool that never drains. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Continuation of slice C2 — Assimp's .mesh reader only handles v1.41+, so older OgreXMLConverter / GearGenie-style meshes (robot.mesh is v1.40) failed the scan with a load error. The editor itself reads .mesh through Ogre::MeshSerializer; route the scan the same way. inspectAsset() now branches on .mesh BEFORE the Assimp importer and uses the existing loadAndFillOgreInspect helper. Skeleton + animation metadata is pulled directly from Ogre::SkeletonPtr via a new fillAssetInfoFromOgreSkeleton helper (boneCount + per-anim name/length/maxKeys, mirroring what the Assimp pass extracts for other formats). ACMR continues to flow through computeWeightedAcmrViaOgre. Drops the now-obsolete '**/robot.mesh' exclude from the repo's qtmesh.yml since the scan can read v1.40 .mesh files end-to-end: ninja.mesh 842 v / 1008 tri / 28 bones / 20 anims / ACMR 0.93 robot.mesh 295 v / 308 tri / 18 bones / 5 anims / ACMR 1.06 Redundant-keyframe analysis stays Assimp-only — that's an aiNodeAnim channel-list traversal and not load-bearing for .mesh assets. A future slice can port the analyzer to Ogre tracks if needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…image
The previous commit dropped the robot.mesh exclude because the in-tree
Ogre 14.5.x reads MeshSerializer v1.40 fine — and the scan now routes
.mesh through Ogre. But the scan-assets-qtmesh CI job runs the *published*
fernandotonon/qtmesh Docker image, which still bundles a much older Ogre
("Supported versions: [MeshSerializer_v1.8]") and errors out on robot.mesh.
Reinstating the exclude with a comment explaining when it can come out.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Merged
4 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 6 slice C2 — pipe
qtmesh scan's ACMR through Ogre so the scan and the in-app validator agree on the number for every asset.Before: scan computed ACMR from Assimp's flattened face array; the editor's validator computed it from Ogre's actual index buffer. Operators had to calibrate two separate ceilings — ninja.mesh read 3.0 in scan but 0.93 in the editor.
After: scan loads each asset through
MeshImporterExporter(the editor's own loader) and measures ACMR on the Ogre index buffer. ninja.mesh prints 0.932 in both surfaces.What changed
ScanEngine::computeWeightedAcmrViaOgre()(new): imports each asset throughMeshImporterExporter, walks every entity's submesh index buffer (16/32-bit aware), runsVertexCacheOptimizer::computeAcmrweighted by triangle count, writesAssetInfo::weightedAcmr. Failures are non-fatal — leaves the metric at 0.inspectAsset(): removed the Assimp face-array ACMR loop. Vertex / face / skeleton metadata still come from Assimp's scene graph; ACMR is exclusively Ogre's job now.clearOgreSceneForScanImport(): now also flushesMeshManager::unloadUnreferencedResources(true)andSkeletonManager::unloadUnreferencedResources(true)so scanning a directory of 1000 assets doesn't accumulate state in Ogre's resource pools.Documentation cleanup
Per direction — "Assimp-only was historical, not deliberate; remove framing that suggests otherwise":
clearOgreSceneForScanImport).Performance Conceptsandmax_acmrRuleCard): removed the "scan runs higher than the editor's" caveat. They match now. Updated the calibration hint from1.5→1.0(same ceiling the editor uses).What's NOT in this PR (intentional scope limit)
Full conversion of vertex / face / material / animation extraction to walk Ogre's scene graph instead of Assimp's
aiScene. The current path stays hybrid:ScanEngine, notAnimationMerger) operates directly on Assimp's animation channels.Ogre::Skeleton::getAnimation()is a separate ~500 LoC refactor with its own test coverage and is a candidate for a future C3 slice.This PR focuses on the operator-facing problem (ACMR numbers don't agree across surfaces). Captured as a follow-up task in case we want a full conversion later.
Test plan
qtmesh scan media/models --max-acmr 0.8 --include "**/*.mesh"reports 0.932 on ninja.mesh, matchingqtmesh vertex-cache's 0.932 baseline.inspectAssetthat didn't change; the new helper is exercised end-to-end by the manual scan above.media/modelsdirectory (4 .mesh assets) does not crash or accumulate visible Ogre state across the loop.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Chores