UV: Slice G — CLI/MCP parity for UV editor tools (#465) - #777
Conversation
Centralize headless UV info, projection, seam marking, and partial unwrap in UvPipeline and wire qtmesh uv plus MCP uv_* tools with tests and docs. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 11 minutes and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughIntroduces ChangesUvPipeline CLI/MCP Parity
Sequence Diagram(s)sequenceDiagram
participant CLI as qtmesh CLI / MCP Client
participant cmdUv as CLIPipeline::cmdUv / MCPServer::toolUv*
participant UvPipeline
participant UvProject
participant UvUnwrap
participant OgreEntity as Ogre::Entity
CLI->>cmdUv: --project box / uv_project{mode:"box"}
cmdUv->>UvPipeline: parseProjectMode("box")
cmdUv->>UvPipeline: projectEntity(entity, Box, uvChannel, opts)
UvPipeline->>OgreEntity: load into EditableMesh, preload UVs
UvPipeline->>UvProject: project(editableMesh, Box, opts)
UvPipeline->>OgreEntity: commit updated UVs
UvPipeline-->>cmdUv: UvProject::Report {vertsChanged}
cmdUv-->>CLI: JSON result
CLI->>cmdUv: --set-seams "0:1-2,0:3-4" / uv_set_seams{edges:...}
cmdUv->>UvPipeline: parseSeamEdgeList(spec)
cmdUv->>UvPipeline: setSeamsOnEntity(entity, edges)
UvPipeline->>OgreEntity: load, validate, UvSeamData::setSeam, write back
UvPipeline-->>cmdUv: bool ok
cmdUv-->>CLI: JSON result
CLI->>cmdUv: uv_unwrap_selection{submesh, triangles}
cmdUv->>UvPipeline: unwrapTriangles(entity, subMeshIndex, triangleIndices, opts)
UvPipeline->>OgreEntity: build faceMask, load seamEdgeKeys
UvPipeline->>UvUnwrap: unwrapEntity(entity, opts+faceMask)
UvPipeline-->>cmdUv: UvUnwrapReport
cmdUv-->>CLI: JSON result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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: 9b1bbb0536
ℹ️ 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".
| if (ti >= 0 && ti < triCount) | ||
| mask.includeTriangle[static_cast<size_t>(ti)] = true; | ||
| } | ||
| runOpts.faceMasks.push_back(mask); |
There was a problem hiding this comment.
When uv_unwrap_selection is used on a multi-submesh entity, only the target submesh gets a FaceMask; UvUnwrap::unwrapEntity treats submeshes with no mask as fully included, so every other submesh is xatlas-unwrapped even though the caller selected triangles on one submesh. This can silently replace unrelated UVs; add all-false masks for non-target submeshes or otherwise restrict the unwrap to the requested submesh.
Useful? React with 👍 / 👎.
| SelectionSet* sel = SelectionSet::getSingleton(); | ||
| if (!sel || sel->getEntitiesCount() == 0) | ||
| return makeErrorResult("No selected entity."); |
There was a problem hiding this comment.
hasSelectedEntities() already accepts scene-node and sub-entity selections via getResolvedEntities(), but this gate then rejects them by checking the raw entity list. The normal MCP load_mesh flow leaves the imported object selected as a scene node, so uv_info (and the same repeated check in uv_project, uv_set_seams, and uv_unwrap_selection) returns "No selected entity" immediately after loading a mesh instead of operating on the resolved entity.
Useful? React with 👍 / 👎.
| if (uvChannel != 0) | ||
| applyUvChannelFromEntity(mesh, const_cast<Ogre::Entity*>(entity), uvChannel); |
There was a problem hiding this comment.
For --info --channel N / uv_info with a channel that is absent on some or all submeshes, applyUvChannelFromEntity simply leaves the EditableMesh UVs that loadFromEntity populated from UV0. The subsequent island and overlap report is therefore computed from UV0 while the caller asked about channel N, which gives false coverage/overlap results for missing lightmap channels; clear hasUV or report N/A when the requested channel cannot be read.
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: 8
🧹 Nitpick comments (2)
src/UvPipeline_test.cpp (1)
5-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the accepted projection aliases and normalization here.
parseProjectMode()also accepts trimmed/case-insensitive inputs pluscyl,sph,reset_box, andresetbox. These tests only pin the canonical spellings, so a regression in the CLI/MCP input variants added by this PR would still pass.Example coverage
TEST(UvPipelineParseProjectMode, AcceptsKnownModes) { bool ok = false; + EXPECT_EQ(UvPipeline::parseProjectMode(QStringLiteral(" CYL "), &ok), + UvProject::Mode::Cylinder); + EXPECT_TRUE(ok); + EXPECT_EQ(UvPipeline::parseProjectMode(QStringLiteral("sph"), &ok), + UvProject::Mode::Sphere); + EXPECT_TRUE(ok); + EXPECT_EQ(UvPipeline::parseProjectMode(QStringLiteral("reset_box"), &ok), + UvProject::Mode::ResetBox); + EXPECT_TRUE(ok);🤖 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/UvPipeline_test.cpp` around lines 5 - 29, Expand the UvPipeline::parseProjectMode test coverage to include the accepted alias and normalization inputs, not just the canonical spellings. Update UvPipelineParseProjectMode::AcceptsKnownModes and related checks to verify trimmed and case-insensitive values, plus the additional accepted forms cly, sph, reset_box, and resetbox, so regressions in the CLI/MCP parsing path are caught.src/CLIPipeline_cmduv_coverage_test.cpp (1)
96-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise gate 4 with one of the new mode flags.
MultipleModesSpecifiedstill uses only--infoand--unwrap. Since this PR extends the parser with--projectand--set-seams, a bug where either new flag is not counted by the “one mode only” check would slip past this test.Example tweak
TEST(CLIPipelineCmdUvCoverageError, MultipleModesSpecified) { - UvTestArgv args({"qtmesh", "uv", "model.fbx", "--info", "--unwrap", "-o", "out.glb"}); + UvTestArgv args({"qtmesh", "uv", "model.fbx", "--info", "--project", "box", + "-o", "out.glb"}); EXPECT_EQ(CLIPipeline::cmdUv(args.argc(), args.argv()), 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/CLIPipeline_cmduv_coverage_test.cpp` around lines 96 - 100, The MultipleModesSpecified coverage test only exercises --info and --unwrap, so it misses the new parser modes added in cmdUv. Update the test in CLIPipelineCmdUvCoverageError to include either --project or --set-seams alongside an existing mode flag, and keep asserting CLIPipeline::cmdUv returns the same “multiple modes” error so the one-mode-only check is validated for the new flags too.
🤖 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 7928-7933: The UV command parser/usage in CLIPipeline’s `uv` mode
currently only recognizes `--info`, `--unwrap`, `--project`, and `--set-seams`,
so the required `--pack` workflow is missing. Update the parsing/validation and
help text around the `inputPath`, `outputPath`, `projectMode`, and `seamSpec`
handling to accept `--pack` with the required margin control options, and make
sure the mode dispatch path treats it as a first-class UV mode rather than
rejecting it or leaving it undocumented.
In `@src/MCPServer.cpp`:
- Around line 583-586: The MCP UV tool registry is missing the required uv_pack
entry, so MCP parity is incomplete. Add a uv_pack handler in MCPServer, register
it alongside toolUvInfo/toolUvProject/toolUvSetSeams/toolUvUnwrapSelection in
the UV tools map, and update the tool schema/advertised objectives so uv_pack is
exposed consistently; if the implementation is not ready, do not mark the `#465`
objective as fully closed.
- Around line 1591-1595: The MCP uv projection path is not matching the CLI
because uv_project in MCPServer.cpp forwards invalid scale values directly while
cmdUv normalizes non-positive scale to 1.0. Update the uv_project handling
around UvProject::Options opts so args["scale"] is clamped or rejected
consistently with the CLI before assigning opts.boxScale, and keep the behavior
aligned with cmdUv and the existing MCP argument parsing.
- Around line 6865-6903: The MCP tool schemas for uv_set_seams and
uv_unwrap_selection are missing required-field declarations even though the
handlers expect edges and triangles to always be present. Update the JSON schema
objects built in MCPServer’s tool registration blocks for appendTool so that
uv_set_seams marks edges as required and uv_unwrap_selection marks triangles as
required, matching the expectations of toolUvSetSeams and toolUvUnwrapSelection.
- Around line 1662-1669: The submesh/triangles parsing in the MCPServer request
handler is silently coercing non-integer JSON values to 0 via toInt(), which can
accept malformed input as valid. Update the logic around the args["submesh"] and
args["triangles"] handling to explicitly validate that submesh is a JSON integer
and that every triangles entry is an integer within the expected range before
converting, otherwise return a makeErrorResult failure instead of defaulting to
0.
In `@src/UvPipeline.cpp`:
- Around line 330-354: Validate the selection inputs in UvPipeline before
mutating runOpts: in the submesh index check, reject values that cannot be
represented safely instead of casting subMeshIndex to unsigned short, and use
the same validated index consistently when calling getSubMesh. Also, in the
triangleIndices loop, detect any out-of-range triangle IDs and return a failure
immediately rather than silently skipping them, so UvUnwrapOptions::FaceMask is
only built from fully valid input.
- Around line 62-78: The overlap ratio in the UvPipeline overlap scan only
counts the first triangle in each overlapping pair, so it underreports the
fraction of triangles with overlapping UV AABBs. Update the overlap counting
logic in the loop over order/boxes so both triangles in a detected overlap are
counted, and make the final ratio reflect triangles with at least one overlap
rather than pair starts.
- Around line 95-110: The UV extraction in UvPipeline.cpp assumes every
VES_TEXTURE_COORDINATES element is a 2-float layout before reading p[1]. Update
the loop around baseVertexPointerToElement() to detect the element’s actual type
and only read as Ogre::Vector2 for VET_FLOAT2-compatible data; for VET_FLOAT1
and packed short/ubyte encodings, either skip the channel or convert it safely
before assigning to uvs. Keep the fix localized to the texture-coordinate read
path that uses elem, vbuf, and base.
---
Nitpick comments:
In `@src/CLIPipeline_cmduv_coverage_test.cpp`:
- Around line 96-100: The MultipleModesSpecified coverage test only exercises
--info and --unwrap, so it misses the new parser modes added in cmdUv. Update
the test in CLIPipelineCmdUvCoverageError to include either --project or
--set-seams alongside an existing mode flag, and keep asserting
CLIPipeline::cmdUv returns the same “multiple modes” error so the one-mode-only
check is validated for the new flags too.
In `@src/UvPipeline_test.cpp`:
- Around line 5-29: Expand the UvPipeline::parseProjectMode test coverage to
include the accepted alias and normalization inputs, not just the canonical
spellings. Update UvPipelineParseProjectMode::AcceptsKnownModes and related
checks to verify trimmed and case-insensitive values, plus the additional
accepted forms cly, sph, reset_box, and resetbox, so regressions in the CLI/MCP
parsing path are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5bea19c-1932-416d-8241-11983e7d14ba
📒 Files selected for processing (10)
CLAUDE.mdsrc/CLIPipeline.cppsrc/CLIPipeline_cmduv_coverage_test.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/UvPipeline.cppsrc/UvPipeline.hsrc/UvPipeline_test.cpptests/CMakeLists.txt
| // Parse: | ||
| // uv <file> --info [--json] [--channel N] | ||
| // uv <file> --unwrap [--resolution N] [--padding P] [--channel C] [--no-backup] -o out | ||
| // uv <file> --project box|cylinder|sphere|reset [--axis N] [--scale S] [--channel C] -o out [--json] | ||
| // uv <file> --set-seams "0:1-2,0:2-3" -o out | ||
| QString inputPath, outputPath, projectMode, seamSpec; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Wire the required --pack UV mode before closing Slice G.
The parser/usage/mode validation only accepts --info, --unwrap, --project, and --set-seams, but the linked objectives require qtmesh uv --pack with margin control. Add the pack mode here or keep the issue open/defer it explicitly.
Also applies to: 7981-8001
🤖 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 7928 - 7933, The UV command parser/usage in
CLIPipeline’s `uv` mode currently only recognizes `--info`, `--unwrap`,
`--project`, and `--set-seams`, so the required `--pack` workflow is missing.
Update the parsing/validation and help text around the `inputPath`,
`outputPath`, `projectMode`, and `seamSpec` handling to accept `--pack` with the
required margin control options, and make sure the mode dispatch path treats it
as a first-class UV mode rather than rejecting it or leaving it undocumented.
| {QStringLiteral("uv_info"), &MCPServer::toolUvInfo}, | ||
| {QStringLiteral("uv_project"), &MCPServer::toolUvProject}, | ||
| {QStringLiteral("uv_set_seams"), &MCPServer::toolUvSetSeams}, | ||
| {QStringLiteral("uv_unwrap_selection"), &MCPServer::toolUvUnwrapSelection}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Add the missing uv_pack MCP tool.
The new UV registry/schema advertises four UV tools, but the linked objectives require uv_pack as part of MCP parity. Add the handler, registration, and tool schema, or avoid claiming #465 is fully closed.
Also applies to: 6833-6904
🤖 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/MCPServer.cpp` around lines 583 - 586, The MCP UV tool registry is
missing the required uv_pack entry, so MCP parity is incomplete. Add a uv_pack
handler in MCPServer, register it alongside
toolUvInfo/toolUvProject/toolUvSetSeams/toolUvUnwrapSelection in the UV tools
map, and update the tool schema/advertised objectives so uv_pack is exposed
consistently; if the implementation is not ready, do not mark the `#465` objective
as fully closed.
Use resolved MCP selections, mask non-target submeshes on partial unwrap, validate triangle/submesh indices, fix overlap counting and non-zero channel info, and align MCP scale handling with the CLI. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Review follow-up in 883fdea: Fixed
Intentionally deferred (Slice F rejected)
|
|
1 participant
Summary
UvPipelineas the shared headless layer for UV info, geometric projection, seam marking, and partial xatlas unwrap.qtmesh uvwith--info(islands + overlap),--project, and--set-seams; add MCP toolsuv_info,uv_project,uv_set_seams, anduv_unwrap_selection.CLAUDE.md.Closes #465.
Test plan
./build_local/bin/UnitTests --gtest_filter="UvPipeline*:CLIPipelineCmdUvCoverage*"(23 tests pass)qtmesh uv model.fbx --info --jsonqtmesh uv model.fbx --project box -o out.glbuv_info/uv_projecton a loaded meshMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes