fix(paint): Inspector thumbnail uses image provider — no more blink#541
Merged
Conversation
The Inspector's 2D paint thumbnail was bound to `previewDataUri`, a PNG-encoded + base64-wrapped data URI regenerated on every dirty flush. Each refresh produced a fresh URL string, and Qt's Image element treats a changed `source` as a brand-new load — visibly blinking the thumbnail during every paint stroke. The detached Texture Editor window already routes through `fullResPreviewUrl` (`image://paintbuffer/current?v=N`), which is served by `PaintBufferImageProvider` — a `QImage` view of the live buffer, no PNG encode, no base64. The Inspector consumed the same controller but used the slow path. This switches the Inspector to the same provider-backed channel. Three changes: - Register `paintbuffer` image provider on the Inspector's QQmlEngine (`MainWindow`). Without this, the `image://` URL resolves to a missing-provider error and the thumbnail goes blank. - Bind the Inspector thumbnail to `TexturePaintController.fullResPreviewUrl` and update via `onFullResPreviewChanged`. - Stop populating `previewDataUri` inside `refreshPreviewUri()` — no consumer remains. The property + `previewChanged` signal are retained for binary compatibility but the per-flush PNG encode + base64 churn is gone. Roughly 80 ms/refresh on 2048² is saved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR migrates texture-paint preview rendering from embedded PNG/base64 data URIs to a live image provider. The QML preview now uses a provider-served full-resolution URL and listens to fullResPreviewChanged; the controller stops creating encoded thumbnails and instead bumps a full-res version to invalidate the provider. Tests are rewritten/expanded. ChangesPaint Buffer Preview Provider Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
The controller had three thin tests (singleton brush-tool defaults +
one in-memory UV hit) against a 2796-line implementation. This brings
broad coverage of the public API and the QML-facing properties — the
parts that get touched by every Inspector / Material-Mode interaction.
Two suites:
* `TexturePaintControllerStandalone` (no scene needed)
- default state on instance(), brush-tool/target/uv-overlay setters
(sticky, no-emit on same value, signal emit on transition)
- smart-select tolerance clamping to [0..1]
- brush param mirror: setBrushRadius/Strength/Falloff/Color update
EditModeController; the read-side mirrors (texturePaintRadius
etc.) flow back through
- no-session edge cases — fullResPreviewUrl empty, snapshotBufferImage
null, mask actions / smartSelect / bake / loadPaintBuffer all
no-op cleanly, ensurePaintableTexture without selection returns
false, bakeToOriginalFile empty, beginStrokeUV refused
- paint-target change side-effects (sessionChanged + smartSelectChanged
also emitted, not only paintTargetChanged)
- enable-without-selection is harmless (no crash, no session)
* `TexturePaintControllerSceneTest` (fixture: scene + 3-vertex UV
triangle + material with a `diffuse_map` TUS)
- enable → session created → disable tears it down
- ensurePaintableTexture builds buffer at requested resolution and
reuses on second call
- closeSession resets every observable
- refreshSlots exposes slot model with the expected keys
- fullResPreviewUrl shape (`image://paintbuffer/...?v=N`) when
session is active; URL version bumps after a buffer-mutating op
- snapshotBufferImage matches buffer dimensions
- save / load PNG roundtrip; loadPaintBuffer accepts a known PNG
and writes the seeded pixel into the buffer
- findMeshPointForUV hits each triangle corner, misses outside
- texturePaintRadiusUV maps through mesh half-extent
- smart-select adds pixels; selectAll covers W×H; invert flips
pixel count; clearSelectionMask emits smartSelectChanged
- fillMaskWithFG / fillMaskWithBG / deleteMaskPixels all write
every selected pixel and zero the buffer respectively
- applyPixelSnapshot round-trips (undo path); mismatched size
is rejected without crash
- paint target tex→vertex tears down the session (the "switch
crashes app" regression guard)
- setHoveredUV / clearHoveredUV emit hoveredUVChanged with the
expected payload
- beginStrokeUV/updateStrokeUV/endStrokeUV completes on a session
- smartSelect handles unrecognised combine mode (falls to replace),
exercises add (mode=1) and subtract (mode=2)
- bakeVertexColorsToTexture is callable on an entity without
crashing (return value depends on whether mesh has vertex
colors — we just assert it's ≥ -1)
- opening + closing the editor window flips the flag
Tests will skip cleanly on macOS (test_main FATAL on tryInitOgre is
unchanged); CI runs on Linux under Xvfb where the scene fixture works.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closed
8 tasks
2 tasks
…ler-coverage test(paint): expand TexturePaintController coverage 3 → 65 tests
The 37 scene-fixture tests guarded on `canLoadMeshFiles()`, which returns false in some Linux-CI permutations even when hardware buffer creation works for the simple in-memory meshes the tests use. The pre-existing `FindMeshPointForUVHitsCorrectTriangle` test in the same file uses `tryInitOgre()` and runs successfully in CI; align the new fixture with that same gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three issues surfaced once tests actually executed (the prior canLoadMeshFiles gate was masking them): 1. `BeginAndEndStrokeUVCompletesWithoutCrash` — beginStrokeUV refuses unless m_paintEnabled is true; ensurePaintableTexture alone isn't enough. Enable paint mode before the stroke. 2. `SavePaintBufferProducesReadablePNG` — ensurePaintableTexture floors resolution at 16 when there's no existing texture (std::max(16, resolution)), so `(8)` produced a 16×16 buffer. Request 32 and assert against 32. 3+4. `FullResVersionAdvancesPerRefresh` and `RefreshPreviewUriEmitsFullResSignal` — both touch refreshPreviewUri, which is double-debounced: flushDirtyToOgre is QTimer::singleShot(16ms), then the preview refresh inside is QTimer::singleShot(60ms). The tests didn't pump events, so neither timer fired. Add `pumpEventsFor(150)` helper that spins QCoreApplication::processEvents until both timers can complete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_test.cpp`:
- Around line 683-693: The test incorrectly modifies a local copy instead of the
controller's buffer and asserts the wrong condition; change the setup to write
the sentinel value directly into the controller's buffer (e.g., assign to
ctrl->buffer().data()[0] or use ctrl->buffer()[0]) before calling
TexturePaintController::applyPixelSnapshot, and update the final assertion to
EXPECT_EQ(ctrl->buffer().data()[0], 99) so the test verifies the buffer remains
99 when a mismatched-size snapshot is ignored; references:
TexturePaintController::instance(), ensurePaintableTexture, buffer(),
applyPixelSnapshot.
🪄 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: 840970d1-1fdd-4c9c-b356-ed55ead4ea68
📒 Files selected for processing (1)
src/TexturePaintController_test.cpp
Comment on lines
+683
to
+693
| TEST_F(TexturePaintControllerSceneTest, ApplyPixelSnapshotMismatchedSizeIgnored) { | ||
| ASSERT_TRUE(m_fix.setup(QStringLiteral("ApplyBadSize"))); | ||
| auto* ctrl = TexturePaintController::instance(); | ||
| ASSERT_TRUE(ctrl->ensurePaintableTexture(8)); | ||
| auto good = ctrl->buffer().data(); | ||
| good[0] = 99; | ||
| std::vector<uint8_t> bad(4, 0); // intentionally wrong size | ||
| ctrl->applyPixelSnapshot(bad); | ||
| // Buffer should remain unchanged (and not crash). | ||
| EXPECT_NE(ctrl->buffer().data()[0], 99) << "buffer should not have been touched"; | ||
| } |
There was a problem hiding this comment.
Test logic bug: assertion always passes without verifying the intended behavior.
Line 688 modifies good (a copy of the buffer data), not the actual buffer. The assertion at line 692 checks that the buffer's first byte is not 99, but the buffer was never set to 99 — only the local copy was modified. This test will always pass regardless of whether applyPixelSnapshot correctly rejects mismatched sizes.
🐛 Proposed fix
TEST_F(TexturePaintControllerSceneTest, ApplyPixelSnapshotMismatchedSizeIgnored) {
ASSERT_TRUE(m_fix.setup(QStringLiteral("ApplyBadSize")));
auto* ctrl = TexturePaintController::instance();
ASSERT_TRUE(ctrl->ensurePaintableTexture(8));
- auto good = ctrl->buffer().data();
- good[0] = 99;
+ // Set buffer to a known value
+ ctrl->mutableBuffer().data()[0] = 99;
std::vector<uint8_t> bad(4, 0); // intentionally wrong size
ctrl->applyPixelSnapshot(bad);
// Buffer should remain unchanged (and not crash).
- EXPECT_NE(ctrl->buffer().data()[0], 99) << "buffer should not have been touched";
+ EXPECT_EQ(ctrl->buffer().data()[0], 99) << "buffer should not have been touched";
}🤖 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_test.cpp` around lines 683 - 693, The test
incorrectly modifies a local copy instead of the controller's buffer and asserts
the wrong condition; change the setup to write the sentinel value directly into
the controller's buffer (e.g., assign to ctrl->buffer().data()[0] or use
ctrl->buffer()[0]) before calling TexturePaintController::applyPixelSnapshot,
and update the final assertion to EXPECT_EQ(ctrl->buffer().data()[0], 99) so the
test verifies the buffer remains 99 when a mismatched-size snapshot is ignored;
references: TexturePaintController::instance(), ensurePaintableTexture,
buffer(), applyPixelSnapshot.
|
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
previewDataUri, a PNG-encoded + base64-wrapped data URI regenerated on every dirty flush. Each refresh produced a freshsourceURL, and Qt'sImageelement treats a changed source as a brand-new load — visibly blinking the thumbnail during every paint stroke.fullResPreviewUrl(image://paintbuffer/current?v=N), served byPaintBufferImageProvider— aQImageview of the live buffer, no PNG encode, no base64. This switches the Inspector to the same provider-backed channel.refreshPreviewUri()(no remaining consumer ofpreviewDataUri). Saves ~80 ms/refresh on 2048² textures.Changes
mainwindow.cpp— register thepaintbufferimage provider on the Inspector'sQQmlEngine(without this, theimage://URL has no resolver).qml/PropertiesPanel.qml— Inspector thumbnail Image binds toTexturePaintController.fullResPreviewUrland refreshes viaonFullResPreviewChanged.TexturePaintController::refreshPreviewUri()— stop encoding/populating the legacypreviewDataUri; emitpreviewChangedonly on clear-to-empty for back-compat.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit