UV Editor: UV component selection (issue #460)#760
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe UV editor panel and controller now support vertex, edge, and face selection, box selection, selection overlays, context-island highlighting, and edit-mode synchronization. The controller also rebuilds UV caches and the test suite covers the new selection behaviors. ChangesUV component selection editor
Sequence Diagram(s)sequenceDiagram
participant UVEditorPanel
participant UVEditorController
participant EditModeController
UVEditorPanel->>UVEditorController: setSelectionMode()
UVEditorController-->>UVEditorPanel: selectionModeChanged
UVEditorPanel->>UVEditorController: pickAt() / boxSelect()
UVEditorController->>UVEditorController: applySelectionSet()
UVEditorController-->>UVEditorPanel: uvSelectionChanged
EditModeController-->>UVEditorController: editSelectionChanged
UVEditorController->>UVEditorController: updateContextIslandsFromEdit()
UVEditorController-->>UVEditorPanel: uvSelectionChanged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: e2432c6f5d
ℹ️ 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".
| int globalVertOffset = 0; | ||
| int globalTriIndex = 0; |
There was a problem hiding this comment.
When only a later submesh is selected, filteredEditableMesh removes the earlier submeshes, but the new sync IDs start from zero here. The meshGlobalTri and meshGlobalVert values therefore point at components in the first submesh, so with Sync enabled a UV pick in submesh 1 pushes/pulls the wrong Edit Mode face or vertex. Keep the original submesh's global vertex/triangle offsets when building the filtered UV cache.
Useful? React with 👍 / 👎.
| const uint64_t key = (packUvKey(tri.u[e], tri.v[e]) << 1) | ||
| ^ packUvKey(tri.u[n], tri.v[n]); |
There was a problem hiding this comment.
Adjacent triangles normally store their shared edge in opposite directions, but this key is order-dependent, so the two occurrences do not land in the same bucket and occs.size() stays 1. As a result, shared UV corners are not welded and vertex/edge selection splits across every triangle boundary even inside one continuous UV island. Build the occurrence key from sorted endpoint UV keys and weld reversed corners.
Useful? React with 👍 / 👎.
| if (bestFace >= 0) | ||
| faces.insert(bestFace); |
There was a problem hiding this comment.
In FaceMode, a click outside every triangle still keeps the nearest centroid in bestFace and inserts it unconditionally, so an empty-space click even far from the UV layout selects an arbitrary face instead of clearing/no-oping; edge and vertex modes already guard with pickRadiusSq. Gate the centroid fallback by the pick radius, or only insert when pointInTriangle matched.
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: 4
🧹 Nitpick comments (1)
src/UVEditorController.cpp (1)
104-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd breadcrumbs for the new user-facing UV selection actions.
setSelectionMode()logs a breadcrumb, but the new sync toggle, clear selection, click-pick, and box-select actions do not. As per coding guidelines, “AddSentryReporter::addBreadcrumb(category, message)calls for user-facing actions and significant operations.”Proposed breadcrumb coverage
void UVEditorController::setSelectionSyncEnabled(bool on) { if (m_selectionSyncEnabled == on) return; + SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), + QStringLiteral("UV editor selection sync: %1").arg(on ? QStringLiteral("on") + : QStringLiteral("off"))); m_selectionSyncEnabled = on; @@ void UVEditorController::clearUvSelection() { if (m_selectedUvVerts.isEmpty() && m_selectedUvEdges.isEmpty() && m_selectedUvFaces.isEmpty()) return; + SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), + QStringLiteral("UV editor clear selection")); @@ void UVEditorController::pickAt(double u, double v, int modifiers, double pickRadiusUv) { if (!m_hasMesh || m_uvTris.empty()) return; + SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), + QStringLiteral("UV editor pick selection")); @@ void UVEditorController::boxSelect(double uMin, double vMin, double uMax, double vMax, int modifiers) { if (!m_hasMesh) return; + SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), + QStringLiteral("UV editor box selection"));Also applies to: 307-413
🤖 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.cpp` around lines 104 - 125, The new user-facing UV selection actions in UVEditorController need Sentry breadcrumbs like setSelectionMode() already does. Add SentryReporter::addBreadcrumb(category, message) calls in the sync toggle path (setSelectionSyncEnabled), clear selection (clearUvSelection), and the click-pick/box-select handling mentioned in the diff so each significant user action is recorded with a clear category/message. Use the existing breadcrumb pattern in UVEditorController to place the calls near the action entry points and keep the messages descriptive and consistent.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 `@src/UVEditorController.cpp`:
- Around line 941-943: The UV triangle emission path is using a per-triangle
counter for meshGlobalTri, but triangulated n-gons need to keep the original
mesh face id so sync/context lookups resolve to the correct Edit Mode face.
Update the UvTri creation logic in UVEditorController’s triangulation flow
(including the loop around sub.faces and the UvTri assignment) so every emitted
triangle for a source face carries that face’s original mesh id instead of the
incremented UV triangle index.
- Around line 1017-1043: The UV weld pass in UVEditorController’s
edge-occurrence loop is using a directed UV-only key, which misses shared edges
when adjacent triangles store opposite edge directions and can incorrectly merge
unrelated overlapping UVs. Change the edge matching logic around edgeOccMap and
packUvKey so it builds undirected keys from the actual mesh edge identity (using
the underlying vertex ids, not just UV coordinates), then keep the corner
welding in weldCorner gated by the UV epsilon check.
- Around line 317-335: The FaceMode picking logic in UVEditorController
currently falls back to the nearest triangle centroid even when no UV triangle
contains the click, which causes empty-space clicks to select an arbitrary face.
Update the selection path in the FaceMode block so it only inserts into faces
when pointInTriangle() actually matches, and otherwise leaves the selection
empty or no-ops consistently with the vertex/edge picking behavior in
UVEditorController.
- Around line 1178-1181: The UV rebuild path in
UVEditorController::buildFromEntity/clearUvSelection is pushing an empty
selection too early when sync is enabled and entity != prevEntity. Update the
rebuild flow so clearUvSelection does not call pushSelectionToEdit() in this
transition, or otherwise guard it until after the Edit Mode selection has been
pulled back, to avoid deselectAll() on the source selection before the sync
readback occurs.
---
Nitpick comments:
In `@src/UVEditorController.cpp`:
- Around line 104-125: The new user-facing UV selection actions in
UVEditorController need Sentry breadcrumbs like setSelectionMode() already does.
Add SentryReporter::addBreadcrumb(category, message) calls in the sync toggle
path (setSelectionSyncEnabled), clear selection (clearUvSelection), and the
click-pick/box-select handling mentioned in the diff so each significant user
action is recorded with a clear category/message. Use the existing breadcrumb
pattern in UVEditorController to place the calls near the action entry points
and keep the messages descriptive and consistent.
🪄 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: ebadd4a7-a8a8-43b2-9738-9f540db73d76
📒 Files selected for processing (4)
qml/UVEditorPanel.qmlsrc/UVEditorController.cppsrc/UVEditorController.hsrc/UVEditorController_test.cpp
|
Addressed CodeRabbit/Codex review feedback in eb4da3e:
Added CI |
|
Follow-up in 570f207: removed live Edit Mode sync (Sync checkbox + push/pull). It was freezing the app because every UV pick rebuilt Edit Mode selection overlays on the main thread. Review items — status:
Context island tint (3D → UV read-only) remains. |
|
Re-ran |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/deploy.yml (1)
1441-1496: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSharding silently halves SonarCloud coverage.
gcovr(Line 1441) andsonar-scanner(Line 1496) run only on shard 1, but shard 1 now executes only ~half of theUnitTestssuites (the per-suite skip at Lines 1318-1324). The.gcdafiles for suites that run exclusively on shard 2 are therefore never produced on the shard-1 runner, and shard 2 uploads no coverage and there is no merge step. The result is that SonarCloud receives coverage for only the shard-1 subset (plus the "other test executables"), which can drop reported coverage substantially and trip the quality gate.Two options:
- Download shard-2's
.gcda/coverage artifact and merge before runninggcovr/sonar; or- Run the full (unsharded)
UnitTestsset on shard 1 purely for coverage, keeping shard 2 for parallelism/pass-fail only.Please confirm the intended coverage semantics before merge.
🤖 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 @.github/workflows/deploy.yml around lines 1441 - 1496, Shard 1 is only generating and uploading coverage for a partial test set, so SonarCloud sees incomplete coverage. Update the coverage flow in the deploy workflow around the “Generate coverage data” and “Run sonar-scanner” steps so the shard-1 job either merges shard-2 .gcda/coverage artifacts before running gcovr, or runs the full UnitTests coverage suite on shard 1 while keeping sharding only for execution. Use the existing shard-1 condition and coverage generation block to locate the change.src/UVEditorController.cpp (1)
976-985: 🗄️ Data Integrity & Integration | 🟠 MajorPair welded corners by shared mesh vertex, not by
c0/c1order.EdgeOcc::c0/c1keep each triangle’s local edge direction, and shared manifold edges are expected to run in opposite directions. The currenta.c0↔b.c0/a.c1↔b.c1pairing can compare different mesh vertices and miss a valid weld, leaving adjacent triangles split. Match corners bymeshGlobalVert, and add a regression test with two triangles that actually share a mesh edge; the existing seamed-quad test uses duplicated vertices and does not cover this path.🤖 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.cpp` around lines 976 - 985, The corner-welding logic in the UVEditorController weld helper is pairing `a.c0/c1` with `b.c0/c1`, which can mismatch shared manifold edges because `EdgeOcc::c0/c1` are local edge directions. Update the welding in this block to match corners by shared `meshGlobalVert` for each `EdgeOcc`, so the correct corresponding corners are united regardless of edge orientation. Add a regression test around the relevant UV welding path in `UVEditorController` using two triangles that truly share a mesh edge, since the current seamed-quad coverage with duplicated vertices does not exercise this case.
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
939-941: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep
UNIT_TEST_SHARD_TOTALin sync with the matrix.
UNIT_TEST_SHARD_TOTAL: 2is duplicated frommatrix.shard: [1, 2]. Adding/removing a shard requires editing both; deriving it from the matrix length (${{ strategy.job-total }}) removes the drift risk.🤖 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 @.github/workflows/deploy.yml around lines 939 - 941, Keep UNIT_TEST_SHARD_TOTAL aligned with the shard matrix instead of hardcoding it. In the deploy workflow’s unit test job, replace the duplicated total in the env block with a value derived from the job matrix/strategy so changes to the shard list only need to be made in one place; use the existing matrix shard configuration and the job-total/strategy-derived value to keep the total in sync.
🤖 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 @.github/workflows/deploy.yml:
- Line 938: The workflow job is over-permissioned by using permissions:
read-all; narrow the permissions block to only the minimal read access the
analysis job actually needs. Update the relevant job in deploy.yml where
SONAR_TOKEN and GITHUB_TOKEN are used so it requests explicit repo read access
instead of every read scope, keeping least-privilege without changing the
analysis behavior.
- Around line 1430-1434: The workflow step using actions/setup-python is still
on the retired v4 tag, so update the Set up Python 3.8 for gcovr step to a
current Node 20-based release such as v6.3.0 and pin it to the release commit
SHA instead of the mutable major tag. Keep the existing matrix.shard condition
and python-version setting, and make the change in the setup-python action
reference only.
---
Outside diff comments:
In @.github/workflows/deploy.yml:
- Around line 1441-1496: Shard 1 is only generating and uploading coverage for a
partial test set, so SonarCloud sees incomplete coverage. Update the coverage
flow in the deploy workflow around the “Generate coverage data” and “Run
sonar-scanner” steps so the shard-1 job either merges shard-2 .gcda/coverage
artifacts before running gcovr, or runs the full UnitTests coverage suite on
shard 1 while keeping sharding only for execution. Use the existing shard-1
condition and coverage generation block to locate the change.
In `@src/UVEditorController.cpp`:
- Around line 976-985: The corner-welding logic in the UVEditorController weld
helper is pairing `a.c0/c1` with `b.c0/c1`, which can mismatch shared manifold
edges because `EdgeOcc::c0/c1` are local edge directions. Update the welding in
this block to match corners by shared `meshGlobalVert` for each `EdgeOcc`, so
the correct corresponding corners are united regardless of edge orientation. Add
a regression test around the relevant UV welding path in `UVEditorController`
using two triangles that truly share a mesh edge, since the current seamed-quad
coverage with duplicated vertices does not exercise this case.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 939-941: Keep UNIT_TEST_SHARD_TOTAL aligned with the shard matrix
instead of hardcoding it. In the deploy workflow’s unit test job, replace the
duplicated total in the env block with a value derived from the job
matrix/strategy so changes to the shard list only need to be made in one place;
use the existing matrix shard configuration and the job-total/strategy-derived
value to keep the total in sync.
🪄 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: ce7969fd-7d21-412e-896b-14cbe465f78a
📒 Files selected for processing (5)
.github/workflows/deploy.ymlqml/UVEditorPanel.qmlsrc/UVEditorController.cppsrc/UVEditorController.hsrc/UVEditorController_test.cpp
💤 Files with no reviewable changes (1)
- qml/UVEditorPanel.qml
| matrix: | ||
| shard: [1, 2] | ||
| timeout-minutes: 45 | ||
| permissions: read-all |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Scope down permissions: read-all.
This job only needs to read the repo (and uses SONAR_TOKEN/GITHUB_TOKEN from secrets for analysis). read-all grants read on every scope unnecessarily; prefer the least-privilege set.
🔒 Suggested scoping
- permissions: read-all
+ permissions:
+ contents: read
+ pull-requests: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| permissions: read-all | |
| permissions: | |
| contents: read | |
| pull-requests: read |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 938-938: overly broad permissions (excessive-permissions): uses read-all permissions
(excessive-permissions)
🤖 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 @.github/workflows/deploy.yml at line 938, The workflow job is
over-permissioned by using permissions: read-all; narrow the permissions block
to only the minimal read access the analysis job actually needs. Update the
relevant job in deploy.yml where SONAR_TOKEN and GITHUB_TOKEN are used so it
requests explicit repo read access instead of every read scope, keeping
least-privilege without changing the analysis behavior.
Source: Linters/SAST tools
| - name: Set up Python 3.8 for gcovr | ||
| if: matrix.shard == 1 | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: 3.8 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🌐 Web query:
What is the latest release tag and commit SHA of actions/setup-python, and which Node runtime does v4 vs v5 use?
💡 Result:
As of June 25, 2026, the latest release tag for actions/setup-python is v6.3.0 [1]. The commit SHA associated with the v6.3.0 release is a309ff8b426b58ec0e2a45f0f869d46889d02405 [2]. Regarding the Node runtime environments: - The v4 version of actions/setup-python used Node 16 [3]. - The v5 version of actions/setup-python updated the Node runtime to Node 20 [4][3].
Citations:
- 1: https://github.com/actions/setup-python/releases/tag/v6.3.0
- 2: actions/setup-python@83679a8...a309ff8
- 3: Bump actions/setup-python from 4 to 5 PyGithub/PyGithub#3283
- 4: actions/setup-python@v4...v5
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/deploy.yml')
text = p.read_text()
lines = text.splitlines()
for i in range(1424, 1438):
print(f"{i+1}: {lines[i]}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 579
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/deploy.yml')
text = p.read_text()
lines = text.splitlines()
for i in range(1424, 1438):
print(f"{i+1}: {lines[i]}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 579
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/deploy.yml')
lines = p.read_text().splitlines()
for i in range(1370, 1455):
print(f"{i+1}: {lines[i]}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 4407
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/deploy.yml')
lines = p.read_text().splitlines()
for i in range(1370, 1455):
print(f"{i+1}: {lines[i]}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 4407
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/deploy.yml')
lines = p.read_text().splitlines()
for i in range(1370, 1455):
print(f"{i+1}: {lines[i]}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 4407
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/deploy.yml')
lines = p.read_text().splitlines()
for i in range(1370, 1455):
print(f"{i+1}: {lines[i]}")
PYRepository: fernandotonon/QtMeshEditor
Length of output: 4407
Update actions/setup-python to a Node 20 release and pin the SHA.
actions/setup-python@v4 uses the retired Node 16 runtime. Switch this step to a current release such as v6.3.0 and pin it to the release commit instead of the mutable major tag.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 1432-1432: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 zizmor (1.26.1)
[error] 1432-1432: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/deploy.yml around lines 1430 - 1434, The workflow step
using actions/setup-python is still on the retired v4 tag, so update the Set up
Python 3.8 for gcovr step to a current Node 20-based release such as v6.3.0 and
pin it to the release commit SHA instead of the mutable major tag. Keep the
existing matrix.shard condition and python-version setting, and make the change
in the setup-python action reference only.
Source: Linters/SAST tools
Add independent UV-vertex/edge/face selection with click, multi-select, and box-select; optional bidirectional sync with Edit Mode; and context island highlighting when sync is off. Co-authored-by: Cursor <cursoragent@cursor.com>
Gate face picks by radius, weld UV corners on undirected mesh edges, preserve original submesh global indices for filtered views, map n-gon fan triangles to one Edit Mode face id, and avoid pushing empty UV selection during entity-switch rebuild before pull. Co-authored-by: Cursor <cursoragent@cursor.com>
Bidirectional selection sync caused main-thread freezes (Edit Mode overlay rebuilds on every pick). Drop the Sync checkbox and push/pull paths; UV selection stays local and 3D Edit Mode selection still tints context islands. Co-authored-by: Cursor <cursoragent@cursor.com>
fernandotonon
force-pushed
the
feature/uv-component-selection-460
branch
from
a7d3b32 to
feda14f
Compare
Address CodeRabbit nitpick: log clear/pick/box selection alongside the existing selection-mode breadcrumb. Co-authored-by: Cursor <cursoragent@cursor.com>
|
CI on rebased branch ( Code review status (3573075):
|
|
1 participant
Summary
UVEditorController(notSelectionSet); supports click, Shift-add, Ctrl-toggle, and drag-box select1/2/3shortcuts (panel focus)Closes #460
Review feedback addressed
pickRadiusUv(no arbitrary face on empty click)meshGlobalTrifor context lookupTest plan
./build_local/bin/UnitTests --gtest_filter="UVEditorControllerTest.*"(14/14 pass)Summary by CodeRabbit
New Features
Bug Fixes
Tests