feat(scan): retire Assimp from inspection — route through extractMeshInfo (Phase 6 slice C3) - #502
Conversation
…Info (Phase 6 slice C3) Continuation of slice C2. The Ogre-backed ACMR pass already loaded every asset through MeshImporterExporter; this slice consolidates the rest of the scan's metadata extraction onto that same Ogre scene so the scan, the in-app overlay, and the CLI `info` subcommand all agree on what's in a file. Before: inspectAsset() ran Assimp's aiScene traversal for vertex / face / material / bone / animation counts, then re-imported through Ogre purely to measure ACMR. Redundant-keyframe analysis was a 200-LoC scan-specific analyzer over aiNodeAnim channels. After: inspectAssetViaOgre() loads once via MeshImporterExporter, walks Manager::getEntities() and calls CLIPipeline::extractMeshInfo per entity — the same extractor MeshInfoOverlay uses for the floating viewport stats. Redundant-keyframe analysis now delegates to AnimationMerger::analyzeRedundantKeyframes, the analyzer behind `qtmesh anim --simplify` and the Inspector's "Simplify" button — so the percentage the scan flags is exactly the percentage the fix will remove. What changed in the surface: - ninja.mesh: materialCount was 2, now 1 (matches editor / CLI info; the Assimp count was double-counting via aiScene's per-submesh material index array). - Rumba Dancing.fbx: redundant_keyframes 41.8% → 42.0% — same value the Inspector / `qtmesh anim --simplify --dry-run` reports because it IS the same analyzer. - ACMR / vertex / face / bone counts unchanged — the Ogre walk produces the same numbers as before, and the editor was already using the same routine. Embedded-texture detection now uses Ogre::Texture::isManuallyLoaded(), which is true when MaterialProcessor::loadTexture called loadImage() from an in-memory stream (i.e. the texture was packed inside the source asset). What did NOT change: Assimp still lives in the `--fix` write-back path where we have to re-export a mutated scene via Assimp::Exporter for redundant-keyframe simplification. That's a separate slice. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughScanEngine inspection is consolidated into a single Ogre-backed pass: assets import once via MeshImporterExporter, CLIPipeline extracts mesh/material/animation info, AnimationMerger computes redundant-keyframe stats, and Assimp is retained only for texture-reference enumeration and a fallback path. ChangesUnified Ogre-backed asset inspection flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 docstrings
🧪 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: 4bb75fef85
ℹ️ 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".
| if (!inspectAssetViaOgre(filePath, info, AnimationMerger::SimplifyTolerances{})) { | ||
| if (!info.loadError) { | ||
| info.loadError = true; | ||
| info.errorMessage = QStringLiteral("Failed to load asset via Ogre"); | ||
| } |
There was a problem hiding this comment.
inspectAssetViaOgre treats an import with zero entities as failure, and inspectAsset then converts that false return into loadError. That regresses animation-only assets (e.g., FBX clips with skeleton/animations but no mesh): they now get flagged as failed loads instead of being scanned, which also suppresses downstream rules like require_animations and redundant-keyframe checks.
Useful? React with 👍 / 👎.
| for (const QString& t : mi.textures) { | ||
| if (t.isEmpty()) continue; | ||
| if (seenTextures.insert(t.toStdString()).second) { | ||
| info.texturePaths.append(t); | ||
| info.textureRefCount++; |
There was a problem hiding this comment.
This now stores CLIPipeline::extractMeshInfo texture names directly into asset.texturePaths, but those names are Ogre resource names (often basename-only or embedded IDs like *0), not the original material paths. require_textures_exist later treats them as filesystem paths, so valid textures in subfolders (or embedded textures) are reported missing even when the asset is correct.
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
🧹 Nitpick comments (1)
src/ScanEngine.cpp (1)
1044-1078: ⚡ Quick winRe-imports the asset a second time per file when the rule is enabled — consider a shared helper or skipping when tolerances match defaults.
inspectAsset()already imports the asset viaMeshImporterExporter::importerand callsfillRedundancyFromOgreSkeleton. Whenredundant_keyframes_pctis configured, this block imports the file again, walks the samegetEntities()list, and runsAnimationMerger::analyzeRedundantKeyframesover the same skeleton tracks — duplicating most of whatfillRedundancyFromOgreSkeletonalready does, just with config-specific tolerances.For large directory scans of animation-heavy assets (e.g. FBX scenes), this roughly doubles per-file load cost. Two ways to reduce the impact:
- If the configured tolerances equal the defaults that
inspectAssetalready used, reuseasset.totalKeyframes/asset.redundantKeyframesdirectly and skip the re-import.- Otherwise, extract a shared
analyzeEntitiesForRedundancy(entities, tol, …)so the rule body shrinks to: tolerances → import → reuse helper → cleanup, mirroringfillRedundancyFromOgreSkeleton.🤖 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 1044 - 1078, The current block re-imports the file and re-walks entities duplicating work already done by inspectAsset()/fillRedundancyFromOgreSkeleton; change it to first check if the configured tolerances (redundantKeyframesTranslationTol, redundantKeyframesRotationDegTol, redundantKeyframesScaleTol) match the defaults used by inspectAsset and if so reuse asset.totalKeyframes and asset.redundantKeyframes and skip the import/analysis; otherwise refactor the repeated logic into a shared helper (e.g., analyzeEntitiesForRedundancy(const QList<Ogre::Entity*>& entities, const AnimationMerger::SimplifyTolerances& tol, int* total, int* redundant)) that calls AnimationMerger::analyzeRedundantKeyframes for each skeleton/animation so the rule can import once with MeshImporterExporter::importer, call the helper, then clearOgreSceneForScanImport.
🤖 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 1088-1099: The projection uses originalSize * (1 - pct/100)
(variables projectedSize and savedBytes) which wrongly assumes uniform binary
scaling; change the code that computes projectedSize/savedBytes (and the
user-facing message that uses formatBytes) to either (A) estimate byte savings
from the fraction of the file dedicated to animation data (derive an
animPayloadFraction from parsed asset metadata/channels and compute
projectedSize = originalSize - animPayloadFraction * savedBytes) or (B) remove
byte-savings claims entirely and reword the message to report only the
number/percentage of redundant keyframes (e.g., "could remove N redundant
keyframes") while leaving formatBytes for any exact measured --fix output;
update the logic around pct, projectedSize, savedBytes and the message
generation accordingly.
---
Nitpick comments:
In `@src/ScanEngine.cpp`:
- Around line 1044-1078: The current block re-imports the file and re-walks
entities duplicating work already done by
inspectAsset()/fillRedundancyFromOgreSkeleton; change it to first check if the
configured tolerances (redundantKeyframesTranslationTol,
redundantKeyframesRotationDegTol, redundantKeyframesScaleTol) match the defaults
used by inspectAsset and if so reuse asset.totalKeyframes and
asset.redundantKeyframes and skip the import/analysis; otherwise refactor the
repeated logic into a shared helper (e.g., analyzeEntitiesForRedundancy(const
QList<Ogre::Entity*>& entities, const AnimationMerger::SimplifyTolerances& tol,
int* total, int* redundant)) that calls
AnimationMerger::analyzeRedundantKeyframes for each skeleton/animation so the
rule can import once with MeshImporterExporter::importer, call the helper, then
clearOgreSceneForScanImport.
🪄 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: 10ab81b9-d669-4be6-b3fa-cea0aa1f107e
📒 Files selected for processing (2)
CLAUDE.mdsrc/ScanEngine.cpp
| const qint64 originalSize = asset.fileSize; | ||
| const qint64 projectedSize = static_cast<qint64>( | ||
| originalSize * (1.0 - (pct / 100.0))); | ||
| const qint64 savedBytes = originalSize - projectedSize; | ||
|
|
||
| auto formatBytes = [](qint64 bytes) -> QString { | ||
| if (bytes >= 1024 * 1024) | ||
| return QString("%1 MB").arg(bytes / (1024.0 * 1024.0), 0, 'f', 2); | ||
| if (bytes >= 1024) | ||
| return QString("%1 KB").arg(bytes / 1024.0, 0, 'f', 1); | ||
| return QString("%1 B").arg(bytes); | ||
| }; |
There was a problem hiding this comment.
Projected-size formula assumes file size scales linearly with keyframe-removal percentage, which doesn't hold for binary container formats.
const qint64 projectedSize = static_cast<qint64>(
originalSize * (1.0 - (pct / 100.0)));
const qint64 savedBytes = originalSize - projectedSize;FBX, glTF-binary, etc. contain meshes, textures, materials, embedded media, and headers — none of which simplifying keyframes reduces. A file that's 80% texture payload won't shrink by pct% of total size when keyframes are removed; the message will mislead users with overstated savings (e.g. "Simplify it to save ~12 MB" when the actual rewrite saves ~200 KB).
The actual --fix path on line 1419 reports correct numbers because it measures real before/after bytes. Either compute a tighter projection from anim-channel proportion of file, or soften the wording to "could remove N redundant keyframes" without claiming byte savings.
🤖 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 1088 - 1099, The projection uses
originalSize * (1 - pct/100) (variables projectedSize and savedBytes) which
wrongly assumes uniform binary scaling; change the code that computes
projectedSize/savedBytes (and the user-facing message that uses formatBytes) to
either (A) estimate byte savings from the fraction of the file dedicated to
animation data (derive an animPayloadFraction from parsed asset
metadata/channels and compute projectedSize = originalSize - animPayloadFraction
* savedBytes) or (B) remove byte-savings claims entirely and reword the message
to report only the number/percentage of redundant keyframes (e.g., "could remove
N redundant keyframes") while leaving formatBytes for any exact measured --fix
output; update the logic around pct, projectedSize, savedBytes and the message
generation accordingly.
…ng assets CI surfaced a regression on InspectAsset_ObjParsesGeometryAndTextureReferences: the test creates an .obj+.mtl pair referencing a texture file that doesn't exist on disk. The scan needs to: 1. extract vertex/face/material counts, 2. report the texture reference, 3. let require_textures_exist flag the missing file. C3 broke (2) and (3) for any asset where MaterialProcessor::loadTexture threw — the Ogre import aborted, leaving Manager::getEntities() empty, and the scan emitted a generic "load_error" finding instead. Two fixes here: - Catch Ogre/std exceptions around MeshImporterExporter::importer so a texture-load failure doesn't abort the whole inspection. If getEntities() comes back empty we fall back to a tiny Assimp aiScene walk (fillAssetInfoFromAssimpFallback) for the count metadata — enough for the rule evaluator to keep working. - Extract the texture-refs-via-Assimp pass into its own helper (enumerateTextureRefsViaAssimp) and call it from BOTH the Ogre-success and Assimp-fallback paths. Texture references now come from the source-file aiMaterial regardless of whether Ogre managed to bind the texture to a real TextureUnitState, which is what require_textures_exist needs to do anything useful. Verified locally: the failing test fixture now produces vertexCount=3, faceCount=1, materialCount>=1, textureRefCount=1, loadError=false — matching the test's EXPECT/ASSERT. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
Phase 6 slice C3 — consolidate all scan metadata extraction onto the same Ogre walk that
MeshInfoOverlayandqtmesh infoalready use, so the scan, the floating viewport stats, and the CLI report identical numbers for the same file.Before:
inspectAsset()ran Assimp'saiScenetraversal for vertex / face / material / bone / animation counts, then re-imported through Ogre purely to measure ACMR. Redundant-keyframe analysis was a 200-LoC scan-specific analyzer overaiNodeAnimchannels.After:
inspectAssetViaOgre()loads once viaMeshImporterExporter, walksManager::getEntities()and callsCLIPipeline::extractMeshInfoper entity — the same extractorMeshInfoOverlay::formatStatsuses. Redundant-keyframe analysis delegates toAnimationMerger::analyzeRedundantKeyframes, the analyzer behindqtmesh anim --simplifyand the Inspector's "Simplify" button — so the percentage the scan flags is exactly the percentage the fix will remove.Verification
BaseWhite)vertex-cache)anim --simplify --dry-run)The ninja.mesh material count was previously double-counted via Assimp's per-submesh material index array. C3 fixes it by routing through the same code that powers the in-app material list — which is what the user actually sees.
What changed
inspectAssetViaOgre()(new): single Ogre pass filling vertex/face/material/texture/bone/animation/ACMR/redundancy/embedded-flag fields.mergeOgreEntityIntoAssetInfo()/fillRedundancyFromOgreSkeleton()/detectEmbeddedTexturesFromEntities(): helpers that aggregate per-entity output into the scan'sAssetInfo.Ogre::Texture::isManuallyLoaded(). WhenMaterialProcessor::loadTexturereads from an in-memory stream (i.e. the FBX/glTF carried the texture inside it), the resulting Texture is manually-loaded; when it loads from disk, it isn't.redundant_keyframes_pctre-imports via Ogre + callsAnimationMerger::analyzeRedundantKeyframeswith config-specific tolerances (instead of Assimp + the old scan-specific analyzer).sampleVecKeys,sampleQuatKeys,unionTimes,nodeKeyIsRedundant,countRedundantNodeKeys,analyzeAnimationRedundancy) and the helper for.meshskeleton metadata (subsumed by the new path).What's NOT in this PR (intentional scope limit)
Assimp still lives in the
--fixwrite-back path: when--fixactually rewrites a file to remove redundant keyframes, it loads viaAssimp::Importer, mutates theaiScene, and re-exports viaAssimp::Exporter. Porting the write-back to FBX/glTF re-export throughMeshImporterExporter::exporteris a meaningfully larger refactor with its own format-by-format test coverage — captured as a follow-up.Test plan
qtmesh scan media/models --include "**/*.mesh"reports 0.932 ACMR on ninja.mesh, 1.058 on robot.mesh — matchesqtmesh vertex-cacheon both.qtmesh scanredundant_keyframes count for Rumba Dancing.fbx is identical toqtmesh anim --simplify --dry-run(1156/2750, 42.0%).clearOgreSceneForScanImportcontinues to clear MeshManager / SkeletonManager between files.🤖 Generated with Claude Code
Summary by CodeRabbit