quads chunk 4b: n-gon-aware face/edge selection - #334
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 17d138076d
ℹ️ 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".
| const size_t n = f.indices.size(); | ||
| if (n < 3) continue; // invalid face — triangulateFaces skipped it | ||
| const size_t triCount = n - 2; |
There was a problem hiding this comment.
faceIndexForTriangle advances running for any face with n >= 3, but triangulateFaces and HalfEdgeMesh::buildFromEditableMesh both skip faces that fail EditableFace::isValid() (for example, consecutive duplicate indices). On such meshes, the triangle-to-face mapping drifts after the first invalid face, so face-mode ops that rely on this mapper (extrude/delete/dissolve/subdivide) can target the wrong polygon even though the clicked triangle is valid.
Useful? React with 👍 / 👎.
| running += subs[s].faces.empty() | ||
| ? static_cast<int>(subs[s].triangles.size()) | ||
| : static_cast<int>(subs[s].faces.size()); |
There was a problem hiding this comment.
The HE face base offset uses subs[s].faces.size() whenever faces is non-empty, but HalfEdgeMesh::buildFromEditableMesh only appends valid faces (face.isValid()). If any earlier submesh contains invalid face entries, offsets for later submeshes become too large, so selectedFacesAsHEFaceIndices() can return indices that point at the wrong HE faces (or miss entirely), causing topology ops to mutate the wrong region.
Useful? React with 👍 / 👎.
Selection in Edit Mode now operates over n-gon polygons rather than
the artificial fan-triangulation:
- Click on a quad → highlights all of its triangles (the whole
polygon), not just one half. Visual selection matches the
user's mental model of "I clicked a face".
- Edge mode hit-test ignores fan diagonals (the artificial edge
between two halves of a quad) and only picks real polygon
perimeter edges. Backface culling added so backside edges
aren't pickable from the front, matching face-selection
behaviour.
- Topology ops (delete / dissolve / extrude / subdivide) consume
the selection through `selectedFacesAsHEFaceIndices()`, which
deduplicates triangle picks down to unique HE face indices —
so a quad selected via either of its triangles is processed
as a single face.
Subdivide-on-quad now actually does something:
- New `HalfEdgeMesh::subdivideFacesToQuads` splits each selected
n-gon into N sub-quads (face point + edge midpoints + corner),
sharing edge midpoints between adjacent selected faces in the
same submesh so the result stays manifold.
- `subdivideSelection` dispatches by face arity: triangles → the
existing 1-to-4 split; n-gons → the new quad split.
Plumbing:
- `EditableSubMesh` gains a `faceIndexForTriangle()` free helper
that maps a fan triangle back to its source face index, used
everywhere the controller needs to dilate triangle selections.
- `EditableMesh::loadFromAssimpFile` reads tangents from the
source file (when present) so the editable mesh carries them
end-to-end. We deliberately do NOT request
aiProcess_CalcTangentSpace because that flag implicitly
triangulates the mesh, defeating the n-gon path.
- `MeshImporterExporter::applyNormalMapsToEntity` is now public
so future fixes can re-attach RTSS bump-map state from any
Edit-Mode op.
Tests (+9 standalone + 4 HE):
- `faceIndexForTriangle` — legacy / quad / mixed / out-of-range /
null-output cases.
- `subdivideFacesToQuads` — empty, single quad → 4 sub-quads,
triangle → 3 sub-quads, two adjacent quads share midpoints.
Known issue (deferred to a follow-up fix-PR):
Bump-mapped meshes loaded through the n-gon import path lose
their bump map (and sometimes basic per-pixel lighting) after a
topology op. Triangle-only and procedural assets are unaffected.
Tracked separately because the fix needs proper RTSS / shader
pipeline instrumentation rather than continued blind permutation.
None of the various invalidate / validate / re-attach
permutations attempted reproduce the import-time behaviour
reliably; the real fix likely lives in tangent / vertex-
declaration handling on the round-trip path.
Towards #326.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related findings, same root cause: `faceIndexForTriangle` and `selectedFacesAsHEFaceIndices` were counting *all* faces with at least three indices, but `triangulateFaces` and `HalfEdgeMesh::buildFromEditableMesh` only consume faces that pass `EditableFace::isValid()` (which additionally rejects consecutive duplicate indices). On meshes with any invalid face entry — e.g. a 4-vertex face like `[0,0,1,2]` — the triangle-to-face mapping and the per-submesh HE face base-offset would drift, so face-mode ops (extrude / delete / dissolve / subdivide) could mutate the wrong polygon. Fix: align both call sites to skip faces by `!isValid()` (matching what buildFromEditableMesh actually appends), and base the HE face offset on the count of valid faces only. Adds a regression test (FaceIndexForTriangleSkipsInvalidFaces) covering the consecutive-duplicate-index case explicitly. All 190 standalone tests still pass.
17d1380 to
d05b23f
Compare
|
1 participant
Summary
Stacked on #333 (chunk 5a). Makes face/edge selection and Standard Subdivide n-gon-aware so quad meshes behave correctly in Edit Mode.
faceIndexForTriangle()maps a fan triangle to its source face.EditableSubMesh::faces(when populated) so fan-triangulation diagonals are skipped. Adds Newell-normal backface culling so edges on the back of the mesh aren't picked through the model.subdivideFaces; n-gons → newsubdivideFacesToQuads(each N-gon splits into N sub-quads via centroid + edge midpoints). Selection is cleared after the op so stale verts don't linger.selectedFacesAsHEFaceIndices()so ops that need face indices (subdivide, future extrude/bevel) don't process the same n-gon multiple times.loadFromAssimpFilereads tangents from the source if present (noaiProcess_CalcTangentSpace, which would force triangulation).MeshImporterExporter::applyNormalMapsToEntitymade public so the controller can re-apply RTSS sub-render-states after a topology op.Known issue (deferred)
Bump-mapped meshes loaded through the n-gon import path lose their bump map (and sometimes basic per-pixel lighting) after a topology op (subdivide / extrude). Many permutations of deferred RTSS, tangent rebuild, and
_deinitialise/_initialiseordering were tried without success; root cause appears to be in how RTSS material schemes survive the rewrite path in n-gon mode. Tracking as a follow-up fix-PR — selection/topology behavior in this PR is correct, only the shader scheme is regressing.Test plan