Turntable PNG export (CLI + renderer) - #643
Merged
Merged
Conversation
Headless Ogre render-to-texture captures orbit frames from imported meshes. New `qtmesh turntable` writes a sprite sheet or a %02d frame sequence. Co-authored-by: Cursor <cursoragent@cursor.com>
Qualify ModelTurntableRenderer::shutdown() so ensureRenderTarget compiles. Co-authored-by: Cursor <cursoragent@cursor.com>
Use white ambient plus directional light, hide Ogre selection bounding boxes, orbit on a single selectable axis (y/x/z), and accept --camera-height as an elevation alias. Co-authored-by: Cursor <cursoragent@cursor.com>
Orbit camera with a stable world-up view matrix instead of lookAt roll, and render through the ShaderGenerator viewport scheme with per-material RTSS validation so normal maps shade correctly. Co-authored-by: Cursor <cursoragent@cursor.com>
Recenter imported meshes at the world origin before capture and compute orbit distance from all eight bounding-box corners so the model stays framed in the viewport at any elevation. Co-authored-by: Cursor <cursoragent@cursor.com>
Use a pivot node so the camera always looks at the orbit point, dedupe scene-node translation on multi-entity imports, and apply a small vertical framing bias for upright assets. Co-authored-by: Cursor <cursoragent@cursor.com>
Import already wires RTSS like the editor; re-applying wirePbrSlotsForFFP and applyNormalMap could leave the normal map in the FFP texture stack while RTSS also sampled it. Rely on MSN_SHADERGEN viewport + imported materials. Co-authored-by: Cursor <cursoragent@cursor.com>
Cover cmdTurntable success paths (sprite sheet, sequence, columns, JSON), parseAxis edge cases, composeSpriteSheet layouts, axis orbit variants, and null output rejection. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR adds a new ChangesTurntable Rendering Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 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: d5fdbd9216
ℹ️ 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".
| restDir = cameraRestOffset(axis, 1.0f, 0.0f); | ||
| restDir.normalise(); | ||
|
|
||
| const Ogre::Real distance = fitOrbitDistance(bounds, pivotPoint, restDir, st.camera, paddingFactor); |
There was a problem hiding this comment.
placeCameraOnAxis always calls fitOrbitDistance with restDir (the unrotated view direction), so every frame uses a radius sized for angle 0 instead of the actual camera angle. For non-square bounds this underestimates distance on parts of the orbit (for example, Y-axis orbit when Z-extent is larger than X-extent), which crops frames in the generated turntable. The fit calculation should use the rotated per-frame view direction (or precompute the max radius over all sampled angles).
Useful? React with 👍 / 👎.
Comment on lines
+2918
to
+2924
| const bool sequenceOutput = outputPath.contains(QLatin1Char('%')); | ||
| QStringList writtenPaths; | ||
|
|
||
| if (sequenceOutput) { | ||
| for (int f = 0; f < frames.size(); ++f) { | ||
| char buf[2048]; | ||
| snprintf(buf, sizeof(buf), outputPath.toUtf8().constData(), f); |
There was a problem hiding this comment.
Any % in -o switches to sequence mode, and the raw user string is then used as a snprintf format string. A filename like "100%done.png" (or any unsupported specifier) is treated as a format pattern, producing undefined behavior and potentially failing/crashing instead of writing files. Restrict sequence mode to an explicit validated placeholder (e.g. %0Nd/%d) and escape or reject other % usages.
Useful? React with 👍 / 👎.
Cover --size WxH parsing, --cli flag handling, camera_height alias, null-only entity lists, and turntable option clamping. 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: 6
🧹 Nitpick comments (1)
src/ModelTurntableRenderer_test.cpp (1)
13-17: ⚡ Quick winAlign fixture prerequisites with Ogre test convention
On Line 15, add
ASSERT_TRUE(canLoadMeshFiles())alongsidetryInitOgre()inSetUp()to keep Ogre-dependent CI prerequisites explicit.Based on learnings: In QtMeshEditor tests that depend on Ogre, fixture SetUp must fail loudly in CI by using `ASSERT_TRUE(tryInitOgre())` and `ASSERT_TRUE(canLoadMeshFiles())`.Suggested fix
void SetUp() override { ASSERT_TRUE(tryInitOgre()) << "Ogre init failed (Xvfb/GL required in CI)"; + ASSERT_TRUE(canLoadMeshFiles()) << "Required mesh resources are unavailable"; ModelTurntableRenderer::shutdown(); }🤖 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/ModelTurntableRenderer_test.cpp` around lines 13 - 17, The SetUp fixture must assert both Ogre initialization and mesh-loading capability: inside the overridden SetUp() in ModelTurntableRenderer_test.cpp, after calling ASSERT_TRUE(tryInitOgre()) and before ModelTurntableRenderer::shutdown(), add ASSERT_TRUE(canLoadMeshFiles()) so tests that depend on Ogre + mesh files fail loudly in CI; reference functions: SetUp(), tryInitOgre(), canLoadMeshFiles(), and ModelTurntableRenderer::shutdown().
🤖 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 `@action.yml`:
- Line 11: The description field in action.yml currently lists only a subset of
subcommands ("Subcommand: scan, info, validate, convert, fix, anim, lod, pose,
turntable") which may be out of sync with the CLI; update the description value
to either enumerate all supported subcommands exactly as implemented in the CLI
or change the text to indicate these are “common examples” (e.g., "Subcommands
(examples): ...") so users aren’t misled—edit the description entry in
action.yml to reflect the full, accurate command set or to clearly mark it as
examples.
In `@src/CLIPipeline.cpp`:
- Around line 2816-2849: Numeric flag parsing currently uses
QString::toInt()/toFloat without validation (e.g., parsing into frameCount,
columns, width, height, elevation, and the sizeArg parts), causing malformed
inputs to coerce to 0; change each conversion to use the ok boolean overload
(QString::toInt(bool *ok) / toFloat(bool *ok)), validate ok and for the "--size"
branch validate both left and right parts (or single value) before assigning,
and if any parse fails print a usage/error and return exit code 2 instead of
silently using 0; update both "--elevation" and ("--camera-height" ||
"--camera_height") handling to validate the float parse the same way.
- Around line 2886-2949: The code logs a generic "cli.turntable" breadcrumb but
doesn't emit file I/O breadcrumbs for the actual model import and image exports;
add SentryReporter::addBreadcrumb("file.import", QString("Import
%1").arg(fi.absoluteFilePath())) immediately before calling
MeshImporterExporter::importer({fi.absoluteFilePath()}) and add
SentryReporter::addBreadcrumb("file.export", QString("Export
%1").arg(framePath)) inside the sequenceOutput loop (before frames.at(f).save),
plus a file.export breadcrumb before the single-image save (before
frames.first().save(outputPath)) and before the sprite-sheet save (before
sheet.save(outputPath)) so Sentry captures the concrete input/output paths for
MeshImporterExporter::importer,
ModelTurntableRenderer::renderToImages/composeSpriteSheet and the image write
operations.
In `@src/ModelTurntableRenderer.cpp`:
- Around line 94-100: The fast-path that returns early when st.renderTarget
exists and st.rttWidth == width && st.rttHeight == height skips refreshing the
render target background; update the code in ModelTurntableRenderer.cpp so
before the early return you call st.renderTarget->setBackgroundColour(bg) (using
the same bg derived from TurntableOptions.background) and, if needed, also
propagate the color to the existing viewport (via
st.renderTarget->getViewport(0)->setBackgroundColour(bg)) so background changes
are applied even on the same-size RTT reuse.
- Around line 407-482: Add Sentry breadcrumbs for the turntable render lifecycle
in ModelTurntableRenderer::renderToImages: call SentryReporter::addBreadcrumb
with an appropriate category (e.g., "turntable") and messages for start
("renderToImages:start" or similar) immediately at the top of the function
(after null checks and before heavy work), for success
("renderToImages:success") just before returning true, and for failures in both
catch blocks ("renderToImages:error" with the exception description for the
Ogre::Exception catch and a generic failure message for the catch-all). Also add
an intermediate breadcrumb if desired inside the main loop after each frame or
before state().renderTarget->update() to indicate progress (e.g.,
"renderToImages:frame:N"). Ensure you reference the existing functions/state
used here (renderToImages, state().renderTarget, placeCameraOnAxis,
readRenderTarget) when inserting the SentryReporter::addBreadcrumb calls.
- Around line 376-404: The cleanup currently only runs when sm is non-null,
leaving cached pointers (st.light, st.lightNode, st.camera, st.cameraNode,
st.pivotNode, st.hasSavedAmbient/st.savedAmbient) unchanged if sceneMgr() is
null; change the logic so any calls that require sm (e.g., sm->setAmbientLight,
detachObject, destroySceneNode, destroyLight, destroyCamera) are guarded by if
(sm) but the cached pointer resets and flag clears always run afterwards—i.e.,
move st.light = nullptr, st.lightNode = nullptr, st.camera = nullptr,
st.cameraNode = nullptr, st.pivotNode = nullptr and st.hasSavedAmbient = false
out of the sm guard so pointers are nulled even when sm is null.
---
Nitpick comments:
In `@src/ModelTurntableRenderer_test.cpp`:
- Around line 13-17: The SetUp fixture must assert both Ogre initialization and
mesh-loading capability: inside the overridden SetUp() in
ModelTurntableRenderer_test.cpp, after calling ASSERT_TRUE(tryInitOgre()) and
before ModelTurntableRenderer::shutdown(), add ASSERT_TRUE(canLoadMeshFiles())
so tests that depend on Ogre + mesh files fail loudly in CI; reference
functions: SetUp(), tryInitOgre(), canLoadMeshFiles(), and
ModelTurntableRenderer::shutdown().
🪄 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: 2277fbe7-e0c8-4465-a7e1-1fb53f366680
📒 Files selected for processing (11)
CLAUDE.mdaction.ymlsrc/CLIPipeline.cppsrc/CLIPipeline.hsrc/CLIPipeline_test.cppsrc/CMakeLists.txtsrc/ModelTurntableRenderer.cppsrc/ModelTurntableRenderer.hsrc/ModelTurntableRenderer_test.cppsrc/main.cpptests/CMakeLists.txt
| inputs: | ||
| command: | ||
| description: 'Subcommand: scan, info, validate, convert, fix, anim, lod, pose' | ||
| description: 'Subcommand: scan, info, validate, convert, fix, anim, lod, pose, turntable' |
There was a problem hiding this comment.
Keep the command list in sync with actual CLI coverage.
Line 11 still documents only a subset of supported subcommands, which can mislead action users about what’s valid. Please either list all supported commands or explicitly label this as “common examples”.
📝 Suggested doc update
- description: 'Subcommand: scan, info, validate, convert, fix, anim, lod, pose, turntable'
+ description: 'Subcommand (examples): scan, info, validate, convert, fix, anim, lod, pose, turntable, material, pack-textures, normal-from-height, atlas, atlas-apply, optimize'📝 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.
Suggested change
| description: 'Subcommand: scan, info, validate, convert, fix, anim, lod, pose, turntable' | |
| description: 'Subcommand (examples): scan, info, validate, convert, fix, anim, lod, pose, turntable, material, pack-textures, normal-from-height, atlas, atlas-apply, optimize' |
🤖 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 `@action.yml` at line 11, The description field in action.yml currently lists
only a subset of subcommands ("Subcommand: scan, info, validate, convert, fix,
anim, lod, pose, turntable") which may be out of sync with the CLI; update the
description value to either enumerate all supported subcommands exactly as
implemented in the CLI or change the text to indicate these are “common
examples” (e.g., "Subcommands (examples): ...") so users aren’t misled—edit the
description entry in action.yml to reflect the full, accurate command set or to
clearly mark it as examples.
Strip duplicate normal-map texture units from the FFP multitexture chain while keeping SRS_NORMALMAP sampling, and run that sync before turntable capture. Also resolve normal maps via qtme.normal_map UOB when TUS names are missing (typical on FBX imports like Jump.fbx). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 1051-1054: The test uses a hardcoded machine-specific path
(jumpPath) and GTEST_SKIP(), making it non-portable; replace the hardcoded
"/home/fernando/Downloads/Jump.fbx" with the repository test fixture path (use
testDataDir() or equivalent to build the path to "Jump.fbx"), remove the
GTEST_SKIP() branch, and assert presence with
ASSERT_TRUE(QFile::exists(jumpPath)) << "Jump.fbx must be present in test data:
" << jumpPath.toStdString(); ensure jumpPath is constructed via
testDataDir().filePath("Jump.fbx") (or equivalent) so the asset is loaded from
the repo testData directory and the test fails loudly on CI.
In `@src/ModelTurntableRenderer_test.cpp`:
- Around line 189-210: This test calls RTShaderHelper::initialize(sceneMgr) but
never restores global RTShaderHelper state; add a teardown to undo that by
calling the RTShaderHelper cleanup API (e.g. RTShaderHelper::finalize() or the
library's corresponding shutdown/uninitialize function) after the assertions —
either append the call at the end of this test or, better, put it in the test
fixture TearDown/AfterEach so RTShaderHelper::initialize and
RTShaderHelper::finalize/uninitialize are always paired and global listener
state is not left installed for subsequent tests.
In `@src/ModelTurntableRenderer.cpp`:
- Around line 328-341: The deduplication uses mat->getName() which conflates
materials from different resource groups; instead deduplicate by the material
object identity. Change processed from std::unordered_set<std::string> to
something like std::unordered_set<const Ogre::Material*> (or uintptr_t) and use
mat.get() (or the raw pointer) as the key when inserting/checking, leaving calls
to RTShaderHelper::excludeNormalMapFromFfpChain(mat) and
RTShaderHelper::wirePbrSlotsForFFP(mat.get()) unchanged so each distinct
Material instance is processed exactly once.
In `@src/RTShaderHelper.cpp`:
- Around line 335-345: The loop that intends to remove duplicate texture units
is decrementing canonicalIdx whenever it encounters a non-albedo unit before
canonicalIdx, even if no removal occurred; fix by only decrementing canonicalIdx
when you actually remove a texture unit. In the loop in RTShaderHelper.cpp where
you iterate texture units (the block using pass->getTextureUnitState,
isAlbedoSlotName, and pass->removeTextureUnitState), change the logic so you
check if tus->getTextureName() == canonicalTex, then call
pass->removeTextureUnitState(...) and only after that, if i < canonicalIdx,
decrement canonicalIdx; do not decrement canonicalIdx in any other branch.
🪄 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: 4cc192c3-54c1-42dc-8cb1-450e01ad7fb5
📒 Files selected for processing (7)
src/Assimp/MaterialProcessor.cppsrc/CLIPipeline_test.cppsrc/MeshImporterExporter.cppsrc/ModelTurntableRenderer.cppsrc/ModelTurntableRenderer_test.cppsrc/RTShaderHelper.cppsrc/RTShaderHelper.h
Defer ShaderGenerator wiring until all PBR texture units exist, remove extra Bump/NormalMap units, dedupe diffuse+albedo FFP modulation, and rebuild the shader technique on turntable capture (Jump.fbx and similar). Co-authored-by: Cursor <cursoragent@cursor.com>
Load Ogre resources after the headless render window so turntable matches the GUI normal-map path. Harden CLI flag parsing, sequence output patterns, orbit framing per angle, and CI tests (no skipped Jump.fbx fixture). Co-authored-by: Cursor <cursoragent@cursor.com>
Add turntable to the CLI reference, quick start, Docker examples, and the landing-page pipeline tab with usage for sprite sheets and frame sequences. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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
qtmesh turntablefor PNG sprite sheets / frame sequences from OBJ/glb/fbx/etc., with--axis,--json,--columns, framing/lighting fixes, and pivot-camera orbit so vertical meshes frame consistently.ModelTurntableRenderer(parseAxis,composeSpriteSheet, orbit axes, null guards, double shutdown) andCLIPipeline::cmdTurntableintegration paths.Closes #294.
Test plan
cmake --build build_local --target UnitTests && ./build_local/bin/UnitTests --gtest_filter='ModelTurntableRenderer*:CLIPipelineCmdTurntable*:CLIPipelineCmdTurntableError*'qtmesh turntable model.obj -o sheet.png --frames 8 --size 512Made with Cursor
Summary by CodeRabbit
New Features
turntableCLI command to render model turntables, outputting per-frame PNGs or composed sprite sheets with configurable frames, axis, camera height, resolution, and layout.Documentation
turntableand its parameters.Tests
turntableargument validation and rendering outcomes.Bug Fixes