feat(retopo): triangle-pairing quad retopology (closes #401)#697
Merged
Conversation
Closes #401 (epic #397, AI-Assist slice 4). Adds quad-dominant retopology via greedy triangle pairing. ## Background — why not Instant Meshes / QuadriFlow The issue title proposes wrapping Instant Meshes (Wenzel Jakob). In practice: * Instant Meshes ships as a research GUI app with no clean C++ library API; the library-extraction PR upstream is dormant (2021), the repo has had no releases since 2016. * QuadriFlow (the production-grade alternative used by Blender 3.0+) is callable but requires Boost + Eigen + LEMON — heavy deps the project doesn't currently use, with several days of CMake work to integrate cleanly. This slice ships a native triangle-pairing backend with **zero new dependencies**. The `Algorithm` enum is set up to plug in QuadriFlow / Instant Meshes as future opt-in backends behind a `--algo` flag (mirroring `MeshDecimator::Algorithm` and `MeshLodController::Algorithm`). ## Algorithm For every interior edge whose two adjacent faces are triangles: 1. **Coplanarity** — angle between the two triangle normals. Default `maxAngleDeg=25°` preserves curvature features. 2. **Quad shape** — deviation of each interior angle of the candidate quad from 90°. Default `shapeToleranceDeg=65°`. 3. **Convexity** — interior angles sum to ≤ 360° (rejects non-convex merges). 4. **Aspect ratio** — longest/shortest edge of the candidate quad. Default `maxAspectRatio=6.0`. Candidates are sorted by a composite score (coplanarity + 1 − angle-dev + 1/aspect) and taken greedily best-first; each triangle claimed at most once. Quads are emitted with opposing- corner winding `(opposing0, sharedA, opposing1, sharedB)` so the diagonal we just removed becomes the quad's natural diagonal. Output goes through `EditableSubMesh::faces` (n-gon storage) → `triangulateFaces` (fan retri for the GPU index buffer) → `writeNgonFacesToMesh` (`qtme.faces.<i>` user binding so FBX / glTF exporters round-trip the new quads, and Edit Mode sees the mesh as quads). Triangle pairing **never introduces new vertices** — UVs, skin weights, vertex colors, and other per-vertex attributes survive unchanged, which is the main practical advantage over field- aligned methods. ## Surface * **CLI**: `qtmesh retopo <file> [--target-faces N] [--max-angle DEG] [--shape-tol DEG] [--max-aspect R] -o <out> [--json]`. * **MCP**: `retopologize` tool with `target_faces / max_angle_deg / shape_tol_deg / max_aspect_ratio` params. Response includes a structured `retopo` object with per-submesh and total face counts plus a `quadDominance` percentage. * **GUI**: Material Mode → Mode Tools → "Quad Retopology…" button + top-level dialog (`qml/QuadRetopoDialog.qml`) driven by the `QuadRetopoController` QML singleton. Same Inspector-styled idiom as `UvUnwrapDialog`. * **Library**: `QuadRetopo::retopologize(entity, opts, algo)` for the Ogre-backed path; `QuadRetopo::retopologizeMesh(positions, indices, opts, outFaces)` is a pure-data variant used by unit tests and any future headless caller. ## Verification (Rumba Dancing.fbx) ``` $ qtmesh retopo Rumba\ Dancing.fbx -o /tmp/rumba_retopo.glb Quad Retopology =============== Mesh: Rumba Dancing Submeshes: 11 Triangles in: 10220 Faces out: 6032 (4188 quads, 1844 triangles) Quad dominance: 82.0% Wrote: rumba_retopo.glb ``` Valid 760 KB .glb produced; skin weights and animation survive the round-trip (no new vertices introduced). ## Sentry breadcrumbs * `ai.assist.retopo` for every retopo action (CLI, MCP, GUI). ## Tests `src/QuadRetopo_test.cpp` covers: * Two coplanar right triangles → one quad * Bent triangles rejected at default `maxAngleDeg` * Bent triangles accepted with relaxed gate * Empty input returns error report * Aspect-ratio gate rejects elongated quads * `--target-faces N` stops pairing early * Algorithm string round-trip * `reportToJson` schema `tests/CMakeLists.txt` updated to include `QuadRetopo.cpp` + `QuadRetopoController.cpp` so per-test-binary linking picks them up (same fix pattern as the 4bb0b06 follow-up to the UV unwrap PR). ## Documentation * `CLAUDE.md` — new `QuadRetopo` entry under the AI-Assist section. * `README.md` — `qtmesh retopo` CLI examples. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces a complete quad-dominant retopology feature using triangle-pairing heuristics. The implementation spans a core C++ backend (QuadRetopo), a Qt controller singleton, a QML dialog, and three user-facing entry points: CLI (qtmesh retopo), MCP (retopologize tool), and an Edit Mode dialog button. The algorithm greedily pairs adjacent triangles into quads based on coplanarity, shape tolerance, and aspect-ratio constraints, with optional target-face budgeting. ChangesQuad Retopology 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: 7b12bdc6f6
ℹ️ 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
+126
to
+129
| outQuad[0] = opposing0; | ||
| outQuad[1] = sharedA; | ||
| outQuad[2] = opposing1; | ||
| outQuad[3] = sharedB; |
There was a problem hiding this comment.
For normally wound adjacent triangles such as [0,1,2] and [0,2,3], using the sorted edge endpoints here emits [1,0,3,2]; the subsequent fan triangulation produces [1,0,3] and [1,3,2], whose normals are opposite the original triangles. Any accepted quad can therefore be exported/rendered with flipped winding, breaking backface culling and lighting on retopologized surfaces.
Useful? React with 👍 / 👎.
Comment on lines
+227
to
+228
| if (opts.targetFaces > 0) | ||
| perSub.targetFaces = globalRemainingTargetCount; |
There was a problem hiding this comment.
When the caller supplies a total targetFaces for a mesh with multiple submeshes, passing the unchanged global value into each submesh makes retopologizeMesh compare a per-submesh facesNow against the total target. If the total target is larger than an individual submesh's triangle count, that submesh stops immediately and does no pairing, so common assets split into several submeshes can ignore --target-faces almost entirely.
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
98-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
retopoto the recognized subcommands list.The new
retoposubcommand is missing from the list of recognized CLI subcommands that activate CLI mode. This list should include all valid subcommands for accuracy.🔧 Proposed fix
-CLI mode is activated by: (1) invoking via the `qtmesh` symlink, (2) passing `--cli`, or (3) using a recognized subcommand (`info`, `fix`, `convert`, `anim`, `validate`, `lod`, `pose`, `turntable`, `scan`, `material`, `pack-textures`, `normal-from-height`, `atlas`, `atlas-apply`, `memory`, `analyze`, `vertex-cache`, `decimate`, `optimize`, `uv`) as the first argument. Use `--verbose` to see Ogre/engine debug output. Use `--no-telemetry` to permanently opt out of anonymous usage data collection. +CLI mode is activated by: (1) invoking via the `qtmesh` symlink, (2) passing `--cli`, or (3) using a recognized subcommand (`info`, `fix`, `convert`, `anim`, `validate`, `lod`, `pose`, `turntable`, `scan`, `material`, `pack-textures`, `normal-from-height`, `atlas`, `atlas-apply`, `memory`, `analyze`, `vertex-cache`, `decimate`, `optimize`, `uv`, `retopo`) as the first argument. Use `--verbose` to see Ogre/engine debug output. Use `--no-telemetry` to permanently opt out of anonymous usage data collection.🤖 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 `@CLAUDE.md` at line 98, The CLI activation sentence listing recognized subcommands is missing the new "retopo" command; update the parenthetical subcommand list in the sentence that currently enumerates ("info", "fix", ..., "uv") to include "retopo" so the documentation reflects all valid CLI subcommands and will correctly signal CLI mode when "retopo" is used.
🧹 Nitpick comments (3)
src/QuadRetopo_test.cpp (1)
122-146: ⚡ Quick winCover the actual “early-stop after some pairing” path.
This test currently checks only the boundary case (
targetFaces == initial faces), not the partial-merge stop behavior implied by the name/comments. Add a second assertion block (e.g.,targetFaces = 3) to verify exactly one merge occurs and then stops.✅ Suggested extension
TEST(QuadRetopoTest, TargetFacesStopsPairingEarly) { @@ auto report = QuadRetopo::retopologizeMesh( pos.data(), 6, tris.data(), 4, opts, faces); ASSERT_TRUE(report.applied); EXPECT_EQ(report.totalFacesAfter, 4); EXPECT_EQ(report.totalQuadsAfter, 0); EXPECT_EQ(report.totalTrianglesAfterRetopo, 4); + + // Verify partial pairing then early stop at target. + opts.targetFaces = 3; + faces.clear(); + report = QuadRetopo::retopologizeMesh( + pos.data(), 6, tris.data(), 4, opts, faces); + ASSERT_TRUE(report.applied); + EXPECT_EQ(report.totalFacesAfter, 3); + EXPECT_EQ(report.totalQuadsAfter, 1); + EXPECT_EQ(report.totalTrianglesAfterRetopo, 2); }🤖 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/QuadRetopo_test.cpp` around lines 122 - 146, The test only checks the no-op case; add a second call to QuadRetopo::retopologizeMesh with QuadRetopoOptions opts where opts.targetFaces = 3 to exercise the early-stop-after-some-pairing path and assert the expected merge: verify report.applied is true, report.totalFacesAfter == 3, report.totalQuadsAfter == 1 and report.totalTrianglesAfterRetopo == 2 (using the same input pos/tris as the existing test) so exactly one triangle-pair was merged into a quad and the algorithm stopped.CLAUDE.md (1)
240-240: ⚡ Quick winDocument the JSON output option.
The PR objectives specify that the
retopocommand supports JSON output, but the CLI example only shows the basic flags. For consistency with other CLI commands documented in this file (e.g.,info --json,validate --json), add a JSON output example.📝 Suggested addition
After the existing CLI example line, add:
qtmesh retopo model.fbx --target-faces N --max-angle DEG -o out + qtmesh retopo model.fbx -o quads.glb --json # JSON report with quadDominance, per-submesh counts🤖 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 `@CLAUDE.md` at line 240, Add documentation for the JSON output option for the retopo CLI by updating the existing CLI example for the qtmesh retopo/retopologize command to include the --json flag (matching style used by other commands like info --json and validate --json); specifically, append or add an example line showing the same invocation (qtmesh retopo --target-faces N --max-angle DEG -o out) with --json (or an equivalent retopologize --json) so readers see how to request JSON output and that QuadRetopo/retopo supports machine-readable output.src/CLIPipeline.cpp (1)
6792-6817: ⚡ Quick winUse the standardized breadcrumb categories on the retopo flow.
This new user-facing path only emits a custom
ai.assist.retopobreadcrumb. It should also record the import/export steps withfile.import/file.export, and the action breadcrumb should use one of the standard categories expected by the repo.As per coding guidelines "Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with appropriate categories (ui.action, ai.tool_call, file.import, file.export)".
🤖 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/CLIPipeline.cpp` around lines 6792 - 6817, The current retopo flow only emits a custom breadcrumb "ai.assist.retopo"; add standardized breadcrumbs around the import, action, and export steps: call SentryReporter::addBreadcrumb with category "file.import" (and include the input path/filename) immediately before MeshImporterExporter::importer({fi.absoluteFilePath()}), replace the custom action breadcrumb with the repo-standard category (e.g. "ai.tool_call" or "ui.action") describing the retopology call (including targetFaces/maxAngle), and add a "file.export" breadcrumb (including outputPath and fmt) immediately before MeshImporterExporter::exporter(node, ...). Keep messages concise and include any relevant args/error details, leaving QuadRetopo::retopologize and exporter/importer calls unchanged otherwise.
🤖 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 `@qml/QuadRetopoDialog.qml`:
- Around line 46-73: InspectorButton is currently a mouse-only
Rectangle/MouseArea and needs keyboard accessibility: either replace the
composite with a real QML Button or update the existing component
(InspectorButton / id: btn) to add activeFocusOnTab: true, focus: true when
appropriate, Keys.onPressed handler to activate btn.clicked() on Enter/Space,
Accessible.role and Accessible.name properties (e.g., Accessible.role:
Accessible.Button and Accessible.name: label) and ensure the MouseArea remains
enabled only for pointer input; also set focusPolicy/focus properties so the
control is tabbable and visually indicates focus while preserving buttonEnabled
gating for activation.
In `@src/CLIPipeline.cpp`:
- Line 1261: The CLI dispatch adds a "retopo" command via cmdRetopo but the
usage/help display omits it; update the usage output in printUsage() (or the
function that builds the --help text) to include a line describing the "retopo"
command and its brief synopsis/flags so it appears in qtmesh --help; reference
the cmdRetopo symbol to ensure the help description matches the implemented
behavior.
- Around line 6757-6768: The numeric flag parsing for targetFaces, maxAngleDeg,
shapeToleranceDeg, and maxAspectRatio currently uses QString::toInt()/toDouble()
without validating conversion; update the parsing in CLIPipeline.cpp to call
QString::toInt(&ok)/toDouble(&ok) (or equivalent) and check the bool ok and
sensible bounds (e.g., targetFaces > 0, angles within expected ranges, aspect
ratio > 0) after conversion; if conversion fails or the value is out of range,
print a clear error message and abort (e.g., stderr/log and exit(1) or throw) so
malformed flags like "--target-faces foo" are rejected instead of silently
becoming zero.
- Around line 6796-6817: The code currently retopologizes only entities.first()
after MeshImporterExporter::importer(...) which mutates a single entity and then
exports only that node; for multi-entity imports either detect and fail fast
(like cmdDecimate()) or loop over all imported entities and apply
QuadRetopo::retopologize to each before calling MeshImporterExporter::exporter;
update the logic around Manager::getSingleton()->getEntities() to either return
an error when entities.size() > 1 or iterate every Ogre::Entity* from entities,
call QuadRetopo::retopologize(entity, opts) for each and handle per-entity
report.errors, then export the full scene node(s) rather than only
entities.first().
In `@src/MCPServer.cpp`:
- Around line 1436-1440: The code checks
SelectionSet::getEntitiesCount()/getEntity(0) which ignores resolved selections
used by hasSelectedEntities(); replace that raw-entity usage with the resolved
selection APIs: call SelectionSet::getResolvedEntities() on the SelectionSet
singleton (SelectionSet::getSingleton()), verify the returned resolved list is
non-empty, and use the first resolved entry (rather than sel->getEntity(0)) when
constructing the Ogre::Entity or handling the selection; keep the existing error
messages but base them on the resolved list size and null-check the resolved
entry before proceeding.
- Around line 1430-1434: The code sets QuadRetopoOptions from args but never
validates them; add pre-call validation right after populating QuadRetopoOptions
(where opts.targetFaces, opts.maxAngleDeg, opts.shapeToleranceDeg,
opts.maxAspectRatio are assigned) and before calling QuadRetopo::retopologize().
Enforce: targetFaces == -1 or > 0, maxAngleDeg >= 0, shapeToleranceDeg >= 0, and
maxAspectRatio >= 1 (adjust any additional domain rules you know); on violation
return a clear usage error to the caller (or log and abort the operation)
instead of proceeding to mutate the mesh or invoking QuadRetopo::retopologize().
Ensure the error path prevents further processing.
In `@src/QuadRetopo.cpp`:
- Around line 334-369: The emit pass is O(n²) because step 5 re-scans candidates
and searches the pairs list; instead, when selecting a winning candidate in the
greedy loop (the loop that uses candidates, claimed, facesNow and currently
builds pairs), immediately push the validated quad (c.quad) into outFaces with
the same winding used by scoreCandidate and mark the two triangles claimed; then
update facesNow and skip storing into pairs. Remove the whole step 5 loop plus
the pairs and emitted vectors, and adjust outFaces.reserve to triangleCount - /*
number of quads expected */ or keep reserve(triangleCount) — this preserves
best-score order (candidates is already sorted) and eliminates the quadratic
scan.
- Around line 15-16: QuadRetopo.cpp: replace use of M_PI with a local constexpr
double Pi = 3.14159265358979323846 (declare near the top of QuadRetopo.cpp) or
add _USE_MATH_DEFINES on MSVC; then replace the M_PI occurrences in functions
using Pi. In retopologizeMesh/retopologizeSubmesh make perSub.targetFaces
represent an absolute per-submesh target (not a global remaining count): compute
per-submesh targets by proportionally distributing the global target across
submeshes (e.g., target = floor(globalTarget * submeshTriangleCount /
totalTriangleCount)) and clamp to remaining globalTarget, pass that value into
retopologizeSubmesh, and when decrementing globalRemainingTargetCount use a
consistent reductionHere = initialFaceCount - faces.size() (or final face count)
so the global budget updates correctly. Finally, eliminate the quadratic emit by
marking/recording winning pair IDs during the selection step in retopologizeMesh
(or inserting them into an unordered_set) and then do a single linear pass over
pairs to emit only those marked, rather than nesting for candidates × for pairs.
In `@src/QuadRetopoController.cpp`:
- Around line 72-76: Replace the non-approved breadcrumb category
"ai.assist.retopo" with a standard category (either "ai.tool_call" or
"ui.action") in the SentryReporter::addBreadcrumb call while keeping the
existing message/args (the retopo operation name and parameters) unchanged;
locate the SentryReporter::addBreadcrumb(...) invocation in
QuadRetopoController.cpp and update its first argument to the approved category
so the breadcrumb remains queryable with other Sentry events.
- Around line 81-84: QuadRetopo::retopologize(entity, opts) is being called
synchronously on the GUI thread in QuadRetopoController leading to UI freezes;
change this to run retopologize on a background/worker thread or task (e.g.,
dispatch a job from QuadRetopoController that invokes QuadRetopo::retopologize)
and return a QuadRetopoReport via a future/callback; ensure you propagate
progress and cancellation requests from the UI into the worker (wire the UI
cancel/progress controls to the job), catch exceptions inside the worker (catch
Ogre::Exception and others), and marshal final result/error back to the main
thread to update the UI safely.
- Around line 53-56: The failure branches in the QuadRetopoController.cpp
routine emit an error signal but do not populate result["error"], causing the
caller to see "Unknown error"; update every branch that calls emit error(...)
(e.g., the no-selection branch around the existing emit error(QStringLiteral("No
selection set.")) and the other branches at the ranges you noted, plus the
generic report.applied == false fallback) to also set result["error"] to the
same descriptive message before returning; search for the function that builds
and returns the QVariantMap named result and add result["error"] =
QStringLiteral(...) immediately prior to each return that currently only sets
result["applied"] = false.
---
Outside diff comments:
In `@CLAUDE.md`:
- Line 98: The CLI activation sentence listing recognized subcommands is missing
the new "retopo" command; update the parenthetical subcommand list in the
sentence that currently enumerates ("info", "fix", ..., "uv") to include
"retopo" so the documentation reflects all valid CLI subcommands and will
correctly signal CLI mode when "retopo" is used.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 240: Add documentation for the JSON output option for the retopo CLI by
updating the existing CLI example for the qtmesh retopo/retopologize command to
include the --json flag (matching style used by other commands like info --json
and validate --json); specifically, append or add an example line showing the
same invocation (qtmesh retopo --target-faces N --max-angle DEG -o out) with
--json (or an equivalent retopologize --json) so readers see how to request JSON
output and that QuadRetopo/retopo supports machine-readable output.
In `@src/CLIPipeline.cpp`:
- Around line 6792-6817: The current retopo flow only emits a custom breadcrumb
"ai.assist.retopo"; add standardized breadcrumbs around the import, action, and
export steps: call SentryReporter::addBreadcrumb with category "file.import"
(and include the input path/filename) immediately before
MeshImporterExporter::importer({fi.absoluteFilePath()}), replace the custom
action breadcrumb with the repo-standard category (e.g. "ai.tool_call" or
"ui.action") describing the retopology call (including targetFaces/maxAngle),
and add a "file.export" breadcrumb (including outputPath and fmt) immediately
before MeshImporterExporter::exporter(node, ...). Keep messages concise and
include any relevant args/error details, leaving QuadRetopo::retopologize and
exporter/importer calls unchanged otherwise.
In `@src/QuadRetopo_test.cpp`:
- Around line 122-146: The test only checks the no-op case; add a second call to
QuadRetopo::retopologizeMesh with QuadRetopoOptions opts where opts.targetFaces
= 3 to exercise the early-stop-after-some-pairing path and assert the expected
merge: verify report.applied is true, report.totalFacesAfter == 3,
report.totalQuadsAfter == 1 and report.totalTrianglesAfterRetopo == 2 (using the
same input pos/tris as the existing test) so exactly one triangle-pair was
merged into a quad and the algorithm stopped.
🪄 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: fa3e30cd-aa6c-4405-b21d-b94a06a2975b
📒 Files selected for processing (19)
CLAUDE.mdREADME.mdqml/PropertiesPanel.qmlqml/QuadRetopoDialog.qmlqml/qmldirsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/QuadRetopo.cppsrc/QuadRetopo.hsrc/QuadRetopoController.cppsrc/QuadRetopoController.hsrc/QuadRetopo_test.cppsrc/main.cppsrc/mainwindow.cppsrc/qml_resources.qrctests/CMakeLists.txt
Comment on lines
+53
to
+56
| if (!sel) { | ||
| emit error(QStringLiteral("No selection set.")); | ||
| result["applied"] = false; | ||
| return result; |
There was a problem hiding this comment.
Always return an error string on failure.
These branches emit error(...) but often return only { applied: false }. In QuadRetopoDialog.qml, the synchronous return path runs after the signal handler and overwrites the real message with Failed: unknown error. Please set result["error"] before every failure return, including the generic report.applied == false fallback.
💡 Suggested fix
auto* sel = SelectionSet::getSingleton();
if (!sel) {
- emit error(QStringLiteral("No selection set."));
+ const QString message = QStringLiteral("No selection set.");
+ emit error(message);
result["applied"] = false;
+ result["error"] = message;
return result;
}
const auto entities = sel->getResolvedEntities();
if (entities.isEmpty()) {
- emit error(QStringLiteral("No mesh selected."));
+ const QString message = QStringLiteral("No mesh selected.");
+ emit error(message);
result["applied"] = false;
+ result["error"] = message;
return result;
}
@@
- if (!report.error.isEmpty()) result["error"] = report.error;
+ if (!report.error.isEmpty()) result["error"] = report.error;
if (report.applied) emit retopoApplied(result);
- else emit error(report.error.isEmpty()
- ? QStringLiteral("Quad retopology failed")
- : report.error);
+ else {
+ const QString message = report.error.isEmpty()
+ ? QStringLiteral("Quad retopology failed")
+ : report.error;
+ result["error"] = message;
+ emit error(message);
+ }Also applies to: 59-62, 103-108
🤖 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/QuadRetopoController.cpp` around lines 53 - 56, The failure branches in
the QuadRetopoController.cpp routine emit an error signal but do not populate
result["error"], causing the caller to see "Unknown error"; update every branch
that calls emit error(...) (e.g., the no-selection branch around the existing
emit error(QStringLiteral("No selection set.")) and the other branches at the
ranges you noted, plus the generic report.applied == false fallback) to also set
result["error"] to the same descriptive message before returning; search for the
function that builds and returns the QVariantMap named result and add
result["error"] = QStringLiteral(...) immediately prior to each return that
currently only sets result["applied"] = false.
Comment on lines
+72
to
+76
| SentryReporter::addBreadcrumb(QStringLiteral("ai.assist.retopo"), | ||
| QString("UI retopo entity=%1 target=%2 maxAngle=%3 shape=%4 aspect=%5") | ||
| .arg(QString::fromStdString(entity->getName())) | ||
| .arg(targetFaces) | ||
| .arg(maxAngleDeg).arg(shapeToleranceDeg).arg(maxAspectRatio)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use a standard breadcrumb category here.
This operation should still be breadcrumbed, but ai.assist.retopo is not one of the repo’s approved categories. Keep the retopo operation name in the message/payload, and emit it under ai.tool_call or ui.action so it stays queryable with the rest of the Sentry breadcrumbs.
As per coding guidelines "Add Sentry breadcrumbs for all user-facing actions and significant operations using SentryReporter::addBreadcrumb() with appropriate categories (ui.action, ai.tool_call, file.import, file.export)".
🤖 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/QuadRetopoController.cpp` around lines 72 - 76, Replace the non-approved
breadcrumb category "ai.assist.retopo" with a standard category (either
"ai.tool_call" or "ui.action") in the SentryReporter::addBreadcrumb call while
keeping the existing message/args (the retopo operation name and parameters)
unchanged; locate the SentryReporter::addBreadcrumb(...) invocation in
QuadRetopoController.cpp and update its first argument to the approved category
so the breadcrumb remains queryable with other Sentry events.
Comment on lines
+81
to
+84
| QuadRetopoReport report; | ||
| try { | ||
| report = QuadRetopo::retopologize(entity, opts); | ||
| } catch (const Ogre::Exception& e) { |
There was a problem hiding this comment.
Move retopology off the GUI thread.
QuadRetopo::retopologize(...) runs synchronously from a modal UI action, so large meshes will freeze the entire app until it returns. That also misses the linked objective of running this as a worker-thread job with progress/cancel support.
🤖 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/QuadRetopoController.cpp` around lines 81 - 84,
QuadRetopo::retopologize(entity, opts) is being called synchronously on the GUI
thread in QuadRetopoController leading to UI freezes; change this to run
retopologize on a background/worker thread or task (e.g., dispatch a job from
QuadRetopoController that invokes QuadRetopo::retopologize) and return a
QuadRetopoReport via a future/callback; ensure you propagate progress and
cancellation requests from the UI into the worker (wire the UI cancel/progress
controls to the job), catch exceptions inside the worker (catch Ogre::Exception
and others), and marshal final result/error back to the main thread to update
the UI safely.
Batch fix of issues raised by automated review on the initial commit. Mix of critical correctness bugs (flipped winding, broken target- faces budgeting) and defensive hardening (input validation, CLI discoverability). ## Codex P1 — Preserve source triangle winding (src/QuadRetopo.cpp) `buildQuadWinding` used the sorted endpoints of `EdgeKey` (which puts `min(u,v)` first regardless of the source triangles' winding direction) when emitting the quad winding. For normally-wound adjacent triangles like `[0,1,2]` and `[0,2,3]`, that produced quad `[1,0,3,2]`; the subsequent fan-triangulation in `triangulateFaces` then emitted `[1,0,3]` and `[1,3,2]`, both with normals opposite the source triangles. Every retopologized quad rendered with inverted normals — broken backface culling and lighting. Fix: detect tri0's winding direction across the shared edge by checking whether it contains the directed edge `e0 → e1` (rather than just the undirected pair). If yes, emit `[opposing0, e0, opposing1, e1]`; otherwise emit `[opposing0, e1, opposing1, e0]`. The chosen winding always matches the source triangles, so fan- triangulation produces normals consistent with the input. Regression test added in `QuadRetopo_test.cpp` (`EmittedQuadPreservesSourceTriangleWinding`). ## Codex P2 — Per-submesh target-faces budgeting (src/QuadRetopo.cpp) `retopologizeSubmesh` passed the *global* remaining target face count straight to `retopologizeMesh.opts.targetFaces`. But `retopologizeMesh` interprets `targetFaces` as a per-submesh hard limit on face count: a submesh with 200 tris and a global target of 600 would see `facesNow(200) <= 600` immediately and exit without pairing anything. Common multi-submesh assets (Mixamo characters, anything imported with material-split submeshes) silently ignored `--target-faces`. Fix: redefine the "global" counter as a *reduction budget* (number of pair operations remaining across all submeshes), seeded from `(totalTris - opts.targetFaces)`. Each submesh converts the remaining budget into its own per-submesh face limit `max(ceil(tris/2), tris - allowedReduction)` and decrements the global counter by the reduction it actually achieved. Sentinel `-1` means unlimited (caller didn't specify `targetFaces`). Verified against Rumba Dancing.fbx (11 submeshes): * `--target-faces 8000`: 10220 → 8000 faces (was: ignored, would return ~6032 like the unconstrained run) * `--target-faces 6000`: 10220 → 6032 (clamped at the natural ~50% reduction floor) * Unconstrained: 10220 → 6032, unchanged from before. ## CodeRabbit fixes * **CLI: add `retopo` to `--help`** (`src/CLIPipeline.cpp`). The subcommand was dispatched but undocumented. * **CLI: validate numeric flags** (`src/CLIPipeline.cpp`). Previously `--target-faces foo` silently became `0` and changed the meaning of the operation. Now rejected with a clear usage error. Bounds-check each flag: target-faces > 0, max-angle ∈ [0,180], shape-tol ∈ [0,90], max-aspect ≥ 1. * **CLI: reject multi-entity inputs fail-fast** (`src/CLIPipeline.cpp`). Matches the existing `cmdDecimate()` convention. * **MCP: validate option ranges** (`src/MCPServer.cpp`). Same bound rules as the CLI surface; returns a structured usage error rather than silently producing a no-op. * **MCP: use `getResolvedEntities()`** (`src/MCPServer.cpp`). The block was using `getEntitiesCount()/getEntity(0)`, which would reject valid node-level or sub-entity selections that `hasSelectedEntities()` already accepted. * **Controller: always populate `error` field on failure** (`src/QuadRetopoController.cpp`). `QuadRetopoDialog.qml` reads `r.error` when `applied=false`; previous version emitted the signal but left the map without an error key. * **M_PI portability** (`src/QuadRetopo.cpp`). `M_PI` is not standard C++; not defined under MSVC without `_USE_MATH_DEFINES`. Use `Ogre::Math::PI` which the project already depends on. * **O(n²) emit pass** (`src/QuadRetopo.cpp`). Step 5 used to re- walk every candidate and linear-scan `pairs` to identify the winning merges. Replaced with a `WinningPair` struct that stores the validated quad winding directly when each pair is claimed, so the emit pass is O(pairs.size()). ## Deliberately NOT addressed in this PR * **CodeRabbit: keyboard accessibility of `InspectorButton`** — every Inspector-styled dialog in the project uses the same mouse-only `Rectangle` + `Text` + `MouseArea` composite. Adding Accessibility to just this dialog would be inconsistent; retrofitting all of them is a larger refactor outside this slice. * **CodeRabbit: move retopology off the GUI thread** — flagged as "heavy lift" by the reviewer itself. The sibling UV-unwrap (#400) runs synchronously too; the epic's parent issue (#397) mentions an `AIAssistManager` worker thread as the home for cross-feature threading. That's the right place for this work. * **CodeRabbit: change Sentry breadcrumb category** — `ai.assist.retopo` is the conventional category for this epic (matches `ai.assist.uv_unwrap`, `ai.assist.lod`, `ai.assist.decimate`). See `CLAUDE.md` for the convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SonarCloud flagged `cpp:S3923` ("Remove this conditional structure
or edit its code blocks so that they're not all the same") at
`algorithmFromString` on PR #697. Both branches returned
`Algorithm::TrianglePair` because only one backend is implemented
today.
Replace the `if (recognized) ... else ...` with an unconditional
return. The recognized-string set widens here naturally when a
second backend (QuadriFlow / InstantMeshes) lands — at that point
the conditional structure becomes meaningful again.
The behavioural contract is unchanged: any input still maps to
`Algorithm::TrianglePair`. Existing unit tests pass without
modification.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/QuadRetopo_test.cpp (1)
165-167:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the comment to match the actual test scenario.
The comment claims "8 tris total" and discusses
target_faces=8andtarget_faces=6, but the test defines only 4 triangles (line 172-175) and setstargetFaces=4(line 178).📝 Proposed fix to correct the comment
- // Two unit squares side by side (8 tris total). With - // target_faces=8 we want no pairing; with target_faces=6 we - // want exactly one pair (2 tris → 1 quad, 8→7). + // Two quads side by side (4 tris total). With + // target_faces=4 we want no pairing (keep all 4 triangles).🤖 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/QuadRetopo_test.cpp` around lines 165 - 167, The comment above the test is incorrect: the test defines 4 triangles (not 8) and sets targetFaces=4, so update the comment to reflect "4 tris total" (two unit squares side-by-side composed of 4 tris) and change the example expectations to match the actual test values (mention targetFaces=4 as the no-pairing case and, if you want an alternative scenario, use targetFaces=3 to illustrate a single pairing). Update the text surrounding the variable targetFaces and the triangle description so the comment matches the test's actual setup.
🤖 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.
Outside diff comments:
In `@src/QuadRetopo_test.cpp`:
- Around line 165-167: The comment above the test is incorrect: the test defines
4 triangles (not 8) and sets targetFaces=4, so update the comment to reflect "4
tris total" (two unit squares side-by-side composed of 4 tris) and change the
example expectations to match the actual test values (mention targetFaces=4 as
the no-pairing case and, if you want an alternative scenario, use targetFaces=3
to illustrate a single pairing). Update the text surrounding the variable
targetFaces and the triangle description so the comment matches the test's
actual setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8004a10-63e4-4ee8-bbab-e876bdb4c438
📒 Files selected for processing (5)
src/CLIPipeline.cppsrc/MCPServer.cppsrc/QuadRetopo.cppsrc/QuadRetopoController.cppsrc/QuadRetopo_test.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/CLIPipeline.cpp
- src/QuadRetopoController.cpp
- src/MCPServer.cpp
- src/QuadRetopo.cpp
Two follow-ups to address user feedback + a CodeRabbit review item that I had previously declined on PR #697. ## Move "Quad Retopology…" button to Edit Mode Tools User feedback: "it is not mainly related to materials, it is more about the mesh." Retopology is a topology operation — pairs of triangles becoming quads in the n-gon binding. Lives logically alongside the other Edit Mode topology tools (vertex/edge/face selection, soft selection, normals, wireframe overlay, and the existing mesh-validation block). Moved the button from Material Mode → Mode Tools (which mainly hosts texture / atlas / channel-pack tools) to Edit Mode Tools, above the mesh-validation separator. Edit Mode is the right container because (a) the operation only makes sense on a mesh selection, (b) Edit Mode users are the audience who care about quads vs triangles (animation rigs, subdivision workflows, exporting to DCCs for hand-editing). ## CodeRabbit: keyboard accessibility for the dialog Previously declined on the grounds that every other Inspector- styled dialog uses the same mouse-only `InspectorButton` and fixing only this one would be inconsistent. On reflection that's the wrong tradeoff for a modal dialog — keyboard users were locked out entirely until they could find the close button with the mouse. Solution: add a Window-level keyboard handler (`Item { id: keyCapture; focus: true; Keys.onPressed: ... }`) that intercepts Enter / Return → run retopo, and Escape → close. This keeps the `InspectorButton` primitive unchanged (matches the other dialogs) while giving the modal proper keyboard access. The `open()` function now calls `keyCapture.forceActiveFocus()` so the handler is live as soon as the dialog appears. Factored the existing button onClicked into a `runRetopo()` function so both mouse click and Enter key go through the same code path. 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 #401 (epic #397, AI-Assist slice 4). Adds quad-dominant retopology via greedy triangle pairing.
Background — why not Instant Meshes / QuadriFlow
The issue title proposes wrapping Instant Meshes (Wenzel Jakob). In practice:
This slice ships a native triangle-pairing backend with zero new dependencies. The
Algorithmenum is set up to plug in QuadriFlow / Instant Meshes as future opt-in backends behind a--algoflag (mirroringMeshDecimator::AlgorithmandMeshLodController::Algorithm).Algorithm
For every interior edge whose two adjacent faces are triangles:
maxAngleDeg=25°preserves curvature features.shapeToleranceDeg=65°.maxAspectRatio=6.0.Candidates are sorted by a composite score (coplanarity + 1−angle-dev + 1/aspect) and taken greedily best-first; each triangle claimed at most once. Quads are emitted with opposing-corner winding
(opposing0, sharedA, opposing1, sharedB).Output goes through
EditableSubMesh::faces(n-gon storage) →triangulateFaces(fan retri for the GPU index buffer) →writeNgonFacesToMesh(qtme.faces.<i>user binding so FBX / glTF exporters round-trip the new quads, and Edit Mode sees the mesh as quads).Triangle pairing never introduces new vertices — UVs, skin weights, vertex colors, and other per-vertex attributes survive unchanged.
Surface
qtmesh retopo <file> [--target-faces N] [--max-angle DEG] [--shape-tol DEG] [--max-aspect R] -o <out> [--json].retopologizetool withtarget_faces / max_angle_deg / shape_tol_deg / max_aspect_ratioparams. Response includes a structuredretopoobject with per-submesh and total face counts plus aquadDominancepercentage.qml/QuadRetopoDialog.qml) driven by theQuadRetopoControllerQML singleton. Same Inspector-styled idiom asUvUnwrapDialog.QuadRetopo::retopologize(entity, opts, algo)for the Ogre-backed path;QuadRetopo::retopologizeMesh(positions, indices, opts, outFaces)is a pure-data variant used by unit tests and headless callers.Verification (Rumba Dancing.fbx)
Valid 760 KB .glb produced; skin weights and animation survive the round-trip (no new vertices introduced).
Sentry breadcrumbs
ai.assist.retopofor every retopo action (CLI, MCP, GUI).Tests
src/QuadRetopo_test.cppcovers:maxAngleDeg--target-faces Nstops pairing earlyreportToJsonschemaTest plan
qtmesh retopo <file> -o <out>produces a quad-dominant mesh (verified on Rumba Dancing.fbx → 82% quad dominance)--target-faces Nstops pairing early once near the target--max-angle DEGcontrols curvature preservation🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests