UV: Slice D — Seams, sewing, and selection-scoped re-unwrap (#462) - #775
Conversation
Add seam/pin data on EditableMesh with Ogre mesh bindings, Edit Mode and UV panel tools (mark, pin, sew, split, unwrap selected), undo support, and a detached UV editor window under Material Mode tools. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 5 minutes and 9 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 (13)
📝 WalkthroughWalkthroughImplements UV Slice D: adds seam/pin persistence on ChangesUV Seam / Pin / Sew / Split / Unwrap (Slice D)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 85b31b7479
ℹ️ 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 (!faceIgnore.empty()) | ||
| decl.faceIgnoreData = reinterpret_cast<const bool*>(faceIgnore.data()); |
There was a problem hiding this comment.
When unwrapSelectedFaces() passes a mask with only some triangles selected, setting faceIgnoreData here does not preserve the skipped triangles' existing UVs: no current UV stream is provided to xatlas, and the code later replaces the entire submesh vertex/index buffers with xatlas output. As a result, unselected faces in the same submesh can lose or change their UVs even though this is advertised as selection-scoped re-unwrap; splice generated UVs only into included triangles or explicitly copy old UVs back for ignored faces.
Useful? React with 👍 / 👎.
| const Ogre::Vector2 avg( | ||
| (sub.vertices[a].uv.x + sub.vertices[b].uv.x) * 0.5f, | ||
| (sub.vertices[a].uv.y + sub.vertices[b].uv.y) * 0.5f); |
There was a problem hiding this comment.
For a normal selected UV edge (a,b), this averages the two edge endpoints together and then assigns that single midpoint UV to every vertex at either endpoint position. After splitting an edge, selecting one side and sewing it will collapse both ends of the edge to one UV point rather than pairing coincident duplicates per endpoint, corrupting the UV layout; sew should average duplicates at pa separately from duplicates at pb.
Useful? React with 👍 / 👎.
| if (auto* uv = UVEditorController::instance()) { | ||
| if (auto* wm = uv->workingMeshForEntity(entity)) | ||
| return wm; | ||
| } | ||
| return nullptr; |
There was a problem hiding this comment.
Seam/pin/topology undo commands become no-ops if the user changes modes or closes the UV editor before undoing, because meshForEntity() only returns the edit-mode mesh or the UV editor working mesh and otherwise returns nullptr. In that scenario the undo stack still advances but the seam/pin/topology change remains on the Ogre mesh; mirror UVEditCommand's fallback by loading a temporary EditableMesh from the entity and committing it back.
Useful? React with 👍 / 👎.
Destroy the seam overlay ManualObject in destroySelectionOverlay so Edit Mode tests can re-enter without duplicate-name crashes. Sew only the selected edge endpoints, load EditableMesh from entity for undo when UV/edit controllers are inactive, and preserve source UVs on verts outside a partial unwrap selection. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the Codex review items in b9b3fa5:
FBX/glTF round-trip for seams/pins remains out of scope for this slice (Ogre UOB bindings only), as noted in the PR description. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
src/EditableMesh.cpp (1)
418-419: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd breadcrumbs around seam-binding load/persist paths.
These new hooks mutate persistent mesh state during load/commit/resize, but they currently leave no breadcrumb trail when seam/pin state is rehydrated or flushed back to Ogre. As per coding guidelines,
src/**/*.{cpp,h}must “Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message)` using the established breadcrumb categories.Also applies to: 809-809, 1374-1374
🤖 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/EditableMesh.cpp` around lines 418 - 419, The seam-binding load/persist flow is missing breadcrumb tracking around significant state changes. Add SentryReporter::addBreadcrumb calls in the EditableMesh paths that rehydrate or flush seam/pin state, including the UvSeamData::readBindingsFromMesh load path and the corresponding commit/resize persistence hooks referenced by this change. Use the established breadcrumb categories and include a clear message for each operation so user-facing mesh-state mutations are traceable.Source: Coding guidelines
src/UvSeamData_test.cpp (1)
67-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an integration test for the new
EditableMeshhooks too.This only round-trips through
UvSeamData::writeBindingsToMesh()directly, so it will not catch regressions whereEditableMesh::commitToEntity()orEditableMesh::resizeEntityBuffers()stop persisting seam/pin bindings. As per coding guidelines,src/**/*_test.cppshould “Add Google Test unit tests for new functionality.”🤖 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/UvSeamData_test.cpp` around lines 67 - 84, The current test only verifies direct round-tripping through UvSeamData::writeBindingsToMesh(), so it misses regressions in the new EditableMesh persistence hooks. Extend UvSeamDataTest by adding an integration test that exercises EditableMesh::commitToEntity() and/or EditableMesh::resizeEntityBuffers() after calling setSeam and setPinned on a submesh, then reloads the mesh and asserts the bindings still persist. Keep the existing round-trip coverage, but add the new test around EditableMesh, mesh.loadFromMesh(), and the seam/pin accessors to cover the hook path.Source: Coding guidelines
src/UvUnwrap_test.cpp (1)
131-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify untouched faces, not just that unwrap succeeded.
This only checks
report.appliedandchartCount, so it will not catch regressions where ignored triangles are rewritten anyway. Seed the mesh with known UVs and assert that the six unselected triangles keep those coordinates after unwrap.🤖 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/UvUnwrap_test.cpp` around lines 131 - 151, The UvUnwrapTest_partial test only verifies that unwrapEntity succeeds and creates charts, so it misses regressions where non-masked triangles get modified. Update the test around UvUnwrap::unwrapEntity and UvUnwrapOptions::FaceMask to seed the mesh with known UVs before unwrapping, then assert that the unselected triangles in the masked submesh keep their original UV coordinates after the call while only the selected triangles are changed.src/UvSeamOps_test.cpp (1)
43-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise
sewEdges()with a real selected edge.This test uses
makeEdgeKey(0, 2), but0and2are coincident duplicate vertices, not an edge the caller can produce. That blesses the current implementation rather than the public contract, so a real split→sew regression on edge(a, b)would still pass here. Build the seam viasplitEdges()and then sew that same edge key back.🤖 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/UvSeamOps_test.cpp` around lines 43 - 70, The UvSeamOpsTest::SewAveragesCoincidentUvPositions case is using makeEdgeKey(0, 2) for coincident duplicate vertices instead of a real selected edge, so it does not exercise the public sewEdges() contract. Update the test to create the seam through splitEdges() and then call sewEdges() with that returned edge key, so the test covers an actual split-to-sew flow and validates UvSeamData::setSeam/isSeam behavior on a real edge.
🤖 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/PropertiesPanel.qml`:
- Around line 2554-2558: The UV Edit panel keeps inspectorEmbedded enabled when
sectionVisible hides the outer section but the Loader remains active, so the
cleanup in the Loader’s Component.onDestruction never runs. Update the UV Edit
visibility handling in PropertiesPanel.qml to explicitly clear
UVEditorController.setInspectorEmbedded(false) whenever the UV Edit section is
hidden by mode/selection changes, using the existing UVEditorController and
Loader/Component.onCompleted/Component.onDestruction flow as the place to hook
the reset.
In `@qml/UVEditorPanel.qml`:
- Around line 651-653: The selection overlay in UVEditorPanel.qml is being
covered by the pin markers, so selected pinned vertices lose their selected
appearance. Update the draw order around drawSelection and drawPinnedVertices,
or adjust the pinned-vertex rendering so selected vertices remain visibly
selected when both states apply. Use the existing drawSeamEdges, drawSelection,
and drawPinnedVertices rendering flow to keep the selection state on top for
pinned vertices.
In `@src/commands/UvSeamCommands.cpp`:
- Around line 207-219: UvSeamTopologyCommand::undo() and
UvSeamTopologyCommand::redo() mutate the mesh but do not record a breadcrumb
like the seam/pin command paths do. Update these methods in
UvSeamTopologyCommand to add the same breadcrumb recording used elsewhere before
calling applyMesh(m_before) or applyMesh(m_after), while preserving the existing
m_firstRedo behavior in redo().
- Around line 92-110: The seam/pin apply helpers currently update the mesh and
refresh the UV editor, but they skip the normal edit-mode change notification
path. Update UvSeamMarkCommand::apply and the corresponding pin command helper
to invoke EditModeController::notifyMeshDataChanged() after syncEditMeshFrom(),
or refactor them to reuse commitUvMeshState() so the initial action and
undo/redo both notify observers consistently.
In `@src/EditModeController.cpp`:
- Around line 796-801: The mark/clear seam path updates the editable mesh and
entity data but leaves the UV editor’s working state stale, so sync the UV view
after seam changes. Update the seam-edit flow around
UVSeamData::writeBindingsToMesh, UndoManager::push with UvSeamMarkCommand, and
the related clear-seam path to refresh UVEditorController and call
refreshAfterUvEdit() whenever seams change. Keep the existing selection/mesh
notifications, but ensure the UV editor’s m_workingMesh and seam overlay/query
data are rebuilt immediately after these operations.
In `@src/UVEditorController.cpp`:
- Around line 1982-1990: The sew flow in UVEditorController::sewEdges() updates
seam state in m_workingMesh but never persists the changed qtme.seams.* bindings
to the entity mesh. After UvSeamOps::sewEdges() succeeds, write the updated seam
bindings back using the same persistence path used by the mark/clear, pin/unpin,
split, and undo helpers, then keep commitWorkingMeshUvs() and the
undo/breadcrumb logic as-is.
- Around line 752-764: In UVEditorController::syncWorkingMeshFromEntity, avoid
always calling m_workingMesh.loadFromEntity(m_activeEntity) when
EditModeController::instance()->isEditModeActive() and edit->editEntity()
matches the active entity, because edit->currentMesh() is the authoritative mesh
state in that case. Update the sync logic to prefer the existing
currentMesh/subMeshes data from EditModeController when edit mode owns the mesh,
and only fall back to loading from the entity when edit mode is inactive or not
targeting m_activeEntity; keep applyUvChannel and the submesh copy path aligned
with that source of truth.
- Around line 1996-2036: splitSelectedEdges() and unwrapSelectedFaces() update
the mesh but do not follow the controller’s normal change-notification path.
After the topology/UV edits succeed, bump m_meshRevision and emit
meshDataChanged() the same way the other mutators in UVEditorController do,
keeping the existing refresh() call if needed. Use the splitSelectedEdges() and
unwrapSelectedFaces() flows as the place to restore the missing notification so
downstream listeners are informed of the mesh change.
In `@src/UvSeamData_test.cpp`:
- Around line 69-70: The Ogre-dependent test setup currently skips execution
when tryInitOgre() fails, which hides regressions; update the test
initialization in UvSeamData_test.cpp to use ASSERT_TRUE(tryInitOgre()) instead
of GTEST_SKIP(), and also assert canLoadMeshFiles() where that prerequisite is
required so failures surface loudly. Use the existing test setup code around
tryInitOgre() to apply the change without altering the rest of the test body.
In `@src/UvSeamData.cpp`:
- Around line 130-144: readBindingsFromMesh currently accepts persisted seam/pin
local ids without validating them, so stale or corrupt bindings can repopulate
subMeshes[i].seamEdges and subMeshes[i].pinnedVertices with out-of-range
vertices. Update readBindingsFromMesh to filter the deserialized EdgeKeyList and
PinList against the valid local vertex range for subMeshes[i] before inserting,
using the existing seamsKey/pinsKey and subMeshes[i].vertices data to locate the
checks. Keep only bindings that resolve to vertices present in the current
submesh and discard invalid entries during reload.
In `@src/UvSeamOps.cpp`:
- Around line 91-95: The seam-duplication path in UvSeamOps::splitEdge / related
seam copy logic creates new vertices with copyVertex but does not preserve
pinned state, so the duplicated seam endpoints can become movable unexpectedly.
Update the vertex duplication flow for newA/newB to copy the source vertex’s pin
flag (and any other pin-related state tracked separately) from sub.vertices[a]
and sub.vertices[b] onto the newly pushed vertices, including the same fix in
the matching logic referenced by the second location.
- Around line 165-177: The sewEdges/sewOne path is using the passed edge
endpoints directly, which averages the wrong UVs and makes splitEdges() not
reversible. Update sewOne to resolve the duplicated seam-side vertices for the
mesh-edge key (a, b) before averaging, so it sews the two split copies rather
than collapsing the original edge; keep UvSeamData::setSeam in sync with the
actual paired vertices being rejoined.
In `@src/UvUnwrap.cpp`:
- Around line 149-165: buildIncludedSourceVerts() is too coarse because it marks
a whole source vertex as included whenever any connected triangle is selected,
which causes shared boundary vertices to pull ignored faces into
buildUnwrappedSubmesh(). Update the selection tracking so inclusion is per
source-face corner or per xatlas clone/xref rather than one boolean per source
vertex, and use that finer-grained mask when deciding which UVs to rewrite. Keep
the fix localized around buildIncludedSourceVerts() and the selection handling
in buildUnwrappedSubmesh().
- Around line 47-115: buildFaceMaterials() is doing an unnecessary O(T²)
neighbor prepass even when no seam partitioning is needed; update the function
to short-circuit the chart-building path when seamEdges is empty (or equivalent
no-seam case) so the default unwrap flow skips the pairwise triangle comparison
entirely. Make the change in buildFaceMaterials() and ensure the call site in
runUnwrap() still passes through the fast path for submeshes without seam data,
while preserving the existing chart logic when seamEdges is present.
---
Nitpick comments:
In `@src/EditableMesh.cpp`:
- Around line 418-419: The seam-binding load/persist flow is missing breadcrumb
tracking around significant state changes. Add SentryReporter::addBreadcrumb
calls in the EditableMesh paths that rehydrate or flush seam/pin state,
including the UvSeamData::readBindingsFromMesh load path and the corresponding
commit/resize persistence hooks referenced by this change. Use the established
breadcrumb categories and include a clear message for each operation so
user-facing mesh-state mutations are traceable.
In `@src/UvSeamData_test.cpp`:
- Around line 67-84: The current test only verifies direct round-tripping
through UvSeamData::writeBindingsToMesh(), so it misses regressions in the new
EditableMesh persistence hooks. Extend UvSeamDataTest by adding an integration
test that exercises EditableMesh::commitToEntity() and/or
EditableMesh::resizeEntityBuffers() after calling setSeam and setPinned on a
submesh, then reloads the mesh and asserts the bindings still persist. Keep the
existing round-trip coverage, but add the new test around EditableMesh,
mesh.loadFromMesh(), and the seam/pin accessors to cover the hook path.
In `@src/UvSeamOps_test.cpp`:
- Around line 43-70: The UvSeamOpsTest::SewAveragesCoincidentUvPositions case is
using makeEdgeKey(0, 2) for coincident duplicate vertices instead of a real
selected edge, so it does not exercise the public sewEdges() contract. Update
the test to create the seam through splitEdges() and then call sewEdges() with
that returned edge key, so the test covers an actual split-to-sew flow and
validates UvSeamData::setSeam/isSeam behavior on a real edge.
In `@src/UvUnwrap_test.cpp`:
- Around line 131-151: The UvUnwrapTest_partial test only verifies that
unwrapEntity succeeds and creates charts, so it misses regressions where
non-masked triangles get modified. Update the test around UvUnwrap::unwrapEntity
and UvUnwrapOptions::FaceMask to seed the mesh with known UVs before unwrapping,
then assert that the unselected triangles in the masked submesh keep their
original UV coordinates after the call while only the selected triangles are
changed.
🪄 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: 4245a6db-1570-4732-af38-cac59d3a8fc5
📒 Files selected for processing (25)
qml/PropertiesPanel.qmlqml/UVEditorPanel.qmlqml/UVEditorWindow.qmlsrc/CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController.hsrc/EditableMesh.cppsrc/EditableMesh.hsrc/UVEditorController.cppsrc/UVEditorController.hsrc/UvSeamData.cppsrc/UvSeamData.hsrc/UvSeamData_test.cppsrc/UvSeamOps.cppsrc/UvSeamOps.hsrc/UvSeamOps_test.cppsrc/UvUnwrap.cppsrc/UvUnwrap.hsrc/UvUnwrap_test.cppsrc/commands/UvSeamCommands.cppsrc/commands/UvSeamCommands.hsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrctests/CMakeLists.txt
💤 Files with no reviewable changes (1)
- src/mainwindow.h
Clear UV inspector embedded state when the section hides, fix draw order for selected pins, sync edit/UV controllers on seam changes, persist seam bindings after sew, skip seam chart prep when unused, restore ignored-tri UVs per-triangle after partial unwrap, validate binding reload, and expand unit test coverage for split/sew, commit persistence, and partial unwrap. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the open review threads in 915086d: Codex
CodeRabbit
FBX/glTF round-trip for seams/pins remains deferred (Ogre UOB only), as noted in the PR description. |
|
1 participant
Summary
EditableMeshwith Ogre mesh UOB persistence (qtme.seams.*,qtme.uv_pins.*).UvSeamCommands.Test plan
UnitTests --gtest_filter="UvSeam*:UvUnwrap*"(22 tests pass locally)Notes
Closes #462
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes