Add in-app isometric sprite export UI (#724) - #742
Conversation
Surfaces the existing isometric renderer in Animation Mode with a dialog, hides editor grid chrome during capture, and adds controller/MCP regression tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
More reviews will be available in 40 minutes and 26 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughIntroduces an end-to-end isometric sprite export feature: a new ChangesIsometric Sprite Export
Sequence Diagram(s)sequenceDiagram
participant User
participant PropertiesPanel as PropertiesPanel (QML)
participant Dialog as IsometricSpritesDialog (QML)
participant Ctrl as IsometricSpritesController (C++)
participant MainWindow as MainWindow (C++)
participant Renderer as ModelIsometricRenderer (C++)
User->>PropertiesPanel: clicks "Export Isometric Sprites…"
PropertiesPanel->>Dialog: openIsometricSpritesDialog() / open()
User->>Dialog: clicks "Browse…"
Dialog->>Ctrl: requestOutputPathPick(startPath)
Ctrl->>MainWindow: outputPathPickRequested(startPath)
MainWindow->>User: QFileDialog (Save PNG)
User-->>MainWindow: selects file
MainWindow->>Ctrl: outputPathPicked(path)
Ctrl->>Dialog: outputPath updated via Connections
User->>Dialog: clicks "Export PNG"
Dialog->>Ctrl: exportSelected(outputPath, animName, directions, frames, ...)
Ctrl->>Ctrl: setExporting(true)
Ctrl->>Renderer: renderToGrid(entity, IsometricOptions)
Renderer-->>Ctrl: image grid (EditorCaptureGuard hides/restores UI)
Ctrl->>Ctrl: compose sheet + save PNG
Ctrl->>Dialog: exportFinished(ok, outputPath, error)
Ctrl->>Ctrl: setExporting(false)
Dialog->>User: status message updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 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: 8af52c5d0a
ℹ️ 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".
|
|
||
| IsometricSpritesController *IsometricSpritesController::qmlInstance(QQmlEngine *, QJSEngine *) | ||
| { | ||
| return instance(); |
There was a problem hiding this comment.
When the properties panel first references IsometricSpritesController, qmlRegisterSingletonType lets the QQmlEngine own the returned QObject unless qmlInstance() marks it with QQmlEngine::CppOwnership. MainWindow destroys the QQuickWidget/engine via delete ui before calling IsometricSpritesController::kill(), so any GUI session that loaded the panel can have the engine delete this singleton and then hit a second delete through the static kill() path. Return the instance after setting C++ ownership, as the other manually-killed controllers do.
Useful? React with 👍 / 👎.
| sel->clear(); | ||
| for (Ogre::Entity *entity : entityList) { | ||
| if (entity) | ||
| sel->append(entity); |
There was a problem hiding this comment.
When the export starts from a selected scene node/group or sub-entity, entityList is the flattened result of SelectionSet::getResolvedEntities(), but this restore path clears the real selection and appends only entities. After an export attempt the user's node/submesh selection is replaced by child entity selections, changing the inspector/gizmo context. Save and restore getNodesSelectionList(), getEntitiesSelectionList(), and getSubEntitiesSelectionList() instead of restoring only entityList.
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: 9
🧹 Nitpick comments (1)
qml/PropertiesPanel.qml (1)
1145-1194: ⚡ Quick winUse
ThemeManagercolors for the new Isometric Sprites controls.This new UI block is styled with
PropertiesPanelController.*colors; the repository guideline requires new QML UI components to take theme colors fromThemeManagerfor cross-platform consistency.Proposed adjustment
- color: PropertiesPanelController.textColor + color: ThemeManager.textColor ... - ? PropertiesPanelController.highlightColor - : PropertiesPanelController.headerColor + ? ThemeManager.highlightColor + : ThemeManager.headerColor ... - ? PropertiesPanelController.highlightColor - : PropertiesPanelController.borderColor + ? ThemeManager.highlightColor + : ThemeManager.borderColor ... - color: PropertiesPanelController.textColor + color: ThemeManager.textColorAs per coding guidelines: “QML components should use theme colors from ThemeManager singleton for consistent styling across all platforms.”
🤖 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 1145 - 1194, The isometric sprites controls block (isoBtn Rectangle and associated Text elements) is currently using PropertiesPanelController color properties instead of ThemeManager colors as required by repository guidelines. Replace all instances of PropertiesPanelController.textColor, PropertiesPanelController.highlightColor, PropertiesPanelController.headerColor, and PropertiesPanelController.borderColor throughout the isoBtn Rectangle definition and its child elements with their equivalent ThemeManager color properties to ensure cross-platform consistency with the rest of the application.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 `@qml/IsometricSpritesDialog.qml`:
- Around line 18-19: Replace all color property assignments in
IsometricSpritesDialog that reference PropertiesPanelController.panelColor (line
18) and any hardcoded color values (line 320) with the corresponding
ThemeManager singleton color properties. Update the color assignments at the
specified locations (lines 18, 108-111, 145-148, 193-195, and 320) to use
ThemeManager instead, ensuring all styling throughout the dialog component
consistently uses the theme system rather than panel controller or hardcoded
values.
- Around line 99-128: The InspectorButton component only supports mouse
interaction through MouseArea and does not handle keyboard input, preventing
keyboard-only users from activating the button needed for the export flow. Add
keyboard accessibility to the InspectorButton by enabling focus through the
focusPolicy property, adding a Keys.onPressed handler to detect Space or Enter
key presses, and emit the clicked() signal when these keys are pressed.
Additionally, add visual focus feedback by modifying the color property to
distinguish the focused state from the hover state, and update the border or
other visual properties to indicate when the button has keyboard focus.
In `@src/IsometricSpritesController.cpp`:
- Around line 121-123: The condition for the seed path fallback in
IsometricSpritesController.cpp is inverted compared to MainWindow. Currently,
the code uses `!QFileInfo(seed).isDir()` which discards file paths and preserves
directories. To align with MainWindow's behavior and preserve file paths, remove
the negation operator (!) from the condition so it checks
`QFileInfo(seed).isDir()` instead. This ensures that the fallback to Pictures
location only occurs when the seed is actually a directory, not when it's a
file.
In `@src/mainwindow.cpp`:
- Around line 681-698: The export path-pick flow in the
IsometricSpritesController signal handler is missing Sentry breadcrumbs required
for tracking user-facing actions. Add a SentryReporter::addBreadcrumb call with
ui.action category before the QFileDialog::getSaveFileName call that opens the
file picker dialog, and add another SentryReporter::addBreadcrumb call with
file.export category after QFileDialog::getSaveFileName returns with the chosen
path to track the outcome of the export operation (whether a path was selected
or cancelled). This ensures the export flow is properly instrumented for
debugging and monitoring.
- Around line 689-690: The seed path normalization logic incorrectly assumes
that any non-`.png` input is a directory. Instead of blindly treating seed as a
directory when it doesn't end with `.png`, you need to first verify whether seed
is actually a directory or a file path. Replace the condition in the block
starting with `if (!seed.endsWith(QStringLiteral(".png"), Qt::CaseInsensitive))`
to check if seed is a directory using `QDir(seed).exists() &&
QDir(seed).isDir()`. If seed is a directory, append `isometric_sprites.png`
using `QDir(seed).filePath()`. If seed is a file path without the `.png`
extension, extract its directory component using `QFileInfo(seed).dir()` and
then append the default filename to that directory instead of treating the file
path as a directory.
In `@src/MCPServerGenerateIsometricSprites_coverage_test.cpp`:
- Line 141: Replace the hardcoded path "/nonexistent/missing_mesh.fbx" in the
args["file"] assignment with a platform-independent temporary file path. Use the
tmp object's filePath() method (e.g., tmp.filePath("missing_mesh.fbx")) to
construct a guaranteed path within the temporary directory, then verify that
this file does not exist before invoking the tool to ensure the test reliably
reproduces the missing-file scenario across Windows, Linux, and macOS.
- Around line 92-94: In the SetUp() method of the test fixture, add a missing
precondition assertion after the existing ASSERT_TRUE(tryInitOgre()) call.
Insert ASSERT_TRUE(canLoadMeshFiles()) with an appropriate error message before
or after the createStandardOgreMaterials() call to ensure mesh and resource file
loading capability is verified early. This will cause the test to fail loudly
instead of skipping when mesh files cannot be loaded in the environment.
In `@src/ModelIsometricRenderer_test.cpp`:
- Around line 120-125: The test performs dimension checks using EXPECT_EQ which
do not halt execution if they fail, but then immediately accesses sheet.pixel(0,
0) without verifying the dimensions are valid. Replace the EXPECT_EQ assertions
for width and height with ASSERT_EQ to add a guard that will stop test execution
before attempting to read pixels. This prevents brittle CI failures when the
sheet dimensions are not as expected. Apply this change before the
sheet.pixel(0, 0) call and also at the location marked in line 135.
In `@src/ModelIsometricRenderer.cpp`:
- Around line 84-101: The code calls getAttachedObject(0) without verifying that
attached objects exist, which will throw std::out_of_range in Ogre 14.x if a
scene node has zero attached objects. In the
ModelIsometricRenderer::ModelIsometricRenderer method, replace the direct
getAttachedObject(0) calls at the gridNode visibility check and the node
visibility check with a bounds-safe approach by first checking the attachment
count before accessing the object. Additionally, move the guard object
instantiation from line 670 into the try block that begins at line 688 to ensure
any exceptions during guard creation are properly caught by the error handling
mechanism.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 1145-1194: The isometric sprites controls block (isoBtn Rectangle
and associated Text elements) is currently using PropertiesPanelController color
properties instead of ThemeManager colors as required by repository guidelines.
Replace all instances of PropertiesPanelController.textColor,
PropertiesPanelController.highlightColor, PropertiesPanelController.headerColor,
and PropertiesPanelController.borderColor throughout the isoBtn Rectangle
definition and its child elements with their equivalent ThemeManager color
properties to ensure cross-platform consistency with the rest of the
application.
🪄 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: 7280cdd8-4cf6-456e-9dd0-c57a28896b14
📒 Files selected for processing (12)
CLAUDE.mdqml/IsometricSpritesDialog.qmlqml/PropertiesPanel.qmlsrc/CMakeLists.txtsrc/IsometricSpritesController.cppsrc/IsometricSpritesController.hsrc/IsometricSpritesController_test.cppsrc/MCPServerGenerateIsometricSprites_coverage_test.cppsrc/ModelIsometricRenderer.cppsrc/ModelIsometricRenderer_test.cppsrc/mainwindow.cppsrc/qml_resources.qrc
| color: PropertiesPanelController.panelColor | ||
|
|
There was a problem hiding this comment.
Use ThemeManager colors instead of PropertiesPanelController/hardcoded colors.
Line 18 and the dialog styling throughout use PropertiesPanelController.*, and Line 320 uses hardcoded status colors. This violates the repo’s theming contract for QML and can drift across platforms.
As per coding guidelines, "QML components should use theme colors from ThemeManager singleton for consistent styling across all platforms."
Also applies to: 108-111, 145-148, 193-195, 320-320
🤖 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/IsometricSpritesDialog.qml` around lines 18 - 19, Replace all color
property assignments in IsometricSpritesDialog that reference
PropertiesPanelController.panelColor (line 18) and any hardcoded color values
(line 320) with the corresponding ThemeManager singleton color properties.
Update the color assignments at the specified locations (lines 18, 108-111,
145-148, 193-195, and 320) to use ThemeManager instead, ensuring all styling
throughout the dialog component consistently uses the theme system rather than
panel controller or hardcoded values.
Source: Coding guidelines
| connect(IsometricSpritesController::instance(), &IsometricSpritesController::outputPathPickRequested, | ||
| this, [this](const QString &startPath) { | ||
| QTimer::singleShot(0, this, [this, startPath]() { | ||
| QString seed = startPath; | ||
| if (seed.isEmpty() || QFileInfo(seed).isDir()) | ||
| seed = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation); | ||
| if (seed.isEmpty()) | ||
| seed = QDir::homePath(); | ||
| if (!seed.endsWith(QStringLiteral(".png"), Qt::CaseInsensitive)) | ||
| seed = QDir(seed).filePath(QStringLiteral("isometric_sprites.png")); | ||
|
|
||
| const QString chosen = QFileDialog::getSaveFileName( | ||
| this, tr("Save isometric sprite sheet"), seed, | ||
| tr("PNG image (*.png)"), nullptr, | ||
| QFileDialog::DontUseNativeDialog | QFileDialog::DontUseCustomDirectoryIcons); | ||
| emit IsometricSpritesController::instance()->outputPathPicked(chosen); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add Sentry breadcrumbs for the export path-pick flow.
Line 681 introduces a user-facing export path action and Lines 692-696 execute file-export path selection, but this flow currently has no breadcrumb. Please add SentryReporter::addBreadcrumb(...) with ui.action for opening the picker and file.export for the chosen/cancel outcome.
As per coding guidelines, “All user-facing actions and significant operations must be tracked with Sentry breadcrumbs…” and file I/O should use file.export.
Suggested patch
connect(IsometricSpritesController::instance(), &IsometricSpritesController::outputPathPickRequested,
this, [this](const QString &startPath) {
+ SentryReporter::addBreadcrumb("ui.action", "Isometric Sprites: output path picker requested");
QTimer::singleShot(0, this, [this, startPath]() {
QString seed = startPath;
if (seed.isEmpty() || QFileInfo(seed).isDir())
seed = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation);
@@
const QString chosen = QFileDialog::getSaveFileName(
this, tr("Save isometric sprite sheet"), seed,
tr("PNG image (*.png)"), nullptr,
QFileDialog::DontUseNativeDialog | QFileDialog::DontUseCustomDirectoryIcons);
+ SentryReporter::addBreadcrumb(
+ "file.export",
+ chosen.isEmpty()
+ ? QStringLiteral("Isometric Sprites: save dialog canceled")
+ : QStringLiteral("Isometric Sprites: output path selected"));
emit IsometricSpritesController::instance()->outputPathPicked(chosen);
});
});📝 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(IsometricSpritesController::instance(), &IsometricSpritesController::outputPathPickRequested, | |
| this, [this](const QString &startPath) { | |
| QTimer::singleShot(0, this, [this, startPath]() { | |
| QString seed = startPath; | |
| if (seed.isEmpty() || QFileInfo(seed).isDir()) | |
| seed = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation); | |
| if (seed.isEmpty()) | |
| seed = QDir::homePath(); | |
| if (!seed.endsWith(QStringLiteral(".png"), Qt::CaseInsensitive)) | |
| seed = QDir(seed).filePath(QStringLiteral("isometric_sprites.png")); | |
| const QString chosen = QFileDialog::getSaveFileName( | |
| this, tr("Save isometric sprite sheet"), seed, | |
| tr("PNG image (*.png)"), nullptr, | |
| QFileDialog::DontUseNativeDialog | QFileDialog::DontUseCustomDirectoryIcons); | |
| emit IsometricSpritesController::instance()->outputPathPicked(chosen); | |
| }); | |
| }); | |
| connect(IsometricSpritesController::instance(), &IsometricSpritesController::outputPathPickRequested, | |
| this, [this](const QString &startPath) { | |
| SentryReporter::addBreadcrumb("ui.action", "Isometric Sprites: output path picker requested"); | |
| QTimer::singleShot(0, this, [this, startPath]() { | |
| QString seed = startPath; | |
| if (seed.isEmpty() || QFileInfo(seed).isDir()) | |
| seed = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation); | |
| if (seed.isEmpty()) | |
| seed = QDir::homePath(); | |
| if (!seed.endsWith(QStringLiteral(".png"), Qt::CaseInsensitive)) | |
| seed = QDir(seed).filePath(QStringLiteral("isometric_sprites.png")); | |
| const QString chosen = QFileDialog::getSaveFileName( | |
| this, tr("Save isometric sprite sheet"), seed, | |
| tr("PNG image (*.png)"), nullptr, | |
| QFileDialog::DontUseNativeDialog | QFileDialog::DontUseCustomDirectoryIcons); | |
| SentryReporter::addBreadcrumb( | |
| "file.export", | |
| chosen.isEmpty() | |
| ? QStringLiteral("Isometric Sprites: save dialog canceled") | |
| : QStringLiteral("Isometric Sprites: output path selected")); | |
| emit IsometricSpritesController::instance()->outputPathPicked(chosen); | |
| }); | |
| }); |
🤖 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 681 - 698, The export path-pick flow in the
IsometricSpritesController signal handler is missing Sentry breadcrumbs required
for tracking user-facing actions. Add a SentryReporter::addBreadcrumb call with
ui.action category before the QFileDialog::getSaveFileName call that opens the
file picker dialog, and add another SentryReporter::addBreadcrumb call with
file.export category after QFileDialog::getSaveFileName returns with the chosen
path to track the outcome of the export operation (whether a path was selected
or cancelled). This ensures the export flow is properly instrumented for
debugging and monitoring.
Source: Coding guidelines
mainwindow.cpp references the controller in qtmesh_test_common; link the implementation so UnitTests builds on Linux CI. Co-authored-by: Cursor <cursoragent@cursor.com>
CppOwnership for the QML singleton, full selection restore after export, shared save-seed normalization, safer scene-node visibility checks, keyboard Browse/Export buttons, and expanded controller/MCP test coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Sync pinned doc refs via scripts/sync-doc-versions-from-cmake.sh. Covers the features merged since 3.7.0: - AI PBR map synthesis from albedo (ONNX) + multi-slot PBR texture UI (#404, #738) - QtMesh Cloud scan-report upload (#748) - In-app isometric sprite export UI (#724, #742) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
IsometricSpritesController+IsometricSpritesDialog.qml) for exporting the live selection to a directions×frames PNG atlas.generate_isometric_spritestool (file-in / PNG-out path, validation branches, tools/list).MCP
Already shipped in #741 / 3.7.0 as
generate_isometric_sprites— same renderer asqtmesh isometric. Supportsfile,output,animation,frames,directions,resolution,elevation,start_azimuth,camera_distance,camera_padding/padding.Test plan
UnitTests --gtest_filter="IsometricSpritesController*"UnitTests --gtest_filter="MCPServerGenerateIsometricSprites*"UnitTests --gtest_filter="ModelIsometricRendererTest.StaticGrid*"generate_isometric_spriteswith a mesh file + output pathMade with Cursor
Summary by CodeRabbit
Release Notes
New Features
Documentation
--elevationand--camera-heightoptions for isometric sprite export.