fix(export): guard texture serialization + pause timers during GUI export (3.3.1)#693
Merged
Conversation
…port (3.3.1) Two coupled fixes for the GUI "File → Export Selected" crash that reproduced on Ogre .mesh + every Assimp-backed format (glTF, OBJ, Collada, STL, PLY, 3DS, X, Assimp Binary). FBX and PS1 (.tmd / .rsd) survived because their custom exporters snapshot inputs up front; the others walk the live mesh. * Root cause: `MeshImporterExporter::exportTextures` called `Ogre::Texture::convertToImage()` unconditionally. When a material's TUS references a texture that isn't GPU-resident (null `_getTexturePtr`, `isLoaded()=false`, or zero-size from a placeholder TUS), `convertToImage` reads from a null buffer and SIGSEGVs inside Ogre (`Ogre::Texture::convertToImage + 116`). Now guard with `!tex || !TEX_TYPE_2D || !isLoaded() || zero size`, attempt a `load()` first, and wrap the actual save in catch-all so a single bad texture can't take down the export. Errors are logged to Ogre LogManager; the rest of the export continues. * Secondary: pause render-loop and animation-poll timers for the whole `on_actionExport_Selected_triggered` action. The QFileDialog opens a nested event loop; without pausing, queued timer callbacks fire inside the dialog AND mid-export. `MainWindow::m_pTimer` calls `renderOneFrame()` while the serializer walks the mesh, and `AnimationControlController::m_pollTimer` (60fps) advances the SkeletonInstance time position — both could mutate the data the serializer is reading. Added `AnimationControlController::suspendPollTimer/resumePollTimer`, called around the export. Render timer paused too. * `MeshImporterExporter::exporter(node)` now takes an optional `QWidget* parent` so the file dialog is anchored to MainWindow instead of nullptr (cleaner modal cycle on macOS). Callers unchanged because the param defaults to nullptr. Bumps version to 3.3.1. Per CLAUDE.md ran `scripts/sync-doc-versions-from-cmake.sh` so README + website hook stay aligned with the CMake project version. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR releases 3.3.1: it adds AnimationControlController suspend/resume APIs to pause 60fps polling, hardens MeshImporterExporter (parent-anchored dialogs, early validation, safer texture export, adjusted exception handling), updates MainWindow export flow to suspend/resume timers, and bumps doc/CI version pins and sync script to 3.3.1. ChangesExport crash prevention (issue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
![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: f9b65adf6e
ℹ️ 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".
| file.setFile(_uri); | ||
|
|
||
| if(!Manager::getSingleton()->getSceneMgr()->hasEntity(_sn->getName())) return -1; | ||
| if(!Manager::getSingleton()->getSceneMgr()->hasEntity(_sn->getName())) { |
There was a problem hiding this comment.
When the non-dialog exporter is called with a null scene node, this line now dereferences _sn before any validation. The public API previously returned -1 for this case (and the repo has MeshImporterExporterStandaloneTest.Exporter_NullSceneNode_ReturnMinusOne covering exporter(nullptr, "", "")), so invalid CLI/MCP/test inputs can now crash the process instead of failing cleanly. Keep the early _sn guard before accessing _sn->getName().
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MeshImporterExporter.cpp (1)
2335-2353:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd breadcrumbs for the user-triggered export dialog flow.
This UI path is user-facing and significant, but Line 2344 and the cancel path at Line 2348 are currently untracked.
💡 Suggested fix
QString MeshImporterExporter::exporter(const Ogre::SceneNode *_sn, QWidget* parent) { if(!_sn) { QMessageBox::warning(parent,"No object","Which object are you trying to export?",QMessageBox::Ok); return QString(); } + SentryReporter::addBreadcrumb( + "ui.action", + QStringLiteral("Opened Export Selected dialog")); + QString filter = "Ogre Mesh (*.mesh)"; QString fileName = QFileDialog::getSaveFileName(parent, QObject::tr("Export Mesh"), _sn->getName().data(), exportFileDialogFilter(),&filter, QFileDialog::DontUseNativeDialog); - if(fileName.isEmpty()) return QString(); + if(fileName.isEmpty()) { + SentryReporter::addBreadcrumb("ui.action", QStringLiteral("Export Selected canceled")); + return QString(); + } QString uri = formatFileURI(fileName, filter); + SentryReporter::addBreadcrumb("file.export", QStringLiteral("Export Selected started: %1").arg(uri)); exporter(_sn, uri, filter); return uri; }As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message) using categories like 'ui.action', 'ai.tool_call', 'file.import', 'file.export'."
🤖 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/MeshImporterExporter.cpp` around lines 2335 - 2353, In MeshImporterExporter::exporter add SentryReporter::addBreadcrumb calls to track the user export flow: record a 'ui.action' breadcrumb just before showing the save dialog with a message including the node name (use _sn->getName()), record a 'file.export' breadcrumb if the user cancels (when fileName.isEmpty()) with a clear "export canceled" message, and record a 'file.export' breadcrumb immediately before calling exporter(_sn, uri, filter) indicating the export started (include the URI and filter); use the same helper symbols already present (formatFileURI, exportFileDialogFilter) to build messages so breadcrumbs reflect the dialog open, cancel, and start-export events.
🤖 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 `@README.md`:
- Around line 54-57: The README shows the action ref set to
fernandotonon/QtMeshEditor@3.3.1 but still pins image-tag: "3.1.0"; update all
image-tag values to "3.3.1" and any adjacent “current version” text to match the
bumped action ref (look for the snippet containing
fernandotonon/QtMeshEditor@3.3.1 and the image-tag field, and other repeated
occurrences in the file), then run ./scripts/sync-doc-versions-from-cmake.sh to
propagate the CMake VERSION change to README and
website/src/hooks/useQtmeshActionRef.js so docs remain synchronized.
In `@src/AnimationControlController.cpp`:
- Around line 79-95: Replace the direct stderr prints in
AnimationControlController::suspendPollTimer and
AnimationControlController::resumePollTimer with SentryReporter::addBreadcrumb
calls: remove fprintf(stderr, ...) and instead call
SentryReporter::addBreadcrumb("ui.action", "[anim] suspendPollTimer") when
stopping the timer in suspendPollTimer and
SentryReporter::addBreadcrumb("ui.action", "[anim] resumePollTimer") when
starting the timer in resumePollTimer, leaving the existing checks for
m_pollTimer/m_pollSuspended and the m_pollTimer->stop()/start(16) and
m_pollSuspended toggles intact.
In `@src/MeshImporterExporter.cpp`:
- Around line 206-223: The code ignores the boolean result of img.save(...)
leading to silent texture export failures; update the block around img.save to
check its return value and handle failures explicitly: call img.save(...) into a
bool saved variable (using the same path built from file.path() + "/" +
exportTextureName(QString::fromStdString(tex->getName()))), and if saved is
false log an error via Ogre::LogManager::getSingleton().logError including
texture id (tex->getName()) and full path, then either mark the export as failed
(set a failure flag/return false from the surrounding export function) or throw
an exception so callers know the export did not fully succeed; keep existing
catch blocks for exceptions but ensure non-exception failures are treated the
same way.
- Around line 2368-2371: The code dereferences _sn via _sn->getName() before
checking that _sn is non-null; update the overload to first check if _sn is
nullptr (e.g., if (!_sn) return -1 or appropriate error) before calling
_sn->getName(), so the subsequent calls to
Manager::getSingleton()->getSceneMgr()->hasEntity(...) and getEntity(...) are
safe; apply this null guard at the start of the block that uses _sn to prevent
null-pointer crashes.
---
Outside diff comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 2335-2353: In MeshImporterExporter::exporter add
SentryReporter::addBreadcrumb calls to track the user export flow: record a
'ui.action' breadcrumb just before showing the save dialog with a message
including the node name (use _sn->getName()), record a 'file.export' breadcrumb
if the user cancels (when fileName.isEmpty()) with a clear "export canceled"
message, and record a 'file.export' breadcrumb immediately before calling
exporter(_sn, uri, filter) indicating the export started (include the URI and
filter); use the same helper symbols already present (formatFileURI,
exportFileDialogFilter) to build messages so breadcrumbs reflect the dialog
open, cancel, and start-export events.
🪄 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: b45af125-7db4-43e4-bbf1-8ec322bc0445
📒 Files selected for processing (8)
CMakeLists.txtREADME.mdsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/MeshImporterExporter.cppsrc/MeshImporterExporter.hsrc/mainwindow.cppwebsite/src/hooks/useQtmeshActionRef.js
* Restore the `!_sn` and `_uri.isEmpty()` early-returns in
`MeshImporterExporter::exporter(SceneNode, uri, format, ...)`.
They were dropped when I stripped the printf traces — Codex P1
flagged that
`MeshImporterExporterStandaloneTest.Exporter_NullSceneNode_ReturnMinusOne`
exercises this exact case and would crash without the guard.
Also re-add the `!e` guard after `getEntity`.
* `AnimationControlController::suspendPollTimer / resumePollTimer`
now emit `SentryReporter::addBreadcrumb("ui.action", …)` instead
of `fprintf(stderr)`. Matches CLAUDE.md guidance that user-facing
/ significant operations need structured breadcrumbs.
* `exportTextures` cleans up the texture-save block per CodeRabbit
feedback: pulls the output path into a named local, adds a
comment explaining why we don't bool-check `Ogre::Image::save`
(it returns void and throws on failure; the existing
catch-all chain already logs).
* README + sync-doc script: `image-tag: "X.Y.Z"` pins now bump
alongside the action ref. The earlier 3.3.1 bump left the
pinned image tags at 3.1.0; CodeRabbit caught the mismatch.
`sync-doc-versions-from-cmake.sh` learns to update + verify
the `image-tag` field so this can't drift again.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rnings SonarCloud flagged `exportTextures` for cognitive complexity 49 > 25 (rule cpp:S3776) and nested-control-flow > 3 levels (rule cpp:S134) because the texture export sat at the bottom of a 4-deep `technique → pass → TUS → try` nest. Also caught the `catch(...)` arm (rule cpp:S1181) and a separate `catch(...)` in sceneExporter (cpp:S2738). Refactor into three small anonymous-namespace helpers: * `isTextureSerializable(tex)` — every null/load/size guard, early-returns. Replaces the inline `if (!tex) continue; if (!tex ->isLoaded()) ...` chain. * `saveTextureAsImage(tex, file)` — the convertToImage + img.save block, with `Ogre::Exception` + `std::exception` catch arms. Dropped the bare `catch(...)` per Sonar — those two together cover every throw the export path can emit (Ogre throws Ogre:: Exception, file-system code throws std::system_error which is std::exception). If anything else escapes the export legitimately wants to crash, which matches how the rest of the pipeline handles unknown exceptions. * `forEachNamedTexture(material, fn)` — flattens the technique → pass → TUS triple loop into one function so the outer call site is a one-liner. Same fix applied to `sceneExporter`'s catch chain — `catch(...)` removed; the `std::exception` arm already covers everything worth catching. `exportTextures` is now 4 lines and has zero nesting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 76-82: The caller currently only invokes apply_perl_replace() for
files matching an action-ref pin (e.g., fernandotonon/QtMeshEditor@X.Y.Z), so
files that only contain image-tag: "X.Y.Z" never get their image-tag updated;
update the caller logic to also run the image-tag replacement (the block that
sets QTMESH_DOC_VERSION and invokes perl with
s/(image-tag:\s*")(\d+\.\d+\.\d+)(")/...) for files that contain an image-tag
even if they lack the action-ref pin — either by removing the action-ref-only
guard and always calling apply_perl_replace(), or by adding an additional
condition that detects /image-tag:\s*"\d+\.\d+\.\d+"/ and runs the image-tag
perl replacement (refer to apply_perl_replace and the image-tag perl
substitution block).
- Around line 55-58: The current grep regex in the imgtags extraction
(imgtags="$(grep -ohE 'image-tag:[[:space:]]*"[0-9]+\.[0-9]+\.[0-9]+"' "$f" ...
)") will match backtick-wrapped examples; change the extraction to only match
occurrences not preceded by a backtick by using a negative-lookbehind-capable
tool (e.g., replace the grep with a Perl one-liner like perl -nle 'print $& if
/(?<!`)image-tag:\s*"[0-9]+\.[0-9]+\.[0-9]+"/' "$f"') and apply the same change
to the other occurrence noted (the block around lines 76-82) so examples wrapped
in backticks are ignored.
🪄 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: ca0ead79-bcd1-4d2f-9e13-a6719f22f358
📒 Files selected for processing (4)
README.mdscripts/sync-doc-versions-from-cmake.shsrc/AnimationControlController.cppsrc/MeshImporterExporter.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- src/AnimationControlController.cpp
CodeRabbit review on PR #693: * The previous regex matched `image-tag: "X.Y.Z"` regardless of surrounding backticks, so inline-code examples in markdown (`` `image-tag: "9.9.9"` ``) would get silently rewritten on every bump. The comment claimed they were skipped but the regex didn't enforce it. Added `(?<!\`)` / `(?!\`)` lookarounds to both the apply-side perl substitution and the verify-side scanner. BSD grep doesn't have lookaround support, so the scanner drops to perl too. * The apply-side caller previously gated `apply_perl_replace` on the file containing an action-ref pin (`fernandotonon/QtMeshEditor@X.Y.Z`). A doc file containing only `image-tag: "X.Y.Z"` would never get rewritten. Caller now matches either pattern. Tested with a synthetic file containing one backticked `image-tag` (preserved) and one unbacked pin (rewritten); `./scripts/sync-doc-versions-from-cmake.sh --check` against the repo continues to pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Actionable comments posted: 0 |
|
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
Fixes the GUI File → Export Selected crash on Ogre
.meshand every Assimp-backed format (.gltf,.glb,.obj,.dae,.stl,.ply,.3ds,.x,.assbin). FBX and PS1 (.tmd/.rsd) survived because their custom exporters snapshot inputs up front; the rest walk the live mesh and tripped one of two bugs.Bumps version to 3.3.1 and re-runs
scripts/sync-doc-versions-from-cmake.shper CLAUDE.md so README + website hook stay aligned.Root cause
MeshImporterExporter::exportTexturescalledOgre::Texture::convertToImage()unconditionally. When a material's TUS references a texture that isn't GPU-resident — null_getTexturePtr,isLoaded() == false, or zero-size from a placeholder TUS —convertToImagereads from a null buffer and SIGSEGVs inside Ogre (Ogre::Texture::convertToImage + 116, confirmed via printf trace to/tmp/qtmesh_export_trace.logsince stderr is consumed by macOS.appbundle launchd).Fix 1 — Texture guards
exportTexturesnow:TexturePtr, non-2D types, zero-size texturestex->load()when not loaded (some imports defer GPU upload until first render)convertToImage+img.save()in catch-all so one bad texture can't take down the export — errors logged viaOgre::LogManagerFix 2 — Timer-pause during the action
The
QFileDialogin the export path opens a nested event loop. Without pausing timers, queued timer callbacks fire inside the dialog AND mid-export:MainWindow::m_pTimercallsrenderOneFrame()→ serializer reads a mesh that the renderer is currently writingAnimationControlController::m_pollTimer(60 fps) advances the SkeletonInstance's animation time position → same race against the serializerAdded
AnimationControlController::suspendPollTimer / resumePollTimerand called both around the wholeon_actionExport_Selected_triggeredaction. Render timer paused too. Viewport freezes during the dialog and the export call but resumes cleanly after.Bonus
MeshImporterExporter::exporter(node)now takes an optionalQWidget* parentso the file dialog is anchored to MainWindow instead ofnullptr(cleaner modal cycle on macOS).Test plan
qtmesh convert ... -o /tmp/x.meshstill works (unchanged path).mesh,.gltf,.glb,.obj,.dae,.stl,.ply,.3ds,.x,.assbin,.mesh.xml,.fbx) — all OK.mesh— no longer crashes (verified by user onHip Hop Dancing.fbx)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Chores