feat(scan): finish-line — Ogre-only --fix path + 5 new quality rules (Phase 6 slice C4) - #504
Merged
Merged
Conversation
…(Phase 6 slice C4) Closes out the scan-side Assimp retirement thread and adds the production-grade quality rules that the C3 doc audit flagged as missing. ## Fix path Slice C2 routed the redundant_keyframes_pct --fix through AnimationMerger::simplifyAnimation only for .fbx/.fbxa. Non-FBX formats still went through Assimp::Importer + a scan-specific stripRedundantAnimKeys + Assimp::Exporter, which: - used a weaker analyzer than what `qtmesh anim --simplify` runs; - was the only remaining Assimp::Exporter site in scan. C4 unifies the fix path: every supported extension (FBX / glTF / glb / DAE / OBJ / PLY / STL / .mesh) loads via MeshImporterExporter, simplifies via AnimationMerger::simplifyAnimation under the configured tolerances, then re-exports via MeshImporterExporter::exporter against the matching format descriptor. Drops ~220 LoC of vecApproxEqualAssimp / quatApproxEqualAssimp / compactVectorTrackInPlace / stripRedundantAnimKeys / totalAnimKeysForScene / assimpExportFormatIdForAssetPath and the <assimp/Exporter.hpp> include. Assimp's role in scan is now narrowly limited to a metadata-only ReadFile for texture-reference enumeration (needed by require_textures_exist when the texture file is missing). ## New quality rules All five new rules are opt-in (zero default = disabled) and consume data collected in the same Ogre scene walk that C3 already runs — no extra import cost when they're off. - max_texture_resolution (warn): largest single-axis pixel dimension of any bound texture exceeds the configured cap. Walks each TUS's resolved Ogre::Texture and checks max(width, height). - require_uv_channels (warn): per-submesh minimum count of VES_TEXTURE_COORDINATES elements. Takes the minimum across submeshes so any one submesh missing its lightmap UV trips the rule. - detect_zero_weight_bones (info): set of bones with no VertexBoneAssignment on any submesh. Common Mixamo bloat. Info-level by design — stripping unused bones needs a DCC round-trip rather than an auto-fix. - detect_overlapping_uvs_pct (warn): fraction of triangles whose UV0 AABB overlaps another triangle's. O(n log n) sweep on xmin-sorted bboxes, cheap upper bound on true UV overlap. Catches the common cases — Mixamo body / clothing UV islands stacked at the same 0-1 range, mirrored geometry on a shared lightmap unwrap, etc. - detect_non_manifold_edges_pct (warn): fraction of edges shared by != 2 triangulated faces. Boundary edges count too — catches open-bottom clothing meshes that break boolean ops / fluid sims / 3D printing. ## Surfaces Wired into: - ScanConfig parser (YAML & JSON), applyRuleOverrides for scoped overrides, and JSON asset report (writes maxTextureDimension, minUvChannelCount, zeroWeightBones, overlappingUvsRatio, nonManifoldEdgesRatio when populated). - ScanEngine ruleDescriptions for SARIF / cloud schema export. - CLI help: new "Quality rules (config only)" section in `qtmesh scan --help` listing every new rule with its key form and example threshold. - bundled qtmesh.yml: example commented-out block with the rules and a note that they're opt-in. - website DocsApp.jsx: new "Quality Rules (Slice C4)" section under the rule reference with a RuleCard for each, plus a "Animation Simplification" section documenting redundant_keyframes_pct and its three tolerance knobs. The walkthrough yaml snippet shows all rules at once. - CLAUDE.md ScanEngine description: updated to reflect the unified Ogre fix path and the new rule set. ## Verification Manual scan of media/models against the bundled config with all C4 rules enabled (against Rumba Dancing.fbx): zeroWeightBones: 15 # Mixamo carries 70 bones, 15 unused overlappingUvsRatio: 1.0 # Mixamo's stacked UV islands nonManifoldEdgesRatio: 0.319 # open clothing edges All three rules fire as expected. The default qtmesh.yml in this repo still scans clean (0 warnings) because the new rules ship disabled. ## Tests 13 new gtest cases: - 9 evaluator tests (one positive + one boundary case per new rule, plus a regression that confirms all five are silent when disabled). - 2 ScanConfig parser tests covering the new YAML keys + scope override pickup. - The existing redundant_keyframes_pct rule tests cover the unified fix path's analyzer side; the FBX export side stays exercised by the pre-existing scan-fix tests. 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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Phase 6 "C4" quality-rule scanning: new ScanConfig fields, AssetInfo extensions, Ogre-based collectors to compute texture/UV/topology metrics, evaluateRules checks that emit findings, JSON/SARIF reporting, tests, docs, and an Ogre-based redundant-keyframes auto-fix pipeline. ChangesC4 Quality Rules Integration
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 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: cea7231ecb
ℹ️ 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".
Comment on lines
+498
to
+500
| const auto& assignments = sub->getBoneAssignments(); | ||
| for (const auto& [vertexIdx, vba] : assignments) | ||
| usedHandles.insert(vba.boneIndex); |
There was a problem hiding this comment.
This detector only reads sub->getBoneAssignments(), but Ogre stores skin weights for shared-vertex meshes in mesh->getBoneAssignments(). For assets using useSharedVertices, usedHandles stays incomplete and valid weighted bones are incorrectly reported as zero-weight, producing false info findings for common rig layouts.
Useful? React with 👍 / 👎.
Comment on lines
+628
to
+631
| uint32_t a = idx[t + k]; | ||
| uint32_t b = idx[t + ((k + 1) % 3)]; | ||
| if (a > b) std::swap(a, b); | ||
| edges[{a, b}]++; |
There was a problem hiding this comment.
The non-manifold collector hashes edges using only raw index pairs, but index buffers are local to each non-shared submesh (and can restart at 0 per submesh/entity). This merges unrelated edges like (0,1) from different submeshes into one bucket, corrupting incidence counts and causing both false positives and false negatives in detect_non_manifold_edges.
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ScanEngine.cpp (1)
1071-1096:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRSD path drops all newly collected C4 metrics.
In the RSD wrapper branch, the copy from
inneromitsmaxTextureDimension,minUvChannelCount,zeroWeightBoneNames,overlappingUvsRatio, andnonManifoldEdgesRatio, so C4 rules silently won’t evaluate for.rsdassets.Proposed fix
info.redundantKeyframes = inner.redundantKeyframes; + info.maxTextureDimension = inner.maxTextureDimension; + info.minUvChannelCount = inner.minUvChannelCount; + info.zeroWeightBoneNames = inner.zeroWeightBoneNames; + info.overlappingUvsRatio = inner.overlappingUvsRatio; + info.nonManifoldEdgesRatio = inner.nonManifoldEdgesRatio;🤖 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 1071 - 1096, The RSD wrapper branch copies many fields from the local AssetInfo named inner into info but omits C4 metric fields (maxTextureDimension, minUvChannelCount, zeroWeightBoneNames, overlappingUvsRatio, nonManifoldEdgesRatio), causing RSD assets to lose these metrics; update the copy block in ScanEngine (where AssetInfo inner = ScanEngine::inspectAsset(...) is used) to assign those missing fields (e.g., info.maxTextureDimension = inner.maxTextureDimension; info.minUvChannelCount = inner.minUvChannelCount; info.zeroWeightBoneNames = inner.zeroWeightBoneNames; info.overlappingUvsRatio = inner.overlappingUvsRatio; info.nonManifoldEdgesRatio = inner.nonManifoldEdgesRatio) so the info returned preserves all C4 metrics for .rsd assets.
🤖 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 1371-1390: The emitted finding rule IDs (the string literals
passed to findings.append: "detect_overlapping_uvs" and
"detect_non_manifold_edges") must be changed to match the config/documentation
keys with the "_pct" suffix ("detect_overlapping_uvs_pct" and
"detect_non_manifold_edges_pct"); update those string IDs in the ScanEngine.cpp
instances that build these findings (the block checking
config.detectOverlappingUvsPct / asset.overlappingUvsRatio and the analogous
block checking config.detectNonManifoldEdgesPct / asset.nonManifoldEdgesRatio)
and also update the other occurrence mentioned (the similar findings emitted
later) so all emitted rule IDs consistently include the "_pct" suffix.
- Around line 1632-1635: The call to MeshImporterExporter::importer is unguarded
and can throw, so wrap the call (the sequence around
clearOgreSceneForScanImport(); QList<Ogre::SkeletonPtr> animOnlySkeletons;
MeshImporterExporter::importer({asset.filePath}, 0, &animOnlySkeletons);) in a
try/catch block that catches std::exception& and a catch-all, logs the error
(include exception.what() when available) and cleans up the Ogre scene state,
then skip/continue processing this file instead of letting the exception
propagate and abort the scan/fix run; do not rethrow the exception.
- Around line 605-642: The non-manifold calculation wrongly collides edges from
different meshes/submeshes because edges are keyed only by local vertex indices;
change the map key to include mesh/submesh identity so edges are unique per
source. Define a composite key (e.g., struct EdgeKey { const void* meshPtr;
unsigned submeshIndex; uint32_t a,b; } with equality and hash) or include the
entity pointer and submesh index in the key, keep the existing a>b swap, and
when inserting/incrementing use EdgeKey{mesh.get(), s, a, b} (or Entity pointer
+ s) instead of std::pair<uint32_t,uint32_t>; update the unordered_map type and
iteration accordingly so nonManifold counts are computed per mesh/submesh
correctly.
In `@website/src/DocsApp.jsx`:
- Around line 941-954: The docs claim the "redundant_keyframes_pct" rule is
fixable but the Auto-Fix table lacks that entry; update the Auto-Fix
table/component to include redundant_keyframes_pct (referencing the RuleCard
name="redundant_keyframes_pct") as a fixable rule, show the example config keys
(redundant_keyframes_pct and the three tolerance keys) and the fix command
(qtmesh scan ... --fix re-export behavior) so the table matches the RuleCard
text and example.
---
Outside diff comments:
In `@src/ScanEngine.cpp`:
- Around line 1071-1096: The RSD wrapper branch copies many fields from the
local AssetInfo named inner into info but omits C4 metric fields
(maxTextureDimension, minUvChannelCount, zeroWeightBoneNames,
overlappingUvsRatio, nonManifoldEdgesRatio), causing RSD assets to lose these
metrics; update the copy block in ScanEngine (where AssetInfo inner =
ScanEngine::inspectAsset(...) is used) to assign those missing fields (e.g.,
info.maxTextureDimension = inner.maxTextureDimension; info.minUvChannelCount =
inner.minUvChannelCount; info.zeroWeightBoneNames = inner.zeroWeightBoneNames;
info.overlappingUvsRatio = inner.overlappingUvsRatio; info.nonManifoldEdgesRatio
= inner.nonManifoldEdgesRatio) so the info returned preserves all C4 metrics for
.rsd assets.
🪄 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: cbf7e8e5-19b2-4b7a-a4af-7fa36534d6c4
📒 Files selected for processing (9)
CLAUDE.mdqtmesh.ymlsrc/CLIPipeline.cppsrc/ScanConfig.cppsrc/ScanConfig.hsrc/ScanEngine.cppsrc/ScanEngine.hsrc/ScanEngine_test.cppwebsite/src/DocsApp.jsx
CodeRabbit + Codex flagged 6 issues. All real; all fixed. 1) detect_zero_weight_bones missed shared-vertex skin weights. Ogre stores per-vertex bone assignments at the *mesh* level when submeshes share a single vertex pool (typical Mixamo / FBX layout); the C4 collector only walked sub->getBoneAssignments(). Result: every bone got flagged as zero-weight on shared-vertex skinned meshes — a false-positive flood. Now reads both mesh->getBoneAssignments() and sub->getBoneAssignments() into the same used-handles set. (Codex P1) 2) detect_non_manifold_edges collided edges across submeshes/entities. The edge key was just std::pair<a,b>, but index buffers are local to each non-shared submesh and restart at 0, so unrelated edges like (0,1) from different submeshes shared a bucket and corrupted the incidence count. Key now carries the vertex-pool pointer (mesh->sharedVertexData when useSharedVertices, sub->vertexData otherwise) so edges are unique per source. (Codex P1, CodeRabbit major) 3) Finding rule IDs didn't match config keys. Config exposed detect_overlapping_uvs_pct / detect_non_manifold_edges_pct but findings/SARIF emitted detect_overlapping_uvs / detect_non_manifold_edges (no _pct suffix). Aligned in evaluateRules emit sites, ruleDescriptions, and the unit-test asserts so config + findings + SARIF + docs all speak the same names. (CodeRabbit minor) 4) MeshImporterExporter::importer in --fix path was unguarded. A throw from a malformed asset (e.g. missing-texture MaterialProcessor crash) would abort the whole scan/fix run mid-pass. Now wrapped in try/catch with separate Ogre::Exception and std::exception arms; reports the failure on the per-finding message and continues to the next file. Matches the analogous guard around inspectAssetViaOgre's loader call. (CodeRabbit major) 5) RSD wrapper branch dropped the new C4 AssetInfo fields. inspectAsset's .rsd handler delegates to the geometry's inspect and copies fields one by one — the new C4 fields (maxTextureDimension, minUvChannelCount, zeroWeightBoneNames, overlappingUvsRatio, nonManifoldEdgesRatio) weren't in the copy block, so C4 rules silently no-op'd on .rsd assets. Added the missing assignments. (CodeRabbit outside-diff major) 6) Auto-Fix docs table didn't list redundant_keyframes_pct. The DocsApp RuleCard already says the rule is fixable, but the Auto-Fix overview table only listed file_name_case. Added a row covering the C2/C4 simplify re-export across every supported format. (CodeRabbit minor) Local smoke test: Rumba Dancing.fbx under all three detect_* rules now emits 1 info + 2 warnings (zero_weight_bones, overlapping_uvs_pct, non_manifold_edges_pct) — matches expectation. Build + tests clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GitHub Actions Linux runner stalled on the aqt Install Qt step for >24 minutes on the previous workflow run, blocking the build-linux status check. All other CI passed (build-macos, build-windows, unit-tests-linux, SonarCloud, scan-assets-qtmesh, verify-doc-versions). No code change — push an empty commit to retrigger CI from scratch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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
Closes out the scan-side Assimp retirement thread started in C2/C3 and adds the production-grade quality rules the C3 audit flagged as missing.
Fix path consolidation
C2 routed the
redundant_keyframes_pctfix throughAnimationMerger::simplifyAnimationfor FBX only. Non-FBX formats went throughAssimp::Importer+ a scan-specificstripRedundantAnimKeys(consecutive-duplicate stripping per channel) +Assimp::Exporter. C4 unifies them: every supported format (FBX / glTF / glb / DAE / OBJ / PLY / STL / .mesh) now loads viaMeshImporterExporter, simplifies viaAnimationMerger::simplifyAnimationunder the configured tolerances, and re-exports viaMeshImporterExporter::exporter.Drops ~220 LoC:
vecApproxEqualAssimp,quatApproxEqualAssimp,compactVectorTrackInPlace,compactQuatTrackInPlace,stripRedundantAnimKeys,totalAnimKeysForScene,assimpExportFormatIdForAssetPath, and the<assimp/Exporter.hpp>include. The only remainingAssimp::Importersite in scan is the metadata-onlyReadFilethat enumerates texture references forrequire_textures_exist.New rules (all opt-in)
max_texture_resolutionrequire_uv_channelsdetect_zero_weight_bonesdetect_overlapping_uvs_pctdetect_non_manifold_edges_pctAll five live inside the same Ogre scene walk C3 already runs — no extra import per asset when they're off.
Verification
Manual scan of
media/models/Rumba Dancing.fbxwith all C4 rules enabled:All three rules fire as expected. Default scan against
media/modelsstill passes clean (0 warnings) because the new rules ship disabled in the bundledqtmesh.yml.What changed in the surface
src/ScanEngine.cpp— fixed path unified; new collectorsmaxTextureDimensionForEntities,minUvChannelCountForEntities,zeroWeightBonesForEntities,overlappingUvsRatioForEntities,nonManifoldEdgesRatioForEntities; rule evaluators for each.src/ScanConfig.h/cpp— new fields + YAML/JSON parsers + scope override pickup.src/ScanEngine.h—AssetInfocarries the new data fields.src/CLIPipeline.cpp— new "Quality rules (config only)" section inqtmesh scan --help.qtmesh.yml— example commented-out block for the new rules with a note that they're opt-in.website/src/DocsApp.jsx— full RuleCard reference for the new rules + a new "Animation Simplification" section documentingredundant_keyframes_pctand its three tolerance knobs (previously undocumented).CLAUDE.md—ScanEnginedescription updated to reflect the unified Ogre fix path and the new rule set.Tests
13 new gtest cases in
ScanEngine_test.cpp:ScanConfigparser tests covering the new YAML keys and scope override pickup.The unified fix path's analyzer side is exercised by the pre-existing
redundant_keyframes_pcttests; the FBX export side stays covered by the existing scan-fix tests.What's NOT in this PR
fernandotonon/qtmesh-cloud/web/app.js. The qtmesh-cloud config UI has a per-rule HTML input; adding the 5 new ones is a small frontend change in a different repo and is captured as a follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests