feat(#461): UV editor transforms with undo and live viewport sync - #772
Conversation
Add move/rotate/scale/mirror UV transforms with Q/W/E/R shortcuts, undo via UVEditCommand, and GPU commits that update skeletal anim buffers without entity re-init. Also fix GL render-target spam from material preview RTTs. Closes #461 Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 41 minutes and 20 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 (9)
📝 WalkthroughWalkthroughThis PR implements UV editor transform operations (move/rotate/scale/mirror) with undo/redo via a new ChangesUV Editor Transform System
Render Target Management and Material Preview Caching
Sequence Diagram(s)sequenceDiagram
participant QML as UVEditorPanel (QML)
participant UVCtrl as UVEditorController
participant UVTransform
participant UndoManager
participant UVEditCommand
participant EditableMesh
participant OgreEntity
QML->>UVCtrl: beginTransformDrag(u, v, modifiers)
UVCtrl->>UVCtrl: collectSelectedUvRefs() → beforeRefs
QML->>UVCtrl: updateTransformDrag(u, v, modifiers)
UVCtrl->>UVTransform: applyTransform(op, refs, settings, delta)
UVTransform-->>UVCtrl: updated VertRefs
UVCtrl->>UVCtrl: applyWorkingMeshUv() per vertex
QML->>UVCtrl: commitTransformDrag()
UVCtrl->>UVCtrl: applyUvRefChanges(refs, before, op, desc)
UVCtrl->>EditableMesh: commitUvsToEntity(entity, uvChannel)
EditableMesh->>OgreEntity: writeUvChannel() → GPU buffer update
UVCtrl->>UndoManager: push(UVEditCommand)
UVCtrl->>UVCtrl: syncUvLayoutFromWorkingMesh() → emit meshDataChanged
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: cf32bbd83d
ℹ️ 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 (!m_activeEntity) | ||
| return false; | ||
|
|
||
| return m_workingMesh.commitUvsToEntity(m_activeEntity, m_uvChannel, nullptr); |
There was a problem hiding this comment.
When the UV editor is opened for a sub-entity selection on UV channel 1, m_workingMesh still contains every submesh, but applyUvChannel(..., m_submeshFilter) only reloads the requested channel for the visible submeshes; the hidden submeshes keep the channel-0 UVs from loadFromEntity(). This unfiltered commit then writes every submesh back to channel 1, so transforming one selected submesh can overwrite channel-1 UVs on all unselected submeshes with their channel-0 coordinates. The commit path should either load the requested channel for all submeshes or restrict commitUvsToEntity to the edited submesh set.
Useful? React with 👍 / 👎.
| } else if (event.key === Qt.Key_Return || event.key === Qt.Key_Enter) { | ||
| applyNumericBuffer() | ||
| event.accepted = numericBuffer.length > 0 | ||
| } else if (event.key >= Qt.Key_0 && event.key <= Qt.Key_9) { |
There was a problem hiding this comment.
Because this new numeric digit handler is placed after the existing Qt.Key_1/2/3 selection-mode shortcuts above, those digits never reach appendNumericChar() while a transform is active. As a result, common numeric transforms such as rotate 180, scale 2, or move 1.25 switch selection mode instead of entering the value; handle numeric input before the selection shortcuts when transform numeric entry is active.
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qml/MaterialListModal.qml (1)
161-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisable QML pixmap caching for these preview thumbnails.
The preview URI is deterministic per material name, so reassigning the same source string after an edit can keep the old bitmap alive.
PropertiesPanel.qmlalready setscache: falsefor the same preview path; without that, this modal can still show stale thumbnails even ifmaterialPreviewUrisis rebuilt.Suggested fix
Image { anchors.horizontalCenter: parent.horizontalCenter width: 52; height: 52 source: materialListModal.materialPreviewUris[modelData] || "" fillMode: Image.PreserveAspectFit asynchronous: true + cache: false sourceSize.width: 52 sourceSize.height: 52🤖 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 `@qml/MaterialListModal.qml` around lines 161 - 168, The preview thumbnail Image in MaterialListModal.qml is still using the default QML pixmap cache, which can keep stale bitmaps around when materialPreviewUris is rebuilt. Update the Image block in the MaterialListModal preview delegate to disable caching the same way PropertiesPanel.qml does by setting cache to false, so reusing the same source string after edits reloads the thumbnail instead of showing an old bitmap.
🧹 Nitpick comments (2)
src/commands/UVEditCommand.cpp (1)
70-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the standard breadcrumb category here.
These are user-facing undo/redo actions, but
mesh.uv.transformdoes not follow the repo's documented Sentry taxonomy. Please log them underui.actionand keep the specific operation in the message so UV editor telemetry stays queryable with the rest of the UI events. As per coding guidelines,**/*.{cpp,h}should track user-facing actions withSentryReporter::addBreadcrumb(category, message)using documented categories such asui.action.🤖 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/commands/UVEditCommand.cpp` around lines 70 - 85, The undo/redo breadcrumbs in UVEditCommand::undo and UVEditCommand::redo are using a nonstandard Sentry category. Update the SentryReporter::addBreadcrumb calls to use the documented ui.action category, while keeping the operation-specific message text like “Undo UV edit” and “Redo UV edit” so these user-facing actions stay consistent with the repo taxonomy.Source: Coding guidelines
src/UVEditorController_test.cpp (1)
401-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd controller undo tests for rotate and scale.
This adds move and mirror undo coverage, but the new controller paths for
RotateTransformandScaleTransformalso need undo/redo round-trip tests. As per coding guidelines,src/**/*_test.cpp: “Add Google Test unit tests for new functionality”; the PR objective also calls for tests for each transform plus undo.🤖 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/UVEditorController_test.cpp` around lines 401 - 467, Add Google Test coverage in UVEditorControllerTest for the missing RotateTransform and ScaleTransform undo/redo paths, similar to MoveSelectionUpdatesGpuAndUndoRoundTrip and MirrorXCommandIsUndoable. Create tests that set up an in-memory mesh, select a face through UVEditorController, call setTransformMode with RotateTransform and ScaleTransform, apply the numeric transform, then verify the UVs change and that UndoManager::undo() and redo() restore/reapply the same values. Keep the assertions aligned with the existing readEntityUv0 helper and controller flow used in the other UVEditorController tests.Source: Coding guidelines
🤖 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/MaterialListModal.qml`:
- Around line 43-56: The thumbnail cache in MaterialListModal.qml is not being
invalidated after edits, so previews stay stale while the modal remains open.
Update the MaterialListModal flow so `editMaterial()` triggers a refresh of
`materialPreviewUris` after the editor applies changes, reusing the same
invalidation pattern already used in `PropertiesPanel.qml`. Ensure the cache is
rebuilt through `rebuildMaterialPreviews()` and then `refreshMaterialList()` (or
equivalent) so `MaterialEditorQML.materialPreview(...)` is re-queried for the
edited material.
In `@qml/PropertiesPanel.qml`:
- Around line 4491-4520: The interactive preview can stay stale when the
selected material is edited and reapplied because
`previewHost.refreshPreviewSource()` only updates on selection/shape/yaw/width
changes and leaves `previewSource` unchanged when
`MaterialEditorQML.interactiveMaterialPreview()` returns an empty string. Update
the `refreshPreviewSource()` flow so it explicitly invalidates or clears
`previewSource` when the current material’s preview can’t be generated, and make
sure the existing `schedulePreviewRefresh()` path is also triggered from the
material-apply/update flow that changes the underlying material content, not
just `onSelectedMaterialNameChanged`, `onPreviewShapeChanged`,
`onPreviewYawChanged`, and `onWidthChanged`.
In `@qml/UVEditorPanel.qml`:
- Around line 183-206: The key handling in UVEditorPanel.qml is letting the
selection hotkeys consume digit keys before the numeric transform path, so when
transform mode is active the key handler should route 1/2/3 to appendNumericChar
instead of the earlier selection branches. Update the main key handler to check
the numeric-transform state before the selection shortcuts, using the existing
appendNumericChar, applyNumericBuffer, and root.draggingTransform logic to keep
transform input active. Also fix the Enter/Return branch so event.accepted is
determined from the buffer before numericBuffer is cleared, ensuring successful
submissions are marked handled.
In `@src/EditableMesh.cpp`:
- Around line 827-830: The first-pass lookup in the EditableMesh UV refresh path
is not fully guarded, since getTechnique(0) can return a technique with zero
passes. Update the logic around mat->getTechnique(0)->getPass(0) to check
getNumPasses() first and skip the material when there are no passes, keeping the
existing getNumTechniques() guard intact. Use the existing
material/technique/pass handling in EditableMesh to ensure the pass pointer is
only accessed when it is valid.
In `@src/MaterialPreviewRenderer.cpp`:
- Around line 349-354: The interactive cache in
MaterialPreviewRenderer::renderMaterialPreview uses a rounded yaw for the cache
key but still renders with the unrounded wrappedYaw, so nearby angles can reuse
the wrong image. Make the cache key and the rendered yaw use the same quantized
value by computing the yaw once at the chosen precision and reusing that value
for both the key and the preview generation.
In `@src/UVEditorController.cpp`:
- Around line 875-889: Rollback the local working/edit UV state if
commitWorkingMeshUvs() fails, because applyWorkingMeshUv() has already mutated
editor state before the GPU commit attempt. In the method that builds
UVEditCommand::VertChange entries, capture enough pre-change UV data to restore
each touched vertex and, on a false return from commitWorkingMeshUvs(), revert
those vertices back to their previous values instead of returning or refreshing
from the uncommitted state. Apply the same rollback behavior in the other
affected path near the second commit call so the working mesh, GPU, and undo
stack stay consistent.
In `@src/UVTransform.cpp`:
- Around line 31-57: The vertex snapping path in snapUv() still does a linear
std::find over selectedIds for every vertex in allVerts, which makes
UVEditorController::updateTransformDrag() too slow on large drags. Change
applyTransform() to build a hash-based membership structure once, update the
selectedIds parameter type in UVTransform.h, and use constant-time lookups
inside snapUv() instead of scanning the vector.
---
Outside diff comments:
In `@qml/MaterialListModal.qml`:
- Around line 161-168: The preview thumbnail Image in MaterialListModal.qml is
still using the default QML pixmap cache, which can keep stale bitmaps around
when materialPreviewUris is rebuilt. Update the Image block in the
MaterialListModal preview delegate to disable caching the same way
PropertiesPanel.qml does by setting cache to false, so reusing the same source
string after edits reloads the thumbnail instead of showing an old bitmap.
---
Nitpick comments:
In `@src/commands/UVEditCommand.cpp`:
- Around line 70-85: The undo/redo breadcrumbs in UVEditCommand::undo and
UVEditCommand::redo are using a nonstandard Sentry category. Update the
SentryReporter::addBreadcrumb calls to use the documented ui.action category,
while keeping the operation-specific message text like “Undo UV edit” and “Redo
UV edit” so these user-facing actions stay consistent with the repo taxonomy.
In `@src/UVEditorController_test.cpp`:
- Around line 401-467: Add Google Test coverage in UVEditorControllerTest for
the missing RotateTransform and ScaleTransform undo/redo paths, similar to
MoveSelectionUpdatesGpuAndUndoRoundTrip and MirrorXCommandIsUndoable. Create
tests that set up an in-memory mesh, select a face through UVEditorController,
call setTransformMode with RotateTransform and ScaleTransform, apply the numeric
transform, then verify the UVs change and that UndoManager::undo() and redo()
restore/reapply the same values. Keep the assertions aligned with the existing
readEntityUv0 helper and controller flow used in the other UVEditorController
tests.
🪄 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: 2f7d18be-9d32-49ab-8bf5-c98553cbcf82
📒 Files selected for processing (26)
qml/MaterialListModal.qmlqml/PropertiesPanel.qmlqml/UVEditorPanel.qmlsrc/CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController.hsrc/EditableMesh.cppsrc/EditableMesh.hsrc/MaterialEditorQML.cppsrc/MaterialPreviewRenderer.cppsrc/MaterialPreviewRenderer.hsrc/MaterialPreviewRenderer_test.cppsrc/MeshDepthRenderer.cppsrc/ModelIsometricRenderer.cppsrc/ModelTurntableRenderer.cppsrc/OgreRenderTargetUtil.hsrc/UVEditorController.cppsrc/UVEditorController.hsrc/UVEditorController_test.cppsrc/UVTransform.cppsrc/UVTransform.hsrc/UVTransform_test.cppsrc/commands/UVEditCommand.cppsrc/commands/UVEditCommand.hsrc/mainwindow.cpptests/CMakeLists.txt
Refresh material preview thumbnails after apply, guard pass lookup, roll back UV edits when GPU commit fails, use hash-set vertex snap, fix numeric input hotkeys, and align interactive preview yaw cache with rendered angle. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
UVEditCommand; live 3D viewport updates during drag without entity re-init (mesh + skeletal anim buffers)setDrawBufferspam from material-preview RTTs and cache inspector preview URIsTest plan
UnitTests --gtest_filter="UVTransformTest.*:UVEditorControllerTest.*"(26 passed)Closes #461
Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests