Fix mesh validation crash on Linux; add Sentry breadcrumbs - #244
Conversation
…tures On Linux/GL3Plus, MeshValidator::validate() called glMapBufferRange (via Ogre HardwareBuffer::lock) between render frames when no OpenGL context was current, causing a crash. Fixed by deferring the actual buffer-locking work to frameStarted() via Ogre::FrameListener, which guarantees the GL context is active. macOS (Metal) is unaffected. Also adds Sentry breadcrumbs to MeshValidator (validate, fixAll), MeshLodController (generate, auto-generate, remove, export LODs), and the animation-only FBX import/merge path in mainwindow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 20 seconds. ⌛ 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 (4)
📝 WalkthroughWalkthroughAdded Sentry breadcrumb logging across mesh UI and LOD workflows. Refactored MeshValidator to defer validation to the next Ogre frame via an Ogre::FrameListener (validate() now schedules doValidate()). Added MCPServer handlers for mesh validation and LOD tooling and bumped project version. Changes
Sequence DiagramsequenceDiagram
participant User
participant App as MeshValidator (caller)
participant Ogre as Ogre::FrameListener
participant Validator as ValidationLogic
User->>App: call validate()
App->>App: set m_pendingValidate = true
App->>Ogre: register frame listener (if not registered)
App-->>User: return immediately
Note over Ogre: Next frame tick
Ogre->>App: frameStarted(evt)
App->>App: if m_pendingValidate
App->>Validator: doValidate()
Validator->>Validator: run validation (GL context active)
App->>App: m_pendingValidate = false
sequenceDiagram
participant Client
participant MCP as MCPServer
participant Sel as Selection
participant Validator as MeshValidator
participant LodCtrl as MeshLodController
Client->>MCP: callTool("generate_lods", {count, reductions})
MCP->>Sel: check selected entities
alt none selected
MCP-->>Client: return error JSON
else selected
MCP->>LodCtrl: generateLods(selection, count, reductions)
LodCtrl-->>MCP: emit error OR success info
MCP-->>Client: return JSON result (error or lod info)
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/MeshValidator.cpp`:
- Around line 94-117: The UI shows empty results because MeshValidator::validate
defers work to frameStarted() and leaves m_validated false and m_issues empty;
add an explicit "validating" state so the UI can show a loader. Introduce a
boolean member (e.g., m_validating) with an accessor and change signal
(validatingChanged), set m_validating = true at the start of
MeshValidator::validate (alongside clearing m_issues and setting
m_pendingValidate), emit validatingChanged (and keep emitting issuesChanged if
you want the empty/placeholder state), then clear m_validating = false and set
m_validated = true when the actual validation completes in frameStarted();
update QML to bind to MeshValidator.validating to show a loading indicator.
🪄 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: 8205b56d-151c-4913-97fe-e8f7b9cb8092
📒 Files selected for processing (4)
src/MeshLodController.cppsrc/MeshValidator.cppsrc/MeshValidator.hsrc/mainwindow.cpp
| void MeshValidator::validate() | ||
| { | ||
| m_issues.clear(); | ||
| m_validated = false; | ||
| emit issuesChanged(); | ||
|
|
||
| auto* sel = SelectionSet::getSingleton(); | ||
| if (!sel || !sel->hasEntities()) | ||
| return; | ||
|
|
||
| SentryReporter::addBreadcrumb("ui.action", "Validate mesh"); | ||
|
|
||
| // On Linux/GL3Plus, glMapBufferRange requires an active OpenGL context. | ||
| // Deferring to frameStarted() guarantees the Ogre context is current. | ||
| if (!m_frameListenerRegistered) { | ||
| if (auto* mgr = Manager::getSingletonPtr()) { | ||
| if (auto* root = mgr->getRoot()) { | ||
| root->addFrameListener(this); | ||
| m_frameListenerRegistered = true; | ||
| } | ||
| } | ||
| } | ||
| m_pendingValidate = true; | ||
| } |
There was a problem hiding this comment.
Deferred validation causes UI to show stale data until next frame.
After validate() returns, m_validated is false and m_issues is empty. The QML in qml/PropertiesPanel.qml (lines 597-608) calls MeshValidator.validate() and immediately displays results from MeshValidator.validated and MeshValidator.issues. With this change, the UI will briefly show no results until frameStarted() fires on the next render frame.
Consider emitting a "validating" state or adding a loading indicator so users know validation is in progress rather than complete with no issues.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshValidator.cpp` around lines 94 - 117, The UI shows empty results
because MeshValidator::validate defers work to frameStarted() and leaves
m_validated false and m_issues empty; add an explicit "validating" state so the
UI can show a loader. Introduce a boolean member (e.g., m_validating) with an
accessor and change signal (validatingChanged), set m_validating = true at the
start of MeshValidator::validate (alongside clearing m_issues and setting
m_pendingValidate), emit validatingChanged (and keep emitting issuesChanged if
you want the empty/placeholder state), then clear m_validating = false and set
m_validated = true when the actual validation completes in frameStarted();
update QML to bind to MeshValidator.validating to show a loading indicator.
![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: 72a6acc498
ℹ️ 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".
| if (!m_frameListenerRegistered) { | ||
| if (auto* mgr = Manager::getSingletonPtr()) { | ||
| if (auto* root = mgr->getRoot()) { |
There was a problem hiding this comment.
validate() only calls root->addFrameListener(this) when m_frameListenerRegistered is false, but that flag is never reset when Manager::kill() destroys the current Ogre::Root. If the process recreates Manager/Root (which happens in several test flows and any in-process restart), subsequent MeshValidator::validate() calls set m_pendingValidate but never run doValidate() because this object is no longer registered on the new root. This makes validation silently stop working in that lifecycle.
Useful? React with 👍 / 👎.
…crumbs; bump 2.20.1 New MCP tools: - validate_mesh: run synchronous mesh validation, returns issues list - generate_lods: generate N LOD levels with optional reduction ratios - generate_auto_lods: auto-generate optimal LOD levels - remove_lods: strip all LOD levels from selected mesh - get_lod_info: query per-LOD triangle counts Sentry breadcrumbs added to MaterialPresetLibrary::applyPreset() (was missing). MeshValidator::doValidate() promoted to public for synchronous MCP/CLI use. Version bumped to 2.20.1 (bugfix: Linux GL context crash in mesh validation). Co-Authored-By: Claude Sonnet 4.6 <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 the current code and only fix it if needed.
Inline comments:
In `@src/MCPServer.cpp`:
- Around line 2141-2163: The response currently echoes the requested 'count'
which may be clamped by MeshLodController::generateLods(); change the code that
builds the header (currently using 'count') to report the actual generated LOD
count by using MeshLodController::instance()->lodLevelInfo().size() (or
QVariantList info.size()) after generateLods(), so the message reflects the
clamped/actual number of levels produced; ensure you reference
MeshLodController::generateLods and lodLevelInfo when making this change.
- Around line 2117-2120: Replace the direct synchronous call to
MeshValidator::doValidate() with MeshValidator::validate() so the validation
runs deferred (via frameStarted()) and will only lock GL buffers when a GL
context is current; subscribe to the MeshValidator::issuesChanged() signal and
use QTimer::singleShot to defer sending the MCP HTTP response until after that
signal fires, ensuring the response is emitted after validate() completes and
avoiding re-entrant GL context crashes.
🪄 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: 244ff8df-3ae4-42b1-a0c5-55f8f6b6752d
📒 Files selected for processing (5)
CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MaterialPresetLibrary.cppsrc/MeshValidator.h
✅ Files skipped from review due to trivial changes (2)
- src/MaterialPresetLibrary.cpp
- CMakeLists.txt
| // doValidate() is synchronous — safe to call here since Ogre is initialized | ||
| // in headless MCP mode (CPU buffers) or from the main thread with GUI. | ||
| MeshValidator* validator = MeshValidator::instance(); | ||
| validator->doValidate(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the specific lines in MCPServer.cpp
sed -n '2117,2120p' src/MCPServer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 313
🏁 Script executed:
# Get context around those lines and the full toolValidateMesh function
sed -n '2109,2133p' src/MCPServer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1059
🏁 Script executed:
# Find MeshValidator.h to understand its API
find . -name "MeshValidator.h" -type f | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 91
🏁 Script executed:
# Look for frame-listener or deferred validation pattern in MeshValidator
rg -A 10 "frameStarted|doValidate" src/MeshValidator.cpp | head -80Repository: fernandotonon/QtMeshEditor
Length of output: 845
🏁 Script executed:
# Check if there's a pending validation queue or deferred path
rg -i "defer|queue|pending.*validat" src/MeshValidator.h src/MeshValidator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 661
🏁 Script executed:
# Check MeshValidator.h to see the public API
cat src/MeshValidator.h | head -100Repository: fernandotonon/QtMeshEditor
Length of output: 2123
🏁 Script executed:
# Check if validate() method exists in MeshValidator
rg -A 5 "void validate\(\)" src/MeshValidator.h src/MeshValidator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 526
🏁 Script executed:
# Examine the MCP response mechanism and how signals/slots work
grep -n "issuesChanged\|makeSuccessResult" src/MCPServer.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 1960
🏁 Script executed:
# Check how other MCP tools handle async operations
sed -n '2135,2169p' src/MCPServer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1364
🏁 Script executed:
# Look for signal connections in MCPServer if any
rg -i "connect.*validator|issuesChanged" src/MCPServer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check MeshValidator.cpp implementation of doValidate to understand buffer operations
sed -n '160,220p' src/MeshValidator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 3134
🏁 Script executed:
# Check if there are any recent commits or comments about doValidate safety
rg -B 3 -A 3 "safe from MCP" src/Repository: fernandotonon/QtMeshEditor
Length of output: 542
🏁 Script executed:
# Verify the exact comment in doValidate() implementation
rg -B 2 "glMapBufferRange" src/MeshValidator.cppRepository: fernandotonon/QtMeshEditor
Length of output: 212
🏁 Script executed:
# Check if MCP server uses QTimer::singleShot anywhere else
rg -n "QTimer::singleShot" src/MCPServer.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 228
🏁 Script executed:
# Check how the tool dispatcher calls the tool functions and if there's async response handling
sed -n '2997,3050p' src/MCPServer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1750
🏁 Script executed:
# Look at the complete tool dispatch in callTool to understand response flow
sed -n '457,475p' src/MCPServer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 881
🏁 Script executed:
# Check if there's a pattern for queued/pending tool results elsewhere
rg -A 15 "struct.*Tool|class.*Tool" src/MCPServer.h | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Verify the exact comment about doValidate() in the header
grep -B 5 -A 5 "GL context must be current" src/MeshValidator.hRepository: fernandotonon/QtMeshEditor
Length of output: 562
Call validate() and defer the MCP response to avoid GL context crashes on Linux.
The current code calls doValidate() directly at line 2120, which locks GPU hardware buffers. On Linux/GL3Plus, buffer locking requires an active OpenGL context. Between render frames, no context is current, causing crashes in GUI mode.
The MeshValidator header explicitly documents: "GL context must be current — safe from MCP/CLI context and from inside the Ogre render loop; use validate() from QML to defer automatically."
Use validate() instead (which defers to frameStarted()) and defer the HTTP response via QTimer::singleShot after the issuesChanged() signal. This aligns with the MCP guideline: "HTTP API in MCP server must use deferred tool execution via QTimer::singleShot to avoid re-entrant crashes from Ogre event processing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer.cpp` around lines 2117 - 2120, Replace the direct synchronous
call to MeshValidator::doValidate() with MeshValidator::validate() so the
validation runs deferred (via frameStarted()) and will only lock GL buffers when
a GL context is current; subscribe to the MeshValidator::issuesChanged() signal
and use QTimer::singleShot to defer sending the MCP HTTP response until after
that signal fires, ensuring the response is emitted after validate() completes
and avoiding re-entrant GL context crashes.
…ation, LOD count - Add MeshValidator.validating property (emits validatingChanged) so QML can show "Validating..." while deferred GPU buffer read is pending in frameStarted - Track registered Ogre::Root* instead of a bool flag so the FrameListener is automatically re-registered if Manager/Root is recreated (e.g. test teardown) - Fix generate_lods MCP response to echo actual clamped LOD count from lodLevelInfo() instead of the raw requested value - Update PropertiesPanel.qml to show the validating indicator Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
1 participant
Summary
MeshValidator::validate()was callingHardwareBuffer::lock()→glMapBufferRange()between render frames when no OpenGL context was current. On Linux/GL3Plus this crashes; macOS (Metal) uses unified memory and is unaffected. Fixed by makingMeshValidatoranOgre::FrameListener—validate()sets am_pendingValidateflag, anddoValidate()(with all the GPU buffer reads) runs inframeStarted()the next time the GL context is guaranteed active.SentryReporter::addBreadcrumbcalls to all new features that had none:MeshValidator::validate()andfixAll()MeshLodController::generateLods(),generateAutoLods(),removeLods(),exportLods()mainwindow.cppTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores