feat(opt): batch optimize pipeline — qtmesh optimize + optimize_mesh (Phase 6 slice G) - #507
Merged
Merged
Conversation
…esh (Phase 6 slice G)
Sequences the slice C / C4 / D optimizations end-to-end on a single
mesh asset and writes the result to -o <path>. Same loaded Ogre scene
flows through every stage with no intermediate file I/O.
## Stages
1. **vertex-cache** (slice C / VertexCacheOptimizer::analyzeEntity) —
Forsyth reorder of every triangulated submesh. Reports before/after
weighted ACMR and the number of submeshes rewritten.
2. **decimate** (slice D / MeshDecimator::decimateEntity) — single-pass
reduction. Runs only when --reduction / --target-tris / --target-verts
is supplied. Multi-entity scenes are rejected when decimate is
requested (same one-entity contract qtmesh decimate already enforces).
3. **simplify-anim** (slice C4 / AnimationMerger::simplifyAnimation) —
strip redundant animation keyframes under configurable tolerances
(Balanced preset: 0.001 / 0.5° / 0.001). Operates on the first
entity's skeleton + any animation-only skeletons MeshImporterExporter
surfaced.
## Surface
CLI:
qtmesh optimize <file> -o <output> [flags] [--json]
Flags:
--vertex-cache | --simplify-anim Explicit per-stage toggles
--all vertex-cache + simplify-anim
--reduction <r> Drop fraction 0..0.95
--target-tris N | --target-verts N Target counts (mutually exclusive)
--simplify-translation-tol T
--simplify-rotation-deg-tol D
--simplify-scale-tol S
--json Structured report
When no flag is passed, defaults to --vertex-cache --simplify-anim.
When *only* a decimation knob is passed, the non-destructive defaults
still run on top — "decimate this and clean it up" is what users mean.
MCP:
optimize_mesh tool with the same shape (file/output + per-stage
toggles + tolerances). Response carries per-stage applied/summary
/details plus inputBytes/outputBytes/bytesDelta. Reuses Ogre's
already-up Root in the editor process.
## Verification
Real-world result on `media/models/Rumba Dancing.fbx` with --reduction 0.5:
6.3 MB → 1.4 MB (77.7% smaller)
ACMR 0.822 → 0.648
10220 → 5048 triangles
1156 / 2750 redundant keyframes removed (42.0%)
Same numbers via JSON --json output (verified shape).
## Docs
- CLAUDE.md: added optimize entries to the cheat sheet + CLIPipeline
subcommand inventory + a new architecture section describing the
pipeline + the Rumba result.
- website/src/DocsApp.jsx: new CmdSection with synopsis, options table,
example output block, MCP API summary, and a callout linking each
stage back to the slice that introduced it.
- src/CLIPipeline.cpp printUsage: new entry under Phase 6 commands.
- src/main.cpp: `optimize` added to the CLI-mode activation set so
`qtmesh optimize ...` works without --cli.
## Slice F status
Phase 6 originally scoped a slice F for Draco glTF/glb compression.
Investigation revealed our installed Assimp was built without
ASSIMP_BUILD_DRACO; properly enabling Draco requires rebuilding Assimp
across Linux/macOS/Windows MinGW CI. Captured the work as issue #506
and proceeded to slice G — the optimize pipeline synthesizes
everything else we shipped this phase, so it's the natural place to
land before swinging back to the Draco rebuild.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an end-to-end "optimize" pipeline (vertex-cache reorder, optional decimation, animation keyframe simplification) exposed as ChangesBatch Mesh Optimization Pipeline
Sequence DiagramsequenceDiagram
participant User
participant CLIPipeline
participant Importer
participant VertexCacheOptimizer
participant MeshDecimator
participant AnimationMerger
participant Exporter
participant Reporter
User->>CLIPipeline: cmdOptimize (input, output, flags)
CLIPipeline->>Importer: importScene(input)
Importer->>CLIPipeline: scene, entities, skeletons
CLIPipeline->>VertexCacheOptimizer: run(vertex-cache) [if enabled]
VertexCacheOptimizer->>CLIPipeline: ACMR/stats
CLIPipeline->>MeshDecimator: run(decimation) [if target provided]
MeshDecimator->>CLIPipeline: decimation report/details
CLIPipeline->>AnimationMerger: simplifyKeyframes(skeletons, tolerances) [if enabled]
AnimationMerger->>CLIPipeline: per-skeleton keyframe stats
CLIPipeline->>Exporter: exportScene(output)
Exporter->>Reporter: bytesOut, bytesDelta, stages[]
Reporter-->>User: JSON or human-readable per-stage report
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 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: c0ad112b4f
ℹ️ 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
+4234
to
+4236
| const DecimationReport report = MeshDecimator::decimateEntity(entity, reduction); | ||
| s.applied = report.applied; | ||
| s.summary = QString("%1% triangle reduction (%2 -> %3)") |
There was a problem hiding this comment.
When MeshDecimator::decimateEntity returns report.applied == false for a positive reduction (for example, meshes with unsupported/empty index data), this path still reports success, exports, and returns exit code 0. That makes qtmesh optimize appear to have completed the requested reduction even though the destructive stage failed. The existing qtmesh decimate command treats this exact condition as an error, so this should also abort here to avoid silently producing an un-decimated output.
Useful? React with 👍 / 👎.
Comment on lines
+3794
to
+3796
| const DecimationReport report = MeshDecimator::decimateEntity(entity, reduction); | ||
| s["applied"] = report.applied; | ||
| s["summary"] = QString("%1% triangle reduction (%2 -> %3)") |
There was a problem hiding this comment.
The MCP optimize_mesh tool also treats a failed decimation pass as success: if reduction > 0 but report.applied is false, it still emits a normal success payload and proceeds. In automation this is misleading because callers cannot distinguish “no-op due target>=current” from an actual decimation failure (e.g., MeshLodGenerator unable to reduce). decimate_mesh already returns an error in this scenario, so this tool should mirror that behavior.
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: 7
🤖 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/CLIPipeline.cpp`:
- Around line 640-645: The help text for the "optimize" subcommand is missing
the new simplify tolerance flags; update the string shown in CLIPipeline.cpp so
the optimize usage (the block around the existing optimize help text) documents
--simplify-translation-tol, --simplify-rotation-deg-tol, and
--simplify-scale-tol (and briefly what units/values they expect), and include
them alongside existing options like
--vertex-cache/--simplify-anim/--reduction/--target-tris/--target-verts/--all so
callers of cmdOptimize() can discover these flags from qtmesh --help.
- Around line 4257-4301: The current code stops at the first skeleton found (the
skel variable and the for (Ogre::Entity* entity : entities) loop with the break)
so AnimationMerger::simplifyAnimation is only run on one rig; instead collect
all unique skeletons from entities and animOnlySkeletons (e.g., use a set of
Ogre::SkeletonPtr keyed by pointer or name), then iterate that set and run the
existing animation-counting, name snapshot and
AnimationMerger::simplifyAnimation logic for each skeleton; after simplifying a
skeleton, ensure any entities that reference that skeleton are refreshed/rebuilt
before export so their vertex/animation state is up-to-date.
- Around line 4215-4243: The decimation stage currently ignores a
DecimationReport with report.applied == false and still allows export; update
the cmdArgs.decimateRequested handling (around MeshDecimator::decimateEntity and
DecimationReport usage) to treat a requested positive reduction that produced
report.applied == false as a fatal error: after calling
MeshDecimator::decimateEntity(entity, reduction) check if reduction > 0.0 &&
!report.applied and then fail the optimize command (e.g. return
non-zero/propagate an error or call the existing failure path) instead of merely
recording the stage, ensuring the process does not write output when requested
decimation did not run.
- Around line 4035-4070: The parseOptimizeArgs function must reject explicitly
provided negative decimation targets; after parsing (in parseOptimizeArgs) check
whether the user requested decimation with a negative value and return error
instead of treating it as "not set". Concretely: after parsing argv and before
the "exactly-one-target" check, validate out.reduction, out.targetTris, and
out.targetVerts for negative values that were explicitly provided (use whatever
presence flag your Apply/arg parser sets, e.g. decimateRequested or per-field
"wasSet" indicators); if any provided target is negative, print an error (same
style as existing err() messages) and return 0. Keep the existing
non-destructive default logic unchanged.
In `@src/MCPServer.cpp`:
- Around line 3742-3755: The import currently uses
MeshImporterExporter::importer which mutates the global Manager
(Manager::getSingletonPtr) and then calls mgr->getEntities(), pulling unrelated
scene entities; instead, call MeshImporterExporter::importer in a contained
context and operate only on the newly imported roots: snapshot the set of entity
IDs before import, run importer, snapshot after, compute the delta
(newEntities), and pass only those newEntities into the
optimize_mesh/decimation/export stages (or alternatively create a
temporary/isolated Manager/Scene for import and run stages there), ensuring you
reference the importer call site and avoid using mgr->getEntities() for global
scene contents so decimate and optimization only target the imported asset.
- Around line 3742-3743: Add a Sentry breadcrumb for the source import around
the MeshImporterExporter::importer call: before invoking
MeshImporterExporter::importer({QFileInfo(filePath).absoluteFilePath()}, 0,
&animOnlySkeletons) call SentryReporter::addBreadcrumb("file.import",
QString("Importing source: %1").arg(QFileInfo(filePath).absoluteFilePath())) (or
equivalent string construction), so the import I/O is tracked; ensure the
message includes the absolute file path and use the exact category
"file.import".
- Around line 3741-3900: The code currently only wraps
MeshImporterExporter::importer in a try/catch; wrap the remainder of the
optimization/export logic (everything that uses
VertexCacheOptimizer::analyzeEntity, MeshDecimator::decimateEntity,
AnimationMerger::simplifyAnimation, MeshImporterExporter::exporter, etc.) in a
try block and add catches for std::exception and Ogre::Exception that return
makeErrorResult(...) with the exception details (use e.what() for std::exception
and e.getFullDescription() or similar for Ogre::Exception), so any failure in
those stages returns an error result instead of propagating out of callTool().
🪄 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: 6be54609-94cc-485f-a9da-ca1024155393
📒 Files selected for processing (7)
CLAUDE.mdsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/MCPServer.cppsrc/MCPServer.hsrc/main.cppwebsite/src/DocsApp.jsx
CodeRabbit + Codex flagged 8 distinct issues across the CLI and MCP optimize paths. All real; all fixed in this commit. ## CLI (cmdOptimize) 1. Negative decimation targets now rejected at parse time. Before: `--reduction -1` parsed, set decimateRequested=true with reduction=-1.0, silently fell into the "target equals or exceeds current count; nothing to do" branch. Now: rejected with a clear error before any work starts. Same guard on --target-tris and --target-verts. (CodeRabbit major) 2. Decimation that fails to apply for a positive reduction now returns exit 1 with an error message, mirroring cmdDecimate. Before: the stage report showed applied=false but the command still exited 0 — automation could not tell a decimation failure from a no-op. The stage report is still emitted on the way out so callers can see the partial work. (Codex P1 / CodeRabbit major) 3. simplify-anim now walks every skeleton in the loaded scene, not just the first one found. Multi-entity assets (a co-loaded animation-only skeleton, or multiple skinned entities sharing one file) used to leave every skeleton after the first untouched. De-duplication by skeleton-name keeps cross-entity shared rigs from being simplified twice. Summary reports the skeleton count. (CodeRabbit major) 4. Global qtmesh --help now lists the simplify tolerance flags (--simplify-translation-tol / --simplify-rotation-deg-tol / --simplify-scale-tol). They were buried in the cmdOptimize usage error block; users running `qtmesh --help | grep -i tol` couldn't find them. (CodeRabbit minor) ## MCP (toolOptimizeMesh) 5. Scene isolation. The MCP server runs inside the editor process, so `MeshImporterExporter::importer(...)` was appending the optimize target into the user's live scene. Before stage 1 ran, the entities list contained both the just-imported asset AND every mesh the user already had loaded — and the optimizer happily mutated all of them. Now snapshot the entity-pointer set before the import, subtract after, and operate only on the delta. A trailing RAII cleanup destroys those scene nodes via Manager::destroySceneNode on every return path so the user's scene returns to exactly the state it was in before — no leaked nodes on success, on error, or on a thrown exception. (CodeRabbit critical) 6. Wrap the entire stage pipeline in a try/catch boundary catching Ogre::Exception, std::exception, and `...`. Before, a throw from any of vertex-cache / decimate / simplify-anim / export propagated up through Qt's signal dispatcher and crashed the editor process. The importer call was already guarded; the broader boundary now covers the rest. (CodeRabbit major) 7. Same decimation-not-applied → error treatment as the CLI: return makeErrorResult instead of silent applied=false in the stage report. (Codex P1) 8. Multi-skeleton simplify (matches CLI fix #3 above). 9. file.import Sentry breadcrumb on the source load — symmetric with the file.export breadcrumb already present. (CodeRabbit major refactor) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User feedback: \"the animation got a bit trembling, probably because of
the optmization\" after running `qtmesh optimize` with default flags on
a Mixamo character clip. Balanced is right at the visual-perception
boundary for 30 FPS Mixamo data — barely-perceptible jitter on subtle
motion.
Simplify is destructive (rewrites the asset's animation tracks in
place), so the safe default should be Conservative — near-lossless,
~3-5× key reduction. Users who want Balanced or Aggressive's heavier
reduction now have to opt in by name. This matches the pattern used
elsewhere (--fix --dry-run, decimation requires explicit target, etc.).
## Where the default lives
`AnimationMerger::SimplifyTolerances{}` is the single source of truth.
Every surface (CLI `anim --simplify`, the Inspector "Simplify" button,
`scan --fix redundant_keyframes_pct`, slice G `qtmesh optimize`, MCP
`simplify_animation` + `optimize_mesh`) flows through that ctor or
through `tolerancesForPreset` which now also returns Conservative for
the empty / unknown preset case.
## What changed
- src/AnimationMerger.h: struct ctor defaults flipped to
1e-4f / 0.05f / 1e-4f (Conservative). Updated the inline comment so
future readers see the new contract.
- src/AnimationMerger.cpp `tolerancesForPreset`: the empty-preset and
unknown-preset branches now return Conservative. The "balanced"
branch sets the old defaults explicitly since they no longer match
the ctor.
- src/AnimationMerger_test.cpp: pin the new default (empty + garbage
preset → Conservative). The explicit "balanced" / "aggressive" cases
still test their respective values.
- src/PropertiesPanelController.h: `analyzeAnimationKeyframes` and
`simplifyAnimation` Q_INVOKABLE defaults flipped from "balanced" to
"conservative" so QML callers that omit the preset arg get the safe
choice.
- qml/PropertiesPanel.qml: the per-entity Simplify preset dropdown now
initializes to "Conservative" (currentIndex 0) instead of "Balanced".
- src/ScanConfig.h: `redundantKeyframesTranslationTol` / RotationDegTol
/ ScaleTol defaults flipped to the Conservative triple. Affects
`scan --fix redundant_keyframes_pct` when the user hasn't set the
three `redundant_keyframes_*_tol` keys in qtmesh.yml.
- src/CLIPipeline.cpp: `OptimizeCmdArgs::animTranslationTol` etc match
the new ctor defaults. `--help` text for `qtmesh anim --simplify`
and `qtmesh optimize` updated to call out Conservative as the safe
default and point users at Balanced / Aggressive for heavier
reduction. The Conservative→Balanced→Aggressive ordering in the
preset dropdown is also documented in the optimize block.
- src/MCPServer.cpp: both `simplify_animation` and `optimize_mesh`
schema descriptions updated.
- website/src/DocsApp.jsx: the `qtmesh optimize` CmdSection options
list shows the new default values (0.0001 / 0.05 / 0.0001) and
notes why Conservative is the safe choice.
## Verification
`qtmesh optimize Rumba Dancing.fbx -o out.fbx` (no flags) now reports:
[OK] simplify-anim: removed 216 / 2750 keyframes (7.9%) across 1 skeleton(s)
Down from 1156 / 2750 (42.0%) under Balanced. The 7.9% reduction is
near-lossless on Mixamo character clips — fixes the trembling without
giving up the bulk of the file-size win (6310 KB → 1437 KB, still
77.2% smaller because FBX export itself is the dominant compression).
Users who want the previous behaviour pass `--preset balanced` (for
`qtmesh anim --simplify`) or the explicit tolerance triple
(`--simplify-translation-tol 0.001 --simplify-rotation-deg-tol 0.5
--simplify-scale-tol 0.001`) on `qtmesh optimize`. The Inspector
"Simplify tolerance" dropdown still lists all three options, just
defaulting to Conservative now.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User-observed bug confirmed via slice G's optimize pipeline verification:
running \`qtmesh optimize\` (and even plain \`qtmesh convert\`) on a Mixamo
FBX with a normal map produces an output FBX that re-imports without
the normal map. Root cause: MaterialProcessor::applyRTSSNormalMap
modifies a MaterialPtr at import time, but at export time
\`sub->getMaterial()\` can return a different instance of the same name
(common when a sidecar .material script and an FBX both register the
material) — the RTSS-created normal_map TUS disappears, and the
exporter writes only the diffuse texture reference.
## Fix
Two-sided fix on the import → export contract:
- **MaterialProcessor::applyRTSSNormalMap** now stashes the normal-map
texture name on the material's first pass via UserObjectBindings
(\`qtme.normal_map = texName\`). This survives the resource-group
disagreement because it's keyed on the material instance the
importer actually configured — and Ogre re-applies UOBs on every
load.
- **FBXExporter::writeTextureObjects** + **the Texture→Material
connection loop** both consult this UOB after walking the
CONTENT_NAMED TUS list. When present, the recorded texture name is
added to the texture-name set (so the Texture / Video FBX nodes
emit it) and a \"NormalMap\" connection is created from the texture
to the material. Result: the normal map round-trips even when the
RTSS-created TUS isn't visible to the exporter's pass walk.
## Verification
\`qtmesh optimize ~/Downloads/Rumba\\ Dancing.fbx -o out.fbx\` (Mixamo
source with embedded normal map):
Before:
\$ qtmesh info out.fbx --verbose | grep -i normal
[no output — normal map dropped]
After:
\$ qtmesh info out.fbx --verbose | grep -i normal
Texture 'Boss_normal.png': Loading 1 faces(PF_B8G8R8,1024x1024x1) ...
applyNormalMapsToEntity: built tangents for 'rumba_optimized'
The diffuse + normal both reach the rendered material on re-import.
\`qtmesh convert\` (no optimize stages) gets the same fix — this is a
generic FBX-export round-trip fix that the optimize pipeline just
happened to surface.
## Scope notes
- Issue #508 originally proposed walking RTSS render-state from
FBXExporter to discover bound textures. Tried that first; the
RTSS-created TUS evaporates by export time when materials cross
resource-group boundaries, so render-state inspection finds the
same empty list. UOB-on-the-pass survives the indirection because
it's data on the *exact* Material instance MaterialProcessor
modified, regardless of which group ends up serving it.
- MaterialEditorQML's normal-map slot changes and EditModeController's
re-apply path also call \`RTShaderHelper::applyNormalMap\` directly.
They aren't routed through this UOB path yet — a future fix can
unify them via a shared helper that records the hint at every
entry point.
- Issue #510 (\`qtmesh info\` should surface RTSS-bound normal map
textures in its report) is still a separate concern. That ticket
is about \*reading\*; this PR fixes \*writing\*.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/PropertiesPanelController.h (1)
229-238:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign
simplifyAnimationdefault preset with the new conservative default.
analyzeAnimationKeyframesnow defaults to conservative, butsimplifyAnimationstill defaults to balanced (Line 238). Any caller that omitspresetcan simplify more aggressively than intended.Proposed fix
Q_INVOKABLE int simplifyAnimation(const QString& entityName, const QString& animName, - const QString& preset = QStringLiteral("balanced")); + const QString& preset = QStringLiteral("conservative"));🤖 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/PropertiesPanelController.h` around lines 229 - 238, The default preset for simplifyAnimation is inconsistent with analyzeAnimationKeyframes; update the simplifyAnimation declaration (Q_INVOKABLE int simplifyAnimation(const QString& entityName, const QString& animName, const QString& preset = QStringLiteral("balanced"))) to use QStringLiteral("conservative") instead of "balanced" so both methods default to the conservative preset; ensure the change is applied to the simplifyAnimation signature and any related documentation/comment if present.
🧹 Nitpick comments (1)
src/AnimationMerger_test.cpp (1)
857-868: ⚡ Quick winAssert the full conservative fallback tuple, not only translation.
These fallback checks currently validate only
translation. Please also assertrotationDegandscaleso tuple regressions can’t slip through.Proposed test hardening
bool ok = false; auto def = AnimationMerger::tolerancesForPreset("", &ok); EXPECT_TRUE(ok); EXPECT_FLOAT_EQ(def.translation, 1e-4f); + EXPECT_FLOAT_EQ(def.rotationDeg, 0.05f); + EXPECT_FLOAT_EQ(def.scale, 1e-4f); @@ bool ok2 = true; auto bad = AnimationMerger::tolerancesForPreset("garbage", &ok2); EXPECT_FALSE(ok2); EXPECT_FLOAT_EQ(bad.translation, 1e-4f); + EXPECT_FLOAT_EQ(bad.rotationDeg, 0.05f); + EXPECT_FLOAT_EQ(bad.scale, 1e-4f);🤖 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/AnimationMerger_test.cpp` around lines 857 - 868, The test currently only asserts the translation component of the conservative fallback from AnimationMerger::tolerancesForPreset; extend the assertions to validate the full fallback tuple by also checking rotationDeg and scale on both returns (the empty-string result stored in def and the unknown-preset result stored in bad) using the same EXPECT_FLOAT_EQ style so regressions in rotationDeg or scale values are caught.
🤖 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/FBX/FBXExporter.cpp`:
- Around line 1809-1835: writeDefinitions() is undercounting Texture/Video and
total object Count because it only counts pass TextureUnitState entries and
ignores the extra UOB fallback names collected (qtme.normal_map) in the export
loop that builds texNames; update writeDefinitions() to include the same UOB
fallback texture names (and deduplicate them) when computing Texture, Video, and
overall object counts so the metadata matches the emitted Texture/Video
objects—reuse the texNames collection logic or accept a list of texture names
from the caller, ensure you call Ogre::Any handling
(getUserObjectBindings()/any_cast for "qtme.normal_map") in the same way and
deduplicate before incrementing the counts.
- Around line 1830-1834: The catch blocks currently catching std::bad_cast after
calls to Ogre::any_cast (e.g. the block that extracts Ogre::String from
normalHint and pushes to texNames, and the other similar any_cast usage later)
should be changed to catch const Ogre::Exception& instead of const
std::bad_cast&; find both occurrences where Ogre::any_cast is followed by catch
(const std::bad_cast&) and replace the exception type so the actual
Ogre::Exception thrown by Ogre::any_cast is handled.
---
Outside diff comments:
In `@src/PropertiesPanelController.h`:
- Around line 229-238: The default preset for simplifyAnimation is inconsistent
with analyzeAnimationKeyframes; update the simplifyAnimation declaration
(Q_INVOKABLE int simplifyAnimation(const QString& entityName, const QString&
animName, const QString& preset = QStringLiteral("balanced"))) to use
QStringLiteral("conservative") instead of "balanced" so both methods default to
the conservative preset; ensure the change is applied to the simplifyAnimation
signature and any related documentation/comment if present.
---
Nitpick comments:
In `@src/AnimationMerger_test.cpp`:
- Around line 857-868: The test currently only asserts the translation component
of the conservative fallback from AnimationMerger::tolerancesForPreset; extend
the assertions to validate the full fallback tuple by also checking rotationDeg
and scale on both returns (the empty-string result stored in def and the
unknown-preset result stored in bad) using the same EXPECT_FLOAT_EQ style so
regressions in rotationDeg or scale values are caught.
🪄 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: 576b5abf-ef39-44d3-9059-099f0c4c091d
📒 Files selected for processing (11)
qml/PropertiesPanel.qmlsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/AnimationMerger_test.cppsrc/Assimp/MaterialProcessor.cppsrc/CLIPipeline.cppsrc/FBX/FBXExporter.cppsrc/MCPServer.cppsrc/PropertiesPanelController.hsrc/ScanConfig.hwebsite/src/DocsApp.jsx
✅ Files skipped from review due to trivial changes (1)
- website/src/DocsApp.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/MCPServer.cpp
- src/CLIPipeline.cpp
Comment on lines
+1830
to
+1834
| try { | ||
| const Ogre::String n = Ogre::any_cast<Ogre::String>(normalHint); | ||
| if (!n.empty()) texNames.push_back(n); | ||
| } catch (const std::bad_cast&) {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Ogre version used by QtMeshEditor, what exception type does Ogre::any_cast throw on a failed cast from Ogre::Any?
💡 Result:
In the Ogre version (14.5.2) used by QtMeshEditor [1], Ogre::any_cast throws an Ogre::Exception on a failed cast from Ogre::Any. This is evidenced by OgreAny.h including OgreException.h [2], the get method (referenced by any_cast) using OGRE_EXCEPT with ERR_INVALIDPARAMS when types don't match [3], and runtime error reports showing "OGRE EXCEPTION(2:InvalidParametersException): Bad cast... in Ogre::any_cast" [4]. Ogre consistently uses its custom exception hierarchy rather than std::bad_any_cast [5][6].
Citations:
- 1: Upgrade dependencies: Qt 6.9.3, Assimp 6.0.4, Ogre 14.5.2 #162
- 2: https://ogrecave.github.io/ogre/api/1.10/_ogre_any_8h.html
- 3: https://ogre3d.org/docs/api/1.9/class_ogre_1_1_any.html
- 4: https://steamcommunity.com/app/270150/discussions/1/591761258616772178/
- 5: https://ogrecave.github.io/ogre/api/1.11/class_ogre_1_1_exception.html
- 6: https://ogrecave.github.io/ogre/api/14/_ogre_exception_8h.html
🏁 Script executed:
# First, check the Ogre version being used
find . -name "CMakeLists.txt" -o -name "vcpkg.json" -o -name "conanfile.txt" -o -name "requirements.txt" | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 180
🏁 Script executed:
# Look at the specific lines mentioned in the review
sed -n '1825,1840p' src/FBX/FBXExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 719
🏁 Script executed:
# Also check the other location mentioned (1998-2005)
sed -n '1993,2010p' src/FBX/FBXExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1010
🏁 Script executed:
# Search for all any_cast usages in FBXExporter to understand the pattern
rg "any_cast" src/FBX/FBXExporter.cpp -A 3 -B 1Repository: fernandotonon/QtMeshEditor
Length of output: 608
🏁 Script executed:
# Check what exceptions are being caught currently and what Ogre headers are included
head -50 src/FBX/FBXExporter.cpp | grep -E "#include|catch"Repository: fernandotonon/QtMeshEditor
Length of output: 573
🏁 Script executed:
# Check the Ogre version requirement in CMakeLists.txt
grep -i "ogre" src/CMakeLists.txt | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 1297
🏁 Script executed:
# Check root CMakeLists.txt for Ogre version
grep -i "ogre\|find_package" CMakeLists.txt | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 944
🏁 Script executed:
# Check if OgreException.h is included or needs to be added
rg "OgreException" src/FBX/FBXExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check the entire include block to see what Ogre headers are available
head -30 src/FBX/FBXExporter.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1397
Change exception catch type from std::bad_cast to Ogre::Exception
In Ogre 14.5.2, Ogre::any_cast throws Ogre::Exception on type mismatch, not std::bad_cast. The current catch blocks won't handle the actual exception and will cause unhandled crashes during export. Change both occurrences at lines 1830–1834 and 1998–2005 from catch (const std::bad_cast&) to catch (const Ogre::Exception&).
🤖 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/FBX/FBXExporter.cpp` around lines 1830 - 1834, The catch blocks currently
catching std::bad_cast after calls to Ogre::any_cast (e.g. the block that
extracts Ogre::String from normalHint and pushes to texNames, and the other
similar any_cast usage later) should be changed to catch const Ogre::Exception&
instead of const std::bad_cast&; find both occurrences where Ogre::any_cast is
followed by catch (const std::bad_cast&) and replace the exception type so the
actual Ogre::Exception thrown by Ogre::any_cast is handled.
CodeRabbit re-reviewed the normal-map fix commit. 3 of 4 findings real: 1. PropertiesPanelController::simplifyAnimation still defaulted to "balanced". The companion analyzeAnimationKeyframes was flipped to "conservative" in ecd90af; this Q_INVOKABLE was missed. Any QML caller that omits the preset arg could now silently simplify more aggressively than intended. Flipped to "conservative" to match. 2. FBXExporter::writeDefinitions() counts Texture/Video objects from pass TUSes only, but writeTextureObjects() emits one extra Texture per pass that has a qtme.normal_map UOB hint (the issue #508 fix). For materials where RTSS dropped the normal-map TUS at runtime, the ObjectType Count is now one short of the actually-emitted Texture/ Video pair → downstream FBX parsers reading the metadata see a mismatch. Mirror the UOB-fallback collection from writeTextureObjects into writeDefinitions so the counts stay in sync. 3. AnimationMergerStandaloneTest.TolerancesForPresetMapping only asserted .translation on the conservative fallback. A regression in .rotationDeg or .scale would slip through. Added the missing tuple asserts (0.05f and 1e-4f) for both the empty-string and unknown- preset paths. Skipped (#2 from CodeRabbit): asked to catch Ogre::Exception instead of std::bad_cast around Ogre::any_cast. Verified OgreAny.h in 14.5 — it throws std::bad_cast, not Ogre::Exception. Current code is correct. Verified locally: re-optimized Rumba Dancing.fbx (10220 → 5048 tris, 6310 KB → 2372 KB, 62.4% saved); qtmesh info on the output still reports `Boss_normal.png` loaded and `applyNormalMapsToEntity: built tangents`. Build green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
4 tasks
fernandotonon
added a commit
that referenced
this pull request
May 14, 2026
…e E2) (#511) * feat(textures): apply-atlas — consume a packed manifest (Phase 6 slice E2) Slice E (#505) shipped the packer + UV-manifest writer. This slice ships the consumption side: a pure-data `ApplyAtlas` module that reads the manifest produced by `TextureAtlasPacker::manifestToJson` and rewrites a mesh so it renders the same scene with one binding instead of N. ## What it does - Reads the manifest JSON strictly — half-baked manifests surface the specific missing field instead of producing wrong UVs downstream. - Two-pass walk per Ogre::Entity: 1. Snapshot every submesh's pre-mutation diffuse texture name. Mixamo-style assets share one Material across many submeshes (one Skin_MAT for face / arms / body); a naive single-pass walk sees the swapped-to-atlas binding on the second submesh and reports "no match" for everything after the first. 2. Rewrite UV0 in place (FLOAT2 only) — scale+bias from `[0..1]` into the matched tile's `[u0..u1, v0..v1]` sub-rect — then retarget each unique Material's diffuse TUS exactly once. - After the swap, strips every non-diffuse TUS on affected materials (normal, AO, emissive, …) because they sample UV0 — which is now diffuse-atlas-relative, so they'd sample the wrong region. Also clears the slice #507 `qtme.normal_map` UOB hint so FBX export doesn't re-emit a stale NormalMap connection. `--keep-extras` (CLI) / `keep_extras: true` (MCP) opts out for users who pre- atlased auxiliary channels to match. - After each unique Material is mutated, calls `RTShaderHelper::wirePbrSlotsForFFP` + `mat->compile()/reload()` so the FFP+RTSS lighting path recomputes against the new binding. Without this, lighting reads back the cached pre-swap binding and looks subtly off. ## UV-clamping policy UVs outside `[0..1]` are clamped before remapping by default — that's what every other game-engine atlas tool does, and tiling is impossible to preserve through a sub-rect anyway. Pass `--no-clamp` / `no_clamp: true` to skip out-of-range UVs and have them surface in the report's `outOfRangeUVs` count instead. ## Surfaces - CLI `qtmesh atlas-apply <file> -o <output> --manifest <atlas.json> --atlas <atlas.png> [--match {basename|fullpath}] [--no-clamp] [--keep-extras] [--json]` - MCP tool `apply_atlas` with the same option set - Inspector "Apply Atlas to Mesh…" button (Material Mode → Mode Tools, sibling of "Pack Atlas…") with `qml/ApplyAtlasDialog.qml`. Three drop-area path inputs (mesh / manifest / atlas) + output + match- mode pills + the two checkboxes. ## Tests Standalone (no Ogre needed) — manifest parser round-trip + rejection of malformed JSON + report JSON shape. Auto-discovered by the gtest glob; Linux CI runs them. ## Verified locally Packed a 2-texture atlas (Boss_diffuse + soccer_ball), applied to Rumba Dancing.fbx via the CLI: 11/11 submeshes rewritten, normal map TUSes stripped (no more "Failed to load Boss_normal.png" on re-import), lighting reads against the atlas correctly. Phase 6 epic #261 — the last remaining slice (apart from Draco / slice F #506) is now done. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ui(atlas): move Apply Atlas button into the Pack Atlas dialog Niche follow-up tools shouldn't take general-UI space. The "Apply Atlas to Mesh…" button now lives inside the Pack Atlas dialog's action row, not in Material Mode → Mode Tools. Clicking it loads the Apply Atlas sub-dialog and auto-fills the just-packed atlas + manifest paths so the typical "pack → apply" flow is one extra click after Pack. The PropertiesPanel button + standalone Loader are removed; the ApplyAtlasDialog.qml file itself is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(atlas): address PR #511 review findings CodeRabbit flagged 6 actionable + 2 nitpick items. 5 are real fixes, one was a stale-doc finding; one was wrong (Manager::getEntities already type-checks for "Entity" — line 799 of Manager.cpp filters non-Entity MovableObjects before push_back). ## Fixes 1. CLAUDE.md recognized-subcommand list was missing memory / analyze / vertex-cache / decimate. Doc-only. 2. CLAUDE.md ApplyAtlas paragraph didn't mention --keep-extras; users reading the doc had no visibility into the strip-extras behavior. Doc-only. 3. qml/ApplyAtlasDialog.qml normaliseDroppedPath stripped "file://" blindly — broken for Windows ("file:///C:/path" → "/C:/path") and for percent-encoded paths from Finder (spaces, accents). Use a proper file:// parse, decodeURIComponent, strip the leading slash before a Windows drive letter, ensure a leading slash on POSIX. 4. ApplyAtlas::rewriteUv0 locked vbuf and only unlocked at function end — an exception in baseVertexPointerToElement would leave the buffer locked. Wrapped in a local LockGuard RAII so unlock runs on every exit path. 5. Same-file guard in MaterialEditorQML::applyAtlas (and the mirror in MCPServer::toolApplyAtlas) used QFileInfo::canonicalFilePath() to compare input vs output — canonicalFilePath returns "" for non-existent paths, and the output doesn't exist yet, so the guard never fired. Compare normalized absolute paths (QDir::cleanPath(absoluteFilePath())) with case-insensitive compare on macOS / Windows. 6. Atlas-image directory was registered into the default resource group without an unregister, accumulating across repeated apply- atlas runs and risking same-name shadowing of other dirs. Wrapped both sites (Inspector + MCP) in a LocationCleanup RAII so the location is removed on every return path. The CLI subcommand uses _exit() per CLAUDE.md so it has no leak risk. ## Skipped - CodeRabbit's "non-Entity MovableObject" finding: Manager::getEntities already filters by getMovableType() == "Entity" inside collectEntitiesRecursive. Confirmed by reading Manager.cpp:788-806. - The 3-file-dialog-helper dedup nitpick: cosmetic, ~40 LoC saved at the cost of an extra static helper indirection. Skipping for now. Verified locally: qtmesh atlas-apply Rumba\ Dancing.fbx -o ... --manifest ... --atlas ... → 11/11 submeshes rewritten (regression test of the apply flow). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 14, 2026
extractMeshInfo only walked CONTENT_NAMED TextureUnitStates, which covers every PBR slot MaterialProcessor binds as a plain TUS (diffuse, albedo, metallic, roughness, ao, emissive) but misses the normal map. The normal map is routed through RTSS's render-state side channel (see RTShaderHelper::applyNormalMap), so it never appears as a base- pass TUS, even though the editor renders it. Recover it from the qtme.normal_map UOB hint the importer leaves on the pass — the same hint slice #507 added so FBX export could round- trip the normal map. The walk reads the Any via Ogre::any_cast and adds the texture name to the dedup set, so qtmesh info / scan engine / overlay all see it. Unit test: build a triangle entity, stash the UOB hint without adding a TUS, assert the texture surfaces. Verified locally: $ qtmesh info Rumba\ Dancing.fbx Textures: Boss_diffuse.png, Boss_normal.png Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 14, 2026
extractMeshInfo only walked CONTENT_NAMED TextureUnitStates, which covers every PBR slot MaterialProcessor binds as a plain TUS (diffuse, albedo, metallic, roughness, ao, emissive) but misses the normal map. The normal map is routed through RTSS's render-state side channel (see RTShaderHelper::applyNormalMap), so it never appears as a base- pass TUS, even though the editor renders it. Recover it from the qtme.normal_map UOB hint the importer leaves on the pass — the same hint slice #507 added so FBX export could round- trip the normal map. The walk reads the Any via Ogre::any_cast and adds the texture name to the dedup set, so qtmesh info / scan engine / overlay all see it. Unit test: build a triangle entity, stash the UOB hint without adding a TUS, assert the texture surfaces. Verified locally: $ qtmesh info Rumba\ Dancing.fbx Textures: Boss_diffuse.png, Boss_normal.png Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 14, 2026
extractMeshInfo only walked CONTENT_NAMED TextureUnitStates, which covers every PBR slot MaterialProcessor binds as a plain TUS (diffuse, albedo, metallic, roughness, ao, emissive) but misses the normal map. The normal map is routed through RTSS's render-state side channel (see RTShaderHelper::applyNormalMap), so it never appears as a base- pass TUS, even though the editor renders it. Recover it from the qtme.normal_map UOB hint the importer leaves on the pass — the same hint slice #507 added so FBX export could round- trip the normal map. The walk reads the Any via Ogre::any_cast and adds the texture name to the dedup set, so qtmesh info / scan engine / overlay all see it. Unit test: build a triangle entity, stash the UOB hint without adding a TUS, assert the texture surfaces. Verified locally: $ qtmesh info Rumba\ Dancing.fbx Textures: Boss_diffuse.png, Boss_normal.png Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 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
Closes Phase 6 with a batch optimize pipeline that synthesizes the per-stage commands from slices C / C4 / D into a single `qtmesh optimize` subcommand + `optimize_mesh` MCP tool. Everything runs on the same loaded Ogre scene — no intermediate file I/O between stages.
Stages
Surface
```
qtmesh optimize -o [flags] [--json]
Flags:
--vertex-cache | --simplify-anim Explicit per-stage toggles
--all vertex-cache + simplify-anim
--reduction Drop fraction 0..0.95
--target-tris N | --target-verts N Target counts (mutually exclusive)
--simplify-translation-tol T
--simplify-rotation-deg-tol D
--simplify-scale-tol S
--json Structured report
```
Default flag set (no flags): `--vertex-cache --simplify-anim`. When only a decimation knob is passed, the non-destructive defaults run on top — "decimate this and clean it up" is what users mean.
MCP: `optimize_mesh` with matching args (`file`, `output`, per-stage toggles, tolerances). Response carries per-stage `applied`/`summary`/`details` + `inputBytes`/`outputBytes`/`bytesDelta`.
Verification (real-world)
`media/models/Rumba Dancing.fbx` with `--reduction 0.5`:
```
File: Rumba Dancing.fbx -> rumba_optimized.fbx
Mesh Optimization
[OK] vertex-cache: ACMR 0.822 -> 0.648 across 10220 triangles, 9 submeshes rewritten
[OK] decimate: 50.6% triangle reduction (10220 -> 5048)
[OK] simplify-anim: removed 1156 / 2750 keyframes (42.0%)
6310 KB -> 1405 KB (4904 KB saved, 77.7%)
```
Same numbers via JSON output (verified shape end-to-end).
What's in this PR
Slice F status
Phase 6 originally scoped a slice F for Draco glTF/glb compression. Investigation found our installed Assimp was built without `ASSIMP_BUILD_DRACO`; enabling Draco properly requires rebuilding Assimp across Linux/macOS/Windows MinGW CI. Captured the work as #506 and proceeded to slice G — the optimize pipeline synthesizes everything else we shipped this phase, so it's the natural place to land before swinging back to the Draco rebuild.
Phase 6 closeout
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Behavior Changes