Isometric 8-direction sprite export (#724) - #741
Merged
Conversation
Introduces ModelIsometricRenderer, qtmesh isometric CLI, and generate_isometric_sprites MCP tool for production isometric sprite atlases (rows=directions, cols=animation frames). Bump version to 3.7.0. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds a new isometric 8-direction animated sprite atlas export capability to QtMeshEditor 3.7.0. A new ChangesIsometric Sprite Atlas Export (v3.7.0)
Sequence DiagramsequenceDiagram
participant User as User/CI
participant CLIPipeline as CLIPipeline::cmdIsometric
participant MCPServer as MCPServer::toolGenerateIsometricSprites
participant Renderer as ModelIsometricRenderer
participant Ogre as Ogre RTT
rect rgba(100, 149, 237, 0.5)
Note over User, CLIPipeline: CLI path
User->>CLIPipeline: qtmesh isometric model.fbx -o out.png --animation Walk --frames 8
CLIPipeline->>CLIPipeline: parse flags, validate, select skinned entity
end
rect rgba(144, 238, 144, 0.5)
Note over User, MCPServer: MCP path
User->>MCPServer: generate_isometric_sprites {file, output, animation, frames, directions}
MCPServer->>MCPServer: validate args, build IsometricOptions, transient import
end
CLIPipeline->>Renderer: renderToGrid(entities, animatedEntity, animationName, frameCount, IsometricOptions)
MCPServer->>Renderer: renderToGrid(entities, animatedEntity, animationName, frameCount, IsometricOptions)
loop directions × frames
Renderer->>Ogre: placeCamera(azimuth, elevation)
Renderer->>Ogre: stepAnimation(frameTime)
Ogre-->>Renderer: readback → QImage
end
Renderer-->>CLIPipeline: outRowsByDirection
Renderer-->>MCPServer: outRowsByDirection
CLIPipeline->>Renderer: composeDirectionGrid(rowsByDirection)
MCPServer->>Renderer: composeDirectionGrid(rowsByDirection)
Renderer-->>CLIPipeline: QImage atlas
Renderer-->>MCPServer: QImage atlas
CLIPipeline->>CLIPipeline: save PNG + optional JSON
MCPServer->>MCPServer: save PNG + return JSON metadata
CLIPipeline->>Renderer: shutdown()
MCPServer->>Renderer: shutdown()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
![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: 05e9397226
ℹ️ 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".
Comment on lines
+457
to
+458
| const int directions = std::clamp(options.directionCount, 1, 64); | ||
| const int frames = std::clamp(frameCount, 1, 360); |
There was a problem hiding this comment.
When a caller uses the accepted maxima, for example --directions 64 --frames 360 at the default 512×512 cell size, these independent clamps let renderToGrid retain 23,040 full-size QImages and then compose a 184,320×32,768 sheet, requiring tens of GiB of memory and likely killing the process instead of returning an error. Please cap the total cells/pixels or stream directly into the final sheet before accepting the request.
Useful? React with 👍 / 👎.
Expose square per-cell resolution on qtmesh isometric and generate_isometric_sprites (range 16–8192), alongside existing --size/--width/--height overrides. Co-authored-by: Cursor <cursoragent@cursor.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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
38-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStale version reference: still shows 3.5.3 instead of 3.7.0.
This inline comment says "(currently 3.5.3)" but CMakeLists.txt now declares VERSION 3.7.0. Update this to "(currently 3.7.0)". Based on learnings, running
./scripts/sync-doc-versions-from-cmake.shshould keep this aligned—verify the script updates this location or add it to the script's substitution list.📝 Suggested fix
-- **Reproducible builds** — pin the action and the container to the same semver as this repository's `project(QtMeshEditor VERSION …)` in `CMakeLists.txt` (currently **3.5.3**). After bumping the version in CMake, run `./scripts/sync-doc-versions-from-cmake.sh` to refresh the pinned refs in `README.md` and the docs site fallback; CI enforces the match with `./scripts/sync-doc-versions-from-cmake.sh --check`. +- **Reproducible builds** — pin the action and the container to the same semver as this repository's `project(QtMeshEditor VERSION …)` in `CMakeLists.txt` (currently **3.7.0**). After bumping the version in CMake, run `./scripts/sync-doc-versions-from-cmake.sh` to refresh the pinned refs in `README.md` and the docs site fallback; CI enforces the match with `./scripts/sync-doc-versions-from-cmake.sh --check`.🤖 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 `@README.md` at line 38, Update the hardcoded version reference in the README.md's reproducible builds section from "3.5.3" to "3.7.0" to match the current VERSION declared in CMakeLists.txt. Additionally, verify that the sync-doc-versions-from-cmake.sh script properly handles this README.md location when substituting version numbers, and if it does not, add the appropriate pattern matching for the "(currently **X.Y.Z**)" text to ensure this stays synchronized automatically in the future.Source: Learnings
🧹 Nitpick comments (2)
src/ModelIsometricRenderer_test.cpp (1)
13-16: ⚡ Quick winAlign Ogre fixture preconditions with repo test convention.
Add
ASSERT_TRUE(canLoadMeshFiles())inSetUp()so CI fails early with explicit environment diagnostics for Ogre-dependent tests.Suggested adjustment
void SetUp() override { ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; + ASSERT_TRUE(canLoadMeshFiles()) << "Mesh resources unavailable in test environment"; ModelIsometricRenderer::shutdown(); }Based on learnings: Ogre-dependent QtMeshEditor tests are expected to assert both
tryInitOgre()andcanLoadMeshFiles()and fail loudly rather than silently skipping.🤖 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/ModelIsometricRenderer_test.cpp` around lines 13 - 16, The SetUp() method in the test fixture currently only asserts that Ogre initialization succeeds via tryInitOgre() but does not check if mesh files can be loaded. Add an ASSERT_TRUE(canLoadMeshFiles()) assertion in the SetUp() method after the existing tryInitOgre() check to align with the repository's test convention for Ogre-dependent tests and ensure CI fails early with explicit diagnostics if the environment cannot load mesh files.Source: Learnings
src/CLIPipeline_cmdisometric_coverage_test.cpp (1)
187-187: 💤 Low valueMinor inconsistency:
ASSERT_EQvsEXPECT_EQfor return code checks.This test uses
ASSERT_EQfor thecmdIsometricreturn code (line 187), while the other coverage tests in this file (lines 136, 151, 167) useEXPECT_EQ. Both are valid, but usingEXPECT_EQconsistently allows better diagnostics when a command fails (the test continues to report whether the output file exists).♻️ Proposed fix for consistency
- ASSERT_EQ(CLIPipeline::cmdIsometric(args.argc(), args.argv()), 0); + EXPECT_EQ(CLIPipeline::cmdIsometric(args.argc(), args.argv()), 0);🤖 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/CLIPipeline_cmdisometric_coverage_test.cpp` at line 187, Replace the ASSERT_EQ statement on line 187 with EXPECT_EQ to maintain consistency with the other coverage tests in this file (lines 136, 151, 167). Change the assertion that checks CLIPipeline::cmdIsometric(args.argc(), args.argv()) return value from ASSERT_EQ to EXPECT_EQ so the test continues execution and provides better diagnostic information if the command fails.
🤖 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/CLIPipeline_test.cpp`:
- Around line 1130-1147: The SetUpTestSuite() method in the CLIPipelineCmdTest
fixture currently uses an early return pattern when tryInitOgre() or
canLoadMeshFiles() fail, which causes tests like
CmdIsometric_StaticGridWritesPng to run with unclear failures instead of failing
explicitly. Replace the conditional early return pattern in SetUpTestSuite()
with ASSERT_TRUE() assertions on both tryInitOgre() and canLoadMeshFiles() calls
to ensure that if Ogre initialization or mesh file loading fails, the test suite
fails loudly and immediately rather than continuing with partially initialized
state.
In `@src/CLIPipeline.cpp`:
- Around line 3322-3328: In the argument parsing block for the --frames flag
where parseCliInt is called on argv values into frameCount, add validation after
the successful parse to ensure frameCount is a positive value greater than zero.
If frameCount is not positive, emit an error message using err() with
appropriate context (similar to the existing "Error: Invalid value for --frames"
message) and return exit code 2 to maintain consistency with other argument
validation errors. This validation should occur before setting
frameCountExplicit to true.
- Around line 3448-3459: The current entity selection logic in the loop
iterating through entityList picks the first entity that has a skeleton using
entity->hasSkeleton(), but it should instead verify that the selected entity
actually contains the requested animation clip specified in animationName.
Modify the condition in the skeletal entity detection to check not only if the
entity has a skeleton but also if it contains the specific animation clip
identified by animationName before assigning it to animatedEntity, ensuring that
animatedEntity is set only when both conditions are met.
In `@src/ModelIsometricRenderer.cpp`:
- Around line 183-204: The recenterEntitiesAtOrigin function permanently
translates scene nodes without restoring their original transforms. To fix this,
store the original position or translation offset of each Ogre::SceneNode in the
shifted unordered_set before applying the translation in the for loop, then
after the refreshEntityBounds(entities) call, restore each node's original
transform by translating them back by the positive center offset. This ensures
the entity transformations are temporary and don't persist after the function
completes.
- Around line 510-556: Replace all instances of the non-standard breadcrumb
category "cli.isometric" with a standardized category that follows the
repository's telemetry taxonomy (such as "file.export"). Update all four
SentryReporter::addBreadcrumb() calls in this rendering block to use the same
standardized category instead of "cli.isometric", while keeping the message
content unchanged for the "render start", "render ok", "render failed: Ogre
exception", and "render failed" breadcrumbs.
---
Outside diff comments:
In `@README.md`:
- Line 38: Update the hardcoded version reference in the README.md's
reproducible builds section from "3.5.3" to "3.7.0" to match the current VERSION
declared in CMakeLists.txt. Additionally, verify that the
sync-doc-versions-from-cmake.sh script properly handles this README.md location
when substituting version numbers, and if it does not, add the appropriate
pattern matching for the "(currently **X.Y.Z**)" text to ensure this stays
synchronized automatically in the future.
---
Nitpick comments:
In `@src/CLIPipeline_cmdisometric_coverage_test.cpp`:
- Line 187: Replace the ASSERT_EQ statement on line 187 with EXPECT_EQ to
maintain consistency with the other coverage tests in this file (lines 136, 151,
167). Change the assertion that checks CLIPipeline::cmdIsometric(args.argc(),
args.argv()) return value from ASSERT_EQ to EXPECT_EQ so the test continues
execution and provides better diagnostic information if the command fails.
In `@src/ModelIsometricRenderer_test.cpp`:
- Around line 13-16: The SetUp() method in the test fixture currently only
asserts that Ogre initialization succeeds via tryInitOgre() but does not check
if mesh files can be loaded. Add an ASSERT_TRUE(canLoadMeshFiles()) assertion in
the SetUp() method after the existing tryInitOgre() check to align with the
repository's test convention for Ogre-dependent tests and ensure CI fails early
with explicit diagnostics if the environment cannot load mesh files.
🪄 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: d1192465-de73-4046-988a-419c4a96a521
📒 Files selected for processing (21)
.github/actions/qtmesh/action.ymlCLAUDE.mdCMakeLists.txtREADME.mdaction.ymlsrc/AppLaunchHandler.cppsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_cmdisometric_coverage_test.cppsrc/CLIPipeline_test.cppsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/ModelIsometricRenderer.cppsrc/ModelIsometricRenderer.hsrc/ModelIsometricRenderer_test.cpptests/CMakeLists.txtwebsite/src/App.jsxwebsite/src/DocsApp.jsxwebsite/src/data/content.jswebsite/src/hooks/useQtmeshActionRef.js
Expose fixed orbit distance and auto-fit padding via CLI, MCP, and IsometricOptions so sprite framing can be tuned without re-scaling the mesh. Co-authored-by: Cursor <cursoragent@cursor.com>
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/CLIPipeline_cmdisometric_coverage_test.cpp`:
- Around line 181-190: The CameraPaddingJsonReport test at line 189 currently
only validates file existence with QFile::exists(out), which doesn't catch
corrupt or empty output files. Replace or augment this assertion with a QImage
decode validation to ensure the output is a valid, decodable image file, similar
to what is done in other coverage tests in this file, and optionally validate
the image dimensions as well.
🪄 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: 887a442d-8240-4baf-8b12-bceb94be8057
📒 Files selected for processing (9)
CLAUDE.mdsrc/CLIPipeline.cppsrc/CLIPipeline_cmdisometric_coverage_test.cppsrc/CLIPipeline_test.cppsrc/MCPServer.cppsrc/ModelIsometricRenderer.cppsrc/ModelIsometricRenderer.hwebsite/src/DocsApp.jsxwebsite/src/data/content.js
✅ Files skipped from review due to trivial changes (2)
- website/src/data/content.js
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (4)
- website/src/DocsApp.jsx
- src/ModelIsometricRenderer.cpp
- src/CLIPipeline.cpp
- src/MCPServer.cpp
Cap atlas size before rendering, restore recentered transforms after capture, pick animated entities by clip name, validate positive frame counts, use file.export breadcrumbs, and sync README version text. Co-authored-by: Cursor <cursoragent@cursor.com>
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 `@scripts/sync-doc-versions-from-cmake.sh`:
- Around line 87-89: The Perl replacement expression with the /e modifier in the
perl -i -pe command at lines 87-89 contains a syntactically invalid replacement
string. The expression (currently **) . $v . (**) fails because the literal
string parts containing parentheses and asterisks are not properly quoted as
Perl strings. Fix this by wrapping the literal string parts in quotes so the
concatenation operator properly joins the quoted string literals with the $v
variable, making the replacement valid Perl code that executes when the /e
modifier is used.
🪄 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: 4b4fbf99-3532-4468-bc00-2eaf676e92cf
📒 Files selected for processing (9)
CLAUDE.mdREADME.mdscripts/sync-doc-versions-from-cmake.shsrc/CLIPipeline.cppsrc/CLIPipeline_cmdisometric_coverage_test.cppsrc/CLIPipeline_test.cppsrc/MCPServer.cppsrc/ModelIsometricRenderer.cppsrc/ModelIsometricRenderer_test.cpp
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (7)
- README.md
- src/CLIPipeline_test.cpp
- src/CLIPipeline_cmdisometric_coverage_test.cpp
- src/ModelIsometricRenderer_test.cpp
- src/ModelIsometricRenderer.cpp
- src/MCPServer.cpp
- src/CLIPipeline.cpp
Extract isometric CLI parsing and grid capture helpers, share animation entity lookup, fix RecenterGuard rule compliance, and repair the doc-sync Perl version replacement. Co-authored-by: Cursor <cursoragent@cursor.com>
Guard render-target update when RTT is unavailable and propagate failure from captureIsometricGrid. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ModelIsometricRenderer.cpp (1)
574-580: Eliminate redundantgetAllAnimationStates()call at line 580.Ogre 14's
AnimationStateSet::getAnimationState()is a const method, so it can be called on the const pointer already fetched at line 574. Reusestatesdirectly instead of callinggetAllAnimationStates()again:animState = states->getAnimationState(animationName.toStdString());🤖 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/ModelIsometricRenderer.cpp` around lines 574 - 580, The code calls getAllAnimationStates() twice unnecessarily - once at the beginning to assign to the states variable, and again at line 580 when retrieving the animation state. Since states is already a const pointer to AnimationStateSet and getAnimationState() is a const method, eliminate the redundant call by replacing animatedEntity->getAllAnimationStates()->getAnimationState(animationName.toStdString()) with states->getAnimationState(animationName.toStdString()) to reuse the previously fetched states pointer.
🤖 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.
Nitpick comments:
In `@src/ModelIsometricRenderer.cpp`:
- Around line 574-580: The code calls getAllAnimationStates() twice
unnecessarily - once at the beginning to assign to the states variable, and
again at line 580 when retrieving the animation state. Since states is already a
const pointer to AnimationStateSet and getAnimationState() is a const method,
eliminate the redundant call by replacing
animatedEntity->getAllAnimationStates()->getAnimationState(animationName.toStdString())
with states->getAnimationState(animationName.toStdString()) to reuse the
previously fetched states pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f37a321-e8b7-4451-b41a-c67b8db4200c
📒 Files selected for processing (5)
scripts/sync-doc-versions-from-cmake.shsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/ModelIsometricRenderer.cppsrc/ModelIsometricRenderer.h
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/sync-doc-versions-from-cmake.sh
- src/ModelIsometricRenderer.h
- src/MCPServer.cpp
- src/CLIPipeline.cpp
|
5 tasks
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
ModelIsometricRendererfor headless 8-direction isometric sprite atlases (rows = compass directions, columns = animation frames), reusing the turntable RTT pipeline with stable rest-pose framing and animated sampling viaAnimationState::setTimePosition.qtmesh isometricCLI and MCPgenerate_isometric_sprites(file-in / file-out parity), with Sentry breadcrumbs and JSON report support.--resolution(16–8192); camera framing via--camera-distance(fixed orbit) and--padding(auto-fit multiplier, default 1.25).--framesvalidation,file.exportbreadcrumbs.Closes #724.
Test plan
xvfb-run ./UnitTests --gtest_filter="*Isometric*"— 24 passed locallyqtmesh isometric model.fbx -o iso.png— static 8-direction gridqtmesh isometric model.fbx --animation "Walk" --frames 8 -o iso.png— 8×8 animated atlasqtmesh isometric model.fbx -o iso.png --padding 1.5 --camera-distance 5— framing controlsgenerate_isometric_spriteswith same params produces matching outputSummary by CodeRabbit
Release Notes
New Features
qtmesh isometricCLI support to export isometric/8-direction sprite atlases (static and animation-sampled), with options for directions, frames, resolution/size, camera distance/padding, and elevation.generate_isometric_spritesfor the same isometric sprite export, with optional JSON output.Documentation
Tests
Chores