feat(perf): single-pass mesh decimation (Phase 6 slice D) - #499
Merged
Conversation
Builds the fourth pillar of #261 with the same shape as slices A/B/C: pure-data analyzer + CLI + MCP + validator integration + DocsApp. MeshDecimator (new) — wraps Ogre::MeshLodGenerator for *single-pass* mesh reduction. Unlike MeshLodController which generates a chain of LOD levels (LOD 0/1/2/3/…) for distance-based rendering, decimate rewrites the base mesh itself — appropriate for "ship this asset at 5,000 triangles regardless of distance". Three target modes: - --reduction <r>: drop the requested fraction (0..0.95) - --target-tris N: reduce until total tri count ≈ N - --target-verts N: reduce until total vertex count ≈ N All three clamp at 95% so we never degenerate to a single triangle. The pure-data conversions (reductionFromTargetTris / FromTargetVerts / clampReduction) are testable without Ogre. Surfaced through: - CLI: `qtmesh decimate <file> -o <out> (--reduction|--target-tris| --target-verts) [--json]`. Always requires -o (decimation is destructive; we never overwrite the input). - MCP tool: `decimate_mesh` taking one of reduction / target_tris / target_verts plus an optional dry_run flag. Returns the report under the `decimation` key. - Validator: when the geometry-ok row reports > 10,000 triangles, emits a "Tri budget" info row suggesting the qtmesh decimate CLI or MCP path. Not auto-fixable from the validator yet — needs UI design (where the slider lives, undo semantics, etc.). Documentation: DocsApp now has a "Decimation (poly reduction)" concepts entry that distinguishes lod (LOD chain) from decimate (single-pass), plus a full CmdSection for the new subcommand. Manual smoke on ninja.mesh: - --target-tris 500: 1,008 → 492 tris (51% effective reduction) - --reduction 0.75: 1,008 → 248 tris (75% effective reduction) JSON output round-trips through python json.tool. Deferred from #261's slice D scope to follow-up slices: - In-app slider with live viewport preview (the validator nudge ships the CLI as the action path; needs UI thinking) - Compare mode (split viewport with original vs decimated) - Per-vertex lock weights (true QEM replacement of the underlying edge-collapse algorithm — Ogre's MeshLodGenerator doesn't expose per-vertex weights) Tests: 14 MeshDecimatorTest cases cover target-mode arithmetic (empty / no-change / halving / floor cases), reduction clamping (bounds + NaN), and JSON / text serialisation. 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:
📝 WalkthroughWalkthroughImplements single-pass mesh decimation: adds MeshDecimator (model, analysis, Ogre mutation), CLI ChangesMesh Decimation Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: b151fe844c
ℹ️ 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
+2853
to
+2863
| for (Ogre::SceneNode* node : Manager::getSingleton()->getSceneNodes()) { | ||
| if (!node) continue; | ||
| for (unsigned i = 0; i < node->numAttachedObjects(); ++i) { | ||
| Ogre::MovableObject* obj = node->getAttachedObject(i); | ||
| if (obj && obj->getMovableType() == "Entity") { | ||
| target = static_cast<Ogre::Entity*>(obj); | ||
| break; | ||
| } | ||
| } | ||
| if (target) break; | ||
| } |
There was a problem hiding this comment.
decimate_mesh checks that something is selected, but then ignores selection and decimates the first Entity found in the scene graph. In scenes with multiple meshes, this can mutate the wrong asset even when the user explicitly selected a different mesh, which is a destructive mismatch for this tool.
Useful? React with 👍 / 👎.
Comment on lines
+3631
to
+3633
| if (arg == "--target-tris" && i < argc) { | ||
| out.targetTris = QString::fromLocal8Bit(argv[i++]).toInt(); | ||
| continue; |
There was a problem hiding this comment.
This conversion path accepts non-numeric --target-tris values as 0 (Qt's toInt() fallback), which then gets interpreted as a request for maximum reduction (kMaxReduction = 95%). A typo like --target-tris foo can therefore trigger an aggressive decimation instead of failing fast with a usage error.
Useful? React with 👍 / 👎.
Comment on lines
+2895
to
+2900
| const DecimationReport report = dryRun | ||
| ? MeshDecimator::projectEntity(target, reduction) | ||
| : MeshDecimator::decimateEntity(target, reduction); | ||
|
|
||
| QJsonObject result = makeSuccessResult(MeshDecimator::toText(report)); | ||
| result["decimation"] = MeshDecimator::toJson(report); |
There was a problem hiding this comment.
The tool always returns makeSuccessResult(...) even when dry_run is false and MeshDecimator::decimateEntity reports applied == false (e.g., generator failure). This makes automation think the mesh was modified when it was not, unless every caller inspects the nested report flags manually.
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
🤖 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 3627-3637: The argument parsing for --reduction, --target-tris and
--target-verts must validate numeric conversion and enforce that exactly one
target mode is specified: when handling each flag (the --reduction,
--target-tris, --target-verts branches that set out.reduction, out.targetTris,
out.targetVerts) use the QString::toDouble(const QString&, bool *ok) /
QString::toInt(const QString&, bool *ok) overload (or
QDoubleValidator/QIntValidator) to detect invalid input and, on failure, log an
error and abort/return a failure; after parsing, count how many of out.reduction
(non-default/was-set), out.targetTris (non-default/was-set) and out.targetVerts
(non-default/was-set) were specified and if not exactly one, emit an error and
fail. Also apply the same strict numeric validation to the other occurrence
noted around the 3684–3687 block.
In `@src/MCPServer.cpp`:
- Around line 2852-2865: The code currently finds the first Ogre::Entity in
getSceneNodes() and ignores the user's selection, causing decimation to run on
the wrong mesh; change the selection logic to use the editor/manager selection
API to find the first selected entity (instead of iterating all scene nodes):
query Manager::getSingleton() for the current selection (e.g. selected
nodes/entities) and iterate those to set target (the Ogre::Entity* target
variable) from the first selected Ogre::MovableObject whose getMovableType() ==
"Entity", falling back to the existing error via makeErrorResult("No entity in
scene to decimate.") only if no selected entity is found.
In `@src/MeshDecimator.cpp`:
- Around line 131-136: The early return in MeshDecimator when Ogre::Exception is
thrown (inside the try around Ogre::MeshLodGenerator::generateLodLevels) leaves
totalTrianglesAfter as 0 while per-submesh trianglesAfter still equal
trianglesBefore, making the returned report inconsistent; update the
exception-handling path to compute and set report.totalTrianglesAfter to the sum
of report.subMeshResults[*].trianglesAfter (or copy report.totalTrianglesBefore
into totalTrianglesAfter) and ensure report.applied is set to false before
returning so the MeshDecimator report remains internally consistent.
- Around line 93-96: The code forces sr.trianglesAfter to at least 1 even when
sr.trianglesBefore is 0, producing triangles for empty submeshes; modify the
logic in MeshDecimator.cpp around the prediction so that if sr.trianglesBefore
== 0 you set sr.trianglesAfter = 0 (and skip the rounding/prediction) instead of
applying the Math.max(...,1) fallback, while still correctly updating
report.totalTrianglesBefore and any other report counters as appropriate; update
references to sr.trianglesBefore, sr.trianglesAfter and report.appliedReduction
accordingly so empty submeshes remain zero in dry-run reports.
🪄 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: 181a0f24-c3cc-4630-a517-b2c6ea590eca
📒 Files selected for processing (12)
src/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MeshDecimator.cppsrc/MeshDecimator.hsrc/MeshDecimator_test.cppsrc/MeshValidator.cppsrc/main.cpptests/CMakeLists.txtwebsite/src/DocsApp.jsx
…499) Four CodeRabbit P1/P2 findings: 1. MCP decimate_mesh was decimating the FIRST scene entity instead of the SELECTED one — destructive mismatch in multi-mesh scenes. Now reads SelectionSet::getResolvedEntities() and operates on the first selected entity. Error if nothing is selected. 2. CLI numeric inputs (--reduction / --target-tris / --target-verts) silently accepted non-numeric values via Qt's toInt() fallback (0), which then triggered max-reduction (95%). Added parseStrictInt / parseStrictDouble helpers that emit a clear error and abort. Also added "exactly one target mode" enforcement so callers can't pass ambiguous combinations like `--reduction 0.5 --target-tris 1000`. 3. MCP decimate_mesh returned makeSuccessResult even when decimateEntity reported applied=false. Now returns an error in that case (only when !dry_run && reduction>0), so automation doesn't assume the mesh was modified. 4. MeshDecimator::projectEntity invented triangles for empty submeshes (the max(1, ...) clamp). Now stays at 0 for empty submeshes; the clamp only kicks in for submeshes that had triangles to begin with. 5. MeshDecimator::decimateEntity on LOD generation failure left totalTrianglesAfter at 0 while per-submesh trianglesAfter mirrored trianglesBefore — internally inconsistent. The failure path now sets total = before so consumers see a clean "nothing happened" report. SonarCloud minors: - MeshDecimator::countBaseline hoisted to a public static so both the CLI and MCP wrappers stop duplicating the tri/vert counting loop. CLI's int currentTris=0, currentVerts=0 declaration now in separate statements (S1659). - cmdDecimate's `node` is now pointer-to-const (S5350). - MCP's countMeshBaseline helper removed in favour of the public one. The toolDecimateMesh's S5817 ("should be const") and the S3776 (cognitive complexity 34) remain — the former is the standard NOSONAR for the non-const ToolHandler convention; the latter sits at 27 now after extracting resolveMcpReduction. Sonar may still flag it; if it does I'll split further. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 3730-3744: The current validation ensures --output is provided but
doesn’t reject an output that equals the input, so add a guard after the
existing QFileInfo fi(cmdArgs.filePath) check: construct a QFileInfo for
cmdArgs.outputPath (e.g., QFileInfo outFi(cmdArgs.outputPath)),
canonicalize/absolute both paths (use canonicalFilePath() or absoluteFilePath())
and compare them; if they are equal, emit an error via err() describing that
output must not point to the input (same wording as the other message) and
return the same failure code (e.g., return 2) to prevent in-place overwrite;
reference cmdArgs.outputPath, cmdArgs.filePath, and QFileInfo/outFi when
implementing this check.
- Around line 3753-3767: The code only decimates the first imported entity
(entities.first()) causing partial exports when multiple entities are imported;
either fail fast if multi-entity scenes aren't supported or aggregate counts and
decimate all entities: after MeshImporterExporter::importer(...) and fetching
entities via Manager::getSingleton()->getEntities(), check entities.size() and
if != 1 return an error; otherwise proceed as today; alternatively compute total
currentTris/currentVerts by iterating all Ogre::Entity* in entities and calling
MeshDecimator::countBaseline for each (or a new countBaseline overload), compute
reduction via resolveReduction, then loop over each entity and apply the
decimation routine to every entity rather than only entities.first(). Ensure all
references to entities.first() are replaced with a per-entity loop when choosing
the multi-entity path.
In `@src/MeshDecimator.cpp`:
- Around line 149-166: The code currently calls mesh->removeLodLevels() and then
runs Ogre::MeshLodGenerator::generateLodLevels(lodConfig); if generation throws
we return with applied=false but the original LOD chain is already deleted;
change this by preserving the original LODs before mutating and restoring them
on failure (or alternatively perform generation on a clone and swap on success):
capture the existing LOD state for mesh (e.g. serialize or copy the LOD
levels/indices from mesh prior to calling mesh->removeLodLevels() or create a
temporary cloned Ogre::Mesh), run Ogre::LodConfig + generator.generateLodLevels
on the clone or after removal, and in the catch block restore the saved LOD data
back to mesh and keep report.applied false and totals consistent; ensure the
code paths reference mesh->removeLodLevels(), Ogre::LodConfig,
generator.generateLodLevels(), and report when implementing the
snapshot-and-restore or clone-and-swap strategy.
🪄 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: 17d7e375-3981-4877-b9ee-d1409eb603a8
📒 Files selected for processing (4)
src/CLIPipeline.cppsrc/MCPServer.cppsrc/MeshDecimator.cppsrc/MeshDecimator.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MeshDecimator.h
Adds the live-preview decimation UI the user asked for, mirroring the existing LOD section's pattern (slider → setMeshLodBias swap → Apply to commit). MeshDecimatorController (new) — QML_SINGLETON parallel to MeshLodController. Owns its own MeshLodGenerator; exposes three Q_INVOKABLE entry points: - previewReduction(r): generates a temporary LOD chain at fraction r and forces display via setMeshLodBias(1, 1, 1). Cheap-ish but not free, so QML debounces with a 150ms Timer to avoid melting Ogre with a per-pixel LOD rebuild as the slider moves. - clearPreview(): drops the temporary LOD and restores the base mesh (setMeshLodBias(1, 0, USHRT_MAX) + removeLodLevels). - applyReduction(r): commits via MeshDecimator::decimateEntity. Emits applied(before, after) for QML feedback. QML — new "Decimate (single-pass)" CollapsibleSection sits right below LOD Generation in the Object-mode inspector. Slider 0..0.95 in 5% steps, live "Tris: 12,345 → 6,200" readout where the second number appears in blue while the preview is active, Apply (primary highlight color) and Reset Preview (red) buttons. Selection-change handler resets the slider to 50% and clears the feedback line, matching slice C's pattern. The Q_PROPERTY-backed baseTriangleCount lazy-fills the first time it's read with a live selection — handles the case where the singleton is created after the initial scene load. Cast away const on the internal cache write because m_baseTriangleCount is implementation metadata, not externally observable state. The CLI / MCP / scan rule from earlier slice D commits stays as-is — the validator's "Tri budget" suggestion now has both an in-app and a headless action path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/MeshDecimatorController.cpp (1)
69-78: 💤 Low valueThe
const_castfor lazy initialization is acceptable but consider mutable.The lazy-refresh pattern with
const_castworks but usingmutableon the cache field would be more idiomatic C++ for this pattern.Alternative using mutable
In the header, change:
- int m_baseTriangleCount = 0; + mutable int m_baseTriangleCount = 0;Then the getter can call
refreshBaseline()without the cast:int MeshDecimatorController::baseTriangleCount() const { if (m_baseTriangleCount == 0 && hasSelection()) { - const_cast<MeshDecimatorController*>(this)->refreshBaseline(); + refreshBaseline(); // with refreshBaseline() also marked const } return m_baseTriangleCount; }🤖 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/MeshDecimatorController.cpp` around lines 69 - 78, The getter baseTriangleCount() currently uses const_cast to lazily call refreshBaseline(); instead, mark the cache field m_baseTriangleCount as mutable in MeshDecimatorController's class declaration so the const method can update it without casting, then remove the const_cast and call refreshBaseline() directly (keeping uses of hasSelection() and refreshBaseline() intact).
🤖 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.
Nitpick comments:
In `@src/MeshDecimatorController.cpp`:
- Around line 69-78: The getter baseTriangleCount() currently uses const_cast to
lazily call refreshBaseline(); instead, mark the cache field m_baseTriangleCount
as mutable in MeshDecimatorController's class declaration so the const method
can update it without casting, then remove the const_cast and call
refreshBaseline() directly (keeping uses of hasSelection() and refreshBaseline()
intact).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06e38808-b945-403f-a5b1-b38284a77084
📒 Files selected for processing (5)
qml/PropertiesPanel.qmlsrc/CMakeLists.txtsrc/MeshDecimatorController.cppsrc/MeshDecimatorController.htests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- src/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/CMakeLists.txt
User feedback: the Decimate slider and Apply button did nothing. Root cause: even though MeshDecimatorController uses QML_ELEMENT + QML_SINGLETON in its header, the QtMeshEditor harness explicitly registers each singleton via qmlRegisterSingletonType in MainWindow::registerQml. Without the registration, the QML import silently fell back to no-op stubs. Added the registration block right next to MeshLodController's so the controllers stay paired. App relaunched; slider now generates the preview LOD and Apply commits the reduction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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/mainwindow.cpp`:
- Around line 490-494: MainWindow's destructor misses tearing down the
MeshDecimatorController singleton; add a call to MeshDecimatorController::kill()
into MainWindow::~MainWindow() alongside the other controller teardown calls
(same place where MeshLodController::kill(), MeshValidator::kill(), etc. are
invoked) so the instance created via
MeshDecimatorController::qmlInstance()/qmlRegisterSingletonType is properly
destroyed and resources are released.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+490
to
+494
| qmlRegisterSingletonType<MeshDecimatorController>( | ||
| "PropertiesPanel", 1, 0, "MeshDecimatorController", | ||
| [](QQmlEngine* engine, QJSEngine*) -> QObject* { | ||
| return MeshDecimatorController::qmlInstance(engine, nullptr); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify MeshDecimatorController singleton lifecycle API and teardown usage.
# 1) Locate controller definition/implementation files.
fd -i "MeshDecimatorController" src
# 2) Check for singleton/lifecycle methods (qmlInstance/instance/kill).
rg -n -C3 '\bMeshDecimatorController::(qmlInstance|instance|kill)\b|\bclass\s+MeshDecimatorController\b' src
# 3) Compare teardown calls in MainWindow destructor area.
rg -n -C5 '::kill\(\)' src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3506
🏁 Script executed:
# Read MeshDecimatorController.h for lifecycle/design comments
head -50 src/MeshDecimatorController.h
# Read MeshDecimatorController.cpp implementation details
head -80 src/MeshDecimatorController.cpp
# Check destructor context for explanatory comments
sed -n '315,350p' src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 5677
Add missing MeshDecimatorController::kill() call in the MainWindow destructor.
The MeshDecimatorController singleton is registered and instantiated via qmlInstance() but lacks teardown, unlike all 11 other singleton controllers (MeshLodController, MeshValidator, etc.) that are explicitly destroyed in MainWindow::~MainWindow(). Add MeshDecimatorController::kill(); in the destructor's teardown sequence to prevent resource leak and maintain lifecycle symmetry.
🤖 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/mainwindow.cpp` around lines 490 - 494, MainWindow's destructor misses
tearing down the MeshDecimatorController singleton; add a call to
MeshDecimatorController::kill() into MainWindow::~MainWindow() alongside the
other controller teardown calls (same place where MeshLodController::kill(),
MeshValidator::kill(), etc. are invoked) so the instance created via
MeshDecimatorController::qmlInstance()/qmlRegisterSingletonType is properly
destroyed and resources are released.
Clears the remaining issues Sonar flagged after the Decimate Inspector section + QML-singleton registration landed: - S859 (const_cast in baseTriangleCount lazy-fill): removed the cast-away trick. baseTriangleCount() is now a plain getter again; QML's Component.onCompleted calls a new Q_INVOKABLE primeBaseline() on section load, which refreshes the cached count and emits baseChanged. Same observable behaviour, no UB. - S3776 (parseDecimateArgs cognitive complexity 35): extracted applyDecimateArg as a per-token helper so the main loop is a thin while + dispatch. Complexity drops below the 25 threshold. - S6004 (init-statement for modesProvided): folded into the if's init-statement form, matching the slice A/B/C convention. - S5350 (sel pointer-to-const) in MCPServer.cpp and MeshDecimatorController.cpp: applied. SelectionSet::getResolvedEntities is const, so the pointer never needs to be mutable. Remaining Sonar items are documented false positives: - MCPServer.cpp S5817 — non-const tool method (NOSONAR'd, matches the rest of the registry). - MeshDecimator.cpp + Controller.cpp S995 — entity pointers are non-const because the functions exist specifically to mutate the underlying mesh. - MeshDecimatorController.cpp S5025 (singleton new/delete) — the whole *Controller fleet uses this pattern; fixing it requires a cross-cutting cleanup of all controllers, out of scope for slice D. Manual smoke: qtmesh decimate media/models/ninja.mesh --target-tris 500 -o /tmp/n.mesh still produces 1,008 → 492 (51.2% effective), exit 0. Strict-arg rejection and exactly-one-target enforcement still work end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/MeshDecimatorController.cpp (2)
136-149: ⚡ Quick winAdd breadcrumb tracking for clear preview action.
This user-facing action should be tracked with
SentryReporter::addBreadcrumbto maintain observability. As per coding guidelines, discrete user actions should be logged for diagnostic purposes.📊 Suggested breadcrumb addition
void MeshDecimatorController::clearPreview() { if (!m_hasPreview) return; + SentryReporter::addBreadcrumb("ui.action", "Clear decimation preview"); + for (Ogre::Entity* entity : decimateTargets()) {As per coding guidelines: "All user-facing actions and significant operations must be tracked with
SentryReporter::addBreadcrumb(category, message)using categories:"ui.action"for toolbar/menu clicks".🤖 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/MeshDecimatorController.cpp` around lines 136 - 149, The clearPreview user action in MeshDecimatorController::clearPreview lacks breadcrumb tracking; add a call to SentryReporter::addBreadcrumb("ui.action", "clearPreview") at the start (or just before m_hasPreview is set false) so the UI action is recorded; modify the function to invoke SentryReporter::addBreadcrumb with category "ui.action" and a descriptive message (e.g., "clearPreview") around the existing logic in clearPreview to satisfy observability guidelines.
89-134: 💤 Low valueConsider breadcrumb tracking for preview operations.
While
applyReductionproperly tracks user actions withSentryReporter::addBreadcrumb,previewReductiondoes not. As per coding guidelines, user-facing actions should be tracked. However, since this method is likely called frequently during slider movements, adding a breadcrumb for every call could create noise in telemetry.Consider either:
- Adding a rate-limited breadcrumb (e.g., only log every Nth call or use a debounce timer)
- Adding a breadcrumb only when the reduction crosses significant thresholds
- Deferring breadcrumb tracking if preview is considered a low-priority diagnostic signal
As per coding guidelines: "All user-facing actions and significant operations must be tracked with
SentryReporter::addBreadcrumb(category, message)using categories:"ui.action"for toolbar/menu clicks".🤖 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/MeshDecimatorController.cpp` around lines 89 - 134, Add Sentry breadcrumbs for user-facing preview actions inside MeshDecimatorController::previewReduction but avoid noise by rate-limiting: implement a simple debounce/count/threshold check (e.g., keep a lastPreviewBreadcrumbTime or a previewBreadcrumbCounter as a member) and only call SentryReporter::addBreadcrumb("ui.action", QString("previewReduction:%1").arg(r)) when the debounce interval has elapsed or when the reduction value crosses a configured significance threshold; mirror the breadcrumb format used in applyReduction so telemetry is consistent and ensure the new member(s) are updated in previewReduction before early returns.
🤖 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.
Nitpick comments:
In `@src/MeshDecimatorController.cpp`:
- Around line 136-149: The clearPreview user action in
MeshDecimatorController::clearPreview lacks breadcrumb tracking; add a call to
SentryReporter::addBreadcrumb("ui.action", "clearPreview") at the start (or just
before m_hasPreview is set false) so the UI action is recorded; modify the
function to invoke SentryReporter::addBreadcrumb with category "ui.action" and a
descriptive message (e.g., "clearPreview") around the existing logic in
clearPreview to satisfy observability guidelines.
- Around line 89-134: Add Sentry breadcrumbs for user-facing preview actions
inside MeshDecimatorController::previewReduction but avoid noise by
rate-limiting: implement a simple debounce/count/threshold check (e.g., keep a
lastPreviewBreadcrumbTime or a previewBreadcrumbCounter as a member) and only
call SentryReporter::addBreadcrumb("ui.action",
QString("previewReduction:%1").arg(r)) when the debounce interval has elapsed or
when the reduction value crosses a configured significance threshold; mirror the
breadcrumb format used in applyReduction so telemetry is consistent and ensure
the new member(s) are updated in previewReduction before early returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd1ca440-5dc6-4c68-b857-836064d14000
📒 Files selected for processing (5)
qml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/MeshDecimatorController.cppsrc/MeshDecimatorController.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/MeshDecimatorController.h
- qml/PropertiesPanel.qml
- src/CLIPipeline.cpp
- src/MCPServer.cpp
…499) User feedback after the QML-registration fix landed: app now crashes on startup (SIGABRT, exit code 134) before the window appears. Root cause: Ogre::MeshLodGenerator is itself an Ogre::Singleton<> CRTP. MeshLodController constructs one in its ctor, and my new MeshDecimatorController was constructing a second one in *its* ctor. The second construction trips Ogre's singleton check and throws, aborting the app during MainWindow's qmlRegisterSingletonType call. Fix: drop the unique_ptr<MeshLodGenerator> member from MeshDecimatorController and reach the shared instance via Ogre::MeshLodGenerator::getSingleton() at the use site (previewReduction). Same instance MeshLodController already owns. Header cleanup falls out: forward decl, include of <memory>, and the out-of-line destructor are no longer needed. App launches cleanly with PID assigned and stays running. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ity (#499) User feedback: preview worked but Apply consistently failed with "Decimation failed: MeshLodGenerator could not produce a reduced mesh" on every mesh — imported assets and primitives alike. Root cause: same singleton-double-construction trap I fixed in the controller earlier, but lurking inside MeshDecimator::decimateEntity itself: try { Ogre::MeshLodGenerator generator; // local construction → throws generator.generateLodLevels(lodConfig); } catch (...) { /* swallowed → applied=false */ } Ogre::MeshLodGenerator is a Singleton<>; MeshLodController owns the process-wide instance. The local construction tripped Ogre's "already has a single instance" assertion, which the catch swallowed into the generic "Decimation failed" message. The preview path worked because the controller's new previewReduction() reaches the shared instance via getSingleton(). Two fixes: - MeshDecimator::decimateEntity now goes through a new sharedLodGenerator() helper that returns the live singleton when it exists (the GUI path) and lazy-constructs it otherwise (the CLI / MCP / test paths where no MeshLodController is around). Mirrors the "Ogre singletons live for the process lifetime" expectation; intentional leak on the lazy branch. - MeshDecimatorController::previewReduction now uses getSingletonPtr() with a null check, so the controller never triggers the lazy-construct path — the GUI always has MeshLodController built first anyway. Verified: - qtmesh decimate media/models/ninja.mesh --target-tris 500 -o /tmp/x.mesh → 1,008 → 492 tris, applied, exit 0. - App launches cleanly; Decimate section's slider + Apply both work on imported meshes and primitives. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…no-op (#499) User feedback: opening the Decimate section pre-loaded a 50% reduction preview onto the viewport, which made the mesh shrink before the user had a chance to interact. Now the slider starts at 0% — the previewer treats r<=0 as "no preview" (clearPreview path) so the original mesh stays visible until the user actually drags. Also updates the selection-change handler to reset to 0%, matching the new default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User feedback: clicking Apply correctly updates the mesh, but the floating mesh-info overlay still shows the old tri/draws/GPU numbers until the user toggles the overlay off and back on. Root cause: the overlay listens for Manager::entityCreated / sceneNodeDestroyed and SelectionSet::selectionChanged. None of those fire when an in-place index-buffer mutation happens — the entity is the same, the scene node is the same, the selection is the same. Fix: subscribe the overlay to MeshDecimatorController::applied so it refreshes on every committed reduction. The signal carries before/ after counts, which we ignore here — refresh() re-reads from Ogre directly so the new numbers come from the source of truth. This is the pattern future in-place mutators should follow: emit a signal from the controller; have any view subscribed to it refresh. Future option for cleanliness is a generic Manager::meshChanged signal, but the current 1:1 connection keeps the layering clear. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three real findings from CodeRabbit's review of the live-preview push: 1. CLI: -o pointing back to the input file silently overwrote the source asset despite the error text promising otherwise. Now compares canonical paths (handles symlinks + relative paths; falls back to absolute when the output file doesn't yet exist) and rejects with exit 2 before importing anything. 2. CLI: multi-entity imports silently decimated only the first entity, exporting a partially-reduced scene while the report looked authoritative. cmdDecimate now fails fast when entities.size() > 1, pointing at a future scene-decimation slice as the path forward. 3. MeshDecimator::decimateEntity: line 168 wiped the existing LOD chain before the generator ran. If generateLodLevels() threw, the function returned applied=false but the mesh had still lost its pre-existing LODs — a "failure" was destructive for in- memory callers. Now snapshots each submesh's mLodFaceList before clearing, restores it on the exception path, and frees the saved IndexData* only on success. Manual smoke: - `qtmesh decimate ninja.mesh -o ninja.mesh ...` → exit 2 with the targeted error. - `qtmesh decimate ninja.mesh -o /tmp/n.mesh --target-tris 500` → exit 0, 1,008 → 492 tris (51% effective). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The LOD-chain snapshot/restore in the previous CR-fix commit pushed MeshDecimator::decimateEntity over Sonar's S3776 complexity ceiling (26 vs 25 allowed). Extracted four single-purpose helpers in the anonymous namespace: - snapshotLodFaceLists(mesh) → save + clear per-submesh mLodFaceList - restoreLodFaceLists(mesh, snapshot) → put them back on failure - freeLodSnapshot(snapshot) → release after success - promoteFirstLodToBase(mesh) → swap LOD-1 index data into the base slot and remove the (now-emptied) LOD chain decimateEntity itself is now a thin orchestrator: count baseline, snapshot, generate, on-failure restore, on-success free + promote + re-count. Behaviour preserved (qtmesh decimate ninja.mesh --target-tris 500 still produces 1,008→492). Remaining Sonar items on the PR are all the documented false- positive class (S5025 singleton new/delete pattern matching every *Controller, S5817 NOSONAR'd tool method, S995/S5350 on must-be-mutable Ogre pointers, S1116 Q_UNUSED) — same call as slices A/B/C accepted before. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
7 tasks
6 tasks
4 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 D (#261) — single-pass mesh decimation with live in-app preview. Same pattern as slices A/B/C (pure-data + CLI + MCP + validator + Inspector controller).
MeshDecimator(new): wrapsOgre::MeshLodGeneratorfor single-pass base-mesh reduction. UnlikeMeshLodController(which builds an LOD chain for distance-based rendering), this rewrites the base mesh itself — appropriate for "ship this asset at 5,000 tris regardless of distance". Three target modes:--reduction <r>(drop the requested fraction),--target-tris N,--target-verts N. All clamp at 95% so we never degenerate to one triangle.MeshDecimatorController(new): QML_SINGLETON parallel toMeshLodController. Owns the live preview / Apply lifecycle. ReachesOgre::MeshLodGeneratorvia the shared singleton (the GUI'sMeshLodControllerowns the live instance; lazy-constructs in CLI/MCP/test contexts where neither controller is around).qtmesh decimate <file> -o <out>(always requires-o— destructive op, never overwrites input). Strict numeric input validation; rejects ambiguous combinations like--reduction 0.5 --target-tris 1000.decimate_mesh— operates on the selected entity (not the first scene entity), supportsdry_run, returns error when applied=false. Structured payload under thedecimationkey.MeshInfoOverlay) listens toMeshDecimatorController::appliedso the tri/draws/GPU lines update immediately after Apply — no need to toggle the overlay off/on.lod(chain) fromdecimate(single-pass), full CmdSection in DocsApp.Manual smoke
In-app: select an entity, expand the Decimate section, drag the slider — viewport updates after the 150ms debounce, tri-count line shows
→ Nin blue. Click Apply to commit; the overlay refreshes on its own.Deliberately deferred from #261's slice D scope
Captured in tracking tasks for future slices:
MeshLodGeneratordoesn't expose per-vertex weights. True QEM-with-locks would replace the algorithm; orthogonal to this slice.Issues fixed during review / iteration
SelectionSet::getResolvedEntities().toInt()fallback to 0) which triggered 95% reduction. AddedparseStrictInt/parseStrictDoublewith explicit error messages and "exactly one target mode" enforcement.makeSuccessResultwhenapplied=false. Now returns an error in that case (gated by!dry_run && reduction > 0).projectEntityinvented triangles for empty submeshes viamax(1, ...)— now stays at 0 for empty inputs.decimateEntityon LOD generation failure lefttotalTrianglesAfterat 0 while per-submesh mirrored before — now consistent.baseTriangleCountwith a Q_INVOKABLEprimeBaseline()that QML calls inComponent.onCompleted.applyDecimateArghelper fromparseDecimateArgs; below threshold.modesProvided, const pointers for SelectionSet handles.Ogre::MeshLodGeneratoris itself aSingleton<>CRTP; the controller's constructor was creating a second instance. Now reaches the shared singleton viagetSingleton().MeshDecimator::decimateEntityconstructing a local generator. Now uses asharedLodGenerator()helper that returns the live instance and lazy-constructs in CLI/test contexts.MeshDecimatorController::applied.Test plan
MeshDecimatorTest.*pure-data tests (target-mode arithmetic, reduction clamping, JSON/text serialisation, NaN handling). Run on every CI build.qtmesh decimateon ninja.mesh with--target-tris 500produces 1,008→492 (51%),--reduction 0.75produces 1,008→248 (75%). JSON output validates.--target-tris foo→ exit 2 with clear error). Ambiguous targets rejected.src/CMakeLists.txtandtests/CMakeLists.txt.🤖 Generated with Claude Code