Add vertex normal visualization (Show Normals)#173
Conversation
Add NormalVisualizer class that draws colored lines from each vertex along its normal direction using ManualObject with OT_LINE_LIST. Normals are color-coded by direction (|X|->Red, |Y|->Green, |Z|->Blue) like standard normal maps. Toggled via Options -> Show Normals menu action. Overlays auto-update when entities are added/removed via Manager signals. ManualObjects are attached to dedicated child scene nodes to avoid crashes from code that does unsafe static_cast<Entity*> on all attached objects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a NormalVisualizer component that renders per-entity normals as Ogre ManualObject overlays, integrates a "Show Normals" UI toggle and MCPServer tool to toggle it remotely, includes integration tests, updates build files and project version, and documents the feature. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as MainWindow UI
participant NV as NormalVisualizer
participant SceneMgr as Ogre::SceneManager
participant Entity as Ogre::Entity
User->>UI: toggle "Show Normals"
UI->>NV: setVisible(on/off)
alt Enable
NV->>SceneMgr: enumerate scene nodes / entities
SceneMgr->>Entity: list entities
loop per entity
Entity-->>NV: mesh vertex & normal data (or animated data)
NV->>NV: buildOverlayForEntity()
NV->>SceneMgr: create ManualObject & attach overlay node
end
NV->>NV: start update timer for animated entities
else Disable
NV->>SceneMgr: destroyAllOverlays()
NV->>NV: stop update timer
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
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: a7d4d445d7
ℹ️ 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".
| return; | ||
|
|
||
| Ogre::String moName = "NormalVisualizer_" + entity->getName(); | ||
| Ogre::ManualObject* mo = mSceneMgr->createManualObject(moName); |
There was a problem hiding this comment.
This call can throw when an entity is recreated with the same name while normals are visible. PrimitiveObject::updatePrimitive destroys the old entity and recreates a new one on the same node/name (src/PrimitiveObject.cpp:457-465), but NormalVisualizer never removes the old overlay entry keyed by the destroyed entity pointer, so createManualObject("NormalVisualizer_<name>") is invoked again for an existing ManualObject and Ogre raises a duplicate-name exception.
Useful? React with 👍 / 👎.
| connect(ui->actionShow_Grid, SIGNAL(toggled(bool)),Manager::getSingleton()->getViewportGrid(),SLOT(setVisible(bool))); | ||
|
|
||
| // show normals | ||
| m_normalVisualizer = new NormalVisualizer(Manager::getSingleton()->getSceneMgr(), this); |
There was a problem hiding this comment.
Constructing NormalVisualizer with MainWindow as parent defers its destruction until after MainWindow::~MainWindow finishes, but that destructor calls Manager::kill() first (src/mainwindow.cpp:209-213). If normals are enabled at exit, NormalVisualizer::~NormalVisualizer runs later and calls destroyAllOverlays() using a freed Ogre::SceneManager*, which can crash during shutdown.
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: 1
🧹 Nitpick comments (1)
src/NormalVisualizer_test.cpp (1)
82-106: Test intent is misleading; this currently verifies only “no crash.”
BuildsOverlayForInMemoryMeshdoes not assert overlay creation and always passes withSUCCEED(). Please either rename it as a crash-smoke test or add a concrete overlay assertion.✏️ Minimal rename to match current behavior
-TEST_F(NormalVisualizerIntegrationTest, BuildsOverlayForInMemoryMesh) +TEST_F(NormalVisualizerIntegrationTest, NoCrashWhenTogglingVisibilityForInMemoryMesh) @@ - // Manually call buildOverlayForEntity since the entity wasn't created via Manager - // (Manager::entityCreated signal was not emitted for direct SceneManager creation) - // Instead, just verify the toggle works without crash + // Smoke test only: toggling visibility should not crash for direct SceneManager entities. visualizer.setVisible(false); SUCCEED();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NormalVisualizer_test.cpp` around lines 82 - 106, The test BuildsOverlayForInMemoryMesh only asserts no crash (ends with SUCCEED()) but its name implies it verifies overlay creation; either rename the test to reflect a crash-smoke (e.g., DoesNotCrashWhenTogglingVisibilityForInMemoryMesh) or add a concrete assertion: call NormalVisualizer::buildOverlayForEntity(entity) (or the appropriate method on NormalVisualizer that triggers overlay creation), then assert the overlay was created by querying the visualizer (e.g., NormalVisualizer::getOverlayForEntity or checking Ogre::OverlayManager for the expected overlay name) and validate it is non-null/has expected contents; update the test name if you choose the rename option (reference: test BuildsOverlayForInMemoryMesh, class NormalVisualizer, methods setVisible and buildOverlayForEntity/getOverlayForEntity).
🤖 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/mainwindow.cpp`:
- Around line 341-343: m_normalVisualizer is a child QObject whose destructor
(NormalVisualizer::~NormalVisualizer -> destroyAllOverlays()) uses the scene
manager, but Manager::kill() is called earlier in MainWindow::~MainWindow(),
invalidating the scene and causing a UAF; fix by explicitly destroying
m_normalVisualizer before calling Manager::kill(): in MainWindow::~MainWindow()
delete (or deleteLater/safely delete synchronously) m_normalVisualizer (or
remove its parent and delete) and set m_normalVisualizer = nullptr prior to
invoking Manager::kill(), ensuring NormalVisualizer::~NormalVisualizer() runs
while the scene manager is still valid.
---
Nitpick comments:
In `@src/NormalVisualizer_test.cpp`:
- Around line 82-106: The test BuildsOverlayForInMemoryMesh only asserts no
crash (ends with SUCCEED()) but its name implies it verifies overlay creation;
either rename the test to reflect a crash-smoke (e.g.,
DoesNotCrashWhenTogglingVisibilityForInMemoryMesh) or add a concrete assertion:
call NormalVisualizer::buildOverlayForEntity(entity) (or the appropriate method
on NormalVisualizer that triggers overlay creation), then assert the overlay was
created by querying the visualizer (e.g., NormalVisualizer::getOverlayForEntity
or checking Ogre::OverlayManager for the expected overlay name) and validate it
is non-null/has expected contents; update the test name if you choose the rename
option (reference: test BuildsOverlayForInMemoryMesh, class NormalVisualizer,
methods setVisible and buildOverlayForEntity/getOverlayForEntity).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9666fa34-9595-4d1a-8976-a3981561449e
📒 Files selected for processing (7)
src/CMakeLists.txtsrc/NormalVisualizer.cppsrc/NormalVisualizer.hsrc/NormalVisualizer_test.cppsrc/mainwindow.cppsrc/mainwindow.hui_files/mainwindow.ui
- Normals now follow skeletal animations in real-time by requesting software-skinned normals via addSoftwareAnimationRequest(true) and updating ManualObject geometry each frame via QTimer - Add toggle_normals MCP tool for enabling/disabling normal visualization programmatically (global toggle, no entity parameter required) - Add Sentry breadcrumb for Show Normals toggle - Expand test coverage: 9 NormalVisualizer tests + 2 MCP toggle_normals tests - Update CLAUDE.md with NormalVisualizer documentation - Bump version to 2.10.0 Co-Authored-By: Claude Opus 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 2432-2448: New public tool "toggle_normals" was added to the MCP
interface so you must update the MCP protocol version; open the MCPServer.h
symbol SERVER_VERSION and increment its semantic version (e.g., 1.0.0 -> 1.1.0)
to reflect the new capability, commit the change, and ensure any code that
references SERVER_VERSION continues to compile against the new string constant.
In `@src/NormalVisualizer.cpp`:
- Line 68: The timer is started with mUpdateTimer.start(0) which can spin the
CPU; change the call so the update loop is capped (e.g., mUpdateTimer.start(16))
to limit frequency to ~60 FPS consistent with MCPServer, ensuring
updateAnimatedOverlays() is not invoked unboundedly and preventing CPU pegging
when it locks GPU vertex buffers and iterates skeletal vertices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c25a67c1-326f-45cc-a508-86553fa4a1c1
📒 Files selected for processing (8)
CLAUDE.mdCMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/MCPServer_test.cppsrc/NormalVisualizer.cppsrc/NormalVisualizer.hsrc/NormalVisualizer_test.cpp
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
- Add NormalVisualizer to tests/CMakeLists.txt (fixes linker errors) - Destroy NormalVisualizer before Manager::kill() to prevent UAF - Clean up stale overlays when entities are recreated with same name - Cap animation timer at ~60 FPS (start(16) instead of start(0)) - Bump MCP SERVER_VERSION to 1.1.0 for toggle_normals addition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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/NormalVisualizer.cpp`:
- Around line 375-389: In destroyAllOverlays(), guard entries in
mSoftwareAnimRequested before calling removeSoftwareAnimationRequest: iterate
the container and for each Ogre::Entity* (from mSoftwareAnimRequested) check the
pointer is non-null (and optionally verify it's still attached via
entity->getParentSceneNode() or similar) before calling
entity->removeSoftwareAnimationRequest(true); this prevents dereferencing
invalid/deleted Entity pointers that may have been destroyed (e.g., by
PrimitiveObject::updatePrimitive) without onSceneNodeDestroyed() being invoked;
update the loop in destroyAllOverlays() to skip null/invalid entities and then
clear mSoftwareAnimRequested as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ffa6cc5-657d-47ad-b05a-8033f97366bc
📒 Files selected for processing (4)
src/MCPServer.hsrc/NormalVisualizer.cppsrc/mainwindow.cpptests/CMakeLists.txt
| void NormalVisualizer::destroyAllOverlays() | ||
| { | ||
| for (auto it = mOverlays.begin(); it != mOverlays.end(); ++it) | ||
| { | ||
| OverlayData& data = it.value(); | ||
| data.node->detachObject(data.manualObject); | ||
| mSceneMgr->destroyManualObject(data.manualObject); | ||
| mSceneMgr->destroySceneNode(data.node); | ||
| } | ||
| mOverlays.clear(); | ||
|
|
||
| for (Ogre::Entity* entity : mSoftwareAnimRequested) | ||
| entity->removeSoftwareAnimationRequest(true); | ||
| mSoftwareAnimRequested.clear(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the NormalVisualizer class and understand mSoftwareAnimRequested usage
rg -n "mSoftwareAnimRequested" --type cpp src/Repository: fernandotonon/QtMeshEditor
Length of output: 979
🏁 Script executed:
#!/bin/bash
# Look for the onSceneNodeDestroyed callback implementation
rg -n "onSceneNodeDestroyed" --type cpp src/Repository: fernandotonon/QtMeshEditor
Length of output: 346
🏁 Script executed:
#!/bin/bash
# Check for entity destruction paths
rg -n "destroyEntity|removeAllEntities|destroySceneNode" --type cpp src/ -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 24735
🏁 Script executed:
#!/bin/bash
# Find the NormalVisualizer class definition
fd -e "cpp" -e "hpp" -e "h" | xargs grep -l "class NormalVisualizer" | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 139
🏁 Script executed:
cat -n src/NormalVisualizer.cpp | sed -n '75,100p'Repository: fernandotonon/QtMeshEditor
Length of output: 923
🏁 Script executed:
cat -n src/NormalVisualizer.cpp | sed -n '375,395p'Repository: fernandotonon/QtMeshEditor
Length of output: 670
🏁 Script executed:
cat -n src/NormalVisualizer.cpp | sed -n '125,145p'Repository: fernandotonon/QtMeshEditor
Length of output: 927
🏁 Script executed:
cat -n src/NormalVisualizer.cpp | sed -n '360,390p'Repository: fernandotonon/QtMeshEditor
Length of output: 1166
🏁 Script executed:
rg -n "destroyEntity" --type cpp src/NormalVisualizer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there's an entity destroyed signal or callback
rg -n "onEntity" --type cpp src/NormalVisualizer.cpp -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 360
🏁 Script executed:
# Look at the full NormalVisualizer.h to see all callbacks
cat -n src/NormalVisualizer.h | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 1641
🏁 Script executed:
# Check how entities are being destroyed in tests to understand risk scenarios
rg -B 2 -A 2 "destroyEntity.*destroySceneNode|destroySceneNode.*destroyEntity" --type cpp src/ | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there's an onEntityDestroyed or similar signal
rg -n "entityDestroyed|EntityDestroyed" --type cpp src/ | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if Manager has entity destruction signals
cat -n src/Manager.h | grep -A 5 -B 5 "signal\|Q_SIGNAL"Repository: fernandotonon/QtMeshEditor
Length of output: 496
🏁 Script executed:
# Look at the destructor to understand cleanup
cat -n src/NormalVisualizer.cpp | sed -n '38,75p'Repository: fernandotonon/QtMeshEditor
Length of output: 1578
🏁 Script executed:
# Let me check the destructor to see how cleanup is done
rg -n "~NormalVisualizer" --type cpp src/ -A 10Repository: fernandotonon/QtMeshEditor
Length of output: 1156
🏁 Script executed:
# Look for any entity destruction in the codebase that doesn't destroy node
rg -B 3 -A 3 "mSceneMgr->destroyEntity" --type cpp src/ | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if there are actual cases where entities are destroyed without node destruction
rg -B 5 "destroyEntity" --type cpp src/ | grep -A 5 "destroyEntity" | head -60Repository: fernandotonon/QtMeshEditor
Length of output: 2382
🏁 Script executed:
# Check how PrimitiveObject destroys entities - this seems to be the main creation path
cat -n src/PrimitiveObject.cpp | sed -n '450,465p'Repository: fernandotonon/QtMeshEditor
Length of output: 820
🏁 Script executed:
# Check how Manager::createEntity works and how it registers signal
rg -n "createEntity|entityCreated" --type cpp src/Manager.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 270
🏁 Script executed:
# Verify the concern is real by looking at test patterns more carefully
cat -n src/NormalVisualizer_test.cpp | sed -n '130,145p'Repository: fernandotonon/QtMeshEditor
Length of output: 778
🏁 Script executed:
# Look at where destroyOverlayForEntity is called to understand cleanup paths
rg -n "destroyOverlayForEntity" --type cpp src/Repository: fernandotonon/QtMeshEditor
Length of output: 318
🏁 Script executed:
# Check if there are any other paths that remove entities from scene without cleanup
rg -n "detachObject|detachAllObjects" --type cpp src/NormalVisualizer.cppRepository: fernandotonon/QtMeshEditor
Length of output: 243
🏁 Script executed:
# Let me check if getParentSceneNode() is the right method to check validity
rg -n "getParentSceneNode" --type cpp src/ | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 1023
🏁 Script executed:
# Check if entity pointers can actually become invalid between addition and use
cat -n src/NormalVisualizer.cpp | sed -n '99,145p'Repository: fernandotonon/QtMeshEditor
Length of output: 2098
Guard entity pointers in destroyAllOverlays() before calling removeSoftwareAnimationRequest().
Entities in mSoftwareAnimRequested can become invalid if destroyed directly (e.g., via PrimitiveObject::updatePrimitive()) without triggering onSceneNodeDestroyed(). The code already acknowledges this risk at lines 112–114. Add a validity check before accessing the entity:
Suggested fix
- for (Ogre::Entity* entity : mSoftwareAnimRequested)
- entity->removeSoftwareAnimationRequest(true);
+ for (Ogre::Entity* entity : mSoftwareAnimRequested)
+ {
+ if (entity && entity->getParentSceneNode())
+ entity->removeSoftwareAnimationRequest(true);
+ }
mSoftwareAnimRequested.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NormalVisualizer.cpp` around lines 375 - 389, In destroyAllOverlays(),
guard entries in mSoftwareAnimRequested before calling
removeSoftwareAnimationRequest: iterate the container and for each Ogre::Entity*
(from mSoftwareAnimRequested) check the pointer is non-null (and optionally
verify it's still attached via entity->getParentSceneNode() or similar) before
calling entity->removeSoftwareAnimationRequest(true); this prevents
dereferencing invalid/deleted Entity pointers that may have been destroyed
(e.g., by PrimitiveObject::updatePrimitive) without onSceneNodeDestroyed() being
invoked; update the loop in destroyAllOverlays() to skip null/invalid entities
and then clear mSoftwareAnimRequested as before.
|
1 participant
Summary
NormalVisualizerclass that draws colored lines from each vertex along its normal direction, color-coded by axis (|X|→Red, |Y|→Green, |Z|→Blue)addSoftwareAnimationRequest(true)and per-frame ManualObject updatestoggle_normalstool for programmatic control (global toggle)static_cast<Entity*>inObjectItemModelandManager::getEntities()Test plan
toggle_normalstool → normals toggle on/offNormalVisualizerIntegrationTestandMCPServerTest.ToggleNormals*suites)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores