feat(paint): image-editing tools — FG/BG colors, smart-select, editor window - #530
Merged
Merged
Conversation
…window)
Three connected pieces of UX work on the texture/vertex paint pipeline:
1) FG / BG colors with swap + reset, exposed in the left toolbar.
- EditModeController gains m_vertexPaintBackgroundColor + swap +
reset APIs. Defaults: FG = Fern green (113,188,120), BG = black.
- mainwindow.cpp adds an inline FG/BG swatch widget directly to
the objects toolbar — two overlapping rectangles with a swap
arrow (⇄) and a reset glyph (◰). Click either swatch to open
QColorDialog. Visible in Material Mode only.
- The Erase brush now paints with BG color instead of being
hard-coded to "paint transparent". Old behaviour is preserved
by setting BG alpha = 0 in the picker.
- Brush popup no longer duplicates color UI — toolbar is the
single source of truth.
2) Smart-select / magic-wand tool.
- New PaintSelectionMask class: per-pixel boolean mask paired 1:1
with the paint buffer. Smart-select via flood-fill against a
seed pixel with per-channel L∞ tolerance. Replace / Add / Sub
combine modes (the GUI exposes Replace only; the SDK is wired
for the rest).
- TexturePaintController exposes Q_INVOKABLE Mask APIs: select-
all / invert / clear / smartSelectAtUV / fillMaskWithFG /
fillMaskWithBG / deleteMaskPixels. All actions push a single
TexturePaintMaskActionCommand for undo.
- Wand toolbar button (green icon drawn via QPainter to match
topology buttons; grey disabled state) lives directly under
the paint brush. Enabled only when texture paint is on AND
target=Texture. Clicking outside the mesh clears the
selection (Photoshop / GIMP convention).
- Tolerance: no separate UI control. Click-and-drag horizontally
during the wand stroke to scrub the tolerance. Each fresh
press resets to the default 15%.
- Selection overlay renders in two views: a 2D yellow-tint +
black-outline marching-ants overlay on the texture preview,
AND a 3D ManualObject-textured copy of the mesh that shows
the selected UV regions on the model. The 3D overlay uses an
unlit transparent material sampling the same mask texture.
3) Detached texture editor window (qml/TextureEditorWindow.qml).
- QML Window opened from the right panel, 720x760 default,
mirrors the paint buffer at full canvas size. Hooked into
the same TexturePaintController.beginStrokeUV /
updateStrokeUV / endStrokeUV path, so live-syncs with the
3D viewport in both directions.
- Bottom action bar: Save, Load, Save to Original, plus the
mask Fill FG / Fill BG / Delete / Invert / All / None
actions. Top toolbar mirrors the brush tool selector.
4) Non-destructive paint on stroke end.
- Stroke end NO LONGER overwrites the user's source texture
file. Painted pixels live only in m_buffer and
EmbeddedTextureCache (which feeds the FBX/glTF exporters).
- To persist to disk the user must click "Save to Original"
explicitly (right panel + editor window) or save to a new
file via "Save…". This prevents the case where someone is
just experimenting with a texture and overwrites their
asset by accident.
5) Crash hardening.
- TexturePaintController listens to Manager::sceneNodeDestroyed
and Manager::sceneClearing. When a doomed scene node holds
m_paintMeshEntity / m_sessionEntity / m_maskOverlayEntity,
closeSession() runs while the entities are still valid.
Was reproducing as a segfault after removing a mesh that
had an active wand selection.
- setPaintTarget() now tears down the texture-paint session
when switching from Texture → Vertex (was leaving GPU
texture + rebind state half-alive, crashing on the next
vertex stroke).
- destroyMeshMaskOverlay() and the brush-ring teardown in
closeSession() now source the SceneManager from the
Manager singleton instead of dereferencing entity pointers
that may already be dangling.
Brush radius default raised from 0.25 -> 0.02 with a 0.001-2.0
slider range so the small-detail use case is the default.
New test: src/PaintSelectionMask_test.cpp covers the mask APIs
(default-empty, set/clear, select-all/invert, smart-select with
exact + tolerance match, Add/Sub combine modes, out-of-bounds
+ size-mismatch guards).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis pull request adds a complete smart-select/magic-wand masking system for texture painting. New ChangesSmart-Select Mask Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: e049a30558
ℹ️ 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 on lines
+2329
to
+2332
| QStringLiteral("Smart select: filled %1 px with FG %2") | ||
| .arg(affected).arg(c.name(QColor::HexRgb))); | ||
| flushDirtyToOgre(); | ||
| return affected; |
There was a problem hiding this comment.
fillMaskWithFG/fillMaskWithBG/deleteMaskPixels mutate the paint buffer and push undo, but they only call flushDirtyToOgre() and never refresh EmbeddedTextureCache. Exports for embedded textures read from that cache, so doing only mask actions (without a later brush stroke that goes through endStroke) can export stale pre-action pixels.
Useful? React with 👍 / 👎.
Comment on lines
+1046
to
+1050
| SentryReporter::addBreadcrumb("ui.action", | ||
| "Wand: cleared selection (click outside mesh)"); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
In the wand miss branch, the handler returns unconditionally, so the click never reaches the regular selection path. Because wouldStrokeHit() only checks the current paint mesh, clicking another mesh while wand is active is treated as a miss and gets swallowed, which blocks viewport selection changes until the tool is toggled off.
Useful? React with 👍 / 👎.
Comment on lines
+212
to
+214
| const u = mx / canvasImg.width | ||
| const v = my / canvasImg.height | ||
| if (u < 0 || u > 1 || v < 0 || v > 1) return null |
There was a problem hiding this comment.
The detached editor uses Image.PreserveAspectFit, but uvAt() divides mouse coordinates by the full canvasImg item size. For non-square textures this includes letterbox margins, so clicks/drags map to incorrect UVs and paint/pick operations are offset or compressed relative to visible pixels.
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: 11
🤖 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 1290-1340: The fixed Row of action buttons (Row + Repeater
creating Rectangle buttons with width: 56) can overflow in narrow inspectors;
replace the Row with a wrapping container (e.g., Flow) or make the layout
horizontally scrollable so buttons wrap instead of being clipped. Update the
container that currently contains the Repeater (Row) to Flow with the same
spacing, keep the Rectangle visuals (radius, border, Text, MouseArea) and the
existing enabled/opacity logic (modelData.needsMask / texPaintCol.hasMask), and
ensure ToolTip/MouseArea behavior is preserved; alternatively, wrap the Repeater
in a Flickable/ScrollView if wrapping is undesirable.
In `@qml/TextureEditorWindow.qml`:
- Around line 28-33: The window's detached state can show stale preview/mask
after session teardown because only hasSession is updated; add an
onHasSessionChanged handler in TextureEditorWindow.qml that reacts to hasSession
becoming false by clearing previewUri and maskOverlayUri (set to empty string)
and/or set window.visible = false (or call close()) so the detached window
doesn't display stale data; reference the properties previewUri, maskOverlayUri,
hasSession and the TexturePaintController source when implementing the handler.
- Around line 253-315: The bottomBar Row (id: bottomBar) currently lays out nine
Button children in a single line and will clip controls at small window widths;
replace the Row with a wrapping container such as a Flow (keeping id: bottomBar,
spacing: 6 and the same anchors/margins) so buttons automatically wrap to
additional lines when there isn't enough horizontal space, preserve each
Button's enabled/onClicked properties (including the ToolTip settings for the
"Save to Original" button) and adjust/remove the fixed height (or let
implicitHeight drive it) so the bar grows vertically as needed.
In `@src/EditModeController.h`:
- Around line 235-236: The comment for Q_INVOKABLE void resetPaintColors()
incorrectly states "FG=black, BG=white"; update the documentation to reflect the
actual class defaults used by EditModeController (fern green foreground and
black background) so QML/API consumers see the true behavior, and ensure the
summary matches the resetPaintColors() behavior and the defaults defined near
the class (the fern green/black defaults referenced in the class default
definitions).
In `@src/mainwindow.cpp`:
- Around line 1539-1545: Add Sentry breadcrumbs inside the swap and reset button
click handlers: when calling EditModeController::instance()->swapPaintColors()
in the lambda bound to swap and when calling
EditModeController::instance()->resetPaintColors() in the lambda bound to reset,
invoke SentryReporter::addBreadcrumb("ui.action", "<appropriate message>")
before or after the paint mutation (e.g., "swatch.swap" and "swatch.reset");
keep the existing syncSwatches() calls unchanged and place the breadcrumb calls
in the same lambdas that reference syncSwatches to ensure these toolbar actions
are tracked.
- Around line 1557-1567: refreshPaintBrushVisibility currently hides
paintColorsAction whenever not in MaterialMode, which removes the FG/BG swatch
in Edit Mode; modify the closure so paintColorsAction remains visible in Edit
Mode by checking EditorModeController::instance()->currentMode() and only hiding
paintColorsAction when the mode is neither MaterialMode nor EditMode (i.e., keep
paintColorsAction->setVisible(true) for Edit Mode), leaving vertexPaintAction,
vertexPaintButton, and wandAction visibility logic unchanged; update the lambda
(refreshPaintBrushVisibility) to explicitly set
paintColorsAction->setVisible(material || mode ==
EditorModeController::EditMode) so the swatch stays available in Edit Mode.
In `@src/PaintSelectionMask.h`:
- Around line 58-59: The public non-const data() accessor in PaintSelectionMask
lets callers mutate m_data without rebuilding cached summaries (selectedCount,
bbox), so remove or restrict direct mutation: make data() only return const
(remove or change std::vector<uint8_t>& data()), and if external mutation is
required add a controlled method (e.g., updateData(const std::vector<uint8_t>&)
or editDataWithCallback(std::function<void(std::vector<uint8_t>&)>) that updates
m_data and then calls the internal cache-rebuild methods used by selectedCount()
and bbox(); ensure all places using data() are updated to use the new API so
caches are consistently refreshed.
In `@src/TexturePaintController.cpp`:
- Around line 2088-2093: wouldStrokeHit currently returns false when m_paintMesh
is null, causing a real click-on-model to be treated as a miss; fix by lazily
initializing the editable paint mesh/session the same way beginStroke() does
before bailing out: move the mesh/session-creation logic from beginStroke() into
a small helper (e.g., ensurePaintMeshInitialized() or initializePaintSession())
and call that helper at the top of wouldStrokeHit (or call the safe
initialization path used by beginStroke()) so hitTestUV(...) runs against an
initialized paint mesh instead of returning early when m_paintMesh is null.
- Around line 1399-1423: The cache write that calls EmbeddedTextureCache::store
currently runs only at freehand stroke end, causing stale cache data when other
code paths mutate the pixel buffer; add a small helper (e.g.,
updateEmbeddedTextureCache or storeEmbeddedTextureCache) that encapsulates the
QImage->QByteArray->EmbeddedTextureCache::store and SentryReporter breadcrumb
logic used in TexturePaintController, and invoke this helper from every place
that mutates m_buffer — specifically inside fillMaskWithFG, fillMaskWithBG,
deleteMaskPixels, and the undo/redo handlers (the functions that perform buffer
restores) so the embedded cache is updated immediately after each pixel
mutation.
- Around line 2221-2252: Selection-only actions clearSelectionMask,
selectAllMask, and invertSelectionMask currently mutate m_mask directly and
don't push undo entries; change each to record the previous mask state,
create/execute an undoable command that restores the previous mask on undo (and
reapplies the new mask on redo), and push that command onto the existing undo
stack instead of mutating m_mask in-place. Specifically: before calling
m_mask.clear(), m_mask.selectAll(), or m_mask.invert(), capture the current
m_mask (and related overlay URI/state), construct an undo command (matching your
app's undo-command pattern used by fill/delete actions) that in its redo applies
the new mask and in its undo restores the captured mask; call
destroyMeshMaskOverlay()/scheduleMaskOverlayRefresh(),
SentryReporter::addBreadcrumb(...) and emit smartSelectChanged() from the
redo/command as appropriate so UI and breadcrumbs remain identical. Ensure
selectAllMask() and invertSelectionMask() still resize m_mask when necessary
before creating the command so the command has correct dimensions.
- Around line 1317-1339: The ToolSmartSelect press handler currently overwrites
the user's configured m_smartSelectTolerance to the hardcoded
kDefaultWandTolerance on every stroke; instead, stop resetting
m_smartSelectTolerance in the ToolSmartSelect case (leave m_smartSelectTolerance
untouched), only set m_wandStartTolerance (or a per-stroke working tolerance) to
a sensible default if needed, and move the initial default assignment for
m_smartSelectTolerance into the controller's initialization/constructor so the
default is applied once at startup; also remove the emit smartSelectChanged()
call from the press path (only emit when the actual stored
m_smartSelectTolerance value is changed by user code).
🪄 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: eaab4eca-b087-4ae8-a924-32cc48c5e5a3
📒 Files selected for processing (13)
qml/PropertiesPanel.qmlqml/TextureEditorWindow.qmlsrc/CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController.hsrc/PaintSelectionMask.cppsrc/PaintSelectionMask.hsrc/PaintSelectionMask_test.cppsrc/TexturePaintController.cppsrc/TexturePaintController.hsrc/TransformOperator.cppsrc/mainwindow.cppsrc/qml_resources.qrc
Comment on lines
+2221
to
+2252
| void TexturePaintController::clearSelectionMask() | ||
| { | ||
| if (m_mask.isEmpty()) return; | ||
| m_mask.clear(); | ||
| m_maskOverlayUri.clear(); | ||
| destroyMeshMaskOverlay(); | ||
| SentryReporter::addBreadcrumb("ui.action", "Smart select: cleared"); | ||
| emit smartSelectChanged(); | ||
| } | ||
|
|
||
| void TexturePaintController::selectAllMask() | ||
| { | ||
| if (!hasActiveSession()) return; | ||
| if (m_mask.width() != m_buffer.width() || m_mask.height() != m_buffer.height()) | ||
| m_mask.resize(m_buffer.width(), m_buffer.height()); | ||
| m_mask.selectAll(); | ||
| SentryReporter::addBreadcrumb("ui.action", "Smart select: select all"); | ||
| scheduleMaskOverlayRefresh(); | ||
| emit smartSelectChanged(); | ||
| } | ||
|
|
||
| void TexturePaintController::invertSelectionMask() | ||
| { | ||
| if (!hasActiveSession()) return; | ||
| if (m_mask.width() != m_buffer.width() || m_mask.height() != m_buffer.height()) | ||
| m_mask.resize(m_buffer.width(), m_buffer.height()); | ||
| m_mask.invert(); | ||
| SentryReporter::addBreadcrumb("ui.action", | ||
| QStringLiteral("Smart select: invert (%1 px now)").arg(m_mask.selectedCount())); | ||
| scheduleMaskOverlayRefresh(); | ||
| emit smartSelectChanged(); | ||
| } |
There was a problem hiding this comment.
Selection-only mask actions still bypass undo.
clearSelectionMask(), selectAllMask(), and invertSelectionMask() mutate the selection immediately and never push an undo command. That makes “None / All / Invert” inconsistent with fill/delete and misses the advertised undoable mask-action behavior.
🤖 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/TexturePaintController.cpp` around lines 2221 - 2252, Selection-only
actions clearSelectionMask, selectAllMask, and invertSelectionMask currently
mutate m_mask directly and don't push undo entries; change each to record the
previous mask state, create/execute an undoable command that restores the
previous mask on undo (and reapplies the new mask on redo), and push that
command onto the existing undo stack instead of mutating m_mask in-place.
Specifically: before calling m_mask.clear(), m_mask.selectAll(), or
m_mask.invert(), capture the current m_mask (and related overlay URI/state),
construct an undo command (matching your app's undo-command pattern used by
fill/delete actions) that in its redo applies the new mask and in its undo
restores the captured mask; call
destroyMeshMaskOverlay()/scheduleMaskOverlayRefresh(),
SentryReporter::addBreadcrumb(...) and emit smartSelectChanged() from the
redo/command as appropriate so UI and breadcrumbs remain identical. Ensure
selectAllMask() and invertSelectionMask() still resize m_mask when necessary
before creating the command so the command has correct dimensions.
CI:
- tests/CMakeLists.txt: add PaintSelectionMask.cpp to the QML
test target sources. Linker was emitting "undefined reference
to PaintSelectionMask::resize/smartSelect" because the file
shipped in src/CMakeLists.txt only, and the CI build-wrapper
step links the QML-test targets too.
CodeRabbit + Codex findings:
1) TexturePaintController::wouldStrokeHit() lazily builds the
editable paint mesh before bailing out. Was returning false
on the very first click against an uninitialised session
(m_paintMesh null), which silently swallowed the first wand
click on the model.
2) TransformOperator wand-miss handler now only swallows the
click when there's an existing selection (Photoshop clear-
on-empty-click). With no mask the click falls through to
normal selection so the user can still pick a different
mesh without first toggling wand off.
3) ToolSmartSelect press no longer overwrites the user's
configured tolerance with the 15% hardcoded default. The
default lives in the member initialiser; per-stroke logic
just snapshots the current value as the wand-drag baseline.
4) updateEmbeddedTextureCache() extracted as a helper and
called from: endStroke, applyPixelSnapshot (undo / redo),
fillMaskWithFG, fillMaskWithBG, deleteMaskPixels. Without
this, FBX export and "Save to Original" saw stale pixels
after any mask action or undo (the buffer was up-to-date
but the cache wasn't).
5) PaintSelectionMask::data() non-const accessor removed so
external code can't mutate raw bits and desync the cached
selectedCount / bbox.
6) mainwindow.cpp paintColorsAction now stays visible in Edit
Mode too — vertex paint runs from Edit Mode and the brush
popup no longer holds a color picker, so without this the
user has no toolbar access to FG / BG / swap.
7) Toolbar swap and reset buttons now emit Sentry breadcrumbs
to match the rest of the paint UI's telemetry trail.
8) EditModeController.h resetPaintColors() doc comment now
correctly says "FG = Fern green, BG = black" — was stale
"FG = black, BG = white" from before the defaults changed.
9) TextureEditorWindow.qml uvAt() maps against
canvasImg.paintedWidth/paintedHeight (the inner letter-
boxed rect) instead of the container size. Non-square
textures used to map clicks to incorrect UVs because the
PreserveAspectFit margins were treated as paintable area.
10) TextureEditorWindow.qml onSessionChanged clears
previewUri / maskOverlayUri / hover state when the
session ends so the detached window doesn't keep
showing the last-session texture after mesh removal.
11) Mask action buttons (Fill FG / BG / Delete / Invert / All
/ None) and the editor window's bottom action bar are now
laid out with Flow instead of Row so they wrap to a
second line on narrow inspectors / windows instead of
being clipped.
Material list filter: also filtered out QMEPaintMaskOverlay_*,
QMEPaint_*, and TexturePaint/* prefixes from the Inspector's
submesh material dropdown (SceneTreeModel::availableMaterials)
and the Material Editor's material list
(MaterialEditorQML::getMaterialList) so the runtime paint-
pipeline materials don't clutter user-facing lists.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes