feat(paint): QML falloff slider + vertex color preview toggle - #351
Conversation
- Add Vertex Paint section to QML Edit Mode Tools with radius/strength/falloff - Expose brush falloff parameter and apply it in distance weighting - Add per-entity vertex color preview material override toggle Made-with: Cursor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a vertex color preview toggle in the QML inspector, implements a render-time material override for preview, introduces a configurable vertex-paint falloff threaded into brush application, and updates related tests and mesh commit behavior. Changes
Sequence DiagramsequenceDiagram
participant QML as "QML/Inspector"
participant EMC as "EditModeController"
participant MatSys as "Material Manager"
participant Renderer as "Renderer"
QML->>EMC: setVertexColorPreviewEnabled(true)
EMC->>EMC: store preview flag
EMC->>MatSys: applyVertexColorPreviewMaterials()
MatSys->>MatSys: record original materials
MatSys->>MatSys: override materials with EditMode/VertexColorPreview
MatSys->>Renderer: update bindings
Renderer->>Renderer: render with vertex colors visible
QML->>EMC: setVertexPaintFalloff(value)
EMC->>EMC: clamp/store falloff
QML->>EMC: apply brush stroke (center, radius, color, strength)
EMC->>EMC: compute per-vertex weight using falloff exponent
EMC->>Renderer: update vertex color buffers / request redraw
QML->>EMC: setVertexColorPreviewEnabled(false)
EMC->>MatSys: removeVertexColorPreviewMaterials()
MatSys->>MatSys: restore original materials
MatSys->>Renderer: update bindings
Renderer->>Renderer: render with original materials
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 4e33126648
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (m_vertexColorPreviewEnabled) { | ||
| removeVertexColorPreviewMaterials(); | ||
| m_vertexColorPreviewEnabled = false; |
There was a problem hiding this comment.
When both wireframe and vertex-color preview are enabled, this exit path removes wireframe materials first and then restores preview-saved materials. The preview snapshot can contain wireframe material names, so after removeWireframeMaterials() deletes those resources, removeVertexColorPreviewMaterials() reapplies stale names instead of the original entity materials. In that state, leaving Edit Mode can restore the wrong material set (or missing resources); preview cleanup needs to run before wireframe teardown.
Useful? React with 👍 / 👎.
| // If preview is on, re-apply after toggling wireframe so we snapshot/restore | ||
| // the current (wireframe or solid) material state correctly. | ||
| if (m_vertexColorPreviewEnabled) | ||
| applyVertexColorPreviewMaterials(); |
There was a problem hiding this comment.
This reapply path runs while preview is already active and applyVertexColorPreviewMaterials() clears/rebuilds m_vertexColorPreviewSavedMaterials from the currently overridden sub-entity materials. If the user toggles wireframe while preview is on, the saved baseline becomes preview-derived materials, so turning preview off no longer returns to the real underlying material state. This makes the preview toggle non-reversible in that workflow.
Useful? React with 👍 / 👎.
Made-with: Cursor
|
Addressed both preview/wireframe edge cases in commit 2c68bfb:
|
…ontrols Made-with: Cursor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/EditModeController_test.cpp (1)
168-179: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winStrengthen this test to verify falloff math, not just partial blending.
The current
v1checks only prove the vertex is partially painted; they don’t prove the newfalloffparameter is applied correctly. Please assert the expected weighted result (or compare two different falloff values) so exponent regressions fail deterministically.Proposed test hardening
// v1 is within radius but falloff should keep it not fully red. - EXPECT_LT(sub.vertices[1].color.g, 1.0f); - EXPECT_GT(sub.vertices[1].color.g, 0.0f); + const float expectedWeight = std::pow(1.0f - (1.0f / 1.1f), 0.5f); + const float expectedG = 1.0f - expectedWeight; + EXPECT_NEAR(sub.vertices[1].color.g, expectedG, 1e-3f);As per coding guidelines
src/**/*_test.cpp: “Add Google Test unit tests for new functionality.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController_test.cpp` around lines 168 - 179, The test currently only checks v1 was partially painted; instead compute and assert the exact expected blended color using the same falloff formula used by EditModeController::applyVertexColorBrush: calculate distance from brush center to sub.vertices[1], compute normalized influence = clamp(1.0f - dist / radius, 0.0f, 1.0f), apply falloff exponent to get weight (e.g. w = pow(influence, falloff) * strength), then compute expected color = lerp(original color, paint.color, w) and replace the loose EXPECT_LT/EXPECT_GT with an EXPECT_NEAR comparing sub.vertices[1].color (r/g/b) to that expected value; alternatively add a second call with a different falloff and assert the color changes accordingly to catch exponent regressions.
🤖 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 241-255: The current setter flips m_vertexColorPreviewEnabled and
emits vertexColorPreviewChanged before verifying buffer setup; change
setVertexColorPreviewEnabled to first attempt preparation (call
ensureVertexColorBuffers or call applyVertexColorPreviewMaterials and check its
success) and only set m_vertexColorPreviewEnabled and emit
vertexColorPreviewChanged if the setup succeeds; update
applyVertexColorPreviewMaterials (or add a boolean-returning helper) to return
success/failure so setVertexColorPreviewEnabled can abort and leave the preview
disabled when buffer creation fails, and keep removeVertexColorPreviewMaterials
behavior for disabling as-is.
- Around line 509-517: The teardown code clears m_vertexColorPreviewEnabled and
emits vertexColorPreviewChanged() which forces the UI toggle off and prevents
enterEditMode()'s reapply path from working; instead, call
removeVertexColorPreviewMaterials() to remove materials but do NOT reset
m_vertexColorPreviewEnabled or emit vertexColorPreviewChanged() here so the
persistent preview flag remains true and enterEditMode() can reapply it. Ensure
only removeVertexColorPreviewMaterials() is invoked during teardown and leave
m_vertexColorPreviewEnabled and the signal handling to the normal
toggle/enterEditMode() flow.
In `@src/mainwindow.cpp`:
- Around line 996-1000: Add breadcrumb logging for falloff changes by attaching
a handler to falloffSlider->sliderReleased that reads the current value (use
falloffSlider->value() or emPaint->vertexPaintFalloff()), formats it (e.g.,
percent or float with two decimals) and calls
SentryReporter::addBreadcrumb("brush.falloff", formattedMessage). Leave the
existing valueChanged connection (connect(falloffSlider, &QSlider::valueChanged,
...)) and the vertexPaintChanged connection intact; the new sliderReleased
handler should only log once per completed drag so telemetry isn't noisy. Ensure
the handler references falloffSlider and emPaint (or
emPaint->vertexPaintFalloff()) and creates a clear message like "Falloff set to
X" before calling SentryReporter::addBreadcrumb.
---
Outside diff comments:
In `@src/EditModeController_test.cpp`:
- Around line 168-179: The test currently only checks v1 was partially painted;
instead compute and assert the exact expected blended color using the same
falloff formula used by EditModeController::applyVertexColorBrush: calculate
distance from brush center to sub.vertices[1], compute normalized influence =
clamp(1.0f - dist / radius, 0.0f, 1.0f), apply falloff exponent to get weight
(e.g. w = pow(influence, falloff) * strength), then compute expected color =
lerp(original color, paint.color, w) and replace the loose EXPECT_LT/EXPECT_GT
with an EXPECT_NEAR comparing sub.vertices[1].color (r/g/b) to that expected
value; alternatively add a second call with a different falloff and assert the
color changes accordingly to catch exponent regressions.
🪄 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: 5fc2b3b1-eaa5-47df-a926-05a8910c2718
📒 Files selected for processing (5)
qml/PropertiesPanel.qmlsrc/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/mainwindow.cpp
| void EditModeController::setVertexColorPreviewEnabled(bool enabled) | ||
| { | ||
| if (m_vertexColorPreviewEnabled == enabled) | ||
| return; | ||
|
|
||
| m_vertexColorPreviewEnabled = enabled; | ||
| if (enabled) | ||
| applyVertexColorPreviewMaterials(); | ||
| else | ||
| removeVertexColorPreviewMaterials(); | ||
|
|
||
| SentryReporter::addBreadcrumb( | ||
| "ui.action", | ||
| QStringLiteral("Vertex color preview %1").arg(enabled ? "enabled" : "disabled")); | ||
| emit vertexColorPreviewChanged(); |
There was a problem hiding this comment.
Only expose preview as enabled after buffer setup succeeds.
beginVertexPaintStroke() already treats ensureVertexColorBuffers() as fallible, but this path sets m_vertexColorPreviewEnabled and emits vertexColorPreviewChanged() before verifying setup, while applyVertexColorPreviewMaterials() ignores the return value entirely. If buffer creation fails, the UI can report preview enabled even though the entity was never prepared for the preview material.
Possible fix
-void EditModeController::setVertexColorPreviewEnabled(bool enabled)
+void EditModeController::setVertexColorPreviewEnabled(bool enabled)
{
if (m_vertexColorPreviewEnabled == enabled)
return;
- m_vertexColorPreviewEnabled = enabled;
- if (enabled)
- applyVertexColorPreviewMaterials();
- else
+ if (enabled) {
+ if (!applyVertexColorPreviewMaterials())
+ return;
+ } else {
removeVertexColorPreviewMaterials();
+ }
+
+ m_vertexColorPreviewEnabled = enabled;
SentryReporter::addBreadcrumb(
"ui.action",
QStringLiteral("Vertex color preview %1").arg(enabled ? "enabled" : "disabled"));
emit vertexColorPreviewChanged();
}-void EditModeController::applyVertexColorPreviewMaterials()
+bool EditModeController::applyVertexColorPreviewMaterials()
{
if (!m_editModeActive || !m_editEntity || !m_editableMesh)
- return;
+ return false;
// Ensure the mesh actually has vertex color buffers; default-initialize to white if missing.
- m_editableMesh->ensureVertexColorBuffers(m_editEntity);
+ if (!m_editableMesh->ensureVertexColorBuffers(m_editEntity))
+ return false;
m_vertexColorPreviewSavedMaterials.clear();
for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) {
auto* subEnt = m_editEntity->getSubEntity(i);
if (!subEnt)
continue;
m_vertexColorPreviewSavedMaterials[i] = subEnt->getMaterialName();
subEnt->setMaterialName(
"EditMode/VertexColorPreview",
Ogre::ResourceGroupManager::INTERNAL_RESOURCE_GROUP_NAME);
}
+ return true;
}Also applies to: 5171-5189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 241 - 255, The current setter flips
m_vertexColorPreviewEnabled and emits vertexColorPreviewChanged before verifying
buffer setup; change setVertexColorPreviewEnabled to first attempt preparation
(call ensureVertexColorBuffers or call applyVertexColorPreviewMaterials and
check its success) and only set m_vertexColorPreviewEnabled and emit
vertexColorPreviewChanged if the setup succeeds; update
applyVertexColorPreviewMaterials (or add a boolean-returning helper) to return
success/failure so setVertexColorPreviewEnabled can abort and leave the preview
disabled when buffer creation fails, and keep removeVertexColorPreviewMaterials
behavior for disabling as-is.
| // Restore material overrides (preview first, then wireframe). | ||
| // If preview was enabled while wireframe was on, the preview baseline can contain | ||
| // wireframe clone names; restoring preview after deleting those would reapply | ||
| // stale names. Always remove preview before tearing down wireframe. | ||
| if (m_vertexColorPreviewEnabled) { | ||
| removeVertexColorPreviewMaterials(); | ||
| m_vertexColorPreviewEnabled = false; | ||
| emit vertexColorPreviewChanged(); | ||
| } |
There was a problem hiding this comment.
Keep the preview toggle out of teardown state-reset.
Line 515 clears m_vertexColorPreviewEnabled on every exit, so the UI toggle flips off even though enterEditMode() already has a reapply path for an enabled preview at Lines 464-465. That makes the setting non-persistent across edit sessions and turns the enter-time reapply into dead code for normal usage.
Suggested fix
if (m_vertexColorPreviewEnabled) {
removeVertexColorPreviewMaterials();
- m_vertexColorPreviewEnabled = false;
- emit vertexColorPreviewChanged();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 509 - 517, The teardown code clears
m_vertexColorPreviewEnabled and emits vertexColorPreviewChanged() which forces
the UI toggle off and prevents enterEditMode()'s reapply path from working;
instead, call removeVertexColorPreviewMaterials() to remove materials but do NOT
reset m_vertexColorPreviewEnabled or emit vertexColorPreviewChanged() here so
the persistent preview flag remains true and enterEditMode() can reapply it.
Ensure only removeVertexColorPreviewMaterials() is invoked during teardown and
leave m_vertexColorPreviewEnabled and the signal handling to the normal
toggle/enterEditMode() flow.
| connect(falloffSlider, &QSlider::valueChanged, this, [this, emPaint, falloffLabel](int v) { | ||
| emPaint->setVertexPaintFalloff(v / 100.0); | ||
| falloffLabel->setText(tr("Falloff: %1").arg(emPaint->vertexPaintFalloff(), 0, 'f', 2)); | ||
| }); | ||
| connect(emPaint, &EditModeController::vertexPaintChanged, this, syncFalloff); |
There was a problem hiding this comment.
Add breadcrumb tracking for falloff adjustments.
This is a new user-facing brush control path, but changes to vertexPaintFalloff are not currently breadcrumbed. Log once per completed drag (sliderReleased) to avoid noisy telemetry while satisfying action tracking.
Proposed instrumentation
connect(falloffSlider, &QSlider::valueChanged, this, [this, emPaint, falloffLabel](int v) {
emPaint->setVertexPaintFalloff(v / 100.0);
falloffLabel->setText(tr("Falloff: %1").arg(emPaint->vertexPaintFalloff(), 0, 'f', 2));
});
+ connect(falloffSlider, &QSlider::sliderReleased, this, [falloffSlider]() {
+ SentryReporter::addBreadcrumb(
+ "ui.action",
+ QStringLiteral("Vertex paint falloff set to %1")
+ .arg(falloffSlider->value() / 100.0, 0, 'f', 2));
+ });As per coding guidelines **/*.cpp: “All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message).”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| connect(falloffSlider, &QSlider::valueChanged, this, [this, emPaint, falloffLabel](int v) { | |
| emPaint->setVertexPaintFalloff(v / 100.0); | |
| falloffLabel->setText(tr("Falloff: %1").arg(emPaint->vertexPaintFalloff(), 0, 'f', 2)); | |
| }); | |
| connect(emPaint, &EditModeController::vertexPaintChanged, this, syncFalloff); | |
| connect(falloffSlider, &QSlider::valueChanged, this, [this, emPaint, falloffLabel](int v) { | |
| emPaint->setVertexPaintFalloff(v / 100.0); | |
| falloffLabel->setText(tr("Falloff: %1").arg(emPaint->vertexPaintFalloff(), 0, 'f', 2)); | |
| }); | |
| connect(falloffSlider, &QSlider::sliderReleased, this, [falloffSlider]() { | |
| SentryReporter::addBreadcrumb( | |
| "ui.action", | |
| QStringLiteral("Vertex paint falloff set to %1") | |
| .arg(falloffSlider->value() / 100.0, 0, 'f', 2)); | |
| }); | |
| connect(emPaint, &EditModeController::vertexPaintChanged, this, syncFalloff); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 996 - 1000, Add breadcrumb logging for
falloff changes by attaching a handler to falloffSlider->sliderReleased that
reads the current value (use falloffSlider->value() or
emPaint->vertexPaintFalloff()), formats it (e.g., percent or float with two
decimals) and calls SentryReporter::addBreadcrumb("brush.falloff",
formattedMessage). Leave the existing valueChanged connection
(connect(falloffSlider, &QSlider::valueChanged, ...)) and the vertexPaintChanged
connection intact; the new sliderReleased handler should only log once per
completed drag so telemetry isn't noisy. Ensure the handler references
falloffSlider and emPaint (or emPaint->vertexPaintFalloff()) and creates a clear
message like "Falloff set to X" before calling SentryReporter::addBreadcrumb.
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
|
Added regression coverage for the 'paint invisible until Convert to Quads' bug: asserts we don't swap the VES_DIFFUSE vertex buffer binding after enabling vertex colors + committing a color (guards against VAO/buffer caching regressions). |
|
Added regression coverage for the 'paint invisible until Convert to Quads' bug: the new gtest asserts we don't swap the vertex buffer binding after enabling vertex colors + committing a color (guards against VAO/buffer caching regressions). |
|
Added regression coverage for the 'paint invisible until Convert to Quads' bug:
|
…x-falloff-preview
|
1 participant
Summary
Closes #315.
Test plan
UnitTestsand runEditModeControllerGeometry.ApplyVertexColorBrushAffectsVerticesWithinRadius.Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests