perf+test: editor window image provider + MaterialEditorQML coverage - #534
Conversation
Perf — Texture Editor window
The detached window used to bind its Image element to
`TexturePaintController.previewDataUri`, which produces a PNG-
encoded + base64-wrapped data URI of the paint buffer. On 2048²
textures that's ~80 ms per refresh on the main thread (encode +
QString churn), and the editor window updates every 60 ms of
the existing debounce — net effect: every stroke felt laggy.
New architecture:
- `PaintBufferImageProvider` (QQuickImageProvider, Image mode)
registered on the editor's QQmlApplicationEngine under the
`paintbuffer` scheme. The provider's requestImage just calls
`TexturePaintController::snapshotBufferImage()` which is a
single `QImage::copy()` of the RGBA buffer — no PNG encode,
no base64. ~3 ms on a 2048² texture.
- New controller property `fullResPreviewUrl` returns
`image://paintbuffer/current?v=N` where N is a monotonic
counter bumped on every refresh. The `?v=N` invalidates QML's
per-URL Image cache so each refresh re-pulls. The provider
itself ignores the parameter.
- New signal `fullResPreviewChanged` so QML can bind to the URL
channel without tickling the heavier `previewChanged` path.
- The inspector thumbnail keeps the cheap `previewDataUri`
(256² downscaled PNG, ~3 ms). Two channels, two consumers.
The Image element in `TextureEditorWindow.qml` sets
`sourceSize: Qt.size(0, 0)` so Qt loads the buffer at its native
resolution and crisp-renders when the user enlarges the window.
Tests — MaterialEditorQML
Added 5 new test cases targeting paths Sonar marked uncovered:
- getMaterialList filtering: QMEPaint_, QMEPaintMaskOverlay_,
TexturePaint/ prefixes are hidden from the user-facing
dropdown. Plus the no-Ogre early-return path.
- Setters' no-Ogre paths verify that the QObject-side property
still updates (so QML bindings refresh) even when no material
is loaded — `setLightingEnabled` / `setAmbientColor` / six more.
- Name-helper lists (getPolygonModeNames etc.) return non-empty
arrays with no empty entries — the QML side relies on this.
- `materialPreview("Unknown")` doesn't crash; returns empty or
a data: URI.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces full-resolution live texture preview delivery via a Qt image provider. The new ChangesFull-resolution texture preview via Qt image provider
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 |
![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.cpp`:
- Around line 1993-1997: Session teardown clears the full-res buffer but doesn't
notify QML, leaving fullResPreviewUrl consumers with a stale image; update the
teardown/cleanup paths in TexturePaintController (where the session buffer is
cleared or the session ends) to mirror the refresh behavior by incrementing
m_fullResVersion and calling emit fullResPreviewChanged() after the buffer is
cleared so bindings to fullResPreviewUrl are invalidated.
🪄 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: df839683-c8f7-4ae9-b7a4-9811617884c3
📒 Files selected for processing (8)
qml/TextureEditorWindow.qmlsrc/CMakeLists.txtsrc/MaterialEditorQML_test.cppsrc/PaintBufferImageProvider.cppsrc/PaintBufferImageProvider.hsrc/TexturePaintController.cppsrc/TexturePaintController.htests/CMakeLists.txt
| // The full-res preview channel doesn't need PNG encoding — | ||
| // QQuickImageProvider serves the live buffer directly on demand. | ||
| // Just bump the version so QML re-fetches. | ||
| ++m_fullResVersion; | ||
| emit fullResPreviewChanged(); |
There was a problem hiding this comment.
Emit fullResPreviewChanged on teardown paths too.
fullResPreviewUrl has a dedicated NOTIFY signal, but session teardown clears the buffer without emitting fullResPreviewChanged(). Consumers bound only to fullResPreviewUrl can keep a stale image until another refresh path runs.
Suggested fix
diff --git a/src/TexturePaintController.cpp b/src/TexturePaintController.cpp
@@
m_previewUri.clear();
emit previewChanged();
+ ++m_fullResVersion;
+ emit fullResPreviewChanged();
emit sessionChanged();
}📝 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.
| // The full-res preview channel doesn't need PNG encoding — | |
| // QQuickImageProvider serves the live buffer directly on demand. | |
| // Just bump the version so QML re-fetches. | |
| ++m_fullResVersion; | |
| emit fullResPreviewChanged(); | |
| m_previewUri.clear(); | |
| emit previewChanged(); | |
| +m_fullResVersion; | |
| emit fullResPreviewChanged(); | |
| emit sessionChanged(); |
🤖 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 1993 - 1997, Session teardown
clears the full-res buffer but doesn't notify QML, leaving fullResPreviewUrl
consumers with a stale image; update the teardown/cleanup paths in
TexturePaintController (where the session buffer is cleared or the session ends)
to mirror the refresh behavior by incrementing m_fullResVersion and calling emit
fullResPreviewChanged() after the buffer is cleared so bindings to
fullResPreviewUrl are invalidated.
![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: 9bd66f3f60
ℹ️ 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".
| QImage view(const_cast<uchar*>(m_buffer.data().data()), | ||
| m_buffer.width(), m_buffer.height(), | ||
| m_buffer.width() * 4, QImage::Format_RGBA8888); | ||
| return view.copy(); |
There was a problem hiding this comment.
Protect snapshotBufferImage() with the same synchronization used by paint writes: it builds a QImage view over m_buffer and copies from it without any lock, while stroke/update paths mutate m_buffer concurrently. Because QQuickImageProvider::requestImage() may run off the UI thread, this can race under asynchronous image loading and produce torn/corrupted frames or intermittent crashes.
Useful? React with 👍 / 👎.
|
1 participant
Summary
Perf — the detached Texture Editor window now uses a
QQuickImageProviderinstead of a base64-PNG data URI, removing the per-stroke encode that made painting feel laggy on 2048² textures.PaintBufferImageProviderregistered underimage://paintbuffer/on the editor's engine.requestImageis justQImage::copy()of the live buffer — no PNG encode, no base64 wrapping.TexturePaintController.fullResPreviewUrlreturnsimage://paintbuffer/current?v=N(monotonic version bump per refresh) so QML invalidates the Image cache without re-encoding.Tests — 5 new
MaterialEditorQMLcases:getMaterialListfiltersQMEPaint_*/QMEPaintMaskOverlay_*/TexturePaint/*from the user dropdown.getMaterialListearly-return path when Ogre isn't available.materialPreview("Unknown")doesn't crash.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests