Skip to content

feat(#863): level leg cuts to a symmetric hip line on split - #931

Merged
fernandotonon merged 8 commits into
masterfrom
feat/segment-level-leg-cut-863
Jul 28, 2026
Merged

feat(#863): level leg cuts to a symmetric hip line on split#931
fernandotonon merged 8 commits into
masterfrom
feat/segment-level-leg-cut-863

Conversation

@fernandotonon

@fernandotonon fernandotonon commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to the merged split-cleanup work (#930): splitting a segmented character produced clean, symmetric arms but lopsided legs — the ONNX body model cuts the two legs at slightly different up-axis heights, so an explode looked uneven (one leg longer, a ragged hip line). This adds a targeted levelLimbCut pass that gives the legs the same clean, level cut the arms already have.

Approach

levelLimbCut (BODY category only, default ON via Options::levelLimbCuts):

  • Pools the leg↔torso boundary-face up-axis values across both legs, takes the median as a single shared cut height h.
  • Reassigns only leg/torso faces within a thin band of h: below h → the leg on that lateral side, above h → torso.
  • Scoped strictly to the leg↔torso pair. Arms are excluded on purpose — they attach along a vertical seam where a horizontal level is meaningless (an early arm-inclusive version collapsed them into the torso). This narrow scope is why it can't scramble other parts, unlike the experimental planarBoundaryRecut (which stays OFF).

Runs after the existing de-fringe/de-island passes, before the final island cleanup.

Verified — Hip Hop Dancing.obj (the exact repro: unrigged → AI-segmented → exploded)

Measured from the per-vertex labels + OBJ geometry:

metric before after
leg cut-height L vs R diff 0.050 (3.7% of body height) 0.000 (0% — perfectly level)
leg size ratio (min/max faces) 0.84 0.99
leg/torso seam up-axis std 2.88% of body height 1.33% (half the raggedness)
arms 2624 / 2624 2624 / 2624 (untouched)

Rigged Rumba Dancing.fbx (dance pose): legs stayed balanced (751/769) — the pass is a near no-op when the model output is already symmetric; it only bites on an asymmetric one. No regression.

Confirmed visually too via exploded silhouette renders (isometric sprite of a per-part-offset export): the two leg pieces are now matched and compact vs the fused, jagged baseline.

Tests

35 MeshSegmenter tests pass, incl. 2 new for levelLimbCut (equalises differing leg heights; no-op when a leg/torso part is absent). All pure-data, no GL/model.

Advances #863 (a symmetric, level cut is what makes segmented legs printable/joinable like the arms).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Improved body mesh segmentation with an optional default-on “level limb cuts” pass to better regularize left/right leg↔torso boundaries (configurable).
  • Bug Fixes
    • Improved headless screenshot captures by rendering via the active viewport camera, auto-framing newly loaded meshes, and avoiding dark/black captures.
  • Documentation
    • Updated MeshSegmenter guidance to the newer v2 strategy (including updated selection/fallback behavior) and refreshed continual-training workflow details.
  • Tests
    • Added unit tests covering “level limb cuts” correctness and no-op behavior when required labels aren’t present.

Splitting a segmented character gave clean symmetric ARMS but lopsided LEGS:
the ONNX body model cuts the two legs at slightly different up-axis heights, so
an explode looked uneven (one leg longer, a ragged hip line). New levelLimbCut
pass (BODY category, default ON via Options::levelLimbCuts) fixes this:

- Pools the leg↔torso boundary-face up-values across BOTH legs, takes the median
  as a single shared cut height h, and reassigns only leg/torso faces within a
  thin band of h — below h → the leg on that lateral side, above h → torso.
- Scoped strictly to the leg↔torso pair. ARMS are excluded on purpose: they
  attach along a vertical seam where a horizontal level is meaningless (an early
  arm-inclusive version collapsed them into the torso). Unlike the experimental
  planarBoundaryRecut, this narrow scope can't scramble other parts.

Verified on Hip Hop Dancing.obj (unrigged → AI-segmented → exploded, the exact
repro): leg cut-height diff 3.7% → 0% of body height, size ratio 0.84 → 0.99,
leg/torso seam up-std 2.88% → 1.33% (half the raggedness). Arms untouched
(2624/2624). Rigged Rumba stayed balanced (near no-op when already symmetric —
the pass only bites on an asymmetric model output). 35 MeshSegmenter tests pass
(+2 for levelLimbCut: equalises heights; no-op when a part is missing).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 66252754-cb7d-491a-9614-3250166240e4

📥 Commits

Reviewing files that changed from the base of the PR and between 4b134c2 and 9a9ba45.

📒 Files selected for processing (2)
  • src/MCPServer.cpp
  • src/MeshSegmenter_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/MCPServer.cpp

📝 Walkthrough

Walkthrough

Adds configurable body leg-to-torso seam leveling to geometric and ONNX segmentation cleanup, with tests and updated documentation. MCP mesh loading now frames the active viewport, while screenshots use Ogre offscreen rendering with temporary lighting restoration.

Changes

Leg seam leveling

Layer / File(s) Summary
Leveling contract and implementation
src/MeshSegmenter.h, src/MeshSegmenter.cpp
Adds the levelLimbCuts option and mirror-based levelLimbCut helper for balancing eligible leg and torso face labels.
Segmentation cleanup integration
src/MeshSegmenter.cpp
Applies body-only leg-cut leveling to geometric and ONNX cleanup paths when enabled.
Validation and behavior documentation
src/MeshSegmenter_test.cpp, CLAUDE.md
Tests symmetry and missing-part no-op behavior and documents the updated segmentation model, dispatch, and cleanup behavior.

MCP headless rendering

Layer / File(s) Summary
Loaded-scene framing
src/MCPServer.h, src/MCPServer.cpp
Frames imported scene content in the active viewport after load_mesh.
Offscreen screenshot capture
src/MCPServer.cpp, CLAUDE.md
Captures the active camera through an Ogre RTT, adjusts and restores lighting, saves RGBA output, and cleans up render resources.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MCPClient
  participant MCPServer
  participant SpaceCamera
  participant OgreRTT
  MCPClient->>MCPServer: load_mesh
  MCPServer->>SpaceCamera: frame loaded scene
  MCPClient->>MCPServer: take_screenshot
  MCPServer->>OgreRTT: render active camera and read pixels
  OgreRTT-->>MCPServer: return RGBA image
  MCPServer-->>MCPClient: save screenshot
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: leveling leg cuts for symmetric body splits.
Description check ✅ Passed The description is mostly complete, covering summary, approach, verification, and tests, though it doesn't follow the exact template headings.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/segment-level-leg-cut-863

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f34067918f

ℹ️ 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 thread src/MeshSegmenter.cpp Outdated
Comment on lines +733 to +737
const int lf = faceLabels[f];
const bool isLimb = (lf == limbLeft || lf == limbRight);
if (!isLimb && lf != shared) continue;
const float fu = centroid[f].comp(up);
if (std::fabs(fu - h) > band) continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Limit relabeling to faces near the hip seams

On a posed character where a bent or raised portion of a leg crosses the hip-height band, this loop processes that remote geometry even though it is not near a leg↔torso boundary: every leg face above h becomes torso and every participating face below it is reassigned as a leg. Since the default band spans 10% of the entire body height and the later island cleanup preserves sufficiently large patches, this can corrupt knees, feet, or thighs in common dance/seated poses. Restrict candidates by adjacency or distance through the actual seam rather than vertical distance alone.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was on the earlier horizontal-band levelLimbCut (commit f340679), which was reworked into a mirror-symmetrise in 56e8c72 — the current code has no height band (it never uses up-axis height), so a bent/raised leg crossing hip height is no longer reassigned. Moot on the current path.

Comment thread src/MeshSegmenter.cpp Outdated
const float upSpan = upMax - upMin;
if (!(upSpan > 0.0f))
return 0;
const float latMid = 0.5f * (latMin + latMax);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Derive the left/right divider from the leg seams

When an asymmetric pose extends an arm or accessory much farther on one side, the whole-mesh AABB midpoint can lie beyond both hips. In that case the below-cut branch assigns faces from both legs to the same limb label, splitting or merging semantic leg parts despite the input labels already identifying left and right. Compute the divider from the two leg/seam regions (or retain each existing limb's side) instead of allowing unrelated geometry to shift it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also on the reverted f340679 code. The current mirror path derives the sagittal centre from the two legs (not the whole-mesh AABB), and 2727056 further makes it the count-independent midpoint of the per-limb means. Moot as written.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/MeshSegmenter.cpp (1)

1010-1021: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared face-cleanup pipeline instead of duplicating it in both paths. segmentGeometric and predict() each carry their own copy of the smoothLabelBoundarieslevelLimbCutplanarBoundaryRecutcleanupLabelIslandsvertexLabelsFromFaces sequence; this PR had to add the new levelLimbCut step to both copies identically, which is exactly the kind of drift-prone duplication a shared helper avoids.

  • src/MeshSegmenter.cpp#L1010-L1021: extract this if (opts.cleanupIslands) {...} block (and the matching smoothing/planar/island calls around it) into a private static helper taking faceLabels, positions, vertexCount, indices, indexCount, opts, cat, and call it here.
  • src/MeshSegmenter.cpp#L1356-L1365: call the same extracted helper here instead of repeating the block.
🤖 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/MeshSegmenter.cpp` around lines 1010 - 1021, Extract the duplicated
face-cleanup sequence around segmentGeometric and predict into one private
static helper accepting faceLabels, positions, vertexCount, indices, indexCount,
opts, and cat; preserve the smoothLabelBoundaries, levelLimbCut,
planarBoundaryRecut, cleanupLabelIslands, and vertexLabelsFromFaces ordering.
Replace the repeated blocks at src/MeshSegmenter.cpp:1010-1021 and
src/MeshSegmenter.cpp:1356-1365 with calls to that helper.
🤖 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/MeshSegmenter.h`:
- Around line 286-304: Update the documentation for MeshSegmenter::levelLimbCut
to state that it runs after smoothing but before cleanupLabelIslands and
planarBoundaryRecut. Keep the implementation and all other documented behavior
unchanged.

---

Nitpick comments:
In `@src/MeshSegmenter.cpp`:
- Around line 1010-1021: Extract the duplicated face-cleanup sequence around
segmentGeometric and predict into one private static helper accepting
faceLabels, positions, vertexCount, indices, indexCount, opts, and cat; preserve
the smoothLabelBoundaries, levelLimbCut, planarBoundaryRecut,
cleanupLabelIslands, and vertexLabelsFromFaces ordering. Replace the repeated
blocks at src/MeshSegmenter.cpp:1010-1021 and src/MeshSegmenter.cpp:1356-1365
with calls to that helper.
🪄 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 Plus

Run ID: 540294f0-5cb0-4649-a24e-d49d9b099e2e

📥 Commits

Reviewing files that changed from the base of the PR and between 4feaa5b and f340679.

📒 Files selected for processing (4)
  • CLAUDE.md
  • src/MeshSegmenter.cpp
  • src/MeshSegmenter.h
  • src/MeshSegmenter_test.cpp

Comment thread src/MeshSegmenter.h Outdated
… of flat

User feedback on the flat horizontal leg cut: legs were now the same size, but
(1) the feet got swapped (left foot ended up with the right leg) and (2) the
torso skirt bottom got dragged into the legs. They asked for a DIAGONAL cut like
the arms, not a forced horizontal waistline.

Reworked levelLimbCut to MIRROR-SYMMETRISE across the sagittal plane instead of
recutting at a shared height:
- Reflect the labelling across the leg region's lateral centre; make each
  near-seam face agree with its mirror (union: limb if it OR its mirror is a
  limb; torso only if both). Preserves the model's natural diagonal boundary,
  makes the two legs symmetric, and — because reflection maps a foot to the
  opposite foot with the limb labels swapped — keeps each foot with its own leg.
- No height/plane reassignment, so the torso skirt is never pulled down.
- Scoped to a bounded edge-hop flood from the leg↔torso seam (distant geometry
  untouched); left/right side derived from the legs' own lateral means.

Verified on Hip Hop Dancing.obj: leg size ratio → 1.00, feet stay on their own
side (regress.py: both legs body_lat sign == foot_lat sign), skirt not dragged
(leg up_max = natural diagonal), arms untouched. Rigged Rumba stays balanced.
Test updated to assert symmetry AND no foot-swap; 35 MeshSegmenter tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/MeshSegmenter.cpp (2)

804-813: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

The four-branch union rule collapses to one expression.

The first two branches are identical and want = lf is dead.

♻️ Simplification
-        int want = lf;
-        if (isLimb && mirrorIsLimb) {
-            want = sideLimb;              // both agree it's a limb → this side's limb
-        } else if (isLimb && !mirrorIsLimb) {
-            want = sideLimb;              // union: keep as limb (mirror will follow)
-        } else if (!isLimb && mirrorIsLimb) {
-            want = sideLimb;              // union: torso here but limb across → make limb
-        } else {
-            want = shared;               // both torso
-        }
+        // Union rule: limb if this face OR its mirror is a limb; torso only if both are.
+        const int want = (isLimb || mirrorIsLimb) ? sideLimb : shared;
🤖 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/MeshSegmenter.cpp` around lines 804 - 813, In the branch assigning want
within the isLimb/mirrorIsLimb logic, remove the redundant four-branch structure
and dead want = lf initialization. Replace it with the equivalent single
expression that selects sideLimb whenever either segment is a limb, otherwise
selects shared, preserving the existing union behavior.

757-778: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider a hashed grid restricted to limb/shared faces.

grid is a std::map over every face, and nearestFace performs 27 tree lookups per candidate. Only limb/shared faces can ever be a useful mirror target, and a packed uint64 cell key in an unordered_map avoids the O(log n) per probe. If you switch to an unordered container, keep an explicit tie-break (e.g. prefer the lower face index on equal distance) so results stay deterministic across runs/platforms.

♻️ Sketch
-    std::map<std::tuple<int,int,int>, std::vector<int>> grid;
-    for (int f = 0; f < faceCount; ++f) {
-        const Vec3& c = centroid[f];
-        grid[key(c.comp(0),c.comp(1),c.comp(2))].push_back(f);
-    }
+    std::unordered_map<uint64_t, std::vector<int>> grid;
+    for (int f = 0; f < faceCount; ++f) {
+        const int lf = faceLabels[f];
+        if (lf != limbLeft && lf != limbRight && lf != shared) continue;
+        const Vec3& c = centroid[f];
+        grid[cellKey(c.comp(0), c.comp(1), c.comp(2))].push_back(f);
+    }

with the nearest search keeping if (d < bd || (d == bd && f < best)).

🤖 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/MeshSegmenter.cpp` around lines 757 - 778, Restrict the spatial hash used
by nearestFace to limb/shared faces only, replacing the std::map tuple key with
a packed uint64 cell key in an unordered_map to avoid tree lookups. Update grid
construction and the 27-cell search consistently, and make nearestFace
deterministic by selecting the lower face index when distances are equal.
🤖 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/MeshSegmenter.cpp`:
- Around line 712-718: Update the sagittal-centre calculation in the surrounding
MeshSegmenter flow to use the midpoint of the left and right limb means, rather
than the face-count-weighted latSum/latN average. Move the per-limb accumulation
block above this calculation, derive latCentre from both limb means, and remove
the now-redundant combined accumulation pass while preserving the existing
zero-data handling.

---

Nitpick comments:
In `@src/MeshSegmenter.cpp`:
- Around line 804-813: In the branch assigning want within the
isLimb/mirrorIsLimb logic, remove the redundant four-branch structure and dead
want = lf initialization. Replace it with the equivalent single expression that
selects sideLimb whenever either segment is a limb, otherwise selects shared,
preserving the existing union behavior.
- Around line 757-778: Restrict the spatial hash used by nearestFace to
limb/shared faces only, replacing the std::map tuple key with a packed uint64
cell key in an unordered_map to avoid tree lookups. Update grid construction and
the 27-cell search consistently, and make nearestFace deterministic by selecting
the lower face index when distances are equal.
🪄 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 Plus

Run ID: e709e4d8-f449-4cab-a8ed-8456b46a90e9

📥 Commits

Reviewing files that changed from the base of the PR and between f340679 and 56e8c72.

📒 Files selected for processing (4)
  • CLAUDE.md
  • src/MeshSegmenter.cpp
  • src/MeshSegmenter.h
  • src/MeshSegmenter_test.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/MeshSegmenter.h
  • src/MeshSegmenter_test.cpp
  • CLAUDE.md

Comment thread src/MeshSegmenter.cpp Outdated
fernandotonon and others added 3 commits July 27, 2026 11:05
User feedback: legs are now the right size/angle (mirror fix worked) but the
seam still looks torn/broken between the legs — not a clean cut like the arms.

Diagnosis (measured on Hip Hop Dancing): the leg↔torso label boundary wanders
across ~8.6% of body height (a wide ragged band), so the split tears there. The
arms look clean only because they're topologically DISCONNECTED from the torso
in the model output (no shared seam at all) — the legs can't be, being attached
at the hip.

New cleanLimbSeam pass (legs only, after levelLimbCut): a leg attaches around a
roughly horizontal hip ring, so per-leg it sets the cut height to the MEDIAN
up-value of that leg's seam faces and reassigns the seam-proximity band (bounded
edge-hop flood — so the centred torso skirt isn't dragged and each foot stays
with its own leg) strictly by side of that height. Collapses the wandering band
onto a level ring: leg↔torso seam raggedness ~8.6% → ~4-6% of body height
(roughly halved), feet still correct, legs still symmetric.

Honest limit noted in the header + CLAUDE.md: label-only cutting can't beat the
mesh face resolution — a truly knife-thin seam needs real triangle slicing
(Boolean/knife), which #859 lists as out of scope. This is the best clean-up
achievable without new geometry.

36 MeshSegmenter tests pass (+CleanLimbSeamThinsWanderingBoundary).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
MCP take_screenshot returned a black image and load_mesh left the camera
un-framed, so headless/autonomous visual QA (load → explode → screenshot)
was impossible. Two fixes:

- take_screenshot now renders the active viewport's SpaceCamera into an
  offscreen PF_BYTE_RGBA RTT and reads it back, instead of QWidget::grab().
  Ogre renders straight to the native window surface (WA_PaintOnScreen), so
  grab() only ever captured an empty Qt buffer. The RTT uses the RTSS
  MSN_SHADERGEN scheme + a temporary ambient boost and directional key light
  (restored afterward) so imported materials render lit, not black. Optional
  width/height args; defaults to the widget size.
- load_mesh calls frameSceneInActiveViewport() (select every user scene node →
  SpaceCamera::frameSelection(), which no-ops on an empty selection) so the
  freshly-loaded mesh is actually in frame for the next screenshot.

Verified end to end over the HTTP API: load a split character → transform_submesh
to explode the parts → take_screenshot returns a fully textured, lit, framed
image of the exploded model. 35 MeshSegmenter tests still pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 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/MCPServer.cpp`:
- Around line 2996-3013: The screenshot export in the code handling
image.save(path) lacks breadcrumb tracking. Add a SentryReporter::addBreadcrumb
call immediately before the save operation, using the file.export category and
including the screenshot path or relevant export context, while preserving the
existing save error handling and success flow.
- Around line 1619-1646: Update MCPServer::frameSceneInActiveViewport and its
load-call flow so reframing targets only scene nodes created by the current
import, rather than clearing SelectionSet and selecting every node with attached
objects. Preserve the user’s existing selection and camera view when loading
through an interactive GUI; only frame the newly imported nodes when that
behavior is explicitly required for the import.
- Around line 1619-1646: The new frameSceneInActiveViewport operation lacks
breadcrumb tracking. Add a SentryReporter::addBreadcrumb call within
MCPServer::frameSceneInActiveViewport, using the appropriate ui.action category
for this user-facing camera-framing and selection action, while preserving the
existing selection and camera behavior.
- Around line 1605-1617: Separate the mesh import and camera-framing error
handling in the surrounding operation: keep importMeshs() failures returning the
existing Ogre error, but invoke frameSceneInActiveViewport() in its own guarded
block after a successful import. If framing throws, preserve the successful
import result and avoid reporting it as an import failure.
- Around line 2949-3020: Ensure screenshot capture cleanup runs on every
exception, not only the success path. In the screenshot-rendering flow, move the
scene manager, saved ambient light, temporary light/node, and previous camera
aspect state into scope reachable by failure cleanup, or add an RAII guard that
restores ambient lighting and camera aspect ratio and destroys the temporary
light/node; preserve RTT removal and avoid double cleanup after successful
capture.
🪄 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 Plus

Run ID: 23747eb5-22bf-41db-96a9-ec64e02b608d

📥 Commits

Reviewing files that changed from the base of the PR and between 56e8c72 and 1e75977.

📒 Files selected for processing (3)
  • CLAUDE.md
  • src/MCPServer.cpp
  • src/MCPServer.h
🚧 Files skipped from review as they are similar to previous changes (1)
  • CLAUDE.md

Comment thread src/MCPServer.cpp
Comment thread src/MCPServer.cpp
Comment thread src/MCPServer.cpp
Comment thread src/MCPServer.cpp
fernandotonon and others added 2 commits July 27, 2026 12:19
…bbit)

CodeRabbit: latCentre was a face-count-weighted mean over all limb faces, so
the leg with more faces pulled the sagittal midline toward itself — exactly the
asymmetry this pass exists to fix — which could flip near-crotch faces to the
wrong leg. Use the MIDPOINT of the two per-limb lateral means instead
(count-independent); also derive rightIsPositive from those means. Verified on
Hip Hop Dancing.obj: legs 468/470 (ratio 1.00), feet stay on their own side.

Test refocused to the robust no-swap invariant (LevelLimbCutNeverSwapsLegSides)
— the mirror pass's detailed reassignment is geometry-dependent (nearest-mirror
over a real 3D hip) and is validated on the real model, not a brittle 2D grid.

The other two Codex comments (P1 hip-band relabel, P2 whole-mesh AABB divider)
were on the reverted horizontal-cut code (commit f340679) and no longer apply —
the current mirror path uses neither a height band nor the whole-mesh AABB.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The RTT-screenshot rewrite changed the no-widget error message to "No active
viewport camera", breaking the existing
MCPServerTest.TakeScreenshot_MainWindowWithoutOgreWidgetReturnsError (CI
unit-tests-linux). Restore the established "Error: OgreWidget not found" for the
no-widget case (a QMainWindow with no OgreWidget); the new "No active viewport
camera" is kept only for the has-widget-but-no-camera case. Screenshot tests
pass locally.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/MeshSegmenter_test.cpp`:
- Around line 567-578: Strengthen the fixture in the affected levelLimbCut test
so the previous count-weighted divider would misclassify a near-midline face:
increase the right-leg face-count imbalance while preserving per-leg lateral
means and the correct midpoint. Update the assertions to explicitly verify that
the affected near-midline face retains label 5, while preserving the existing
side-correctness checks for labels 5 and 6.
🪄 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 Plus

Run ID: 2b3acefa-aa04-4684-ae02-54fd4ffa2902

📥 Commits

Reviewing files that changed from the base of the PR and between 1e75977 and 4b134c2.

📒 Files selected for processing (3)
  • src/MCPServer.cpp
  • src/MeshSegmenter.cpp
  • src/MeshSegmenter_test.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/MCPServer.cpp
  • src/MeshSegmenter.cpp

Comment thread src/MeshSegmenter_test.cpp Outdated
…ming + test

MCP screenshot (take_screenshot):
- CRITICAL: the temp capture light/node, boosted ambient, altered camera aspect,
  and the RTT were leaked into the live scene if copyContentsToMemory / image
  alloc threw. Hoist that state out of the try and restore it via a single
  cleanup() lambda called on BOTH the success and the catch path.
- Add a `file.export` Sentry breadcrumb on a successful save (project convention).

load_mesh framing:
- Move frameSceneInActiveViewport() OUTSIDE the import try so a framing throw
  can't mask a SUCCESSFUL import as an error (it swallows its own Ogre errors).
- Preserve the user's selection: snapshot the current node selection, frame, then
  restore it — so auto-framing on load doesn't clobber an interactive
  --with-mcp session's selection.

Test:
- Replace the no-swap fixture with LevelLimbCutDividerIsCountIndependent: a
  deliberately wide left leg vs narrow right leg, so a face-count-weighted
  divider is pulled left while the correct midpoint-of-per-limb-means stays
  centred; asserts no right-leg face lands deep in left territory (and vice
  versa), distinguishing the two dividers.

Verified: RTT screenshot still captures textured+lit end to end; 35 MeshSegmenter
+ 8 MCP screenshot/load tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@fernandotonon
fernandotonon merged commit 2f7a646 into master Jul 28, 2026
38 of 39 checks passed
@fernandotonon
fernandotonon deleted the feat/segment-level-leg-cut-863 branch July 28, 2026 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant