Vertex paint: toolbar tool, correct colour packing, brush UX - #336
Merged
Conversation
Add vertex-color accessors and ensure VES_DIFFUSE data is written back on commit and preserved when rebuilding buffers. Includes a test helper mesh with vertex colors and a round-trip test. Made-with: Cursor
- Add checkable paintbrush tool after Fill on objects toolbar; arrow opens menu with swatches, QColorDialog, radius/strength sliders. - Clear vertex paint when exiting edit mode; disable component selection and box marquee while paint mode is on (miss on mesh does not select). - Custom brush cursor on viewports when paint mode is active. - Pack VET_COLOUR with ColourValue::getAsBYTE for correct RGB on OpenGL; mirror in Assimp MeshProcessor and EditableMesh read/write helpers. - Remove duplicate vertex paint controls from PropertiesPanel (toolbar only). Made-with: Cursor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughEnd-to-end vertex-painting was added: interactive brush UI and cursor, stroke lifecycle with local hit-testing, per-vertex color storage and GPU commit paths, tests and helpers, one Assimp color-packing tweak, and a project version bump. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant UI as "MainWindow"
participant TO as "TransformOperator"
participant Controller as "EditModeController"
participant Mesh as "EditableMesh"
participant Ogre as "Ogre3D"
User->>UI: toggle "Vertex paint" on
UI->>Controller: setVertexPaintEnabled(true)
User->>TO: LMB press on mesh
TO->>Controller: beginVertexPaintStroke(widget, screenPos)
Controller->>Mesh: hitTestLocalPointOnMesh(screenPos)
Mesh->>Ogre: perform pick/raycast -> triangle, local pos
Ogre-->>Mesh: local hit pos + normal
Controller->>Mesh: applyVertexColorBrush(localPos, radius, color, strength)
Mesh->>Mesh: ensureVertexColorBuffers()
Mesh->>Mesh: writeVertexColors(...) and commitVertexColorsToEntity()
User->>TO: mouse move (LMB held)
TO->>Controller: updateVertexPaintStroke(widget, newPos)
Controller->>Mesh: applyVertexColorBrush(newLocalPos,...)
User->>TO: LMB release
TO->>Controller: endVertexPaintStroke(commitUndo=true)
Controller->>Mesh: commitVertexColorsToEntity(entity)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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. Review rate limit: 0/1 reviews remaining, refill in 55 minutes and 58 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/EditableMesh_test.cpp (1)
247-267: Re-check the untouched vertices after the reload too.This proves the edited vertex survives
commitToEntity()+resizeEntityBuffers(), but the regression here is buffer rebuilds dropping existingVES_DIFFUSEdata. Re-assert vertices 0 and 2 afterreloaded.loadFromEntity(entity)so the test also catches non-edited colors being clobbered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditableMesh_test.cpp` around lines 247 - 267, After reloading the entity into EditableMesh reloaded (after reloaded.loadFromEntity(entity)), also re-check the untouched vertices 0 and 2 to ensure their VES_DIFFUSE colors weren't clobbered; call reloaded.getVertexColor(0,0) and reloaded.getVertexColor(0,2) and add EXPECT_NEAR assertions matching the original expected values (1.0f on r for vertex 0 and 1.0f on b for vertex 2) similar to the existing checks for c1b, so the test verifies both the edited vertex survives commitToEntity() + resizeEntityBuffers() and that non-edited colors are preserved.src/EditableMesh.cpp (1)
1048-1092: Consider dirty-range updates for brush strokes.
writeVertexColors()still round-trips the entire diffuse buffer on every paint update. On dense meshes that makes brush cost scale with total vertex count instead of touched vertices, so this is a likely hotspot if paint lag shows up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditableMesh.cpp` around lines 1048 - 1092, writeVertexColors currently reads and writes the whole diffuse buffer (bufSize) each paint, causing brush cost to scale with total vertices; change it to perform a dirty-range update: scan vertices[0..count) to compute the first and last index where hasColor is true (minIndex, maxIndex), compute byte offset = minIndex*vertexSize and length = (maxIndex-minIndex+1)*vertexSize, read only that subrange from vbuf (instead of full bufCopy), modify only those elements using elem->baseVertexPointerToElement with the per-vertex pointer into the subrange, then lock/update only that range on vbuf (use lock(offset,length, appropriate access flag such as HBL_NORMAL or writeData with offset) and avoid HBL_DISCARD unless replacing entire buffer); reference functions/vars: writeVertexColors, vbuf, bufCopy, vertexSize, count, elem->baseVertexPointerToElement, vbuf->lock/unlock and HardwareBuffer::HBL_DISCARD to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/EditModeController_test.cpp`:
- Around line 163-169: This test mutates the global EditModeController singleton
brush color and doesn't restore it; capture the current color via
EditModeController::instance()->vertexPaintColor() at the start of
TEST(EditModeControllerGeometry, VertexPaintBrushColorFromHexString) and after
your EXPECTs call EditModeController::instance()->setVertexPaintBrushColor(...)
to restore the saved original color so subsequent tests are not order-dependent;
use the existing instance(), setVertexPaintBrushColor, and vertexPaintColor
symbols to locate and implement the save/restore.
In `@src/EditModeController.cpp`:
- Around line 415-416: The vertex-paint shutdown currently unconditionally calls
setVertexPaintEnabled(false) which triggers endVertexPaintStroke(true) and
creates an undo referencing a mesh that may be destroyed; update
exitEditMode(bool commitChanges) to match the bevel/knife pattern by only
committing vertex-paint changes when commitChanges is true — either call
setVertexPaintEnabled(false) only if commitChanges is true or ensure
endVertexPaintStroke is invoked with commitChanges (use
endVertexPaintStroke(commitChanges) or propagate commitChanges into
setVertexPaintEnabled) so no undo is created when discarding changes; adjust
references to m_vertexPaintEnabled, setVertexPaintEnabled, and
endVertexPaintStroke accordingly.
In `@src/mainwindow.cpp`:
- Around line 785-788: Replace the emoji text on the QToolButton with a real
icon: instead of calling vertexPaintButton->setText(...) use
vertexPaintButton->setIcon(...) with a bundled SVG/PNG resource (e.g.
":/icons/vertex_paint.svg" or similar) and set an appropriate icon size; ensure
the new icon file is added to the Qt resource (.qrc) and referenced by the same
symbol (vertexPaintButton / ui->objectsToolbar) so the button shows a consistent
icon across Windows, Linux and macOS.
- Around line 891-897: When vertex paint is enabled it must be mutually
exclusive with transform tools: update the shared transform-switch path (the
code that calls setTransformState / handles Q/W/E/R toolbar actions) to clear
vertex paint by calling
EditModeController::instance()->setVertexPaintEnabled(false) and unchecking
vertexPaintButton whenever setTransformState is called with a state !=
TransformOperator::TS_SELECT; alternatively, connect the global transform-state
change signal to a slot/lambda that tests the new state and if it is not
TS_SELECT calls vertexPaintButton->setChecked(false) and
EditModeController::instance()->setVertexPaintEnabled(false), keeping the
existing behavior that enabling paint forces
setTransformState(TransformOperator::TS_SELECT).
- Around line 812-818: Add Sentry breadcrumbs for all user-facing paint-tool
interactions: inside the colorBtn click lambda (around QColorDialog::getColor /
emPaint->setVertexPaintColor and before/after syncPaintColorBtn), add
SentryReporter::addBreadcrumb("ui.action", "<event>") calls for events like
"brush.color.picker.opened" and "brush.color.changed"; also add a breadcrumb in
the emPaint->vertexPaintChanged connection handler. Repeat the same pattern for
the brush swatch handlers and the radius/strength slider callbacks referenced
near the existing paint handlers (use messages like "brush.swatch.selected",
"brush.radius.changed", "brush.strength.changed"), ensuring each handler calls
SentryReporter::addBreadcrumb("ui.action", ...) with a concise event string
before returning.
In `@src/OgreWidget.cpp`:
- Around line 497-501: When the pointer is idle and paint mode is toggled off
the brush cursor can remain stuck; modify the existing branch around
e->buttons() == Qt::NoButton so that when no mouse buttons are pressed you set
the brush cursor if EditModeController::instance()->isEditModeActive() &&
vertexPaintEnabled(), otherwise clear/reset the cursor. Concretely: in the block
containing EditModeController::instance(), call
QWidget::setCursor(vertexPaintBrushCursor()) when vertexPaintEnabled() is true,
and in the else case (still under e->buttons() == Qt::NoButton) call
QWidget::unsetCursor() or set a default cursor (e.g., QWidget::unsetCursor() or
setCursor(Qt::ArrowCursor)); optionally only unset if QWidget::cursor() equals
vertexPaintBrushCursor() to avoid clobbering other cursors.
---
Nitpick comments:
In `@src/EditableMesh_test.cpp`:
- Around line 247-267: After reloading the entity into EditableMesh reloaded
(after reloaded.loadFromEntity(entity)), also re-check the untouched vertices 0
and 2 to ensure their VES_DIFFUSE colors weren't clobbered; call
reloaded.getVertexColor(0,0) and reloaded.getVertexColor(0,2) and add
EXPECT_NEAR assertions matching the original expected values (1.0f on r for
vertex 0 and 1.0f on b for vertex 2) similar to the existing checks for c1b, so
the test verifies both the edited vertex survives commitToEntity() +
resizeEntityBuffers() and that non-edited colors are preserved.
In `@src/EditableMesh.cpp`:
- Around line 1048-1092: writeVertexColors currently reads and writes the whole
diffuse buffer (bufSize) each paint, causing brush cost to scale with total
vertices; change it to perform a dirty-range update: scan vertices[0..count) to
compute the first and last index where hasColor is true (minIndex, maxIndex),
compute byte offset = minIndex*vertexSize and length =
(maxIndex-minIndex+1)*vertexSize, read only that subrange from vbuf (instead of
full bufCopy), modify only those elements using elem->baseVertexPointerToElement
with the per-vertex pointer into the subrange, then lock/update only that range
on vbuf (use lock(offset,length, appropriate access flag such as HBL_NORMAL or
writeData with offset) and avoid HBL_DISCARD unless replacing entire buffer);
reference functions/vars: writeVertexColors, vbuf, bufCopy, vertexSize, count,
elem->baseVertexPointerToElement, vbuf->lock/unlock and
HardwareBuffer::HBL_DISCARD to locate and implement the change.
🪄 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: 88d43b21-3962-488d-b4a4-0e488c5208df
📒 Files selected for processing (13)
src/Assimp/MeshProcessor.cppsrc/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/EditableMesh.cppsrc/EditableMesh.hsrc/EditableMesh_test.cppsrc/OgreWidget.cppsrc/OgreWidget.hsrc/TestHelpers.hsrc/TransformOperator.cppsrc/TransformOperator.hsrc/mainwindow.cpp
Keep the brush cursor fixed-size to avoid misleading world-space scale changes with camera distance, and ensure the toolbar paint settings menu shows strength directly after radius. Made-with: Cursor
Show a projected brush-radius ring while hovering in paint mode (before first click), keep the paint tool visibly active, and refresh cursor state immediately when toggling paint. Also bump app/docs action examples to 2.32.0 so CI snippets and site fallbacks match the new release line. Made-with: Cursor
Prevent undo creation when leaving edit mode with discarded paint changes, enforce paint/tool exclusivity on transform switches, replace the emoji toolbar glyph with a resource icon, add paint interaction breadcrumbs, and harden related tests for singleton state and color preservation. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/EditModeController.cpp (1)
416-417:⚠️ Potential issue | 🟠 MajorDiscarding edit mode still commits the paint stroke.
exitEditMode(false)still funnels throughsetVertexPaintEnabled(false), and that unconditionally callsendVertexPaintStroke(true). The discard path should not enqueue a"Vertex Paint"undo entry.Proposed fix
- if (m_vertexPaintEnabled) - setVertexPaintEnabled(false); + if (m_vertexPaintEnabled) { + if (m_vertexPaintStrokeActive) + endVertexPaintStroke(commitChanges); + m_vertexPaintEnabled = false; + clearVertexPaintPreview(); + SentryReporter::addBreadcrumb("edit_mode", "Vertex paint disabled"); + emit vertexPaintChanged(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 416 - 417, The discard path currently calls setVertexPaintEnabled(false) which always calls endVertexPaintStroke(true) and thus commits a "Vertex Paint" undo; change the flow so that when exitEditMode(false) (discard) is used it signals a non-commit to the paint stroke: propagate a commit flag from exitEditMode into setVertexPaintEnabled (or add an overload/optional parameter) and ensure setVertexPaintEnabled(false) calls endVertexPaintStroke(false) when invoked from the discard path; update exitEditMode, setVertexPaintEnabled, and endVertexPaintStroke usages so only the save/commit path enqueues the undo entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/EditModeController.cpp`:
- Around line 1217-1221: When hitTestLocalPointOnMesh(...) returns false the
function currently returns without clearing the brush preview leaving a stale
ring; call clearVertexPaintPreview(widget) just before the early return so the
preview is removed when the drag leaves the mesh, keeping
updateVertexPaintPreview(widget, screenPos) for the hit case.
- Around line 818-872: applyVertexColorBrush currently scans all vertices every
call and immediately writes colors back, causing per-mouse-move full-mesh
repaints; change it to batch updates by recording which submeshes and vertex
index ranges were modified (e.g., collect touched submesh indices and min/max
vi) inside applyVertexColorBrush and set v.hasColor/v.color there but do NOT
push to the GPU or merge into the entity per-vertex; return the changed flag and
expose the dirty regions so the caller (or a coalescing painter) can flush only
the touched submesh ranges once per frame or on stroke end (refer to
applyVertexColorBrush and where its results are merged) to avoid linear scans
and repeated entity updates.
- Around line 239-288: Add Sentry breadcrumbs for paint control changes: in
setVertexPaintColor(const QColor&) emit a
SentryReporter::addBreadcrumb("ui.action", ...) when you actually change the
color (i.e., right after m_vertexPaintColor = rgb and before/after emit
vertexPaintChanged()) with a concise message containing the new color (use
rgb.name() or rgba string). Do not duplicate in setVertexPaintBrushColor since
it delegates to setVertexPaintColor. Likewise, in setVertexPaintRadius(double)
and setVertexPaintStrength(double) call
SentryReporter::addBreadcrumb("ui.action", ...) when the value is changed (after
m_vertexPaintRadius/m_vertexPaintStrength is updated) with messages like "vertex
paint radius: <value>" and "vertex paint strength: <value>" so all UI paint
control changes are tracked.
---
Duplicate comments:
In `@src/EditModeController.cpp`:
- Around line 416-417: The discard path currently calls
setVertexPaintEnabled(false) which always calls endVertexPaintStroke(true) and
thus commits a "Vertex Paint" undo; change the flow so that when
exitEditMode(false) (discard) is used it signals a non-commit to the paint
stroke: propagate a commit flag from exitEditMode into setVertexPaintEnabled (or
add an overload/optional parameter) and ensure setVertexPaintEnabled(false)
calls endVertexPaintStroke(false) when invoked from the discard path; update
exitEditMode, setVertexPaintEnabled, and endVertexPaintStroke usages so only the
save/commit path enqueues the undo entry.
🪄 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: c2d8740d-1a06-4499-9ab4-54d0a2b88fe6
📒 Files selected for processing (9)
CMakeLists.txtREADME.mdsrc/EditModeController.cppsrc/EditModeController.hsrc/OgreWidget.cppsrc/TransformOperator.cppsrc/mainwindow.cppwebsite/src/DocsApp.jsxwebsite/src/hooks/useQtmeshActionRef.js
✅ Files skipped from review due to trivial changes (4)
- website/src/hooks/useQtmeshActionRef.js
- website/src/DocsApp.jsx
- CMakeLists.txt
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/EditModeController.h
Comment on lines
+818
to
+872
| bool EditModeController::applyVertexColorBrush(EditableMesh& mesh, | ||
| const Ogre::Vector3& localCenter, | ||
| float radius, | ||
| const Ogre::ColourValue& color, | ||
| float strength) | ||
| { | ||
| if (radius <= 0.0f) | ||
| return false; | ||
|
|
||
| strength = std::clamp(strength, 0.0f, 1.0f); | ||
| if (strength <= 0.0f) | ||
| return false; | ||
|
|
||
| bool changed = false; | ||
| const float invR = 1.0f / radius; | ||
|
|
||
| for (size_t si = 0; si < mesh.subMeshes().size(); ++si) { | ||
| auto& sub = mesh.subMeshes()[si]; | ||
| for (size_t vi = 0; vi < sub.vertices.size(); ++vi) { | ||
| auto& v = sub.vertices[vi]; | ||
| const float d = v.position.distance(localCenter); | ||
| if (d > radius) | ||
| continue; | ||
|
|
||
| // Smooth falloff: w = (1 - d/r)^2 | ||
| const float t = 1.0f - (d * invR); | ||
| const float w = t * t; | ||
| const float a = strength * w; | ||
| if (a <= 0.0f) | ||
| continue; | ||
|
|
||
| const Ogre::ColourValue src = v.hasColor ? v.color : Ogre::ColourValue::White; | ||
| Ogre::ColourValue dst; | ||
| dst.r = src.r + (color.r - src.r) * a; | ||
| dst.g = src.g + (color.g - src.g) * a; | ||
| dst.b = src.b + (color.b - src.b) * a; | ||
| dst.a = src.a + (color.a - src.a) * a; | ||
|
|
||
| // Small epsilon to avoid dirtying on no-op blends. | ||
| const float eps = 1e-6f; | ||
| if (!v.hasColor || | ||
| std::abs(dst.r - src.r) > eps || | ||
| std::abs(dst.g - src.g) > eps || | ||
| std::abs(dst.b - src.b) > eps || | ||
| std::abs(dst.a - src.a) > eps) | ||
| { | ||
| v.hasColor = true; | ||
| v.color = dst; | ||
| changed = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return changed; | ||
| } |
There was a problem hiding this comment.
This stroke path repaints the whole mesh on every mouse-move.
Each sample linearly scans every vertex in applyVertexColorBrush(), then immediately pushes vertex colors back to the entity again. That will become the bottleneck on dense meshes and make the brush feel sticky. Consider tracking touched submeshes / dirty ranges before merging this UX work.
Also applies to: 1236-1246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 818 - 872, applyVertexColorBrush
currently scans all vertices every call and immediately writes colors back,
causing per-mouse-move full-mesh repaints; change it to batch updates by
recording which submeshes and vertex index ranges were modified (e.g., collect
touched submesh indices and min/max vi) inside applyVertexColorBrush and set
v.hasColor/v.color there but do NOT push to the GPU or merge into the entity
per-vertex; return the changed flag and expose the dirty regions so the caller
(or a coalescing painter) can flush only the touched submesh ranges once per
frame or on stroke end (refer to applyVertexColorBrush and where its results are
merged) to avoid linear scans and repeated entity updates.
Owner
Author
There was a problem hiding this comment.
Good catch. I kept this PR scoped to correctness/UX and CI stability, and deferred the brush performance refactor to the next epic task (batched dirty submesh/range uploads per frame/stroke). Current path is acceptable for our existing test meshes, but I agree we should not ship this as final for large meshes without coalescing. I will follow up with a dedicated perf patch/benchmark PR.
There was a problem hiding this comment.
@fernandotonon, understood — keeping this PR scoped to correctness and UX is reasonable. The linear-scan path is fine for the mesh sizes targeted by the current test suite; the bottleneck will only become noticeable on production-density meshes (tens of thousands of vertices).
Would you like me to open a GitHub issue to track the follow-up performance work? It could cover:
- Collecting touched
(submeshIndex, vertexIndex)ranges insideapplyVertexColorBrushinstead of pushing to the GPU per call. - Coalescing dirty-range flushes to once per frame (or once per stroke end) via
commitVertexColorsToEntity. - Adding a benchmark against a high-density mesh to validate the improvement.
Just say the word and I'll file it with a link back to this PR and comment.
…entry - EditableMeshTest: relax purple channel asserts after GPU byte packing - EditModeController: clear brush ring when stroke ray misses mesh - Log vertex paint color/radius/strength in controller; drop duplicate UI crumbs - Tests: hex restore via name(); empty/invalid brush strings leave color unchanged Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/OgreWidget.cpp (2)
525-532: Minor cursor handling inconsistency.
mouseMoveEventusesQWidget::unsetCursor()(line 515) whilemouseReleaseEventuses explicitsetCursor(Qt::ArrowCursor)(lines 529-531). These behave differently:unsetCursor()inherits the parent's cursor, whilesetCursor()explicitly sets it.The verbose pattern in
mouseReleaseEventcould also be simplified:♻️ Simplify and make consistent
if (EditModeController::instance()->isEditModeActive() && EditModeController::instance()->vertexPaintEnabled()) { QWidget::setCursor(vertexPaintBrushCursor()); } else { - QCursor cursor = this->cursor(); - cursor.setShape(Qt::ArrowCursor); - QWidget::setCursor(cursor); + QWidget::unsetCursor(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OgreWidget.cpp` around lines 525 - 532, Replace the explicit arrow-cursor setting in mouseReleaseEvent with the same unsetCursor() call used in mouseMoveEvent to keep behavior consistent and inherit the parent cursor; specifically, in the branch that currently calls QWidget::setCursor(cursor) (and constructs a QCursor and sets Qt::ArrowCursor), call QWidget::unsetCursor() instead when EditModeController::instance()->isEditModeActive() is false (and likewise ensure vertexPaintBrushCursor() is still used when vertexPaintEnabled() is true).
196-210: UsedevicePixelRatioF()instead of hardcoded macOS ÷2.The hardcoded
widthMod = 2assumes all macOS displays are Retina with exactly 2.0 DPI scaling. This fails for non-Retina Macs (1.0), fractional-DPI displays (1.25, 1.5), and external monitors with different scaling ratios. Qt'sdevicePixelRatioF()returns the actual DPI scaling for the current display and is the standard approach for high-DPI handling.♻️ Proposed fix using device pixel ratio
void OgreWidget::pixelSizeForCameraPicking(int& outW, int& outH) const { if (!mViewport) { outW = width(); outH = height(); return; } -#ifdef Q_OS_MACOS - const int widthMod = 2; -#else - const int widthMod = 1; -#endif - outW = static_cast<int>(mViewport->getActualWidth()) / widthMod; - outH = static_cast<int>(mViewport->getActualHeight()) / widthMod; + const qreal dpr = devicePixelRatioF(); + outW = static_cast<int>(mViewport->getActualWidth() / dpr); + outH = static_cast<int>(mViewport->getActualHeight() / dpr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OgreWidget.cpp` around lines 196 - 210, Replace the hardcoded macOS division with Qt's devicePixelRatioF() in OgreWidget::pixelSizeForCameraPicking: compute a float scale = devicePixelRatioF() (falling back to 1.0 if <= 0), use it to divide the sizes (when mViewport is present use mViewport->getActualWidth()/scale and getActualHeight()/scale; when mViewport is null use width()/scale and height()/scale), and cast the results to int for outW/outH; update references inside pixelSizeForCameraPicking accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/OgreWidget.cpp`:
- Around line 525-532: Replace the explicit arrow-cursor setting in
mouseReleaseEvent with the same unsetCursor() call used in mouseMoveEvent to
keep behavior consistent and inherit the parent cursor; specifically, in the
branch that currently calls QWidget::setCursor(cursor) (and constructs a QCursor
and sets Qt::ArrowCursor), call QWidget::unsetCursor() instead when
EditModeController::instance()->isEditModeActive() is false (and likewise ensure
vertexPaintBrushCursor() is still used when vertexPaintEnabled() is true).
- Around line 196-210: Replace the hardcoded macOS division with Qt's
devicePixelRatioF() in OgreWidget::pixelSizeForCameraPicking: compute a float
scale = devicePixelRatioF() (falling back to 1.0 if <= 0), use it to divide the
sizes (when mViewport is present use mViewport->getActualWidth()/scale and
getActualHeight()/scale; when mViewport is null use width()/scale and
height()/scale), and cast the results to int for outW/outH; update references
inside pixelSizeForCameraPicking accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3274ac7f-9336-4fc5-9f3e-bddb1b073c52
📒 Files selected for processing (5)
src/EditModeController.cppsrc/EditModeController_test.cppsrc/EditableMesh_test.cppsrc/OgreWidget.cppsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/EditableMesh_test.cpp
- src/EditModeController_test.cpp
Use the original toolbar paintbrush glyph for the vertex paint button so it matches the existing topology tool visual style across Linux themes. Made-with: Cursor
Replace the paint emoji glyph with a packaged green brush SVG so the toolbar icon renders consistently across Linux, macOS, and Windows while matching the existing tool theme. Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/mainwindow.cpp (3)
881-884:⚠️ Potential issue | 🟡 MinorMissing Sentry breadcrumb for strength slider changes.
The strength slider handler doesn't log the interaction.
🔧 Proposed fix
connect(strSlider, &QSlider::valueChanged, this, [this, emPaint, strLabel](int v) { + SentryReporter::addBreadcrumb("ui.action", + QStringLiteral("Vertex paint strength: %1").arg(v / 100.0)); emPaint->setVertexPaintStrength(v / 100.0); strLabel->setText(tr("Strength: %1").arg(emPaint->vertexPaintStrength(), 0, 'f', 2)); });As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 881 - 884, The slider valueChanged lambda for strSlider (the connect block capturing emPaint and strLabel) must call SentryReporter::addBreadcrumb to record the UI action; inside that lambda (where emPaint->setVertexPaintStrength(...) and strLabel->setText(...) are called) add a breadcrumb such as SentryReporter::addBreadcrumb("ui.interaction", QStringLiteral("strength_slider=%1").arg(v)); so the interaction is logged whenever the strength is changed by the user.
846-849:⚠️ Potential issue | 🟡 MinorMissing Sentry breadcrumb for swatch selection.
The coding guidelines require breadcrumbs for all user-facing actions. The swatch button click handler modifies the brush color but doesn't log this interaction.
🔧 Proposed fix
connect(sw, &QPushButton::clicked, this, [emPaint, hex, syncPaintColorBtn]() { + SentryReporter::addBreadcrumb("ui.action", + QStringLiteral("Vertex paint swatch: %1").arg(hex)); emPaint->setVertexPaintBrushColor(hex); syncPaintColorBtn(); });As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 846 - 849, The swatch click handler doesn't record a Sentry breadcrumb; update the lambda connected to sw (the QPushButton) so after calling emPaint->setVertexPaintBrushColor(hex) and before/after syncPaintColorBtn() it calls SentryReporter::addBreadcrumb("ui.action", "swatch.selected: " + hex) (or equivalent string composition) to record the user action; modify the anonymous slot where sw, emPaint and syncPaintColorBtn are referenced to invoke SentryReporter::addBreadcrumb with category "ui.action" and a message that includes the selected hex value.
864-867:⚠️ Potential issue | 🟡 MinorMissing Sentry breadcrumb for radius slider changes.
The radius slider handler doesn't log the interaction.
🔧 Proposed fix
connect(radSlider, &QSlider::valueChanged, this, [this, emPaint, radLabel](int v) { + SentryReporter::addBreadcrumb("ui.action", + QStringLiteral("Vertex paint radius: %1").arg(v / 100.0)); emPaint->setVertexPaintRadius(v / 100.0); radLabel->setText(tr("Radius (local): %1").arg(emPaint->vertexPaintRadius(), 0, 'f', 2)); });As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 864 - 867, The slider handler for radSlider (connected to QSlider::valueChanged) updates emPaint->setVertexPaintRadius(...) and radLabel but does not record a Sentry breadcrumb; modify the lambda connected to radSlider so after updating emPaint and radLabel it calls SentryReporter::addBreadcrumb("ui.slider", QStringLiteral("radius_changed=%1").arg(v).toStdString()) (or similar descriptive message) to record the user action; ensure the call uses the same lambda capture list (this, emPaint, radLabel) and executes on each value change in the existing connect for radSlider.
🧹 Nitpick comments (1)
src/mainwindow.cpp (1)
2011-2015: Good: Mutual exclusivity with transform tools is implemented.When switching away from
TS_SELECT, vertex paint is correctly disabled. ThesetVertexPaintEnabled(false)call in EditModeController properly terminates the stroke viaendVertexPaintStroke().Minor consideration: When this path disables vertex paint mid-stroke,
TransformOperator::mVertexPaintDragActiveisn't explicitly cleared. Subsequent mouse events will still route through paint handling untilmouseReleaseEventresets the flag. This is harmless since the paint functions checkvertexPaintEnabled(), but you may wantTransformOperatorto listen forvertexPaintChangedand resetmVertexPaintDragActivewhen paint is externally disabled.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 2011 - 2015, Add a listener in TransformOperator to clear its mVertexPaintDragActive flag when vertex paint is turned off externally: connect EditModeController's vertexPaintChanged (or equivalent) signal to a new/private slot on TransformOperator that checks EditModeController::vertexPaintEnabled() and sets mVertexPaintDragActive = false when paint is disabled; this ensures any mid-stroke state is reset immediately when EditModeController::setVertexPaintEnabled(false) is called (the existing endVertexPaintStroke()/mouseReleaseEvent checks remain intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/mainwindow.cpp`:
- Around line 881-884: The slider valueChanged lambda for strSlider (the connect
block capturing emPaint and strLabel) must call SentryReporter::addBreadcrumb to
record the UI action; inside that lambda (where
emPaint->setVertexPaintStrength(...) and strLabel->setText(...) are called) add
a breadcrumb such as SentryReporter::addBreadcrumb("ui.interaction",
QStringLiteral("strength_slider=%1").arg(v)); so the interaction is logged
whenever the strength is changed by the user.
- Around line 846-849: The swatch click handler doesn't record a Sentry
breadcrumb; update the lambda connected to sw (the QPushButton) so after calling
emPaint->setVertexPaintBrushColor(hex) and before/after syncPaintColorBtn() it
calls SentryReporter::addBreadcrumb("ui.action", "swatch.selected: " + hex) (or
equivalent string composition) to record the user action; modify the anonymous
slot where sw, emPaint and syncPaintColorBtn are referenced to invoke
SentryReporter::addBreadcrumb with category "ui.action" and a message that
includes the selected hex value.
- Around line 864-867: The slider handler for radSlider (connected to
QSlider::valueChanged) updates emPaint->setVertexPaintRadius(...) and radLabel
but does not record a Sentry breadcrumb; modify the lambda connected to
radSlider so after updating emPaint and radLabel it calls
SentryReporter::addBreadcrumb("ui.slider",
QStringLiteral("radius_changed=%1").arg(v).toStdString()) (or similar
descriptive message) to record the user action; ensure the call uses the same
lambda capture list (this, emPaint, radLabel) and executes on each value change
in the existing connect for radSlider.
---
Nitpick comments:
In `@src/mainwindow.cpp`:
- Around line 2011-2015: Add a listener in TransformOperator to clear its
mVertexPaintDragActive flag when vertex paint is turned off externally: connect
EditModeController's vertexPaintChanged (or equivalent) signal to a new/private
slot on TransformOperator that checks EditModeController::vertexPaintEnabled()
and sets mVertexPaintDragActive = false when paint is disabled; this ensures any
mid-stroke state is reset immediately when
EditModeController::setVertexPaintEnabled(false) is called (the existing
endVertexPaintStroke()/mouseReleaseEvent checks remain intact).
|
This was
linked to
issues
Apr 29, 2026
10 tasks
This was referenced May 12, 2026
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
ColourValue::getAsBYTE()for correct RGB on OpenGL (plus Assimp import alignment).Test plan
Made with Cursor
Summary by CodeRabbit