quads follow-up: knife works on quad meshes; fill produces n-gons - #337
Merged
Conversation
Addresses both Codex P1 findings on PR #332 (chunk 4) before they reach master. Issue 1 — bone-handle drift on skinned meshes loadFromAssimpFile stored aiBone mesh-local indices in EditableBoneAssignment::boneIndex. The GUI import path (MeshProcessor) instead resolves aiBone->mName against the loaded Ogre::Skeleton and stores Ogre::Bone::getHandle(). When a topology op re-emitted VertexBoneAssignments via resizeEntityBuffers, those mesh-local indices were re-interpreted as Ogre handles, so vertices rebound to whichever bones happened to occupy those handle slots. Fix: add an optional Ogre::Skeleton* parameter to loadFromAssimpFile. When non-null, aiBone->mName is resolved against it (matching MeshProcessor) and the resulting handle is stored. Bones that don't resolve are skipped so we never emit wild handles. EditModeController passes the live entity's skeleton when entering edit mode. Issue 2 — Z-up overlay rotation on FBX/glTF assets MeshProcessor bakes a +90°-around-X rotation into rendered buffers for assets declared Z-up (FBX UpAxis = 2), so the Ogre scene-graph stays Y-up without a node rotation. loadFromAssimpFile read raw aiMesh vertices unchanged, so on Z-up assets the editable representation lived in pre-bake space while rendered buffers were post-bake — vertex/edge/face overlays appeared rotated 90° relative to the on-screen geometry, and a commit would write the rotated positions back, silently rotating the entity. Fix: add an `isZup` parameter to loadFromAssimpFile. When true, apply the same +90°-around-X bake to position, normal, and tangent before storing them. MeshImporterExporter caches the source up-axis under "qtme.source_up_axis" alongside the existing source-path / convert-LH caches, and EditModeController reads it back when re-entering edit mode. EditableMesh::commitToEntity / resizeEntityBuffers now also erase this cache key when the live buffers diverge from the source. Tests: two new standalone regression tests cover the Z-up bake math and the unskinned-mesh shape of the new bone-skeleton parameter. The skinned-mesh skeleton-lookup case is exercised by EditModeController integration tests at run time.
Addresses the deferred lighting/RTSS regression flagged on PR #334. Root cause After any Edit-Mode topology op (subdivide, extrude, bevel, knife, merge, delete/dissolve, undo/redo, …) bump-mapped meshes loaded through the n-gon import path went dark and lost their normal map. Two compounding issues: 1. EditableMesh::buildSubMeshBuffers checks only `vertices[0] .hasTangent` to decide whether to add a VES_TANGENT element to the rebuilt declaration. New vertices created by the op default- construct EditableVertex (zero-valued Vector4 tangent), so: (a) if the first vertex retained tangents the declaration kept VES_TANGENT but new vertices wrote (0,0,0,0), making RTSS's SRS_NORMALMAP TBN math collapse to zeros; (b) if the asset came in via loadFromAssimpFile (which deliberately omits aiProcess_CalcTangentSpace because that flag forces triangulation), every vertex has hasTangent=false and the declaration drops VES_TANGENT entirely. Either way the bump map was effectively gone. 2. Each topology op had its OWN inline _deinitialise/_initialise + invalidateMaterial block; the undo/redo path (EditMeshTopologyCommand::applyMeshState) had a third copy that skipped the RTSS hook entirely. So a Subdivide-then-Ctrl-Z lost lighting even when the redo would have restored it. Fix Centralise post-topology-op refresh into a new static method EditModeController::rewriteEntityAfterTopologyChange(Entity*). It: a) Detects bump-map intent by scanning every subentity's material for a `normal_map`/`NormalMap` TUS. If any subentity is bump- mapped, force `Mesh::buildTangentVectors` (storeParityInW=true) BEFORE _deinitialise/_initialise. Order matters: calling it AFTER _initialise is too late — the SubEntity already linked against the old (no-tangent) declaration and the RTSS shaders compile against that stale layout. b) Saves per-subentity material overrides before the deinit/init (Ogre resets them to the SubMesh default), restores after. c) Re-runs MeshImporterExporter::applyNormalMapsToEntity so RTSS re-attaches its SRS_NORMALMAP sub-render-state against the fresh tangents. invalidateMaterial alone only drops cached shader programs; the SRS_NORMALMAP gets dropped on removeShaderBasedTechnique inside applyNormalMap, so we must call it again to re-add it. d) Final invalidateMaterial pass to keep behaviour identical to the old per-op blocks for materials that aren't bump-mapped. All five inline copies in EditModeController.cpp (extrude, bevel commit, bevel cancel, knife commit, generic post-op via applyTopologyMutationNoSurvivor) now call this helper, and so does EditMeshTopologyCommand::applyMeshState in TransformCommands.cpp — so undo/redo gets the same treatment. Tests Standalone regression test confirms the helper is reachable as a static method (so TransformCommands.cpp's qualified call survives a refactor that might shove it back into an anonymous namespace) and null-tolerant. Full bump-map / RTSS exercise needs a GL context which the test infra doesn't provide on macOS; coverage there is via hand smoke tests on the bump-mapped Mixamo asset (subdivide / extrude / undo / redo all confirmed visually preserving the bump map and per-pixel lighting).
Two regressions surfaced after chunks 4 / 4b / 5a landed on feat/quads.
1. Fill produced fan triangles instead of an n-gon
`HalfEdgeMesh::fillSelection` always called `appendTriangle` in a
fan loop and emitted N-2 triangles for N inputs. After the n-gon
round-trip, `toEditableMesh` saw N-2 separate triangles and wrote
them as N-2 EditableFaces, so a 4-vertex fill ended up as 2
triangles with a visible fan diagonal — and Standard Subdivide
on the result treated them as triangles, not as a quad.
Fix: for n=3 still call appendTriangle (matches existing output);
for n>=4 call `appendFace` once, creating a single n-gon HEFace.
The result round-trips through toEditableMesh as one EditableFace
with N indices. Return value semantics changes from "triangles
created" to "polygons created" (always 1 here); callers used the
value as a success/fail flag, so the surface contract holds.
2. Knife silently failed on quad-imported meshes
`splitEdge` (the primitive `cutPath` uses) is a triangle-only MVP
— it bails immediately if either adjacent face has != 3 vertices.
On a quad-imported mesh, every face is an n-gon and the very
first split fails, so knife appears to do nothing.
Workaround until a proper n-gon-aware splitEdge lands: in the
knife commit path, build the HE from a triangle-mode COPY of the
editable mesh (clear `.faces` so buildFromEditableMesh falls back
to the fan-triangulated `.triangles` mirror). The cost is
materialising the fan diagonals on every submesh the cut touches;
the benefit is a working knife. Tracked as a follow-up.
Tests
- 4-vertex fill now expects `1` polygon, not `2`, AND asserts the
result round-trips as a single quad EditableFace.
- 5-vertex fill same: `1` polygon, asserts a pentagon survives the
HE round-trip.
- 4-orphan fill updated for the new return value.
- Existing `FillSelectionRejectsLargerFanThatDuplicatesExistingTri`
still passes — the fan-vs-existing-triangle dedup check at the
head of fillSelection still walks fan triangles, so duplicate
rejection is unchanged.
Codex P2 on PR #335: applyNormalMapsToEntity is now called from rewriteEntityAfterTopologyChange on every topology op AND every undo/redo, so any single broken/unresolvable material would now abort the entire edit op via an unhandled `mat->load()` throw — a regression from the old path that only invalidated RTSS without forcing a load. Wrap the load call in a try/catch and skip the offending sub-entity on failure. Logs the material name + Ogre exception description so we don't lose the diagnostic.
…it-test Two correctness fixes on top of the earlier knife/fill work. 1. Fill produced inward-facing normals `fillSelection` walked the input vertices in user-supplied order (typically `std::set` ascending), which is whatever order the selection produced — nothing guaranteed it matched the winding of the surrounding mesh. On a hole's boundary loop, the new face often ended up oriented INTO the volume. Fix: before building the new face, compute its Newell normal and compare against the average Newell normal of every existing face that shares at least one of the selected vertices. If the dot product is negative the winding is inverted; reverse it. n=3 triangles and n>=4 n-gons go through the same orientation step. 2. Knife was a no-op on quad-imported meshes `knifeHitTest` built the HE from the live `*m_editableMesh` (n-gon path, since `.faces` is populated for quad-imported assets), but `commitKnife` builds the HE from a triangle-mode copy (clears `.faces` so the splitEdge MVP — triangle-only — can run). The `edgeIndex` recorded at click time pointed at edges in the n-gon HE; the commit-time HE had different edge numbering, so cutPath tried to split an unrelated edge and either no-op'd or mutated the wrong region. Fix: build the SAME triangle-mode HE in `knifeHitTest`, mirroring the commit pipeline. Edge indices now line up between hit-test and commit. Triangle-only meshes (welded cube, primitives) were unaffected since their `.faces` is already empty — the existing knife tests still pass.
Symptom: on the FBX quad asset the knife appeared to do nothing — clicks went through, the commit ran, but cutPath was never called. Root cause: knifeHitTest has a Priority-1 vertex snap (10px radius). On a dense imported mesh (Mixamo character ≈ 19k edges), almost any click also lands within 10px of a vertex, so points came back as KnifePoint::OnVertex. commitKnife only accepted OnEdge — the kind check rejected silently via a Sentry breadcrumb. Fix: at commit time, translate OnVertex points into edge clicks by finding any incident edge to that vertex in the (triangle-mode) HE and using t=0 or t=1. cutPath is unchanged — its splitEdge primitive clamps t away from the endpoints by 1e-4 to keep faces non-degenerate, so the resulting cut vertex sits ≈ 1e-4 of an edge length off the original. Fine for an MVP; a follow-up can teach cutPath to start / end at an existing vertex without splitting at all. OnFace clicks are still rejected (no edge-walk path can represent them).
On dense imported meshes the knife was picking edges/vertices on the far side of the model — invisible to the user but inside the screen- space pixel radius. Mirrors the chunk-4b front-facing fix that hitTestEdge / face selection already use. Implementation Build front-facing vertex / edge sets once at the top of knifeHitTest, using the n-gon `m_editableMesh` so quad meshes are filtered correctly. A polygon is front-facing when its Newell normal (rotated into world space) points toward the camera. A vertex is front-facing iff at least one incident polygon does; an edge iff at least one of its two adjacent polygons does. Priority 1 (vertex snap): only return OnVertex if `snapVert` is in the front-facing set. Priority 2 (edge snap): skip edges not in the front-facing set. Edge keys use (min,max) global-vertex pairs which line up directly with the triangle-mode HE the snap loop walks. Fan-triangulation diagonals — which the triangle-mode HE generates internally for quad faces — are NOT in `frontEdges` (it's built from polygon perimeters only), so the knife also can't snap to fake interior edges that don't exist on the n-gon mesh. Trade-off note Knife still cuts as triangles on quad meshes (the build-from- triangle-copy workaround materialises fan diagonals at commit). The proper fix is an n-gon-aware splitEdge, tracked as a follow-up before loop cut since both ops will share that infrastructure.
PR #335's SonarCloud quality gate failed on cognitive complexity: - rewriteEntityAfterTopologyChange: 47 (limit 25) - enterEditMode: 32 (limit 25) plus a deprecation warning on the old buildTangentVectors overload. Refactor (no behaviour change): - File-scope `entityWantsTangents`, `rebuildMeshTangents`, `invalidateEntityRtssMaterials` factor out the three loops inside `rewriteEntityAfterTopologyChange`. Switches to the non-deprecated buildTangentVectors signature. - File-scope `tryLoadEditableMeshNGonPath` factors out the four- nested-try-block n-gon-import attempt from `enterEditMode`. `rewriteEntityAfterTopologyChange` and `enterEditMode` now read top- to-bottom as plain sequences of named steps. All 234 standalone tests still pass.
|
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: 4c96d0f71d
ℹ️ 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
+2517
to
+2519
| triOnly.subMeshes() = m_editableMesh->subMeshes(); | ||
| for (auto& sub : triOnly.subMeshes()) | ||
| sub.faces.clear(); |
There was a problem hiding this comment.
The knife workaround clears faces on every submesh before building the half-edge mesh, so after toEditableMesh the write-back is triangle-only for the whole mesh, not just the region being cut. On quad/n-gon assets this silently discards polygon topology in untouched areas (e.g., one knife action can convert unrelated submeshes to fan triangles), which changes subsequent edit behavior and exposes artificial diagonals. Restrict the triangle-only conversion to affected geometry or restore original faces for untouched submeshes before assigning updated back.
Useful? React with 👍 / 👎.
Codex P1 on PR #337: the knife workaround cleared `.faces` on EVERY submesh before building the half-edge mesh, so after `toEditableMesh` the write-back was triangle-only for the whole mesh — a single knife action could silently convert unrelated submeshes to fan triangles. Fix: snapshot `wasNGonSub[]` (which submeshes were originally n-gon- canonical) before the clear. After cutPath, walk the returned `cutVerts` and collect the set of submeshes the cut actually touched (via `facesAroundVertex` + `face.subMeshIndex`). For every submesh that was originally n-gon AND wasn't touched, restore it verbatim from `originalSubMeshes` before assigning back. Touched submeshes keep the post-cut triangulation (the workaround the PR already documented), and untouched submeshes preserve their quad topology.
|
10 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
Stacks on PR #335 (lighting/bones/transforms). Three knife/fill regressions on
feat/quadsafter chunks 4 / 4b / 5a, plus a follow-up culling polish.1. Fill produced fan triangles instead of an n-gon
HalfEdgeMesh::fillSelectionalways calledappendTrianglein a fan loop. A 4-vertex fill ended up as 2 triangles with a visible diagonal. Fix:appendTrianglefor n=3,appendFacefor n>=4 (single n-gon HEFace). Returns "polygons created" instead of "triangles created" — callers used the value as a success/fail flag, so the contract holds.2. Fill normals pointed inward
Input vertices walked in
std::setorder, which is whatever the selection produced. Fix: compute the candidate face's Newell normal, compare to the average Newell normal of every existing face that shares a selected vertex, reverse winding if the dot product is negative.3. Knife no-op on quad-imported meshes
splitEdgeis a triangle-only MVP — it bails on n-gons. Workaround: in bothknifeHitTestandcommitKnife, build the HE from a triangle-mode COPY of the editable mesh (clear.facessobuildFromEditableMeshfalls back to the fan-triangulated.triangles). Edge indices line up between hit-test and commit. The cost is materialising fan diagonals on every submesh the cut touches; tracked as a follow-up before loop cut since both ops will share an n-gon-aware splitEdge.4. Dense FBX meshes pulled clicks to back-face vertices / vertex snap
commitKnifeended up withOnVertexclicks but only acceptedOnEdge. Fix: at commit time translateOnVertex→ edge click using any incident edge with t=0/t=1.m_editableMeshonce at the top ofknifeHitTest, skip back-face geometry. Fan-triangulation diagonals are correctly excluded sincefrontEdgesis built from polygon perimeters only.Test plan
Known follow-ups
splitEdge— current knife fan-triangulates the cut faces. Properly splitting a quad along an edge needs ear-clip-aware rewiring. Should land BEFORE loop cut since both ops share the same primitive.