feat: half-edge data structure + material preset undo - #288
Merged
Conversation
Phase 4 Item 1: Implement HalfEdgeMesh for topology queries (adjacency, boundary detection) needed by upcoming topology tools (extrude, bevel, loop cut, etc.). Roundtrip conversion to/from EditableMesh preserves UVs, normals, bone weights, and multi-submesh materials. 35 unit tests. Also adds MaterialPresetCommand so material preset application is undoable via Ctrl+Z. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a new HalfEdgeMesh data structure with build/convert/adjacency/boundary/validation APIs and tests, integrates material-preset undo/redo via a new MaterialPresetCommand, and updates CMake to include the HalfEdgeMesh sources. Changes
Sequence Diagram(s)sequenceDiagram
participant EM as EditableMesh
participant HEM as HalfEdgeMesh
participant Q as Query API
EM->>HEM: buildFromEditableMesh()
activate HEM
Note over HEM: clear containers\ncreate per-submesh HE vertices\ncreate faces & half-edges\nlink twins\nbuild boundary half-edges
deactivate HEM
HEM->>Q: facesAroundVertex / edgesAroundVertex / verticesAroundVertex
HEM->>Q: faceVertices / edgeVertices / edgeFaces
HEM->>EM: toEditableMesh()
activate HEM
Note over HEM: remap global HE vertices to per-submesh indices\ncopy attributes and triangles
deactivate HEM
sequenceDiagram
participant User as User/UI
participant MPL as MaterialPresetLibrary
participant Entities as Ogre Entities
participant MPC as MaterialPresetCommand
participant UM as UndoManager
User->>MPL: applyPreset(preset)
activate MPL
Note over MPL: collect entity & sub-entity snapshots (old/new)
MPL->>Entities: setMaterialName(new) -- apply to entities & sub-entities
MPL->>MPC: create MaterialPresetCommand(with snapshots)
deactivate MPL
MPL->>UM: push(MPC)
alt User triggers Undo
UM->>MPC: undo()
activate MPC
MPC->>Entities: setMaterialName(old) -- restore sub-entities (guard null)
deactivate MPC
else User triggers Redo
UM->>MPC: redo()
activate MPC
Note over MPC: first redo is no-op (mFirstRedo toggle)
Note over MPC: subsequent redos apply new materials
MPC->>Entities: setMaterialName(new)
deactivate MPC
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/HalfEdgeMesh.h (3)
32-35: Consider more granular Ogre includes for faster compilation.Including
<Ogre.h>pulls in the entire Ogre library. Since this header only usesOgre::Vector3,Ogre::Vector2, andOgre::ColourValue, you could reduce compile times with more specific includes:`#include` <OgreVector.h> `#include` <OgreColourValue.h>This is a minor optimization and may not be worth changing if the codebase consistently uses
<Ogre.h>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.h` around lines 32 - 35, The header currently includes the monolithic Ogre.h which increases compile time; replace that include with the more granular Ogre headers used here (the Vector and Colour types) by switching the single include of Ogre.h to the specific headers that declare Ogre::Vector3, Ogre::Vector2 and Ogre::ColourValue (e.g., the OgreVector and OgreColourValue headers used in your codebase) so only the needed declarations are pulled into HalfEdgeMesh.h.
159-169: Consider adding bounds-checked accessors or documenting the unchecked behavior.The direct element accessors (
vertex(),face(),edge(),halfEdge()) don't perform bounds checking, which is fine for performance-critical internal use. However, consider either:
- Adding a note that callers must ensure valid indices, or
- Providing optional bounds-checked variants (e.g.,
vertexAt()that throws or returnsstd::optional)This is a minor ergonomics consideration for future API consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.h` around lines 159 - 169, Add bounds-checked accessors and/or document unchecked behavior: either add documented note on the existing unchecked methods vertex(int), face(int), edge(int), halfEdge(int) stating callers must ensure valid indices, or implement new bounds-checked variants (e.g., vertexAt(int), faceAt(int), edgeAt(int), halfEdgeAt(int)) that validate idx against the underlying containers (m_vertices, m_faces, m_edges, m_halfEdges) and either throw std::out_of_range or return std::optional/reference_wrapper on failure; ensure the new names are exported alongside the existing methods and keep the unchecked methods for performance-sensitive code.
291-297:m_vertexOriginsis populated but never used.The
VertexOriginstruct andm_vertexOriginsvector are populated duringbuildFromEditableMesh()but are never read or accessed anywhere in the codebase. If reserved for future topology operations (vertex split, edge collapse, face subdivision), add a comment clarifying this is reserved for future use. Otherwise, consider removing the unused member to reduce memory overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.h` around lines 291 - 297, The member m_vertexOrigins and its inner struct VertexOrigin are populated (in buildFromEditableMesh()) but never read elsewhere (including toEditableMesh()); either remove VertexOrigin and m_vertexOrigins to avoid wasted memory or explicitly mark them as reserved by adding a clarifying comment and leaving them unused; if keeping, update toEditableMesh() (or other relevant methods) to consume m_vertexOrigins when reconstructing per-submesh vertex arrays, otherwise delete the struct and vector declarations (VertexOrigin, m_vertexOrigins) and any code that populates them to eliminate dead state.src/HalfEdgeMesh.cpp (1)
356-404: Potential duplicate faces infacesAroundVertexwhen walking both directions on boundary.When the first loop hits a boundary and breaks (line 379-380), the second loop walks the other direction starting from
startHE. IfstartHE's face was already added in the first loop, it won't be re-added (the second loop starts withtwin->next), but the logic could be clearer. Consider using anstd::unordered_setto deduplicate, or document that the current implementation is correct due to the starting point of the second walk.♻️ Optional: Add deduplication for safety
std::vector<int> HalfEdgeMesh::facesAroundVertex(int vertexIdx) const { std::vector<int> result; + std::unordered_set<int> seen; if (vertexIdx < 0 || vertexIdx >= static_cast<int>(m_vertices.size())) return result; // ... existing walk code ... do { - if (m_halfEdges[he].face >= 0) { + if (m_halfEdges[he].face >= 0 && seen.insert(m_halfEdges[he].face).second) { result.push_back(m_halfEdges[he].face); } // ... rest of loop ... } while (he != startHE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 356 - 404, In facesAroundVertex, walking both directions from startHE can push duplicate face IDs when boundary traversal overlaps; modify the function (facesAroundVertex, local variables startHE/he/result) to deduplicate before appending — e.g., keep an std::unordered_set<int> seen and check seen.count(face) (or use result.contains equivalent) before result.push_back(face) in both loops so faces are only added once; ensure the set is updated whenever you push to result.
🤖 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.cpp`:
- Around line 592-619: Add Sentry breadcrumbs inside MaterialPresetCommand::undo
and MaterialPresetCommand::redo to track preset undo/redo events; call
SentryReporter::addBreadcrumb with a consistent category (e.g.,
"material_preset") and a descriptive message (e.g., "undo preset" or "redo
preset") at the start or immediately after performing the setMaterialName loops
in MaterialPresetCommand::undo and MaterialPresetCommand::redo so each
user-facing undo/redo of presets is recorded.
In `@src/commands/TransformCommands.h`:
- Around line 235-243: TransformCommands.h declares public structs
EntityMaterial and SubEntityMaterial that use std::string but the header does
not include <string>; add a direct include for <string> at the top of
TransformCommands.h so these symbols (EntityMaterial, SubEntityMaterial) no
longer rely on transitive includes and compile correctly when this header is
included standalone.
In `@src/HalfEdgeMesh.h`:
- Around line 129-138: Update the docstring for
HalfEdgeMesh::buildFromEditableMesh in HalfEdgeMesh.h to match the
implementation: state that vertices in different submeshes are NOT merged even
if positions match (within epsilon), and that the method builds per-submesh
vertex sets and tracks submesh provenance per face; ensure the return
description stays the same and remove the misleading sentence about merging
across submeshes so the header aligns with the behavior implemented in
HalfEdgeMesh.cpp.
In `@src/MaterialPresetLibrary.cpp`:
- Around line 115-137: The undo snapshot currently records only
ent->getSubEntity(0) for each Ogre::Entity in resolvedEntities before calling
ent->setMaterialName, which overwrites all sub-entities and loses original
materials; instead, first iterate resolvedEntities and/or subEntities to capture
every affected Ogre::SubEntity into MaterialPresetCommand::SubEntityMaterial
(store subEntity and its oldMaterialName) as well as EntityMaterial entries,
without calling ent->setMaterialName or sub->setMaterialName yet; after the full
old-state capture is complete, apply the new material names (call
ent->setMaterialName/stdMatName or sub->setMaterialName) and then push the new
MaterialPresetCommand(entMats, subMats, name) to UndoManager::getSingleton().
---
Nitpick comments:
In `@src/HalfEdgeMesh.cpp`:
- Around line 356-404: In facesAroundVertex, walking both directions from
startHE can push duplicate face IDs when boundary traversal overlaps; modify the
function (facesAroundVertex, local variables startHE/he/result) to deduplicate
before appending — e.g., keep an std::unordered_set<int> seen and check
seen.count(face) (or use result.contains equivalent) before
result.push_back(face) in both loops so faces are only added once; ensure the
set is updated whenever you push to result.
In `@src/HalfEdgeMesh.h`:
- Around line 32-35: The header currently includes the monolithic Ogre.h which
increases compile time; replace that include with the more granular Ogre headers
used here (the Vector and Colour types) by switching the single include of
Ogre.h to the specific headers that declare Ogre::Vector3, Ogre::Vector2 and
Ogre::ColourValue (e.g., the OgreVector and OgreColourValue headers used in your
codebase) so only the needed declarations are pulled into HalfEdgeMesh.h.
- Around line 159-169: Add bounds-checked accessors and/or document unchecked
behavior: either add documented note on the existing unchecked methods
vertex(int), face(int), edge(int), halfEdge(int) stating callers must ensure
valid indices, or implement new bounds-checked variants (e.g., vertexAt(int),
faceAt(int), edgeAt(int), halfEdgeAt(int)) that validate idx against the
underlying containers (m_vertices, m_faces, m_edges, m_halfEdges) and either
throw std::out_of_range or return std::optional/reference_wrapper on failure;
ensure the new names are exported alongside the existing methods and keep the
unchecked methods for performance-sensitive code.
- Around line 291-297: The member m_vertexOrigins and its inner struct
VertexOrigin are populated (in buildFromEditableMesh()) but never read elsewhere
(including toEditableMesh()); either remove VertexOrigin and m_vertexOrigins to
avoid wasted memory or explicitly mark them as reserved by adding a clarifying
comment and leaving them unused; if keeping, update toEditableMesh() (or other
relevant methods) to consume m_vertexOrigins when reconstructing per-submesh
vertex arrays, otherwise delete the struct and vector declarations
(VertexOrigin, m_vertexOrigins) and any code that populates them to eliminate
dead state.
🪄 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: 145e1758-8ea1-4c68-90e3-8876bca5f765
📒 Files selected for processing (7)
src/CMakeLists.txtsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/HalfEdgeMesh_test.cppsrc/MaterialPresetLibrary.cppsrc/commands/TransformCommands.cppsrc/commands/TransformCommands.h
- Fix critical: snapshot ALL sub-entity materials before applying preset, not just subEntity(0). Undo now restores per-sub-entity materials correctly for entities with mixed materials. - Add Sentry breadcrumbs to MaterialPresetCommand undo/redo. - Add missing #include <string> to TransformCommands.h. - Fix misleading docstring on buildFromEditableMesh (vertices are NOT merged across submeshes). - Use granular Ogre includes (OgreVector.h, OgreColourValue.h) in HalfEdgeMesh.h instead of monolithic Ogre.h. - Remove unused m_vertexOrigins member and VertexOrigin struct. - Add deduplication to facesAroundVertex for boundary vertex safety. - Document unchecked element accessors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
4 tasks
4 tasks
7 tasks
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
HalfEdgeMeshhalf-edge data structure for efficient topology queries (adjacency, boundary detection), foundational for upcoming topology tools (extrude, bevel, loop cut, knife, merge, delete/dissolve, subdivide, fill)MaterialPresetCommandso it participates in the Ctrl+Z/Ctrl+Shift+Z undo/redo historyTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores