Phase 3: Edit Mode — Vertex Editing (#258) - #281
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full Edit Mode: QML header and tools bound to a new EditModeController singleton, a CPU-editable mesh model (EditableMesh), selection/hit-testing/soft-selection/transform logic with overlays, TransformOperator integration and undo command, tests, and build/CI updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QML as PropertiesPanel.qml
participant Ctrl as EditModeController
participant Mesh as EditableMesh
participant Ogre as Ogre::Entity
participant Overlay as SelectionOverlay
User->>QML: Click "Edit" toggle
QML->>Ctrl: toggleEditMode()
Ctrl->>Mesh: loadFromEntity(selectedEntity)
Mesh->>Ogre: Read vertex/index buffers
Mesh-->>Ctrl: Editable data ready
Ctrl->>Overlay: create/update overlays
Ctrl-->>QML: emit editModeChanged / meshDataChanged
QML-->>User: show Edit Mode Tools and counters
sequenceDiagram
participant User
participant Transform as TransformOperator
participant Ctrl as EditModeController
participant Mesh as EditableMesh
participant Undo as UndoManager
User->>Transform: Drag gizmo (vertices selected)
Transform->>Ctrl: snapshotVertexPositions()
Ctrl-->>Transform: startPositions
Transform->>Ctrl: translateSelectedVertices(delta)
Ctrl->>Mesh: setVertexPosition(...) (apply soft weights)
Mesh-->>Ctrl: positions updated
User->>Transform: Release mouse
Transform->>Ctrl: snapshotVertexPositions() -> endPositions
Transform->>Undo: push EditVertexTransformCommand(startPositions,endPositions)
Ctrl->>Mesh: validateMesh() / recalc normals
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
![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: f7b92068da
ℹ️ 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
+1011
to
+1012
| // Re-snapshot so incremental deltas accumulate correctly | ||
| mEditModeStartPositions = editCtrl->snapshotVertexPositions(); |
There was a problem hiding this comment.
The edit-mode drag code overwrites mEditModeStartPositions on every mouse-move, so by mouse release the stored "old" snapshot has already become the current state. That makes the change detection and EditVertexTransformCommand(old,new) creation effectively compare final-to-final, so vertex transform undo is skipped or becomes a no-op after normal drags.
Useful? React with 👍 / 👎.
Comment on lines
+110
to
+111
| // Recalculate normals before writing back | ||
| recalculateNormals(); |
There was a problem hiding this comment.
commitToEntity always calls smooth recalculateNormals() before writing buffers, which overrides any flat normals computed earlier (e.g., via recalculateNormals(false) or flat mode transforms). In practice, choosing flat normals does not persist because every commit forces smooth normals again.
Useful? React with 👍 / 👎.
Comment on lines
+121
to
+124
| if (subMesh->useSharedVertices) { | ||
| if (!wroteShared && mesh->sharedVertexData) { | ||
| writeVertexData(mesh->sharedVertexData, editSub.vertices); | ||
| wroteShared = true; |
There was a problem hiding this comment.
For meshes where multiple submeshes use shared vertex data, only the first shared submesh copy is written back (!wroteShared). Since edit mode stores a separate editable vertex array per submesh, edits made through geometry in later shared submeshes can be silently dropped on commit/exit.
Useful? React with 👍 / 👎.
Comment on lines
+117
to
+121
| for (unsigned short i = 0; i < mesh->getNumSubMeshes(); ++i) { | ||
| Ogre::SubMesh* subMesh = mesh->getSubMesh(i); | ||
| const EditableSubMesh& editSub = m_subMeshes[i]; | ||
|
|
||
| if (subMesh->useSharedVertices) { |
There was a problem hiding this comment.
Topology operations like removeDegenerateTriangles() mutate editSub.triangles, but commitToEntity only writes vertex buffers and never updates indexData buffers/counts. This means triangle removals are not persisted to the Ogre mesh after commit, so the UI action appears to succeed only in the temporary editable copy.
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mainwindow.cpp (1)
913-915:⚠️ Potential issue | 🟠 MajorMove QtInputManager dispatch after edit-mode shortcuts so high-priority shortcuts are consumed first.
Line 916 dispatches the key to
QtInputManagerbefore the edit-mode andTabhandlers run. This means1/2/3,Ctrl+A,Alt+A, andTabcan reach registered listeners even though they are later accepted by the shortcut handlers. Reorder to check these shortcuts first, accept and return if matched, then dispatch toQtInputManageronly if none of the high-priority shortcuts handled the event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 913 - 915, Move the QtInputManager dispatch in MainWindow::keyPressEvent so edit-mode and Tab shortcut handling runs first: call the existing edit-mode shortcut handlers and the Tab key handler at the top of MainWindow::keyPressEvent, and if any of those handlers accept() the QKeyEvent (or otherwise indicate it was handled) immediately return without calling QtInputManager::getInstance().keyPressEvent(event); only dispatch to QtInputManager when none of the high-priority shortcuts matched/accepted the event.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 262-278: Replace the custom Rectangle+MouseArea widgets used for
selection mode, falloff and normals toggles with focusable QtQuick.Controls (use
RadioButton inside a ButtonGroup for exclusive selection of
EditModeController.selectionMode; use Button for standalone actions) so they
gain keyboard focus, tab navigation and proper semantics; specifically, swap the
Rectangle/MousArea constructs that compare modelData.mode to
EditModeController.selectionMode (occurring where modelData.label/modelData.mode
are used in the blocks at the original diff and the other blocks at lines
349–364 and 389–445) for RadioButton/Buttons, bind the RadioButton.checked to
(EditModeController.selectionMode === modelData.mode) and set onClicked to
assign EditModeController.selectionMode = modelData.mode (or call the existing
action handlers for falloff/normals), and apply the existing color/theme values
from PropertiesPanelController (highlightColor, buttonColor, borderColor,
textColor) to the control's background/indicator and font to preserve styling.
In `@src/commands/TransformCommands.cpp`:
- Around line 539-575: EditVertexTransformCommand currently looks up
EditModeController::instance()->currentMesh() inside undo()/redo(), causing
no-ops or wrong-target replays if edit mode or currentMesh changes; change the
constructor to capture a stable handle to the edited mesh/entity (e.g. a pointer
or unique ID) alongside mOldPositions/mNewPositions, store it as a member (e.g.
mTargetMesh or mTargetMeshId), and then in undo() and redo() use that stored
handle instead of calling currentMesh(); also ensure you validate the stored
handle (exists and matches expected mesh) before calling restoreVertexPositions,
recalculateNormals (using normalsMode()), and validateMesh so the command safely
no-ops if the original target was deleted.
In `@src/EditableMesh.cpp`:
- Around line 117-135: removeDegenerateTriangles only updates CPU-side
m_subMeshes[].triangles but the Ogre SubMesh index buffers remain unchanged, so
after committing you must write the updated index lists back into the mesh and
update index counts; after writing vertex data in the loop over
mesh->getNumSubMeshes() (and similarly in the other commit path around the
290-317 region) call a writeIndexData(mesh, subMesh, editSub.triangles) (or
equivalent) and set subMesh->indexData->indexCount (and
mesh->sharedVertexData/indexData if using shared indices) to
editSub.triangles.size() so the GPU uses the new topology before calling
SubMeshTransform::recalculateMeshBounds.
- Around line 48-67: In loadFromEntity(), avoid copying mesh->sharedVertexData
into multiple EditableSubMesh instances; instead create one canonical shared
vertex vector (e.g., sharedVertices) and have every EditableSubMesh that uses
shared vertices reference that single store (or store an index/pointer to it) so
edits are unified; then update commitToEntity() to write that canonical
sharedVertices back to Ogre::Mesh::sharedVertexData (or perform a deterministic
merge/sync of all EditableSubMesh copies into the canonical shared store before
writing) so later edits are not dropped—look for functions/classes
EditableSubMesh, loadFromEntity(), commitToEntity(), mesh->sharedVertexData, and
sharedVertices to implement this change.
- Around line 110-111: commitToEntity() currently always calls
recalculateNormals(), overwriting flat-mode or caller-provided normals; change
it to only recalculate when appropriate by adding/using a boolean condition
(e.g., a member flag or parameter like normalsDirty or shouldRecalculateNormals)
before calling recalculateNormals(). Locate commitToEntity() and remove the
unconditional recalculateNormals() call, instead check the flag or a mode
(consistent with EditModeController::recalculateNormals(false) and the flat
branch used in the vertex transform path) and call recalculateNormals() only
when smooth normals are desired or when normals are actually dirty. Ensure
callers that expect their supplied normals preserved do not set the flag.
- Around line 340-389: readVertexData/readIndexData currently ignore
vertexData->vertexStart and indexData->indexStart causing corrupt reads for
buffer sub-ranges; update readVertexData to add vertexData->vertexStart to the
per-vertex pointer arithmetic (use base + (j + vertexData->vertexStart) *
vbuf->getVertexSize() for position/normal/uv reads) and update readIndexData to
add indexData->indexStart to index pointer calculations (apply indexStart when
computing the starting index into the index buffer); do the same in
writeVertexData by adding vertexData->vertexStart to all attribute write loops
so all base + j*getVertexSize() calculations become base + (j +
vertexData->vertexStart) * getVertexSize(), and ensure index writes use
indexData->indexStart.
In `@src/EditModeController.cpp`:
- Around line 311-320: selectVertex currently returns before clearing selection
when given an invalid index, so calling selectVertex(invalid, false) leaves the
previous selection intact; to fix it, ensure non-additive calls clear selection
even if index validation fails by moving or duplicating the
m_selectedVertices.clear() so that when addToSelection is false the selection is
cleared before any early returns (adjust EditModeController::selectVertex to
clear m_selectedVertices prior to checking
m_editModeActive/m_editableMesh/totalVertexCount or explicitly clear right after
those checks if addToSelection==false), referencing
EditModeController::selectVertex, m_selectedVertices, addToSelection, and
m_editableMesh->totalVertexCount().
- Around line 727-762: The box-select path always adds vertices because
boxSelectVertices only supports add/replace via its addToSelection bool and
handleBoxSelect always calls it; fix by making the box-select flow branch on
m_selectionMode and propagate a tri-state selection intent (replace/add/remove)
from handleBoxSelect through to the selection functions (e.g.,
boxSelectVertices, and analogous boxSelectEdges/boxSelectFaces), not just
shiftHeld; implement removal by erasing indices from
m_selectedVertices/m_selectedEdges/m_selectedFaces when the intent is "remove"
and replace by clearing before adding when intent is "replace"; ensure
handleBoxSelect uses the removal intent computed in
TransformOperator::mouseReleaseEvent() and that updateSelectionOverlay() and
emit editSelectionChanged() are called after the appropriate set modifications.
In `@src/TransformOperator.cpp`:
- Around line 873-875: At drag-start, capture two separate snapshots: keep the
existing mEditModeStartPositions as the rolling baseline used by
restoreVertexPositions(), but create a new immutable snapshot (e.g.,
mGestureStartPositions) to preserve the original pre-drag state for the undo
command; do not overwrite mGestureStartPositions on mouse-move—only update
mEditModeStartPositions for incremental moves and set mEditModeTransformActive =
true as before, and when building EditVertexTransformCommand on mouse-release
use mGestureStartPositions (not mEditModeStartPositions) to compute
newPositions/changed; apply the same separation fix in the other occurrences you
noted (around the blocks at 1010-1013, 1049-1051, 1069-1070, 1352-1370) so the
undo command always gets the immutable gesture-start snapshot.
---
Outside diff comments:
In `@src/mainwindow.cpp`:
- Around line 913-915: Move the QtInputManager dispatch in
MainWindow::keyPressEvent so edit-mode and Tab shortcut handling runs first:
call the existing edit-mode shortcut handlers and the Tab key handler at the top
of MainWindow::keyPressEvent, and if any of those handlers accept() the
QKeyEvent (or otherwise indicate it was handled) immediately return without
calling QtInputManager::getInstance().keyPressEvent(event); only dispatch to
QtInputManager when none of the high-priority shortcuts matched/accepted the
event.
🪄 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: 4675b562-fcb2-4f50-afc4-0918d4add72c
📒 Files selected for processing (15)
qml/PropertiesPanel.qmlsrc/CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/EditableMesh.cppsrc/EditableMesh.hsrc/EditableMesh_test.cppsrc/TransformOperator.cppsrc/TransformOperator.hsrc/commands/TransformCommands.cppsrc/commands/TransformCommands.hsrc/mainwindow.cppsrc/mainwindow.htests/CMakeLists.txt
Comment on lines
+262
to
+278
| Rectangle { | ||
| width: 52; height: 22; radius: 3 | ||
| color: EditModeController.selectionMode === modelData.mode | ||
| ? PropertiesPanelController.highlightColor | ||
| : PropertiesPanelController.buttonColor | ||
| border.color: PropertiesPanelController.borderColor; border.width: 1 | ||
|
|
||
| Text { | ||
| anchors.centerIn: parent | ||
| text: modelData.label | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 10 | ||
| } | ||
| MouseArea { | ||
| anchors.fill: parent | ||
| onClicked: EditModeController.selectionMode = modelData.mode | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the PropertiesPanel.qml file
fd -t f "PropertiesPanel.qml"Repository: fernandotonon/QtMeshEditor
Length of output: 152
🏁 Script executed:
# Check the file size to determine how to read it
wc -l qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 98
🏁 Script executed:
# Read the specific line ranges mentioned in the review
sed -n '262,278p; 349,364p; 389,445p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 5407
🏁 Script executed:
# Look at the beginning of the file to understand imports and available controls
head -50 qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 1748
🏁 Script executed:
# Read lines 262-278 from PropertiesPanel.qml
echo "=== Lines 262-278 ===" && sed -n '262,278p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 1093
🏁 Script executed:
# Read lines 349-364 from PropertiesPanel.qml
echo "=== Lines 349-364 ===" && sed -n '349,364p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 1132
🏁 Script executed:
# Read lines 389-445 from PropertiesPanel.qml
echo "=== Lines 389-445 ===" && sed -n '389,445p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 3386
🏁 Script executed:
# Search for Button usage in PropertiesPanel.qml
rg -n "Button|RadioButton|ToolButton" qml/PropertiesPanel.qml | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 101
🏁 Script executed:
# Check how Button is used in the existing code at line 53
sed -n '45,65p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 1139
🏁 Script executed:
# Search for RadioButton usage to understand if it's used elsewhere
rg -n "RadioButton" qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Look at control styling - check if there's a control palette or style setup
rg -n "palette|style|Material\." qml/PropertiesPanel.qml | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Get more context around the Button usage to understand styling
sed -n '1,80p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 3422
🏁 Script executed:
# Check if there are any custom style files for controls
fd -t f "\.(qml|qss)" | rg -i "style|control" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 99
🏁 Script executed:
# Look for any other interactive controls in the file to understand the pattern
rg -n "CheckBox|Switch|ComboBox|RadioButton" qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 132
🏁 Script executed:
# Check the context around lines 262-278 to see if it's in a repeater/loop
sed -n '245,280p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 1795
🏁 Script executed:
# Check context around lines 349-364
sed -n '330,370p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 2033
🏁 Script executed:
# Check context around lines 389-445
sed -n '370,420p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 2748
🏁 Script executed:
# See how the existing CheckBox at line 298 is styled
sed -n '290,310p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 1050
🏁 Script executed:
# Let me check if RadioButton would be appropriate - see if there are any similar exclusive-choice patterns already using RadioButton elsewhere
rg -n "RadioButton|ExclusiveGroup" qml/ -A 2 -B 2Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check how the existing Button at line 53 applies styling/colors
sed -n '50,70p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 1101
🏁 Script executed:
# Check if there's any button styling setup for this panel
sed -n '1,100p' qml/PropertiesPanel.qml | rg -i "style|palette|button"Repository: fernandotonon/QtMeshEditor
Length of output: 98
🏁 Script executed:
# Look for any custom control definitions that might handle focus + custom styling
fd -t f "*.qml" qml/ | xargs rg -l "focusPolicy|focus: true|Keys.onPressed" | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 479
🏁 Script executed:
# Check what control palette is available
rg -n "palette\." qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check if RadioButton can work in a Repeater context - look for examples
rg -n "Repeater" qml/PropertiesPanel.qml -A 10 | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 1908
🏁 Script executed:
# Look at what the CheckBox implementation shows about custom styling in this file
sed -n '298,325p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 1374
🏁 Script executed:
# Verify the accessibility concern is valid - check Qt documentation references or comments about focus
rg -n "focus|accessibility|keyboard" qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 52
Replace these custom Rectangle+MouseArea controls with focusable Qt Quick Controls.
Lines 262–278, 349–364, and 389–445 implement selection mode, falloff, and normals selection as Rectangle + MouseArea widgets. These lack keyboard focus, tab navigation, and button semantics, making these core edit-mode operations keyboard-inaccessible. The file already imports QtQuick.Controls 2.15 and uses Button, CheckBox, and Slider elsewhere with custom color theming. Use RadioButton (with ButtonGroup for exclusivity) for the mode toggles and Button for action buttons instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/PropertiesPanel.qml` around lines 262 - 278, Replace the custom
Rectangle+MouseArea widgets used for selection mode, falloff and normals toggles
with focusable QtQuick.Controls (use RadioButton inside a ButtonGroup for
exclusive selection of EditModeController.selectionMode; use Button for
standalone actions) so they gain keyboard focus, tab navigation and proper
semantics; specifically, swap the Rectangle/MousArea constructs that compare
modelData.mode to EditModeController.selectionMode (occurring where
modelData.label/modelData.mode are used in the blocks at the original diff and
the other blocks at lines 349–364 and 389–445) for RadioButton/Buttons, bind the
RadioButton.checked to (EditModeController.selectionMode === modelData.mode) and
set onClicked to assign EditModeController.selectionMode = modelData.mode (or
call the existing action handlers for falloff/normals), and apply the existing
color/theme values from PropertiesPanelController (highlightColor, buttonColor,
borderColor, textColor) to the control's background/indicator and font to
preserve styling.
Comment on lines
+539
to
+575
| EditVertexTransformCommand::EditVertexTransformCommand( | ||
| const std::map<int, Ogre::Vector3>& oldPositions, | ||
| const std::map<int, Ogre::Vector3>& newPositions, | ||
| const QString& description, | ||
| QUndoCommand* parent) | ||
| : QUndoCommand(description, parent) | ||
| , mOldPositions(oldPositions) | ||
| , mNewPositions(newPositions) | ||
| , mFirstRedo(true) | ||
| { | ||
| } | ||
|
|
||
| void EditVertexTransformCommand::undo() | ||
| { | ||
| auto* ctrl = EditModeController::instance(); | ||
| if (!ctrl->isEditModeActive() || !ctrl->currentMesh()) | ||
| return; | ||
|
|
||
| ctrl->restoreVertexPositions(mOldPositions); | ||
| ctrl->recalculateNormals(ctrl->normalsMode() == 0); | ||
| ctrl->validateMesh(); | ||
| } | ||
|
|
||
| void EditVertexTransformCommand::redo() | ||
| { | ||
| if (mFirstRedo) { | ||
| mFirstRedo = false; | ||
| return; | ||
| } | ||
|
|
||
| auto* ctrl = EditModeController::instance(); | ||
| if (!ctrl->isEditModeActive() || !ctrl->currentMesh()) | ||
| return; | ||
|
|
||
| ctrl->restoreVertexPositions(mNewPositions); | ||
| ctrl->recalculateNormals(ctrl->normalsMode() == 0); | ||
| ctrl->validateMesh(); |
There was a problem hiding this comment.
Bind this undo command to the edited mesh, not the controller’s current one.
Lines 554 and 570 resolve currentMesh() at execution time, and Lines 554-555 / 570-571 bail out entirely when edit mode is inactive. After the user exits edit mode or moves to a different mesh, Ctrl+Z becomes a silent no-op or replays this snapshot into the wrong target. This command needs a stable mesh/entity handle captured at creation time instead of depending on EditModeController's mutable global state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/TransformCommands.cpp` around lines 539 - 575,
EditVertexTransformCommand currently looks up
EditModeController::instance()->currentMesh() inside undo()/redo(), causing
no-ops or wrong-target replays if edit mode or currentMesh changes; change the
constructor to capture a stable handle to the edited mesh/entity (e.g. a pointer
or unique ID) alongside mOldPositions/mNewPositions, store it as a member (e.g.
mTargetMesh or mTargetMeshId), and then in undo() and redo() use that stored
handle instead of calling currentMesh(); also ensure you validate the stored
handle (exists and matches expected mesh) before calling restoreVertexPositions,
recalculateNormals (using normalsMode()), and validateMesh so the command safely
no-ops if the original target was deleted.
Comment on lines
+48
to
+67
| // Read shared vertex data once if any submesh uses it | ||
| std::vector<EditableVertex> sharedVertices; | ||
| bool hasSharedVertexData = (mesh->sharedVertexData != nullptr); | ||
| if (hasSharedVertexData) { | ||
| readVertexData(mesh->sharedVertexData, sharedVertices); | ||
| } | ||
|
|
||
| for (unsigned short i = 0; i < mesh->getNumSubMeshes(); ++i) { | ||
| Ogre::SubMesh* subMesh = mesh->getSubMesh(i); | ||
| EditableSubMesh editSub; | ||
|
|
||
| editSub.usesSharedVertices = subMesh->useSharedVertices; | ||
| editSub.materialName = subMesh->getMaterialName(); | ||
|
|
||
| // Read vertices | ||
| if (subMesh->useSharedVertices) { | ||
| editSub.vertices = sharedVertices; | ||
| } else { | ||
| readVertexData(subMesh->vertexData, editSub.vertices); | ||
| } |
There was a problem hiding this comment.
Shared-vertex submeshes diverge and later edits are dropped.
loadFromEntity() copies the same mesh->sharedVertexData into every EditableSubMesh, but commitToEntity() only flushes the first shared copy back to Ogre. The same physical vertex is therefore counted and selected multiple times, and any edit made through a later shared submesh is silently lost on commit. This needs one canonical shared-vertex store, or a sync step that reconciles all shared submesh copies before write-back.
Also applies to: 113-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditableMesh.cpp` around lines 48 - 67, In loadFromEntity(), avoid
copying mesh->sharedVertexData into multiple EditableSubMesh instances; instead
create one canonical shared vertex vector (e.g., sharedVertices) and have every
EditableSubMesh that uses shared vertices reference that single store (or store
an index/pointer to it) so edits are unified; then update commitToEntity() to
write that canonical sharedVertices back to Ogre::Mesh::sharedVertexData (or
perform a deterministic merge/sync of all EditableSubMesh copies into the
canonical shared store before writing) so later edits are not dropped—look for
functions/classes EditableSubMesh, loadFromEntity(), commitToEntity(),
mesh->sharedVertexData, and sharedVertices to implement this change.
Comment on lines
+117
to
+135
| for (unsigned short i = 0; i < mesh->getNumSubMeshes(); ++i) { | ||
| Ogre::SubMesh* subMesh = mesh->getSubMesh(i); | ||
| const EditableSubMesh& editSub = m_subMeshes[i]; | ||
|
|
||
| if (subMesh->useSharedVertices) { | ||
| if (!wroteShared && mesh->sharedVertexData) { | ||
| writeVertexData(mesh->sharedVertexData, editSub.vertices); | ||
| wroteShared = true; | ||
| } | ||
| } else { | ||
| if (subMesh->vertexData) { | ||
| writeVertexData(subMesh->vertexData, editSub.vertices); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Recalculate mesh bounds | ||
| SubMeshTransform::recalculateMeshBounds(mesh); | ||
|
|
There was a problem hiding this comment.
Topology edits never reach the Ogre mesh.
removeDegenerateTriangles() only mutates m_subMeshes[].triangles, but this commit path writes vertex buffers only. After the CPU-side fix, subMesh->indexData still contains the original triangle list, so the rendered mesh keeps the degenerates after commit/exit. A writeIndexData() path and indexCount update are required here.
Also applies to: 290-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditableMesh.cpp` around lines 117 - 135, removeDegenerateTriangles only
updates CPU-side m_subMeshes[].triangles but the Ogre SubMesh index buffers
remain unchanged, so after committing you must write the updated index lists
back into the mesh and update index counts; after writing vertex data in the
loop over mesh->getNumSubMeshes() (and similarly in the other commit path around
the 290-317 region) call a writeIndexData(mesh, subMesh, editSub.triangles) (or
equivalent) and set subMesh->indexData->indexCount (and
mesh->sharedVertexData/indexData if using shared indices) to
editSub.triangles.size() so the GPU uses the new topology before calling
SubMeshTransform::recalculateMeshBounds.
Comment on lines
+340
to
+389
| bool EditableMesh::readVertexData(Ogre::VertexData* vertexData, std::vector<EditableVertex>& vertices) | ||
| { | ||
| if (!vertexData || vertexData->vertexCount == 0) | ||
| return false; | ||
|
|
||
| vertices.resize(vertexData->vertexCount); | ||
|
|
||
| auto* decl = vertexData->vertexDeclaration; | ||
| auto* binding = vertexData->vertexBufferBinding; | ||
|
|
||
| // Read positions | ||
| const auto* posElem = decl->findElementBySemantic(Ogre::VES_POSITION); | ||
| if (posElem) { | ||
| auto vbuf = binding->getBuffer(posElem->getSource()); | ||
| auto* base = static_cast<unsigned char*>(vbuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| for (size_t j = 0; j < vertexData->vertexCount; ++j) { | ||
| Ogre::Real* pReal; | ||
| posElem->baseVertexPointerToElement(base + j * vbuf->getVertexSize(), &pReal); | ||
| vertices[j].position = Ogre::Vector3(pReal[0], pReal[1], pReal[2]); | ||
| } | ||
| vbuf->unlock(); | ||
| } | ||
|
|
||
| // Read normals | ||
| const auto* normElem = decl->findElementBySemantic(Ogre::VES_NORMAL); | ||
| if (normElem) { | ||
| auto vbuf = binding->getBuffer(normElem->getSource()); | ||
| auto* base = static_cast<unsigned char*>(vbuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| for (size_t j = 0; j < vertexData->vertexCount; ++j) { | ||
| Ogre::Real* pReal; | ||
| normElem->baseVertexPointerToElement(base + j * vbuf->getVertexSize(), &pReal); | ||
| vertices[j].normal = Ogre::Vector3(pReal[0], pReal[1], pReal[2]); | ||
| vertices[j].hasNormal = true; | ||
| } | ||
| vbuf->unlock(); | ||
| } | ||
|
|
||
| // Read UVs | ||
| const auto* uvElem = decl->findElementBySemantic(Ogre::VES_TEXTURE_COORDINATES); | ||
| if (uvElem) { | ||
| auto vbuf = binding->getBuffer(uvElem->getSource()); | ||
| auto* base = static_cast<unsigned char*>(vbuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| for (size_t j = 0; j < vertexData->vertexCount; ++j) { | ||
| Ogre::Real* pReal; | ||
| uvElem->baseVertexPointerToElement(base + j * vbuf->getVertexSize(), &pReal); | ||
| vertices[j].uv = Ogre::Vector2(pReal[0], pReal[1]); | ||
| vertices[j].hasUV = true; | ||
| } | ||
| vbuf->unlock(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "EditableMesh.cpp" -type fRepository: fernandotonon/QtMeshEditor
Length of output: 92
🏁 Script executed:
cat -n src/EditableMesh.cpp | head -400Repository: fernandotonon/QtMeshEditor
Length of output: 17334
🏁 Script executed:
cat -n src/EditableMesh.cpp | sed -n '390,570p'Repository: fernandotonon/QtMeshEditor
Length of output: 8367
🏁 Script executed:
rg -i "vertexStart|vertexdata.*offset" --max-count=20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
rg "VertexData|IndexData" -A 3 -B 1 src/EditableMesh.cpp | head -60Repository: fernandotonon/QtMeshEditor
Length of output: 1597
🏁 Script executed:
find . -name "*.mesh" -o -name "*.material" | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 233
🏁 Script executed:
rg "shared.*buffer|buffer.*sub|vertexStart|indexStart" src/ --type cppRepository: fernandotonon/QtMeshEditor
Length of output: 2327
🏁 Script executed:
sed -n '1,100p' src/MeshImporterExporter.cpp | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 1959
🏁 Script executed:
rg "indexStart|vertexStart" src/MeshImporterExporter.cpp -B 3 -A 5Repository: fernandotonon/QtMeshEditor
Length of output: 1788
🏁 Script executed:
rg "indexStart|vertexStart" src/MeshValidator.cpp -B 2 -A 4Repository: fernandotonon/QtMeshEditor
Length of output: 682
🏁 Script executed:
rg "vertexStart" src/ -B 3 -A 3 --type cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
rg -i "ogre.*vertexdata.*vertexstart" -A 5 --type cppRepository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
cat -n src/OgreXML/OgreXMLMeshSerializer.cpp | grep -A 5 -B 5 "indexStart"Repository: fernandotonon/QtMeshEditor
Length of output: 1327
🏁 Script executed:
python3 << 'EOF'
# Summary of findings:
files_with_proper_offset_handling = [
("OgreXMLMeshSerializer.cpp", "lines 1401, 1407", "pInt += facedata->indexStart; pShort += facedata->indexStart;"),
("MeshImporterExporter.cpp", "multiple locations", "indexStart + f * 3 + v"),
("MeshValidator.cpp", "validation loop", "id->indexStart + ti")
]
files_with_missing_offset = [
("EditableMesh.cpp", "readIndexData", "lines 429, 434", "reinterpret_cast<uint32_t*>(data) + t * 3 [missing indexStart]"),
("EditableMesh.cpp", "readVertexData", "lines 357, 370, 384, 398", "base + j * vbuf->getVertexSize() [missing vertexStart]")
]
print("=" * 80)
print("OFFSET HANDLING ANALYSIS")
print("=" * 80)
print("\nFiles WITH proper vertexStart/indexStart offset handling:")
for f, loc, code in files_with_proper_offset_handling:
print(f" ✓ {f} ({loc}): {code}")
print("\nFiles WITHOUT proper offset handling (BUG):")
for f, func, lines, issue in files_with_missing_offset:
print(f" ✗ {f} - {func} ({lines}): {issue}")
print("\n" + "=" * 80)
print("CONCLUSION: The review comment is CORRECT")
print("=" * 80)
print("\nEditableMesh::readIndexData and readVertexData ignore vertexStart/indexStart")
print("while other parts of the codebase (MeshImporterExporter, OgreXMLMeshSerializer,")
print("MeshValidator) correctly apply these offsets. This is a genuine major issue.")
EOFRepository: fernandotonon/QtMeshEditor
Length of output: 1286
Buffer sub-ranges are ignored during mesh I/O—vertexStart and indexStart offsets must be applied.
The readVertexData and readIndexData methods read from element 0 of buffers, ignoring Ogre::VertexData::vertexStart and Ogre::IndexData::indexStart offsets. This corrupts mesh data for any submesh using a buffer sub-range. Other parts of the codebase (OgreXMLMeshSerializer, MeshImporterExporter, MeshValidator) correctly apply these offsets, establishing the expected pattern.
Apply offsets in all vertex/index read and write loops:
- In
readIndexData: AddindexData->indexStartto index pointer calculations (lines 429, 434) - In
readVertexData: AddvertexData->vertexStartto vertex offset calculations (lines 357, 370, 384, 398) - In
writeVertexData: Apply samevertexStartoffset to all attribute write loops (410–442, 453–566)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditableMesh.cpp` around lines 340 - 389, readVertexData/readIndexData
currently ignore vertexData->vertexStart and indexData->indexStart causing
corrupt reads for buffer sub-ranges; update readVertexData to add
vertexData->vertexStart to the per-vertex pointer arithmetic (use base + (j +
vertexData->vertexStart) * vbuf->getVertexSize() for position/normal/uv reads)
and update readIndexData to add indexData->indexStart to index pointer
calculations (apply indexStart when computing the starting index into the index
buffer); do the same in writeVertexData by adding vertexData->vertexStart to all
attribute write loops so all base + j*getVertexSize() calculations become base +
(j + vertexData->vertexStart) * getVertexSize(), and ensure index writes use
indexData->indexStart.
Comment on lines
+727
to
+762
| void EditModeController::boxSelectVertices(const QRect& rect, | ||
| Ogre::Camera* camera, | ||
| int viewportWidth, int viewportHeight, | ||
| bool addToSelection) | ||
| { | ||
| if (!m_editableMesh || !m_editEntity || !camera) | ||
| return; | ||
|
|
||
| Ogre::SceneNode* node = m_editEntity->getParentSceneNode(); | ||
| if (!node) | ||
| return; | ||
|
|
||
| if (!addToSelection) | ||
| m_selectedVertices.clear(); | ||
|
|
||
| int globalOffset = 0; | ||
| for (size_t si = 0; si < m_editableMesh->subMeshes().size(); ++si) { | ||
| const auto& sub = m_editableMesh->subMeshes()[si]; | ||
| for (size_t vi = 0; vi < sub.vertices.size(); ++vi) { | ||
| Ogre::Vector3 worldPos = node->convertLocalToWorldPosition(sub.vertices[vi].position); | ||
|
|
||
| // Skip vertices behind camera | ||
| Ogre::Vector3 camToVert = worldPos - camera->getDerivedPosition(); | ||
| if (camToVert.dotProduct(camera->getDerivedDirection()) < 0) | ||
| continue; | ||
|
|
||
| QPoint sp = worldToScreen(worldPos, camera, viewportWidth, viewportHeight); | ||
| if (rect.contains(sp)) | ||
| m_selectedVertices.insert(globalOffset + static_cast<int>(vi)); | ||
| } | ||
| globalOffset += static_cast<int>(sub.vertices.size()); | ||
| } | ||
|
|
||
| updateSelectionOverlay(); | ||
| emit editSelectionChanged(); | ||
| } |
There was a problem hiding this comment.
Box selection only ever adds vertices.
handleBoxSelect() unconditionally routes to boxSelectVertices() and only passes shiftHeld as addToSelection. In EdgeMode/FaceMode it still edits m_selectedVertices, and the Ctrl-removal intent computed in TransformOperator::mouseReleaseEvent() never reaches this API. The selection op needs to branch on m_selectionMode and carry replace/add/remove semantics through the call.
Also applies to: 838-865
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 727 - 762, The box-select path
always adds vertices because boxSelectVertices only supports add/replace via its
addToSelection bool and handleBoxSelect always calls it; fix by making the
box-select flow branch on m_selectionMode and propagate a tri-state selection
intent (replace/add/remove) from handleBoxSelect through to the selection
functions (e.g., boxSelectVertices, and analogous
boxSelectEdges/boxSelectFaces), not just shiftHeld; implement removal by erasing
indices from m_selectedVertices/m_selectedEdges/m_selectedFaces when the intent
is "remove" and replace by clearing before adding when intent is "replace";
ensure handleBoxSelect uses the removal intent computed in
TransformOperator::mouseReleaseEvent() and that updateSelectionOverlay() and
emit editSelectionChanged() are called after the appropriate set modifications.
…, items 1-2 Object Mode / Edit Mode toggle (Tab key) with editable mesh data structure. EditableMesh: - Indexed mesh representation with vertices (position, normal, UV, color, bone weights), triangles, and per-submesh data - loadFromEntity(): reads Ogre vertex/index buffers into editable format - commitToEntity(): writes back with static-to-dynamic buffer upgrade - recalculateNormals(): area-weighted smooth normals - Handles shared vertex data and multi-submesh entities EditModeController: - QML_SINGLETON managing Object/Edit Mode state - Tab key toggle, Enter/Exit with commit or discard - Auto-exits when selection changes away from edited entity - QML properties: editModeActive, modeLabel, vertex/tri/submesh counts UI: - Tab key in mainwindow.cpp keyPressEvent - Status bar mode label (green highlight in edit mode) - Inspector panel mode indicator with stats and Edit/Exit button - 18 unit tests Part of #258 (Phase 3: Edit Mode — Vertex Editing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three selection modes (1/2/3 keys) with hit testing and visual overlays. Selection: - Vertex: click nearest (screen-space projection, 10px radius), box select - Edge: click nearest edge (point-to-segment distance), selects endpoints - Face: click triangle (Moller-Trumbore ray intersection), selects vertices - Shift+click add, Ctrl+click remove, Ctrl+A select all, Alt+A deselect Overlays (ManualObject on child scene node): - Vertices: orange points (5px) - Edges: cyan lines - Faces: semi-transparent blue triangles TransformOperator delegates to EditModeController in edit mode. 27 unit tests (11 pure geometry + 16 Ogre-dependent). Part of #258 (Phase 3: Edit Mode — Vertex Editing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Vertex Transform: - Gizmo translate/rotate/scale operates on selected vertices in edit mode - Soft selection (proportional editing) with linear/smooth falloff and adjustable radius — nearby vertices are influenced by weight - EditVertexTransformCommand for full undo/redo with position snapshots - TransformOperator routes gizmo drags to EditModeController in edit mode Normals Recalculation: - Smooth normals (area-weighted average) and flat normals (face normal) - Toggle between modes via Inspector buttons - Auto-recalculate after vertex edits and on undo/redo Mesh Validation: - Degenerate triangle detection after vertex edits - Warning display in Inspector with count - "Remove Degenerates" auto-fix button QML Inspector: - Edit Mode Tools section with selection mode buttons, soft selection controls, normals mode, and validation warnings 17 new tests (10 standalone + 7 Ogre-dependent). Part of #258 (Phase 3: Edit Mode — Vertex Editing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests were failing because enterEditMode() requires a selected entity with a mesh. Added entity creation and selection in SetUp(), cleanup in TearDown(). Uses static counter for unique mesh names. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- commitToEntity() now respects m_flatNormals flag instead of always recalculating smooth normals (Critical review) - Added setFlatNormals()/isFlatNormals() to EditableMesh - EditModeControllerSelectionTest fixture now creates and selects a triangle mesh entity in SetUp() so enterEditMode() succeeds Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/phase3-edit-mode
branch
from
April 11, 2026 17:58
f7b9206 to
3a608b3
Compare
commitToEntity() now also writes to SubEntity animation blend buffers (same dual-buffer pattern as SubMeshTransform). Skeletal entities render from these buffers, not the mesh VBO directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
src/EditableMesh.cpp (3)
48-67:⚠️ Potential issue | 🔴 CriticalShared-vertex submeshes still diverge.
loadFromEntity()clonesmesh->sharedVertexDatainto everyEditableSubMesh, butcommitToEntity()writes back only the first shared copy. Edits made through any later shared submesh are silently lost, and shared vertices get double-counted in the editable view.Also applies to: 124-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditableMesh.cpp` around lines 48 - 67, loadFromEntity clones mesh->sharedVertexData into each EditableSubMesh causing divergence and duplicate counts; change the logic so all EditableSubMesh instances that have useSharedVertices=true reference a single shared vertex container instead of copying: keep one sharedVertices (from readVertexData(mesh->sharedVertexData, sharedVertices)) and assign editSub.vertices to reference that shared container (or mark editSub as shared) rather than copying. Then update commitToEntity to detect shared submeshes (use the same marker or check editSub.usesSharedVertices) and write back to mesh->sharedVertexData exactly once using the consolidated shared container, skipping per-submesh writes for those that useSharedVertices.
120-133:⚠️ Potential issue | 🔴 CriticalTopology edits never reach Ogre index buffers.
removeDegenerateTriangles()only changesm_subMeshes[].triangles, whilecommitToEntity()writes vertex buffers only. After commit, Ogre still renders the originalindexData, so the removed degenerates come back visually.Also applies to: 293-318
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditableMesh.cpp` around lines 120 - 133, removeDegenerateTriangles only updates m_subMeshes[].triangles but commitToEntity currently writes only vertex buffers (via writeVertexData) so Ogre still uses old indexData; update commitToEntity to also write index buffers from m_subMeshes[i].triangles into the corresponding Ogre::SubMesh indexData (and the mesh's shared indexData when subMesh->useSharedVertices is true) using the existing index-writing routine (e.g. add or call writeIndexData equivalent), mirroring the wroteShared logic used for vertices; ensure you update both per-submesh indexData and any shared indexData so the removed triangles are reflected in Ogre's buffers.
343-408:⚠️ Potential issue | 🟠 MajorRespect
vertexStartandindexStartfor buffer subranges.All pointer arithmetic here starts from element 0 of the underlying buffer. For meshes using buffer subranges, that reads and writes the wrong vertices/indices and can corrupt unrelated geometry.
Also applies to: 428-442, 448-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditableMesh.cpp` around lines 343 - 408, The code in readVertexData reads from buffer element 0 and ignores vertexData subranges, causing wrong vertices for meshes using vertexStart/indexStart; adjust pointer arithmetic to start at base + (vertexData->vertexStart * vbuf->getVertexSize()) (use vertexData->vertexStart) when calling baseVertexPointerToElement for positions, normals, UVs and colors, and similarly respect indexStart when reading index buffers (use indexData->indexStart and indexBuffer->getType()/getIndexSize() offsets) in the other affected methods; update all places that compute base + j * vbuf->getVertexSize() (and index reads) to include the per-VertexData/IndexData start offset obtained from vertexData->vertexStart / indexData->indexStart and the correct buffer source via binding->getBuffer(...).src/TransformOperator.cpp (1)
873-875:⚠️ Potential issue | 🔴 CriticalKeep an immutable pre-drag snapshot for edit-mode undo.
mEditModeStartPositionsis captured at press, then reused as the rolling restore baseline on every mouse-move. By release it already describes the final state, so the undo comparison can see no delta and skip pushing a command. Preserve a separate gesture-start snapshot for undo, and only refresh the incremental baseline during drag.Also applies to: 1006-1012, 1045-1050, 1066-1070, 1347-1370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator.cpp` around lines 873 - 875, At press, capture and preserve an immutable gesture-start snapshot for undo by introducing a new member (e.g., mEditModeGestureStartPositions) and set it from snapshotVertexPositions(); keep mEditModeGestureStartPositions untouched for the final undo comparison, while continuing to use mEditModeStartPositions as the rolling/incremental baseline (refreshing it on mouse-move) and leaving mEditModeTransformActive logic unchanged; update code sites that currently overwrite mEditModeStartPositions at press (and in the listed ranges) to initialize both mEditModeGestureStartPositions and mEditModeStartPositions but only refresh mEditModeStartPositions during drag, and use mEditModeGestureStartPositions when deciding whether to push an undo command on release.src/EditModeController.h (1)
264-275:⚠️ Potential issue | 🟠 MajorBox-select API cannot represent edge/face selection or removal.
boxSelectVertices(...)plushandleBoxSelect(..., bool shiftHeld)can only express vertex replace/add. There is no way for the implementation to box-select edges/faces or perform Ctrl-remove, so box selection can never match the click-selection semantics in EdgeMode and FaceMode.Also applies to: 302-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.h` around lines 264 - 275, The API currently only supports vertex-only box selection via boxSelectVertices and thus cannot represent edge/face selection or removal operations; change the interface to a generic boxSelect function (or overloads) that accepts a SelectionTarget enum (Vertex, Edge, Face) and a SelectionOperation enum (Replace, Add, Remove) so callers like handleBoxSelect can express Ctrl-remove and edge/face modes; update or add boxSelectEdges/boxSelectFaces or a unified boxSelect(const QRect& rect, Ogre::Camera* camera, int viewportWidth, int viewportHeight, SelectionTarget target, SelectionOperation op) and adjust handleBoxSelect(...) to pass the correct target/op based on current EditMode and modifier keys.src/EditModeController.cpp (1)
311-320:⚠️ Potential issue | 🟠 MajorClear replacement selection before the invalid-index early return.
Line 315 returns before Line 318 clears the set, so
selectVertex(invalid, false)keeps the previous vertex selected instead of replacing with an empty selection.💡 Possible fix
void EditModeController::selectVertex(int globalIndex, bool addToSelection) { if (!m_editModeActive || !m_editableMesh) return; + if (!addToSelection) + m_selectedVertices.clear(); if (globalIndex < 0 || globalIndex >= static_cast<int>(m_editableMesh->totalVertexCount())) return; - - if (!addToSelection) - m_selectedVertices.clear(); m_selectedVertices.insert(globalIndex); updateSelectionOverlay(); emit editSelectionChanged(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 311 - 320, selectVertex currently returns on an invalid globalIndex before clearing replacement selection, so calling selectVertex(invalid, false) leaves previous selection; update EditModeController::selectVertex to clear m_selectedVertices when addToSelection is false before any early return for invalid index (and keep the rest of the validity checks intact) so that an invalid-index replacement call results in an empty selection.
🧹 Nitpick comments (1)
src/EditableMesh_test.cpp (1)
230-256: Add regressions for shared-vertex flat normals and committed topology.These cases only use a single-triangle mesh, so they won't catch the shared-vertex flat-normal failure or the missing index-buffer write-back after
removeDegenerateTriangles(). Add one multi-face shared-edge mesh test that commits, reloads, and asserts both normals and triangle count.As per coding guidelines,
src/**/*_test.cpp: Add Google Test unit tests for new functionality.Also applies to: 440-474, 514-547
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditableMesh_test.cpp` around lines 230 - 256, Add a new Google Test in src/EditableMesh_test.cpp (alongside EditableMeshTest::CommitToEntity) that creates a multi-face shared-edge mesh (not the single-triangle createInMemoryTriangleMesh), loads it via EditableMesh::loadFromEntity, modifies/commits topology so removeDegenerateTriangles() path is exercised, calls EditableMesh::commitToEntity(entity), reloads with a fresh EditableMesh::loadFromEntity and then asserts both that the triangle count (index buffer / getTriangleCount) matches expected and that per-vertex normals for shared-vertex flat faces are preserved/updated (compare normal vectors from getVertexNormal or equivalent) to catch the shared-vertex flat-normal regression and the missing index-buffer write-back after removeDegenerateTriangles().
🤖 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/commands/TransformCommands.h`:
- Around line 213-229: EditVertexTransformCommand currently only stores
mOldPositions/mNewPositions so undo()/redo() that rely on transient edit-mode
selection can apply to the wrong mesh after mode/selection changes; modify the
class to carry a durable target identity (e.g., add a member like QString
mTargetId or an immutable scene GUID parameter) and update the constructor to
accept that identity alongside old/new maps; inside undo() and redo() resolve
the target by that ID (not current edit state), apply the stored vertex maps to
the resolved mesh, and handle a missing target gracefully (no-op with a clear
log/error) so undo/redo always targets the intended entity.
In `@src/EditableMesh.cpp`:
- Around line 238-267: recalculateNormalsFlat currently writes a single normal
into shared vertices so the last triangle wins; fix by using per-corner normals
or splitting vertices before assigning face normals: either (A) change the
triangle representation (e.g., Triangle in EditableMesh) to store per-corner
normal indices or normals and assign faceNormal into those per-corner slots
inside EditableMesh::recalculateNormalsFlat, or (B) duplicate vertices for each
triangle corner (create unique vertices for tri.indices in sub.triangles) before
assigning normals so each corner gets the faceNormal; update downstream code
that expects shared-vertex indices (e.g., commit/mesh upload routines) to use
the new per-corner normals or split vertex list accordingly.
In `@src/EditableMesh.h`:
- Around line 157-163: The implementation of recalculateNormalsFlat() claiming
"flat shading" is incorrect for indexed meshes because it writes one normal per
vertex so shared vertices get overwritten depending on triangle order; fix by
generating true flat normals: either (A) duplicate vertices per triangle (create
a non-indexed vertex/normal array by splitting shared vertices) and assign each
new vertex the triangle face normal, or (B) store per-face normals and change
rendering/path to use per-face (flat) interpolation; update
recalculateNormalsFlat(), the mesh data structures it touches, and any
upload/attribute-binding code to use the new non-indexed vertex buffer or
per-face-normal attribute so adjacent triangles do not overwrite each other.
In `@src/EditModeController.cpp`:
- Around line 127-173: The code never initializes or clears
m_degenerateTriangleCount when entering/exiting edit mode, so stale validation
state can persist; update enterEditMode() to call validateMesh() after
successful load (or explicitly set m_degenerateTriangleCount = 0 before/after
load) and update exitEditMode() to reset m_degenerateTriangleCount (and any
other validation counters) to zero when leaving edit mode; reference the
m_degenerateTriangleCount member, validateMesh(), enterEditMode(), and
exitEditMode() to locate where to add the initialization/reset.
- Around line 355-362: The deselectEdge (and similarly the deselectFace) path
removes entries from m_selectedEdges/m_selectedFaces but does not recompute the
derived m_selectedVertices (and deselectFace also leaves related edges
selected), so vertices remain transformable; modify
EditModeController::deselectEdge and EditModeController::deselectFace to rebuild
m_selectedVertices after removing the edge/face (and in deselectFace also erase
any edges belonging exclusively to that face from m_selectedEdges), then call
updateSelectionOverlay() and emit editSelectionChanged() only after
m_selectedVertices is updated so the UI and transform operations reflect the
true selection.
- Around line 180-183: The code reports "Exited Edit Mode: changes committed"
before checking the result of m_editableMesh->commitToEntity(m_editEntity); fix
by capturing the bool return value from commitToEntity and only call
SentryReporter::addBreadcrumb("edit_mode", "Exited Edit Mode: changes
committed") when it returns true; if it returns false, record an error
breadcrumb (e.g. "Exited Edit Mode: commit failed") and invoke the existing
error-path for teardown (or set an error state/return) so edits are not silently
lost; update the EditModeController block that references commitChanges,
m_editableMesh, and m_editEntity to branch based on the commitToEntity return
value.
- Around line 133-165: Replace the nonstandard "edit_mode" breadcrumb category
with the repo-standard UI action category (e.g. "ui.action") for all
SentryReporter::addBreadcrumb calls in this EditModeController code path: change
the calls that log "Cannot enter Edit Mode: no single entity selected", "Failed
to load mesh data for Edit Mode", and the final "Entered Edit Mode: …" message
to use "ui.action" (update the SentryReporter::addBreadcrumb invocations near
m_editableMesh->loadFromEntity, the selection check that returns false, and the
final entry log so telemetry follows the standard ui.action taxonomy).
In `@src/TransformOperator.cpp`:
- Around line 1505-1519: The box-select branch drops the Control modifier: you
compute both shiftHeld and ctrlHeld but call
EditModeController::instance()->handleBoxSelect(mScreenStart, e->pos(),
m_pActiveWidget, shiftHeld) without forwarding ctrlHeld; update the call (or
handleBoxSelect signature) to accept and pass ctrlHeld as well (e.g., add a ctrl
parameter to handleBoxSelect and propagate ctrlHeld) so box selection receives
both modifiers just like handleMouseClick does.
---
Duplicate comments:
In `@src/EditableMesh.cpp`:
- Around line 48-67: loadFromEntity clones mesh->sharedVertexData into each
EditableSubMesh causing divergence and duplicate counts; change the logic so all
EditableSubMesh instances that have useSharedVertices=true reference a single
shared vertex container instead of copying: keep one sharedVertices (from
readVertexData(mesh->sharedVertexData, sharedVertices)) and assign
editSub.vertices to reference that shared container (or mark editSub as shared)
rather than copying. Then update commitToEntity to detect shared submeshes (use
the same marker or check editSub.usesSharedVertices) and write back to
mesh->sharedVertexData exactly once using the consolidated shared container,
skipping per-submesh writes for those that useSharedVertices.
- Around line 120-133: removeDegenerateTriangles only updates
m_subMeshes[].triangles but commitToEntity currently writes only vertex buffers
(via writeVertexData) so Ogre still uses old indexData; update commitToEntity to
also write index buffers from m_subMeshes[i].triangles into the corresponding
Ogre::SubMesh indexData (and the mesh's shared indexData when
subMesh->useSharedVertices is true) using the existing index-writing routine
(e.g. add or call writeIndexData equivalent), mirroring the wroteShared logic
used for vertices; ensure you update both per-submesh indexData and any shared
indexData so the removed triangles are reflected in Ogre's buffers.
- Around line 343-408: The code in readVertexData reads from buffer element 0
and ignores vertexData subranges, causing wrong vertices for meshes using
vertexStart/indexStart; adjust pointer arithmetic to start at base +
(vertexData->vertexStart * vbuf->getVertexSize()) (use vertexData->vertexStart)
when calling baseVertexPointerToElement for positions, normals, UVs and colors,
and similarly respect indexStart when reading index buffers (use
indexData->indexStart and indexBuffer->getType()/getIndexSize() offsets) in the
other affected methods; update all places that compute base + j *
vbuf->getVertexSize() (and index reads) to include the per-VertexData/IndexData
start offset obtained from vertexData->vertexStart / indexData->indexStart and
the correct buffer source via binding->getBuffer(...).
In `@src/EditModeController.cpp`:
- Around line 311-320: selectVertex currently returns on an invalid globalIndex
before clearing replacement selection, so calling selectVertex(invalid, false)
leaves previous selection; update EditModeController::selectVertex to clear
m_selectedVertices when addToSelection is false before any early return for
invalid index (and keep the rest of the validity checks intact) so that an
invalid-index replacement call results in an empty selection.
In `@src/EditModeController.h`:
- Around line 264-275: The API currently only supports vertex-only box selection
via boxSelectVertices and thus cannot represent edge/face selection or removal
operations; change the interface to a generic boxSelect function (or overloads)
that accepts a SelectionTarget enum (Vertex, Edge, Face) and a
SelectionOperation enum (Replace, Add, Remove) so callers like handleBoxSelect
can express Ctrl-remove and edge/face modes; update or add
boxSelectEdges/boxSelectFaces or a unified boxSelect(const QRect& rect,
Ogre::Camera* camera, int viewportWidth, int viewportHeight, SelectionTarget
target, SelectionOperation op) and adjust handleBoxSelect(...) to pass the
correct target/op based on current EditMode and modifier keys.
In `@src/TransformOperator.cpp`:
- Around line 873-875: At press, capture and preserve an immutable gesture-start
snapshot for undo by introducing a new member (e.g.,
mEditModeGestureStartPositions) and set it from snapshotVertexPositions(); keep
mEditModeGestureStartPositions untouched for the final undo comparison, while
continuing to use mEditModeStartPositions as the rolling/incremental baseline
(refreshing it on mouse-move) and leaving mEditModeTransformActive logic
unchanged; update code sites that currently overwrite mEditModeStartPositions at
press (and in the listed ranges) to initialize both
mEditModeGestureStartPositions and mEditModeStartPositions but only refresh
mEditModeStartPositions during drag, and use mEditModeGestureStartPositions when
deciding whether to push an undo command on release.
---
Nitpick comments:
In `@src/EditableMesh_test.cpp`:
- Around line 230-256: Add a new Google Test in src/EditableMesh_test.cpp
(alongside EditableMeshTest::CommitToEntity) that creates a multi-face
shared-edge mesh (not the single-triangle createInMemoryTriangleMesh), loads it
via EditableMesh::loadFromEntity, modifies/commits topology so
removeDegenerateTriangles() path is exercised, calls
EditableMesh::commitToEntity(entity), reloads with a fresh
EditableMesh::loadFromEntity and then asserts both that the triangle count
(index buffer / getTriangleCount) matches expected and that per-vertex normals
for shared-vertex flat faces are preserved/updated (compare normal vectors from
getVertexNormal or equivalent) to catch the shared-vertex flat-normal regression
and the missing index-buffer write-back after removeDegenerateTriangles().
🪄 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: 635c5e31-c73d-4ed1-8374-117e8e1b5243
📒 Files selected for processing (15)
qml/PropertiesPanel.qmlsrc/CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/EditableMesh.cppsrc/EditableMesh.hsrc/EditableMesh_test.cppsrc/TransformOperator.cppsrc/TransformOperator.hsrc/commands/TransformCommands.cppsrc/commands/TransformCommands.hsrc/mainwindow.cppsrc/mainwindow.htests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- src/CMakeLists.txt
- src/mainwindow.h
- src/commands/TransformCommands.cpp
- src/mainwindow.cpp
Comment on lines
+213
to
+229
| // Edit-mode vertex transform: stores full vertex position snapshot for undo/redo | ||
| class EditVertexTransformCommand : public QUndoCommand | ||
| { | ||
| public: | ||
| EditVertexTransformCommand(const std::map<int, Ogre::Vector3>& oldPositions, | ||
| const std::map<int, Ogre::Vector3>& newPositions, | ||
| const QString& description = "Edit Vertex Transform", | ||
| QUndoCommand* parent = nullptr); | ||
|
|
||
| void undo() override; | ||
| void redo() override; | ||
|
|
||
| private: | ||
| std::map<int, Ogre::Vector3> mOldPositions; ///< global index -> old position | ||
| std::map<int, Ogre::Vector3> mNewPositions; ///< global index -> new position | ||
| bool mFirstRedo = true; | ||
| }; |
There was a problem hiding this comment.
Bind this undo command to a concrete mesh/entity.
EditVertexTransformCommand only stores vertex index→position maps. If undo/redo resolves the target through current edit-mode state, Ctrl+Z after leaving edit mode or switching selection can no-op or hit the wrong mesh. Store a durable target identity on the command itself and resolve/apply against that target inside undo() / redo().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/TransformCommands.h` around lines 213 - 229,
EditVertexTransformCommand currently only stores mOldPositions/mNewPositions so
undo()/redo() that rely on transient edit-mode selection can apply to the wrong
mesh after mode/selection changes; modify the class to carry a durable target
identity (e.g., add a member like QString mTargetId or an immutable scene GUID
parameter) and update the constructor to accept that identity alongside old/new
maps; inside undo() and redo() resolve the target by that ID (not current edit
state), apply the stored vertex maps to the resolved mesh, and handle a missing
target gracefully (no-op with a clear log/error) so undo/redo always targets the
intended entity.
Comment on lines
+238
to
+267
| void EditableMesh::recalculateNormalsFlat() | ||
| { | ||
| for (auto& sub : m_subMeshes) { | ||
| // Zero out all normals | ||
| for (auto& v : sub.vertices) { | ||
| v.normal = Ogre::Vector3::ZERO; | ||
| v.hasNormal = true; | ||
| } | ||
|
|
||
| // Assign face normal to each vertex of each triangle | ||
| for (const auto& tri : sub.triangles) { | ||
| if (tri.indices[0] >= sub.vertices.size() || | ||
| tri.indices[1] >= sub.vertices.size() || | ||
| tri.indices[2] >= sub.vertices.size()) | ||
| continue; | ||
|
|
||
| const Ogre::Vector3& v0 = sub.vertices[tri.indices[0]].position; | ||
| const Ogre::Vector3& v1 = sub.vertices[tri.indices[1]].position; | ||
| const Ogre::Vector3& v2 = sub.vertices[tri.indices[2]].position; | ||
|
|
||
| Ogre::Vector3 faceNormal = (v1 - v0).crossProduct(v2 - v0); | ||
| Ogre::Real len = faceNormal.length(); | ||
| if (len > 1e-8f) | ||
| faceNormal /= len; | ||
|
|
||
| // Flat shading: each vertex gets the face normal directly | ||
| sub.vertices[tri.indices[0]].normal = faceNormal; | ||
| sub.vertices[tri.indices[1]].normal = faceNormal; | ||
| sub.vertices[tri.indices[2]].normal = faceNormal; | ||
| } |
There was a problem hiding this comment.
Flat normals are wrong on shared-edge meshes.
recalculateNormalsFlat() overwrites each shared vertex normal once per adjacent triangle, so the last face wins. True flat shading needs per-corner normals or vertex splitting before commit; otherwise “Flat” mode will still shade shared-vertex meshes incorrectly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditableMesh.cpp` around lines 238 - 267, recalculateNormalsFlat
currently writes a single normal into shared vertices so the last triangle wins;
fix by using per-corner normals or splitting vertices before assigning face
normals: either (A) change the triangle representation (e.g., Triangle in
EditableMesh) to store per-corner normal indices or normals and assign
faceNormal into those per-corner slots inside
EditableMesh::recalculateNormalsFlat, or (B) duplicate vertices for each
triangle corner (create unique vertices for tri.indices in sub.triangles) before
assigning normals so each corner gets the faceNormal; update downstream code
that expects shared-vertex indices (e.g., commit/mesh upload routines) to use
the new per-corner normals or split vertex list accordingly.
Comment on lines
+157
to
+163
| /** | ||
| * @brief Recalculate all vertex normals using flat shading. | ||
| * | ||
| * Each vertex gets the face normal of its triangle (no averaging). | ||
| * Note: this means shared vertices get the normal of the last triangle processed. | ||
| */ | ||
| void recalculateNormalsFlat(); |
There was a problem hiding this comment.
recalculateNormalsFlat() cannot produce true flat shading on shared vertices.
This still stores one normal per vertex, so adjacent triangles that share a vertex overwrite each other and the result depends on triangle traversal order. That means the advertised flat-normals mode is incorrect for indexed meshes unless you split vertices per face (or render with a flat-interpolation path).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditableMesh.h` around lines 157 - 163, The implementation of
recalculateNormalsFlat() claiming "flat shading" is incorrect for indexed meshes
because it writes one normal per vertex so shared vertices get overwritten
depending on triangle order; fix by generating true flat normals: either (A)
duplicate vertices per triangle (create a non-indexed vertex/normal array by
splitting shared vertices) and assign each new vertex the triangle face normal,
or (B) store per-face normals and change rendering/path to use per-face (flat)
interpolation; update recalculateNormalsFlat(), the mesh data structures it
touches, and any upload/attribute-binding code to use the new non-indexed vertex
buffer or per-face-normal attribute so adjacent triangles do not overwrite each
other.
Comment on lines
+133
to
+165
| SentryReporter::addBreadcrumb("edit_mode", "Cannot enter Edit Mode: no single entity selected"); | ||
| return false; | ||
| } | ||
|
|
||
| auto* sel = SelectionSet::getSingleton(); | ||
| QList<Ogre::Entity*> entities = sel->getResolvedEntities(); | ||
| m_editEntity = entities.first(); | ||
|
|
||
| // Decompose mesh into editable data | ||
| m_editableMesh = std::make_unique<EditableMesh>(); | ||
| if (!m_editableMesh->loadFromEntity(m_editEntity)) { | ||
| SentryReporter::addBreadcrumb("edit_mode", "Failed to load mesh data for Edit Mode"); | ||
| m_editableMesh.reset(); | ||
| m_editEntity = nullptr; | ||
| return false; | ||
| } | ||
|
|
||
| m_editModeActive = true; | ||
|
|
||
| // Reset selection state | ||
| m_selectionMode = VertexMode; | ||
| m_selectedVertices.clear(); | ||
| m_selectedEdges.clear(); | ||
| m_selectedFaces.clear(); | ||
|
|
||
| // Create overlay materials and initial (empty) overlays | ||
| createOverlayMaterials(); | ||
|
|
||
| SentryReporter::addBreadcrumb("edit_mode", | ||
| QString("Entered Edit Mode: %1 vertices, %2 triangles, %3 submeshes") | ||
| .arg(m_editableMesh->totalVertexCount()) | ||
| .arg(m_editableMesh->totalTriangleCount()) | ||
| .arg(m_editableMesh->subMeshCount())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the standard breadcrumb categories instead of "edit_mode".
These are user-facing actions/significant operations, but they are being emitted under a custom category. That makes the new edit-mode telemetry inconsistent with the repo’s expected Sentry taxonomy.
As per coding guidelines, "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use ui.action for toolbar/menu clicks, ai.tool_call for MCP tool invocations, file.import/file.export for I/O operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 133 - 165, Replace the nonstandard
"edit_mode" breadcrumb category with the repo-standard UI action category (e.g.
"ui.action") for all SentryReporter::addBreadcrumb calls in this
EditModeController code path: change the calls that log "Cannot enter Edit Mode:
no single entity selected", "Failed to load mesh data for Edit Mode", and the
final "Entered Edit Mode: …" message to use "ui.action" (update the
SentryReporter::addBreadcrumb invocations near m_editableMesh->loadFromEntity,
the selection check that returns false, and the final entry log so telemetry
follows the standard ui.action taxonomy).
Comment on lines
+355
to
+362
| void EditModeController::deselectEdge(int v1, int v2) | ||
| { | ||
| auto edge = std::make_pair(std::min(v1, v2), std::max(v1, v2)); | ||
| if (m_selectedEdges.erase(edge) > 0) { | ||
| // Note: we don't remove vertices here as they may be part of other selected edges | ||
| updateSelectionOverlay(); | ||
| emit editSelectionChanged(); | ||
| } |
There was a problem hiding this comment.
Deselecting an edge or face still leaves its vertices transformable.
These paths erase m_selectedEdges/m_selectedFaces, but they do not rebuild the derived m_selectedVertices state (and face removal also leaves m_selectedEdges). Since translate/rotate/scale operate on m_selectedVertices, Ctrl-deselect can still move geometry the UI appears to have removed from the selection.
Also applies to: 402-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 355 - 362, The deselectEdge (and
similarly the deselectFace) path removes entries from
m_selectedEdges/m_selectedFaces but does not recompute the derived
m_selectedVertices (and deselectFace also leaves related edges selected), so
vertices remain transformable; modify EditModeController::deselectEdge and
EditModeController::deselectFace to rebuild m_selectedVertices after removing
the edge/face (and in deselectFace also erase any edges belonging exclusively to
that face from m_selectedEdges), then call updateSelectionOverlay() and emit
editSelectionChanged() only after m_selectedVertices is updated so the UI and
transform operations reflect the true selection.
Comment on lines
+1505
to
+1519
| bool shiftHeld = e->modifiers().testFlag(Qt::ShiftModifier); | ||
| bool ctrlHeld = e->modifiers().testFlag(Qt::ControlModifier); | ||
|
|
||
| // If the rectangle is very small, treat as a click | ||
| QRect rect(mScreenStart, e->pos()); | ||
| rect = rect.normalized(); | ||
|
|
||
| if (rect.width() < 5 && rect.height() < 5) { | ||
| // Point select | ||
| EditModeController::instance()->handleMouseClick( | ||
| mScreenStart, m_pActiveWidget, shiftHeld, ctrlHeld); | ||
| } else { | ||
| // Box select | ||
| EditModeController::instance()->handleBoxSelect( | ||
| mScreenStart, e->pos(), m_pActiveWidget, shiftHeld); |
There was a problem hiding this comment.
Ctrl box-selection is dropped in edit mode.
This code reads both modifiers, but handleBoxSelect() only receives shiftHeld. One of the advertised edit-mode modifiers therefore has no effect for box selection, even though click selection forwards both flags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/TransformOperator.cpp` around lines 1505 - 1519, The box-select branch
drops the Control modifier: you compute both shiftHeld and ctrlHeld but call
EditModeController::instance()->handleBoxSelect(mScreenStart, e->pos(),
m_pActiveWidget, shiftHeld) without forwarding ctrlHeld; update the call (or
handleBoxSelect signature) to accept and pass ctrlHeld as well (e.g., add a ctrl
parameter to handleBoxSelect and propagate ctrlHeld) so box selection receives
both modifiers just like handleMouseClick does.
The NuGet package is Microsoft.WingetCreate, not wingetcreate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All buttons now use headerColor background (dark in dark mode), hover highlight, Behavior on color animation, and local properties with Connections for reactive updates (same pattern as snap settings). Replaced Qt Controls CheckBox for soft selection with themed custom checkbox matching the rest of the UI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Transparent blue glass sphere shows the soft selection influence region, centered at selection centroid, scaled to the radius. Uses procedural sphere with alpha-blended material (no depth write, double-sided). Appears when soft selection enabled + vertices selected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
setSoftSelectionEnabled/Radius/Falloff now call updateSelectionOverlay() so the radius sphere appears/disappears/resizes immediately. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/EditModeController.cpp (5)
314-320:⚠️ Potential issue | 🟠 Major
selectVertex(invalid, false)does not clear previous selection.Line 316 returns before the non-additive clear on Line 320, so replace-selection with an invalid index leaves stale selection.
Suggested fix
void EditModeController::selectVertex(int globalIndex, bool addToSelection) { if (!m_editModeActive || !m_editableMesh) return; + if (!addToSelection) + m_selectedVertices.clear(); if (globalIndex < 0 || globalIndex >= static_cast<int>(m_editableMesh->totalVertexCount())) return; - - if (!addToSelection) - m_selectedVertices.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 314 - 320, In selectVertex in EditModeController.cpp, when addToSelection is false you must clear m_selectedVertices even if the provided globalIndex is invalid; currently the early return on invalid index (globalIndex < 0 || >= totalVertexCount) prevents the non-additive clear, leaving stale selection. Move or duplicate the non-additive clear (m_selectedVertices.clear()) so it runs before the validity check (or run it unconditionally when addToSelection is false), ensuring the selection is cleared when selectVertex(invalid, false) is called; reference selectVertex, globalIndex, m_selectedVertices, m_editModeActive, and m_editableMesh to locate the logic.
151-173:⚠️ Potential issue | 🟠 MajorInitialize and clear validation state on edit-mode transitions.
m_degenerateTriangleCountis never initialized on entry and not reset on exit, so validation warnings can be stale across sessions.Suggested fix
bool EditModeController::enterEditMode() { @@ m_editModeActive = true; + validateMesh(); @@ void EditModeController::exitEditMode(bool commitChanges) { @@ m_editableMesh.reset(); m_editEntity = nullptr; m_editModeActive = false; + if (m_degenerateTriangleCount != 0) { + m_degenerateTriangleCount = 0; + emit validationChanged(); + }Also applies to: 196-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 151 - 173, Reset m_degenerateTriangleCount when toggling edit mode: set m_degenerateTriangleCount = 0 when entering edit mode (alongside m_editModeActive and selection clears) in the same block that creates overlays and emits editModeChanged, and also reset it when leaving edit mode (the block around lines 196-203 you referenced) so validation warnings don't persist across sessions; locate usages of m_degenerateTriangleCount and ensure the variable is initialized/cleared in both the enter and exit edit-mode paths.
181-184:⚠️ Potential issue | 🟠 MajorCheck
commitToEntity()before reporting success and exiting.On Line 182 the return value is ignored; Line 183 logs success even when commit fails, which can silently drop edits.
Suggested fix
- if (commitChanges && m_editableMesh && m_editEntity) { - m_editableMesh->commitToEntity(m_editEntity); - SentryReporter::addBreadcrumb("edit_mode", "Exited Edit Mode: changes committed"); + if (commitChanges && m_editableMesh && m_editEntity) { + const bool committed = m_editableMesh->commitToEntity(m_editEntity); + if (!committed) { + SentryReporter::addBreadcrumb("ui.action", "Exited Edit Mode: commit failed"); + return; // keep edit mode active; do not discard in-memory edits + } + SentryReporter::addBreadcrumb("ui.action", "Exited Edit Mode: changes committed");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 181 - 184, The call to m_editableMesh->commitToEntity(m_editEntity) is ignored and success is always reported; update the EditModeController to capture and check the boolean result of commitToEntity, only call SentryReporter::addBreadcrumb("edit_mode", "Exited Edit Mode: changes committed") when commitToEntity returns true, and handle the failure case by emitting an error breadcrumb/log and avoiding the success-exit path; reference the commitToEntity call on m_editableMesh and the SentryReporter::addBreadcrumb usage so you modify the conditional logic around those symbols.
356-363:⚠️ Potential issue | 🟠 MajorRecompute derived selection after edge/face deselection.
deselectEdge()/deselectFace()erase only primary sets, butm_selectedVertices(and face-related edges) remain stale, so transform operations can still affect deselected geometry.Also applies to: 403-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 356 - 363, deselectEdge() and deselectFace() only erase the primary selection sets (m_selectedEdges / m_selectedFaces) but leave derived sets like m_selectedVertices (and derived edge sets for faces) stale; after removing an edge or face, rebuild the derived selection state instead of leaving it stale: add or call a helper (e.g. recomputeDerivedSelection() or updateDerivedSelectionFromPrimary()) that clears and repopulates m_selectedVertices from the union of vertices in m_selectedEdges and vertices of faces in m_selectedFaces, and for face changes ensure any per-face edge bookkeeping is updated from m_selectedFaces, then call updateSelectionOverlay() and emit editSelectionChanged() as before from deselectEdge() and deselectFace().
133-135:⚠️ Potential issue | 🟠 MajorUse standard Sentry breadcrumb categories instead of
edit_mode.These are user-facing/significant actions but they are logged under a custom category, which breaks telemetry consistency.
As per coding guidelines, "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use ui.action for toolbar/menu clicks, ai.tool_call for MCP tool invocations, file.import/file.export for I/O operations".
Also applies to: 145-146, 162-163, 183-186, 242-243, 261-262, 302-303, 877-878, 971-973, 982-983, 1019-1019
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 133 - 135, Replace custom breadcrumb category "edit_mode" in SentryReporter::addBreadcrumb calls with the standard categories per guidelines: use "ui.action" for user-facing toolbar/menu actions (e.g., the breadcrumb inside the canEnterEditMode check), "ai.tool_call" for MCP/tool invocations, and "file.import"/"file.export" for I/O operations; update every SentryReporter::addBreadcrumb invocation in EditModeController.cpp (including the calls near canEnterEditMode and the other repeated occurrences) to use the appropriate standard category while keeping the original message text.src/EditModeController.h (1)
273-275:⚠️ Potential issue | 🟠 MajorBox-select API cannot express remove semantics or non-vertex mode behavior.
Current signatures only model add/replace via
booland Shift. They cannot carry Ctrl-remove intent or branch cleanly for edge/face box selection.Also applies to: 302-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.h` around lines 273 - 275, The box-select APIs (boxSelectVertices and the similar overloads) only take a bool for addToSelection and so cannot represent Ctrl-remove semantics or distinguish vertex/edge/face modes; change their signatures to accept an explicit enum for selection intent (e.g., SelectionAction { Replace, Add, Remove }) and an enum for element kind (e.g., ElementType { Vertex, Edge, Face }) or a combined SelectionMode so callers can express remove vs add/replace and target element type; update all callers of boxSelectVertices and the related functions (the other box-select declarations around the same area) to pass the new enum values and branch accordingly in implementation to handle remove semantics and non-vertex element selection.
🧹 Nitpick comments (1)
src/EditModeController.cpp (1)
1023-1027: Avoid duplicate GPU commits after degenerate cleanup.
recalculateNormals()already commits to the entity (Line 991), then Line 1026 commits again. This is redundant work on every cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 1023 - 1027, The duplicate GPU commit happens because recalculateNormals(...) already calls commitToEntity, so remove the redundant commit here: delete or guard the m_editableMesh->commitToEntity(m_editEntity) call (or add a boolean parameter to recalculateNormals to control committing and call it accordingly) so that only one commit occurs after degenerate cleanup; updateSelectionOverlay() should remain but ensure you only call commitToEntity from one place (recalculateNormals or this cleanup block) to avoid double GPU commits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/EditModeController.cpp`:
- Around line 314-320: In selectVertex in EditModeController.cpp, when
addToSelection is false you must clear m_selectedVertices even if the provided
globalIndex is invalid; currently the early return on invalid index (globalIndex
< 0 || >= totalVertexCount) prevents the non-additive clear, leaving stale
selection. Move or duplicate the non-additive clear (m_selectedVertices.clear())
so it runs before the validity check (or run it unconditionally when
addToSelection is false), ensuring the selection is cleared when
selectVertex(invalid, false) is called; reference selectVertex, globalIndex,
m_selectedVertices, m_editModeActive, and m_editableMesh to locate the logic.
- Around line 151-173: Reset m_degenerateTriangleCount when toggling edit mode:
set m_degenerateTriangleCount = 0 when entering edit mode (alongside
m_editModeActive and selection clears) in the same block that creates overlays
and emits editModeChanged, and also reset it when leaving edit mode (the block
around lines 196-203 you referenced) so validation warnings don't persist across
sessions; locate usages of m_degenerateTriangleCount and ensure the variable is
initialized/cleared in both the enter and exit edit-mode paths.
- Around line 181-184: The call to m_editableMesh->commitToEntity(m_editEntity)
is ignored and success is always reported; update the EditModeController to
capture and check the boolean result of commitToEntity, only call
SentryReporter::addBreadcrumb("edit_mode", "Exited Edit Mode: changes
committed") when commitToEntity returns true, and handle the failure case by
emitting an error breadcrumb/log and avoiding the success-exit path; reference
the commitToEntity call on m_editableMesh and the SentryReporter::addBreadcrumb
usage so you modify the conditional logic around those symbols.
- Around line 356-363: deselectEdge() and deselectFace() only erase the primary
selection sets (m_selectedEdges / m_selectedFaces) but leave derived sets like
m_selectedVertices (and derived edge sets for faces) stale; after removing an
edge or face, rebuild the derived selection state instead of leaving it stale:
add or call a helper (e.g. recomputeDerivedSelection() or
updateDerivedSelectionFromPrimary()) that clears and repopulates
m_selectedVertices from the union of vertices in m_selectedEdges and vertices of
faces in m_selectedFaces, and for face changes ensure any per-face edge
bookkeeping is updated from m_selectedFaces, then call updateSelectionOverlay()
and emit editSelectionChanged() as before from deselectEdge() and
deselectFace().
- Around line 133-135: Replace custom breadcrumb category "edit_mode" in
SentryReporter::addBreadcrumb calls with the standard categories per guidelines:
use "ui.action" for user-facing toolbar/menu actions (e.g., the breadcrumb
inside the canEnterEditMode check), "ai.tool_call" for MCP/tool invocations, and
"file.import"/"file.export" for I/O operations; update every
SentryReporter::addBreadcrumb invocation in EditModeController.cpp (including
the calls near canEnterEditMode and the other repeated occurrences) to use the
appropriate standard category while keeping the original message text.
In `@src/EditModeController.h`:
- Around line 273-275: The box-select APIs (boxSelectVertices and the similar
overloads) only take a bool for addToSelection and so cannot represent
Ctrl-remove semantics or distinguish vertex/edge/face modes; change their
signatures to accept an explicit enum for selection intent (e.g.,
SelectionAction { Replace, Add, Remove }) and an enum for element kind (e.g.,
ElementType { Vertex, Edge, Face }) or a combined SelectionMode so callers can
express remove vs add/replace and target element type; update all callers of
boxSelectVertices and the related functions (the other box-select declarations
around the same area) to pass the new enum values and branch accordingly in
implementation to handle remove semantics and non-vertex element selection.
---
Nitpick comments:
In `@src/EditModeController.cpp`:
- Around line 1023-1027: The duplicate GPU commit happens because
recalculateNormals(...) already calls commitToEntity, so remove the redundant
commit here: delete or guard the m_editableMesh->commitToEntity(m_editEntity)
call (or add a boolean parameter to recalculateNormals to control committing and
call it accordingly) so that only one commit occurs after degenerate cleanup;
updateSelectionOverlay() should remain but ensure you only call commitToEntity
from one place (recalculateNormals or this cleanup block) to avoid double GPU
commits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88ea2094-c9eb-49b7-8346-6eb97bd9a85c
📒 Files selected for processing (2)
src/EditModeController.cppsrc/EditModeController.h
- NormalVisualizer::refreshEntity(): public method to rebuild normal overlay for a specific entity (destroy + rebuild) - EditModeController calls refreshNormalVisualizer() after every commitToEntity (translate, rotate, scale, normals recalc, degenerate removal) so normal lines update in real-time when Show Normals is on Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Clones sub-entity materials and sets PM_WIREFRAME on enable, restores originals on disable or edit mode exit. QML checkbox follows snap-settings style with local property + Connections. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Reset m_degenerateTriangleCount on exit edit mode, validate on entry - Check commitToEntity return value and log error breadcrumb on failure - Remove duplicate commitToEntity call in removeDegenerateTriangles (recalculateNormals already commits) - Fix indentation of refreshNormalVisualizer calls Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests were using sceneMgr->createEntity() which doesn't register the entity with Manager. SelectionSet::getResolvedEntities() then can't find the entity by node name, so enterEditMode() fails on CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move m_selectedVertices.clear() before bounds check in selectVertex() so invalid index with addToSelection=false still clears selection - Fix modeLabel test: enterEditMode now sets VertexMode, so label is "Edit Mode (Vertex)" not "Edit Mode" - Fix signal emission test: connect after enterEditMode to avoid counting entry emissions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The in-memory triangle mesh has no material name set on its submesh, which is valid. The test incorrectly asserted material name must be non-empty when using shared vertices. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… in edit mode - Wireframe checkbox persists across edit mode sessions: state is kept on exit and re-applied on next enter - Switching selection mode (Vertex/Edge/Face) clears previous selection to avoid stale overlays - Block Delete key and Remove toolbar button in edit mode to prevent crash from deleting the entity being edited - Extract applyWireframeMaterials/removeWireframeMaterials helpers Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The rolling mEditModeStartPositions was re-snapshotted every frame during drag, so by mouse release it matched the final positions and the diff check saw no change. Added mEditModeUndoSnapshot captured once at press time for the undo comparison and command. Also positions gizmo at selected vertices centroid in edit mode. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Moved scan-assets-qtmesh to run first in the pipeline
- build-n-cache-assimp-{windows,linux,macos} now depend on it
- Removed if: github.event_name != 'release' so it runs on
PRs, pushes to master, and releases
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
3 tasks
This was referenced Apr 15, 2026
This was referenced Apr 24, 2026
This was referenced May 2, 2026
This was referenced May 15, 2026
8 tasks
This was referenced Jun 24, 2026
This was referenced Jul 16, 2026
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
Full Edit Mode implementation for direct mesh geometry editing (#258). All 8 items complete.
Item 1-2: Mode Toggle + Mesh Data Structure
Item 3-5: Vertex/Edge/Face Selection
Item 6: Vertex Transform
Item 7: Normals Recalculation
Item 8: Mesh Validation
Stats
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests