Paint v2 Slice C: texture paint layers (#546) - #935
Conversation
Introduce a PaintLayerStack with blend modes, Inspector layer UI, per-layer undo, and export flattening. Fix multi-stroke lag in the UV preview by resetting stamp spacing state, syncing GPU uploads immediately for preview strokes, and deferring embedded-texture cache work off the paint hot path. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds Paint v2 layered texture painting with blend modes, masks, layer-aware strokes and undo, QML layer controls, per-layer previews, generation-safe GPU refresh coordination, and export-time flattening. ChangesPaint v2 layer stack
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: baa5bee2f0
ℹ️ 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".
| void apply(const PaintLayerStack::Snapshot& snap) | ||
| { | ||
| if (!m_controller) return; | ||
| m_controller->applyLayerStackSnapshot(snap); | ||
| } |
There was a problem hiding this comment.
When a layer operation is followed by switching texture slots or painting another mesh, the global undo stack retains this command and apply() unconditionally restores the old stack into the current controller session. Unlike the stroke and mask commands, there is no texture/session identity check, so Undo can replace another mesh's layers and even resize its composite buffer to the previous texture's dimensions. Store and verify the originating texture/session identity before applying the snapshot.
Useful? React with 👍 / 👎.
| if (m_layerStrokeBaseline.empty()) | ||
| m_layerStrokeBaseline = snapshotActiveLayerPixels(); | ||
| m_strokePreSnapshot = std::move(m_layerStrokeBaseline); | ||
| } |
There was a problem hiding this comment.
After selecting, adding, duplicating, merging, or flattening into a different active layer, m_layerStrokeBaseline can still contain pixels from the previously active layer because those operations do not invalidate it. The next stroke moves that stale buffer into its pre-image while recording the new layer index, so Undo copies the previous layer's pixels into the newly painted layer. Track the baseline's layer index or clear/rebuild it whenever the active layer changes.
Useful? React with 👍 / 👎.
| m_buffer.markDirty(0, 0, m_buffer.width(), m_buffer.height()); | ||
| flushDirtyToOgre(); | ||
| } |
There was a problem hiding this comment.
When export runs before a pending layer-operation upload fires, this calls flushDirtyToOgre(), which only schedules a 33 ms timer outside a stroke, and then returns to MeshImporterExporter, which immediately serializes the texture. Programmatic/headless exports, or an export immediately following a layer visibility, opacity, or ordering change, can therefore write the previous GPU texture instead of the composite just produced here. Use the synchronous upload path before returning from this export-specific method.
Useful? React with 👍 / 👎.
| const auto before = m_layerStack.snapshot(); | ||
| m_layerStack.setOpacity(index, static_cast<float>(opacity)); | ||
| recomposeComposite(/*fullBuffer=*/true); | ||
| flushDirtyToOgre(); | ||
| pushLayerOpUndo(QStringLiteral("Layer opacity"), before, m_layerStack.snapshot()); |
There was a problem hiding this comment.
The QML slider calls this method on every onMoved event, and each call copies the entire layer stack twice and retains both snapshots in the unbounded undo stack. A single drag on a multi-layer 1024–4096 texture can consequently allocate hundreds of megabytes or several gigabytes, freeze the UI, and populate Undo with every intermediate slider tick. Merge successive opacity commands or create one snapshot at drag start and commit it on release.
Useful? React with 👍 / 👎.
| src.r * a + dst.r * (1.f - a), | ||
| src.g * a + dst.g * (1.f - a), | ||
| src.b * a + dst.b * (1.f - a), | ||
| src.a * a + dst.a * (1.f - a), | ||
| }; |
There was a problem hiding this comment.
For transparent destination pixels, alpha already contains src.a, but the output alpha multiplies by src.a again, and the straight RGB channels are interpolated without normalizing by the resulting alpha. For example, compositing a 50%-alpha source over a 50%-alpha destination should produce alpha 0.75, while this returns 0.5 and incorrect straight RGB values. This leaves erased or initially transparent texture regions too transparent after overlapping layer strokes.
Useful? React with 👍 / 👎.
| if (auto* tpc = TexturePaintController::instance()) { | ||
| if (!tpc->confirmFlattenLayersForExport(this)) { |
There was a problem hiding this comment.
For Export Selected with a multi-layer paint session, this confirmation is immediately followed by MeshImporterExporter::exporter(node, this), which calls confirmFlattenLayersForExport() again before its file dialog. Consequently a single selected mesh prompts twice, and a multi-selection containing the painted mesh prompts once here plus once for every selected node because the confirmation checks the global selection. Keep the confirmation at only one level of the export flow.
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: 10
🧹 Nitpick comments (6)
src/TexturePaintController.h (1)
324-330: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Q_INVOKABLEon aQWidget*-taking method is unusable from QML.
confirmFlattenLayersForExport(QWidget*)can only be called from C++ (the widget parent is never available to QML), so theQ_INVOKABLEadds meta-object surface without a caller. Dropping it — like the adjacentflushPaintTextureForExport— keeps the QML API honest.🤖 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.h` around lines 324 - 330, Remove the Q_INVOKABLE annotation from confirmFlattenLayersForExport(QWidget* parent) in TexturePaintController, leaving it as a regular C++ method like flushPaintTextureForExport. Preserve its signature and behavior.src/TexturePaintController.cpp (2)
5036-5047: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo breadcrumb for the flatten-export prompt or its outcome.
This is a user-facing modal decision that changes export behavior; add a
ui.actionbreadcrumb recording the layer count and whether the user continued or cancelled. As per coding guidelines, "Track all user-facing actions and significant operations withSentryReporter::addBreadcrumb; useui.actionfor UI actions".🤖 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 5036 - 5047, The export confirmation flow around layerCount() and the QMessageBox::warning call lacks telemetry. Add a SentryReporter::addBreadcrumb entry with category “ui.action” that records the texture layer count and whether the user selected Ok or Cancel, while preserving the existing boolean return behavior.Source: Coding guidelines
4769-4781: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
blendModeNamesfromPaintLayerBlend::modeName.This hardcoded list must stay in lockstep with the
Modeenum andmodeName/modeFromNametables; a new mode added in one place silently desynchronizes the QML combo box index mapping.♻️ Suggested refactor
QStringList TexturePaintController::blendModeNames() const { - return { - QStringLiteral("Normal"), - QStringLiteral("Multiply"), - QStringLiteral("Screen"), - QStringLiteral("Overlay"), - QStringLiteral("Add"), - QStringLiteral("Subtract"), - QStringLiteral("Soft Light"), - QStringLiteral("Hue"), - }; + QStringList names; + for (int m = 0; m <= static_cast<int>(PaintLayerBlend::Mode::Hue); ++m) + names << QString::fromLatin1( + PaintLayerBlend::modeName(static_cast<PaintLayerBlend::Mode>(m))); + return names; }🤖 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 4769 - 4781, Update TexturePaintController::blendModeNames to derive its QStringList from the complete PaintLayerBlend::Mode range using PaintLayerBlend::modeName, rather than maintaining hardcoded names. Preserve enum ordering so the QML combo-box indices remain aligned with modeFromName and automatically include newly added modes.src/PaintLayerBlend_test.cpp (1)
166-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding cases for the gaps this PR's layer ops expose.
Two behaviors are untested and both are where bugs are easy to introduce: solo/active index tracking across
duplicateLayer/moveLayer/removeLayer, and composite output when every layer is hidden (full vs. region paths currently disagree). As per coding guidelines, new functionality should come with Google Test coverage.🤖 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/PaintLayerBlend_test.cpp` around lines 166 - 227, Extend PaintLayerStackTest with Google Test cases covering solo and active-index updates after duplicateLayer, moveLayer, and removeLayer operations, asserting the selected layer remains correct. Add composite tests for both full and region output when all layers are hidden, verifying both paths produce the same expected fully hidden result.Source: Coding guidelines
src/PaintLayerStack.cpp (1)
95-102: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
resizeAlldiscards existing mask data.Any layer with a mask has it reset to fully-visible on resize, silently dropping user mask work. Consider resampling/preserving the overlapping region, or at least documenting the destructive 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/PaintLayerStack.cpp` around lines 95 - 102, Update PaintLayerStack::resizeAll so resizing a layer preserves existing maskAlpha values, resampling them to the new dimensions or copying the overlapping region while initializing only newly added pixels to fully visible. Do not reset every existing mask to 255; retain the current behavior for layers without masks.src/PaintLayerBlend.cpp (1)
196-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive
compositePixelAtinternal linkage.It's defined at namespace scope with external linkage but isn't declared in
PaintLayerBlend.h, so it exports an undocumented symbol. Move it into the anonymous namespace above (or markstatic).🤖 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/PaintLayerBlend.cpp` around lines 196 - 197, Give the namespace-scope compositePixelAt function internal linkage by moving it into the existing anonymous namespace in PaintLayerBlend.cpp or marking it static. Keep its signature and behavior unchanged while preventing export of the undocumented symbol.
🤖 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 4825-4862: Add a per-layer lock toggle alongside the existing
visibility and solo controls in the layer-row delegate. Bind its visual state to
the layer’s locked property and invoke the controller’s corresponding
paint-layer lock setter with the row index and inverted current state,
preserving the existing styling and interaction patterns.
- Around line 4727-4871: Make the layer toolbar and layerList controls keyboard
accessible: replace or augment the toolbar rectangles, layer-selection
MouseArea, and visibility/solo controls with focusable controls that expose
accessible roles and descriptive names, show a visible focus indicator, and
invoke the existing actions on Space or Enter. Preserve the current
enabled-state behavior and layer action symbols, including modelData.action,
modelData.index, and the existing TexturePaintController calls.
In `@src/mainwindow.cpp`:
- Around line 5559-5567: Ensure flatten-layer confirmation occurs exactly once
per selected-export operation: update the selected-export flow around
TexturePaintController::confirmFlattenLayersForExport in src/mainwindow.cpp
lines 5559-5567 and the exporter confirmation in src/MeshImporterExporter.cpp
lines 3384-3390 so one layer owns the prompt and the other receives or honors an
explicit confirmed state/target; preserve confirmation for other callers that
require it and prevent per-item re-prompting.
In `@src/PaintLayerBlend.cpp`:
- Around line 264-285: Update compositeLayersRegion to match compositeLayers’
all-hidden fallback: determine whether any layer is visible for the requested
region, and fill each affected pixel opaque white when none are visible,
including solo selections that reference hidden layers. Preserve the existing
compositePixelAt path when at least one layer is visible.
In `@src/PaintLayerStack.cpp`:
- Around line 200-223: Update PaintLayerStack::mergeDown so its compositeLayers
inputs force both lower and upper layers visible during the merge, rather than
passing their display visibility flags. Preserve all other blend, opacity, mask,
and layer-removal behavior unchanged.
- Around line 159-177: Update PaintLayerStack::duplicateLayer to validate index
before accessing the source, and adjust m_soloIndex when duplicating so a solo
layer at or after the insertion point remains associated with the same original
layer. Update removeLayer to reject out-of-range indices before erasing,
preserving its existing minimum-layer behavior and valid-index solo-index
adjustments.
In `@src/TexturePaintController.cpp`:
- Around line 4721-4727: Update TexturePaintController::pushLayerOpUndo and the
related PaintLayerOpCommand flow to use a lightweight metadata-only undo command
for visibility, opacity, blend mode, solo, and rename operations, avoiding
pixel-buffer copies. Retain full PaintLayerStack::Snapshot undo entries for add,
delete, duplicate, merge, and flatten operations.
- Around line 3214-3218: Update the flow around flushLiveStrokeToGpu so
m_buffer.clearDirty() executes only after a successful GPU flush. Preserve dirty
pixels when the flush fails, and use the existing immediate doFlushDirtyToOgre
fallback if that is the established success path.
- Around line 5050-5060: Make flushPaintTextureForExport synchronously update
the bound Ogre texture before export reads it. In this export-specific path,
bypass flushDirtyToOgre()’s deferred timer by invoking the immediate
doFlushDirtyToOgre path (or otherwise synchronously waiting for completion),
while preserving the existing dirty-region and cache-update behavior.
- Around line 4698-4699: Restrict the single-layer fast path in the surrounding
buffer-composition logic to layers with default opacity, Normal blend mode, no
mask, and visibility enabled. Otherwise use the existing full-buffer compositing
path so layer properties are applied consistently.
---
Nitpick comments:
In `@src/PaintLayerBlend_test.cpp`:
- Around line 166-227: Extend PaintLayerStackTest with Google Test cases
covering solo and active-index updates after duplicateLayer, moveLayer, and
removeLayer operations, asserting the selected layer remains correct. Add
composite tests for both full and region output when all layers are hidden,
verifying both paths produce the same expected fully hidden result.
In `@src/PaintLayerBlend.cpp`:
- Around line 196-197: Give the namespace-scope compositePixelAt function
internal linkage by moving it into the existing anonymous namespace in
PaintLayerBlend.cpp or marking it static. Keep its signature and behavior
unchanged while preventing export of the undocumented symbol.
In `@src/PaintLayerStack.cpp`:
- Around line 95-102: Update PaintLayerStack::resizeAll so resizing a layer
preserves existing maskAlpha values, resampling them to the new dimensions or
copying the overlapping region while initializing only newly added pixels to
fully visible. Do not reset every existing mask to 255; retain the current
behavior for layers without masks.
In `@src/TexturePaintController.cpp`:
- Around line 5036-5047: The export confirmation flow around layerCount() and
the QMessageBox::warning call lacks telemetry. Add a
SentryReporter::addBreadcrumb entry with category “ui.action” that records the
texture layer count and whether the user selected Ok or Cancel, while preserving
the existing boolean return behavior.
- Around line 4769-4781: Update TexturePaintController::blendModeNames to derive
its QStringList from the complete PaintLayerBlend::Mode range using
PaintLayerBlend::modeName, rather than maintaining hardcoded names. Preserve
enum ordering so the QML combo-box indices remain aligned with modeFromName and
automatically include newly added modes.
In `@src/TexturePaintController.h`:
- Around line 324-330: Remove the Q_INVOKABLE annotation from
confirmFlattenLayersForExport(QWidget* parent) in TexturePaintController,
leaving it as a regular C++ method like flushPaintTextureForExport. Preserve its
signature and behavior.
🪄 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 Plus
Run ID: 121c076c-1e5f-4e44-a23a-b678a545da94
📒 Files selected for processing (13)
qml/PropertiesPanel.qmlsrc/CMakeLists.txtsrc/MeshImporterExporter.cppsrc/PaintBufferImageProvider.cppsrc/PaintLayerBlend.cppsrc/PaintLayerBlend.hsrc/PaintLayerBlend_test.cppsrc/PaintLayerStack.cppsrc/PaintLayerStack.hsrc/TexturePaintController.cppsrc/TexturePaintController.hsrc/mainwindow.cpptests/CMakeLists.txt
| // Toolbar — same action-string pattern as the smart-select row. | ||
| Row { | ||
| spacing: 3 | ||
| width: parent.width | ||
| Repeater { | ||
| model: [ | ||
| { label: "+", action: "add", hint: "Add layer" }, | ||
| { label: "Dup", action: "dup", hint: "Duplicate" }, | ||
| { label: "\u2191", action: "up", hint: "Move up" }, | ||
| { label: "\u2193", action: "down", hint: "Move down" }, | ||
| { label: "Mrg", action: "merge", hint: "Merge down" }, | ||
| { label: "Flat", action: "flatten", hint: "Flatten all" }, | ||
| { label: "\u2715", action: "delete", hint: "Delete layer" } | ||
| ] | ||
| Rectangle { | ||
| width: 34; height: 22; radius: 3 | ||
| property bool btnEnabled: modelData.action !== "delete" | ||
| || texPaintCol.layerCount > 1 | ||
| opacity: btnEnabled ? 1.0 : 0.35 | ||
| color: btnEnabled && layerBtnMa.containsMouse | ||
| ? Qt.lighter(PropertiesPanelController.panelColor, 1.4) | ||
| : PropertiesPanelController.headerColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: modelData.label | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 9 | ||
| } | ||
| MouseArea { | ||
| id: layerBtnMa | ||
| anchors.fill: parent | ||
| hoverEnabled: btnEnabled | ||
| enabled: btnEnabled | ||
| cursorShape: btnEnabled ? Qt.PointingHandCursor : Qt.ArrowCursor | ||
| ToolTip.text: !btnEnabled && modelData.action === "delete" | ||
| ? "Cannot delete the last layer" | ||
| : modelData.hint | ||
| ToolTip.visible: containsMouse | ||
| ToolTip.delay: 400 | ||
| onClicked: { | ||
| const idx = texPaintCol.activeLayerIndex | ||
| switch (modelData.action) { | ||
| case "add": TexturePaintController.addPaintLayer(""); break | ||
| case "dup": TexturePaintController.duplicatePaintLayer(idx); break | ||
| case "up": TexturePaintController.movePaintLayerUp(idx); break | ||
| case "down": TexturePaintController.movePaintLayerDown(idx); break | ||
| case "merge": TexturePaintController.mergePaintLayerDown(idx); break | ||
| case "flatten": TexturePaintController.flattenPaintLayers(); break | ||
| case "delete": TexturePaintController.deletePaintLayer(idx); break | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ListView { | ||
| id: layerList | ||
| width: parent.width | ||
| height: Math.min(130, Math.max(36, count * 36)) | ||
| clip: true | ||
| spacing: 2 | ||
| model: texPaintCol.paintLayers | ||
|
|
||
| delegate: Rectangle { | ||
| width: layerList.width | ||
| height: 34 | ||
| radius: 3 | ||
| color: modelData.active | ||
| ? Qt.darker(PropertiesPanelController.highlightColor, 1.2) | ||
| : (layerRowMa.containsMouse ? Qt.lighter(PropertiesPanelController.panelColor, 1.3) | ||
| : PropertiesPanelController.headerColor) | ||
| border.color: modelData.active ? PropertiesPanelController.highlightColor | ||
| : PropertiesPanelController.borderColor | ||
|
|
||
| Row { | ||
| anchors.fill: parent | ||
| anchors.margins: 3 | ||
| spacing: 4 | ||
|
|
||
| Image { | ||
| width: 28; height: 28 | ||
| source: modelData.thumbnailUrl | ||
| fillMode: Image.PreserveAspectFit | ||
| smooth: false | ||
| cache: false | ||
| } | ||
|
|
||
| Text { | ||
| width: Math.max(40, layerList.width - 108) | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| text: modelData.name | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 10 | ||
| elide: Text.ElideRight | ||
| } | ||
|
|
||
| // Visible toggle (eye) | ||
| Rectangle { | ||
| width: 18; height: 18; radius: 3 | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| color: modelData.visible | ||
| ? PropertiesPanelController.highlightColor | ||
| : PropertiesPanelController.controlBgColor | ||
| border.color: PropertiesPanelController.borderColor; border.width: 1 | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: modelData.visible ? "\u2713" : "" | ||
| color: "white"; font.pixelSize: 9 | ||
| } | ||
| MouseArea { | ||
| anchors.fill: parent; cursorShape: Qt.PointingHandCursor | ||
| onClicked: TexturePaintController.setPaintLayerVisible( | ||
| modelData.index, !modelData.visible) | ||
| } | ||
| } | ||
|
|
||
| // Solo toggle | ||
| Rectangle { | ||
| width: 18; height: 18; radius: 3 | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| color: modelData.solo ? "#806622" : PropertiesPanelController.controlBgColor | ||
| border.color: PropertiesPanelController.borderColor; border.width: 1 | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: "S" | ||
| color: modelData.solo ? "#ffcc00" : PropertiesPanelController.textColor | ||
| font.pixelSize: 8; font.bold: true | ||
| } | ||
| MouseArea { | ||
| anchors.fill: parent; cursorShape: Qt.PointingHandCursor | ||
| onClicked: TexturePaintController.setPaintLayerSolo( | ||
| modelData.index, !modelData.solo) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| MouseArea { | ||
| id: layerRowMa | ||
| anchors.fill: parent | ||
| z: -1 | ||
| hoverEnabled: true | ||
| onClicked: TexturePaintController.activeLayerIndex = modelData.index | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Make the layer toolbar and list keyboard accessible.
The toolbar buttons, layer selection, visibility, and solo controls are mouse-only. Use ToolButton/CheckBox/ItemDelegate, or add tab focus, accessible roles/names, focus indicators, and Space/Enter handlers.
🤖 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/PropertiesPanel.qml` around lines 4727 - 4871, Make the layer toolbar and
layerList controls keyboard accessible: replace or augment the toolbar
rectangles, layer-selection MouseArea, and visibility/solo controls with
focusable controls that expose accessible roles and descriptive names, show a
visible focus indicator, and invoke the existing actions on Space or Enter.
Preserve the current enabled-state behavior and layer action symbols, including
modelData.action, modelData.index, and the existing TexturePaintController
calls.
| if (auto* tpc = TexturePaintController::instance()) { | ||
| if (!tpc->confirmFlattenLayersForExport(this)) { | ||
| AnimationControlController::instance()->resumePollTimer(); | ||
| if (wasRendering) m_pTimer->start(); | ||
| SentryReporter::finishTransaction(txn); | ||
| return; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Confirm flattening exactly once per export operation.
The selected-export flow preflights confirmation and then invokes an exporter that confirms again. Because the helper evaluates the current selection, layered selections can produce duplicate prompts or one prompt per exported item.
src/mainwindow.cpp#L5559-L5567: retain one confirmation for the entire selected-export operation, or remove this preflight if the exporter owns confirmation.src/MeshImporterExporter.cpp#L3384-L3390: avoid re-prompting after the selected-export preflight, using an explicit confirmation-state/target contract if other callers require confirmation.
📍 Affects 2 files
src/mainwindow.cpp#L5559-L5567(this comment)src/MeshImporterExporter.cpp#L3384-L3390
🤖 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/mainwindow.cpp` around lines 5559 - 5567, Ensure flatten-layer
confirmation occurs exactly once per selected-export operation: update the
selected-export flow around
TexturePaintController::confirmFlattenLayersForExport in src/mainwindow.cpp
lines 5559-5567 and the exporter confirmation in src/MeshImporterExporter.cpp
lines 3384-3390 so one layer owns the prompt and the other receives or honors an
explicit confirmed state/target; preserve confirmation for other callers that
require it and prevent per-item re-prompting.
| void TexturePaintController::pushLayerOpUndo(const QString& label, | ||
| PaintLayerStack::Snapshot before, | ||
| PaintLayerStack::Snapshot after) | ||
| { | ||
| UndoManager::getSingleton()->push( | ||
| new PaintLayerOpCommand(this, label, std::move(before), std::move(after))); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Every layer op pushes two full pixel-level stack snapshots onto the undo stack.
PaintLayerStack::Snapshot deep-copies every layer's TexturePaintBuffer, so a 4-layer 2048² session costs ~64 MB per snapshot and ~128 MB per undo entry — a handful of visibility/opacity toggles can exhaust memory. Attribute-only ops (visibility, opacity, blend mode, solo, rename) don't need pixel data at all; consider a lightweight metadata-only undo command for those and reserving full snapshots for add/delete/duplicate/merge/flatten.
🤖 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 4721 - 4727, Update
TexturePaintController::pushLayerOpUndo and the related PaintLayerOpCommand flow
to use a lightweight metadata-only undo command for visibility, opacity, blend
mode, solo, and rename operations, avoiding pixel-buffer copies. Retain full
PaintLayerStack::Snapshot undo entries for add, delete, duplicate, merge, and
flatten operations.
Bind layer-op undo to the paint session texture, invalidate stroke baselines on layer changes, sync export flush, coalesce opacity-slider undo, fix normal alpha compositing, solo index on duplicate, merge visibility, and add lock UI. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TexturePaintController.cpp (1)
4808-4823: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winNon-standard Sentry breadcrumb categories for layer operations.
All new layer-operation breadcrumbs use ad-hoc categories (
paint.layer.add,.delete,.duplicate,.reorder,.merge,.flatten,.opacity,.export) instead ofui.action, even though these are all user-triggered UI actions. This contradicts the convention already used in this same file for wand/vertex stroke breadcrumbs (Lines 3210-3212, 3218), which correctly use"ui.action".🔧 Proposed fix (repeat for each site)
- SentryReporter::addBreadcrumb(QStringLiteral("paint.layer.add"), + SentryReporter::addBreadcrumb(QStringLiteral("ui.action"), QStringLiteral("Added layer '%1'").arg(m_layerStack.layer(idx).name));Affected sites:
addPaintLayer(4817),deletePaintLayer(4836),duplicatePaintLayer(4852),movePaintLayerUp/Down(4869, 4884),mergePaintLayerDown(4911),flattenPaintLayers(4927),setPaintLayerOpacity/endPaintLayerOpacityDrag(4971, 4993),flushPaintTextureForExport(5113).As per coding guidelines: "Track all user-facing actions and significant operations with
SentryReporter::addBreadcrumb; useui.actionfor UI actions,ai.tool_callfor MCP calls, andfile.import/file.exportfor I/O."Also applies to: 4825-4841, 4843-4858, 4860-4888, 4902-4916, 4918-4931, 4960-4994, 5103-5117
🤖 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 4808 - 4823, Update the SentryReporter::addBreadcrumb calls in addPaintLayer, deletePaintLayer, duplicatePaintLayer, movePaintLayerUp, movePaintLayerDown, mergePaintLayerDown, flattenPaintLayers, setPaintLayerOpacity, endPaintLayerOpacityDrag, and flushPaintTextureForExport to use the standard "ui.action" category for these user-triggered actions, while preserving their existing breadcrumb messages and behavior.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/TexturePaintController.cpp`:
- Around line 4960-4994: Add a lightweight drag-change flag associated with
beginPaintLayerOpacityDrag, set it only when setPaintLayerOpacity applies an
opacity change during an active drag, and guard endPaintLayerOpacityDrag so
pushLayerOpUndo runs only when that flag is set. Also ensure non-drag
setPaintLayerOpacity calls do not create undo entries when the effective opacity
is unchanged, while preserving existing snapshot and breadcrumb behavior for
real changes.
---
Outside diff comments:
In `@src/TexturePaintController.cpp`:
- Around line 4808-4823: Update the SentryReporter::addBreadcrumb calls in
addPaintLayer, deletePaintLayer, duplicatePaintLayer, movePaintLayerUp,
movePaintLayerDown, mergePaintLayerDown, flattenPaintLayers,
setPaintLayerOpacity, endPaintLayerOpacityDrag, and flushPaintTextureForExport
to use the standard "ui.action" category for these user-triggered actions, while
preserving their existing breadcrumb messages and behavior.
🪄 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 Plus
Run ID: 9610d547-148a-48c8-8557-af5cd9fd2820
📒 Files selected for processing (6)
qml/PropertiesPanel.qmlsrc/MeshImporterExporter.cppsrc/PaintLayerBlend.cppsrc/PaintLayerStack.cppsrc/TexturePaintController.cppsrc/TexturePaintController.h
💤 Files with no reviewable changes (1)
- src/MeshImporterExporter.cpp
Use ui.action/file.export breadcrumbs, derive blend mode names from the enum, guard opacity-drag undo against no-op slider releases, add export flatten confirmation telemetry, preserve mask data on layer resize, give compositePixelAt internal linkage, and extend layer stack tests. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/TexturePaintController.cpp (1)
4794-4801: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a sentinel before hardcoding the blend-mode enum bound.
PaintLayerBlend::Mode::Hueis the last enumerator, so the current loop still covers all defined modes. The real issue is maintainability: future enum values could be inserted afterHueand would stay out of the QML dropdown unless this bound is updated. Add a non-UICount/Lastenum value and iterate up to it instead of relying on the last UI enumerator.🤖 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 4794 - 4801, Update PaintLayerBlend::Mode to include a non-UI Count or Last sentinel after the existing blend modes, then change TexturePaintController::blendModeNames() to use that sentinel as the loop bound. Preserve the existing UI mode ordering and exclude the sentinel itself from the returned names.
🤖 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.
Nitpick comments:
In `@src/TexturePaintController.cpp`:
- Around line 4794-4801: Update PaintLayerBlend::Mode to include a non-UI Count
or Last sentinel after the existing blend modes, then change
TexturePaintController::blendModeNames() to use that sentinel as the loop bound.
Preserve the existing UI mode ordering and exclude the sentinel itself from the
returned names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 178cdb06-d103-439e-a5e7-102c710241b4
📒 Files selected for processing (5)
src/PaintLayerBlend.cppsrc/PaintLayerBlend_test.cppsrc/PaintLayerStack.cppsrc/TexturePaintController.cppsrc/TexturePaintController.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/PaintLayerBlend_test.cpp
- src/PaintLayerBlend.cpp
|
1 participant
Summary
PaintLayerStackwith blend modes (Normal, Multiply, Screen, Overlay, Add, Subtract, Soft Light, Hue), per-layer opacity/visibility/lock/solo, and undoable layer operationsTest plan
PaintLayerBlend_testunit testsMade with Cursor
Summary by CodeRabbit