From f28e7f45dc8dfacdb189243ebdb68016ea9571d9 Mon Sep 17 00:00:00 2001 From: Fernando Date: Thu, 30 Apr 2026 02:58:28 -0400 Subject: [PATCH] fix(quads): bug-fix sweep from PR #347 review (8 of 10 items) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the deferred items from PR #347 review (#326 follow-up). Eight discrete fixes; the remaining two (vertex-knife near-duplicate on OnVertex hits, subdivideFacesToQuads T-junctions) are heavier lifts deferred to a separate PR. 1. HalfEdgeMesh::loopCut rejects mixed quad/tri adjacency upfront. Previously walked f1 and f2 independently, so a quad+tri start edge produced a one-sided cut on the quad side and silently mutated topology. (Codex P1.) +1 unit test. 2. applyWireframeMaterials handles mixed meshes per submesh. Tri-only submeshes inside a mesh that had ANY n-gon submesh were losing wireframe entirely (boundary overlay only emits n-gon submeshes). Now PM_WIREFRAME on tri-only submeshes coexists with the boundary overlay on n-gon submeshes. (Codex P2.) 3. buildSubMeshBuffers explicitly clears the GPU vertex/index buffers when the submesh has no vertices or no triangles to draw. Prior early-return left stale buffers attached, so deleting the last face in a submesh kept the old geometry rendering forever. 4. selectedFacesAsHEFaceIndices compacts invalid faces in the per-face mapping, not just the per-submesh base offset. Without this, a selected face whose raw index is shifted by an earlier invalid face mapped to the wrong HE face — silently breaking face-mode extrude/delete/dissolve/subdivide. 5. canConvertToQuads predicate replaces !isMeshQuadBased on the toolbar gate. Mixed meshes (some submeshes quad, some tri) still have tri-only submeshes worth merging — the previous check wrongly disabled the action. 6. convertToQuads correctly returns non-zero on promote-only runs. Tracks promotion count separately from merge count, returns their sum. Previous return-of-totalMerges falsely reported 0 when only the n-gon promotion happened. 7. deselectFace mirrors selectFace's vertex/edge dilation. Without this, ctrl-deselect erased the face triangles but left perimeter vertex/edge entries — looked like a stuck partial selection. 8. Test in EditModeControllerBevelE2E uses a real (0.001f) translate instead of Vector3::ZERO so the commit-path assertion stays meaningful even if the controller ever short-circuits zero deltas. Two items remain for a follow-up PR: - Vertex knife OnVertex clicks producing near-duplicate points (the click is converted to first-incident-edge + t=0/1 then splitEdge clamps it; need a different splitFace path for true vertex hits). - subdivideFacesToQuads T-junctions on partial n-gon selections (needs adjacent-face retriangulation pass like the existing tri subdivide does). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/EditModeController.cpp | 147 +++++++++++++++++++++++--------- src/EditModeController.h | 12 ++- src/EditModeController_test.cpp | 7 +- src/EditableMesh.cpp | 31 ++++++- src/HalfEdgeMesh.cpp | 15 ++++ src/HalfEdgeMesh_test.cpp | 60 +++++++++++++ src/mainwindow.cpp | 8 +- 7 files changed, 228 insertions(+), 52 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index 57bcb3eac..c2036c8be 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -776,6 +776,38 @@ void EditModeController::deselectFace(int triIndex) const int gi = localTriToGlobal(subIdx, tri); if (m_selectedFaces.erase(gi) > 0) anyErased = true; } + + // Mirror selectFace's vertex/edge dilation on deselect too. + // Without this, ctrl-click-to-deselect leaves the perimeter + // vertices and edges from the previous selectFace call hanging + // around in the overlay, looking like a stuck partial selection. + // (CodeRabbit follow-up on PR #347.) + const int vertOffset = localToGlobal(subIdx, 0); + const int faceK = faceIndexForTriangle(sub, localTri, + &faceFirstTri, &faceTriCount); + + if (faceK >= 0 && faceK < static_cast(sub.faces.size())) { + const auto& f = sub.faces[faceK]; + const size_t n = f.indices.size(); + for (size_t i = 0; i < n; ++i) { + const int g0 = vertOffset + static_cast(f.indices[i]); + const int g1 = vertOffset + static_cast(f.indices[(i + 1) % n]); + m_selectedVertices.erase(g0); + m_selectedEdges.erase({std::min(g0, g1), std::max(g0, g1)}); + } + } else if (faceFirstTri < sub.triangles.size()) { + const auto& triData = sub.triangles[faceFirstTri]; + const int g0 = vertOffset + static_cast(triData.indices[0]); + const int g1 = vertOffset + static_cast(triData.indices[1]); + const int g2 = vertOffset + static_cast(triData.indices[2]); + m_selectedVertices.erase(g0); + m_selectedVertices.erase(g1); + m_selectedVertices.erase(g2); + m_selectedEdges.erase({std::min(g0, g1), std::max(g0, g1)}); + m_selectedEdges.erase({std::min(g1, g2), std::max(g1, g2)}); + m_selectedEdges.erase({std::min(g0, g2), std::max(g0, g2)}); + } + if (anyErased) { updateSelectionOverlay(); emit editSelectionChanged(); @@ -859,19 +891,27 @@ std::vector EditModeController::selectedFacesAsHEFaceIndices() const // match this rule. const auto& subs = m_editableMesh->subMeshes(); std::vector heBaseBySub(subs.size(), 0); + // Per-submesh map: raw face index -> compacted HE face index + // (or -1 if the raw face was invalid and got skipped). Built only + // for n-gon submeshes; tri-only submeshes use the simple + // base + localTri formula. (CodeRabbit follow-up on PR #347: + // when a submesh has invalid faces earlier in the sequence, + // compacting only the BASE offset isn't enough — every selected + // face's faceK also needs to be remapped to the compacted index.) + std::vector> compactedFaceIdxBySub(subs.size()); int running = 0; for (size_t s = 0; s < subs.size(); ++s) { heBaseBySub[s] = running; if (subs[s].faces.empty()) { running += static_cast(subs[s].triangles.size()); } else { - // Match HalfEdgeMesh::buildFromEditableMesh, which only - // appends faces that pass isValid(). Counting raw faces - // here would over-shoot the offset and shift later - // submeshes' HE face indices. + auto& map = compactedFaceIdxBySub[s]; + map.resize(subs[s].faces.size(), -1); int valid = 0; - for (const auto& f : subs[s].faces) { - if (f.isValid()) ++valid; + for (size_t fi = 0; fi < subs[s].faces.size(); ++fi) { + if (subs[s].faces[fi].isValid()) { + map[fi] = valid++; + } } running += valid; } @@ -885,8 +925,12 @@ std::vector EditModeController::selectedFacesAsHEFaceIndices() const const auto& sub = subs[subIdx]; const int faceK = faceIndexForTriangle(sub, localTri, nullptr, nullptr); if (faceK >= 0) { - // n-gon submesh: HE face index = base + faceK. - uniq.insert(heBaseBySub[subIdx] + faceK); + // n-gon submesh: HE face index = base + COMPACTED faceK + // (skipping any invalid faces earlier in sub.faces). + const auto& map = compactedFaceIdxBySub[subIdx]; + if (faceK < static_cast(map.size()) && map[faceK] >= 0) { + uniq.insert(heBaseBySub[subIdx] + map[faceK]); + } } else { // Legacy triangle-only submesh: HE face index = base + // localTri (one HE face per triangle). @@ -3864,36 +3908,24 @@ int EditModeController::convertToQuads(float angleThresholdDeg) const auto preSelectedFaces = m_selectedFaces; int totalMerges = 0; + int submeshesPromoted = 0; for (auto& sub : m_editableMesh->subMeshes()) { // Promote first if the submesh is in legacy triangle-only mode // so the n-gon path takes over even when no merges happen — the // wireframe overlay etc. branch on .faces being non-empty. - if (sub.faces.empty()) promoteTrianglesToFaces(sub); + if (sub.faces.empty() && !sub.triangles.empty()) { + promoteTrianglesToFaces(sub); + ++submeshesPromoted; + } totalMerges += mergeCoplanarTrianglesToQuads(sub, angleThresholdDeg); } - if (totalMerges == 0) { - // Promotion alone counts as a meaningful change (downstream - // n-gon-aware features start working) — but if every submesh - // already had .faces and no merges happened, this is a true - // no-op. Detect by comparing face counts pre/post. - bool topologyChanged = false; - const auto& cur = m_editableMesh->subMeshes(); - if (cur.size() != originalSubMeshes.size()) { - topologyChanged = true; - } else { - for (size_t i = 0; i < cur.size(); ++i) { - if (cur[i].faces.size() != originalSubMeshes[i].faces.size()) { - topologyChanged = true; - break; - } - } - } - if (!topologyChanged) { - // Restore — promote was a no-op too. - m_editableMesh->subMeshes() = std::move(originalSubMeshes); - return 0; - } + // True no-op: every submesh was already n-gon canonical and no + // coplanar pairs got merged. Restore and bail without emitting an + // undo command. + if (totalMerges == 0 && submeshesPromoted == 0) { + m_editableMesh->subMeshes() = std::move(originalSubMeshes); + return 0; } if (m_normalsMode == 0) m_editableMesh->recalculateNormals(); @@ -3918,7 +3950,8 @@ int EditModeController::convertToQuads(float angleThresholdDeg) validateMesh(); SentryReporter::addBreadcrumb("edit_mode", - QString("Convert to Quads (merges=%1)").arg(totalMerges)); + QString("Convert to Quads (merges=%1, promoted=%2)") + .arg(totalMerges).arg(submeshesPromoted)); // Wireframe path may have flipped from PM_WIREFRAME to the n-gon // boundary overlay (or back, if undo is invoked) — re-apply. @@ -3931,7 +3964,11 @@ int EditModeController::convertToQuads(float angleThresholdDeg) refreshNormalVisualizer(); emit editSelectionChanged(); emit meshDataChanged(); - return totalMerges; + // Return non-zero whenever the op actually changed something — + // including the promotion-only case where no triangle pairs got + // merged but legacy submeshes flipped to n-gon canonical form. + // (CodeRabbit follow-up on PR #347.) + return totalMerges + submeshesPromoted; } int EditModeController::subdivideCatmullClarkAll() @@ -5050,22 +5087,42 @@ bool EditModeController::isMeshQuadBased() const return false; } +bool EditModeController::canConvertToQuads() const +{ + if (!m_editableMesh) return false; + // True if any submesh is still triangle-only — that submesh is a + // candidate for promotion + coplanar-tri merging. Mixed meshes + // qualify even though `isMeshQuadBased()` is also true on them, + // because the tri-only submeshes still have work to do. + for (const auto& sub : m_editableMesh->subMeshes()) { + if (sub.faces.empty() && !sub.triangles.empty()) return true; + } + return false; +} + void EditModeController::applyWireframeMaterials() { if (!m_editEntity) return; - // n-gon mesh: keep solid material, use the boundary-edge overlay - // (drawn from updateSelectionOverlay) to show face boundaries - // without the fan-triangulation diagonals. - if (meshHasNgonFaces()) { - m_savedMaterials.clear(); - updateBoundaryEdgeOverlay(); - return; - } - m_savedMaterials.clear(); + + // Mixed-mesh aware: each submesh gets the wireframe path that + // matches ITS canonical face representation. + // - n-gon submesh (.faces non-empty): keep solid material; the + // boundary-edge overlay below draws clean polygon outlines + // without fan-triangulation diagonals. + // - triangle-only submesh: clone the material with PM_WIREFRAME + // so the GPU draws every triangle edge. + // The previous "any n-gon submesh in the mesh → all overlay" rule + // dropped wireframe entirely on tri-only submeshes inside a mixed + // mesh. (Codex P2 follow-up on PR #347.) + const auto& subs = m_editableMesh ? m_editableMesh->subMeshes() + : std::vector{}; for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) { + const bool isNgon = (i < subs.size()) && !subs[i].faces.empty(); + if (isNgon) continue; // boundary overlay handles this one. + auto* subEnt = m_editEntity->getSubEntity(i); m_savedMaterials[i] = subEnt->getMaterialName(); @@ -5082,6 +5139,12 @@ void EditModeController::applyWireframeMaterials() } subEnt->setMaterialName(wireName); } + + // Draw the n-gon boundary overlay if any submesh has .faces. + // Multi-submesh meshes with a mix of tri and n-gon submeshes get + // both paths active simultaneously. + if (meshHasNgonFaces()) + updateBoundaryEdgeOverlay(); } void EditModeController::removeWireframeMaterials() diff --git a/src/EditModeController.h b/src/EditModeController.h index 2209c1979..5f1b462fa 100644 --- a/src/EditModeController.h +++ b/src/EditModeController.h @@ -131,9 +131,17 @@ class EditModeController : public QObject void setWireframeEnabled(bool enabled); /// True if any submesh of the current edit mesh has non-empty - /// n-gon `.faces` — i.e. quad-based already. Drives the - /// "Convert to Quads" toolbar button enable state. + /// n-gon `.faces` — i.e. quad-based already. Q_INVOKABLE bool isMeshQuadBased() const; + + /// True when at least one submesh still has triangle-only + /// representation (`.faces` empty). Drives the "Convert to Quads" + /// toolbar button: a fully n-gon mesh has nothing to do, but a + /// MIXED mesh (some submeshes already n-gon, others tri-only) + /// still benefits — a less precise `isMeshQuadBased()` check + /// would wrongly disable the action on those. (CodeRabbit follow- + /// up on PR #347.) + Q_INVOKABLE bool canConvertToQuads() const; /// @} /// @name Mesh info (only valid when in edit mode) diff --git a/src/EditModeController_test.cpp b/src/EditModeController_test.cpp index 2b03243e8..817f92437 100644 --- a/src/EditModeController_test.cpp +++ b/src/EditModeController_test.cpp @@ -2046,11 +2046,12 @@ TEST_F(EditModeControllerBevelE2ETest, EnterEditModeAfterEditDoesNotReimport) { ASSERT_FALSE(ctrl->currentMesh()->subMeshes()[0].faces.empty()) << "first entry must take the n-gon path"; - // Commit any change (translate one vertex by zero — still triggers - // the commitToEntity path that wipes the source path). + // Commit a real change so the test exercises the commit path even + // if the controller ever short-circuits zero-delta transforms in + // the future. (CodeRabbit follow-up on PR #347.) ctrl->setSelectionMode(EditModeController::VertexMode); ctrl->selectVertex(0); - ctrl->translateSelectedVertices(Ogre::Vector3::ZERO); + ctrl->translateSelectedVertices(Ogre::Vector3(0.001f, 0.0f, 0.0f)); ctrl->exitEditMode(/*commitChanges*/ true); EXPECT_FALSE(mesh->getUserObjectBindings().getUserAny( diff --git a/src/EditableMesh.cpp b/src/EditableMesh.cpp index b36f46f4b..6fe84b971 100644 --- a/src/EditableMesh.cpp +++ b/src/EditableMesh.cpp @@ -883,7 +883,31 @@ bool EditableMesh::ensureVertexColorBuffers(Ogre::Entity* entity) void EditableMesh::buildSubMeshBuffers(Ogre::SubMesh* subMesh, const EditableSubMesh& editSubIn) { - if (!subMesh || editSubIn.vertices.empty()) return; + if (!subMesh) return; + + // Empty-submesh safety net: when an edit deletes the last face in + // a submesh (or the submesh enters with no vertices), explicitly + // tear down any prior vertex/index data on the SubMesh so the GPU + // doesn't keep rendering stale geometry. The previous early-return + // path silently left the old buffers attached. (CodeRabbit Major + // follow-up on PR #347.) + auto clearSubMeshGeometry = [&]() { + if (subMesh->vertexData) { + delete subMesh->vertexData; + subMesh->vertexData = nullptr; + } + subMesh->useSharedVertices = false; + if (subMesh->indexData) { + subMesh->indexData->indexBuffer.reset(); + subMesh->indexData->indexCount = 0; + subMesh->indexData->indexStart = 0; + } + }; + + if (editSubIn.vertices.empty()) { + clearSubMeshGeometry(); + return; + } // n-gon synchronisation: if the caller populated `faces`, that's // canonical and `triangles` is meant to be a fan-triangulation @@ -904,7 +928,10 @@ void EditableMesh::buildSubMeshBuffers(Ogre::SubMesh* subMesh, triangulateFaces(local); editSub = &local; } - if (editSub->triangles.empty()) return; + if (editSub->triangles.empty()) { + clearSubMeshGeometry(); + return; + } // Replace any existing vertex data with a fresh one. if (subMesh->vertexData) delete subMesh->vertexData; diff --git a/src/HalfEdgeMesh.cpp b/src/HalfEdgeMesh.cpp index 5606dfaa3..741f29186 100644 --- a/src/HalfEdgeMesh.cpp +++ b/src/HalfEdgeMesh.cpp @@ -4983,6 +4983,21 @@ std::vector HalfEdgeMesh::loopCut(int startEdgeIdx) }; const auto [f1, f2] = edgeFaces(startEdgeIdx); + + // Loop cut is well-defined only when BOTH faces adjacent to the + // start edge are quads — the algorithm crosses each face via its + // opposite edge and triangles have no such correspondence. The + // EditModeController surfaces a "needs a quad mesh" hint to the + // user when this rejects, so silently producing a half-loop on + // mixed adjacency would be worse than the no-op. Boundary edges + // (one face = -1) are also rejected for the same reason. + // (Codex P1 follow-up on PR #347.) + auto isQuad = [&](int faceIdx) { + if (faceIdx < 0) return false; + return faceVertices(faceIdx).size() == 4; + }; + if (!isQuad(f1) || !isQuad(f2)) return newVertices; + walkDirection(f1, startA, startB); walkDirection(f2, startA, startB); diff --git a/src/HalfEdgeMesh_test.cpp b/src/HalfEdgeMesh_test.cpp index 38e17fd59..0a9420f4d 100644 --- a/src/HalfEdgeMesh_test.cpp +++ b/src/HalfEdgeMesh_test.cpp @@ -3965,6 +3965,66 @@ TEST(HalfEdgeMeshStandalone, LoopCutFailsOnInvalidEdgeIndex) { EXPECT_TRUE(he.loopCut(999).empty()); } +TEST(HalfEdgeMeshStandalone, LoopCutRejectsMixedQuadTriAdjacency) { + // Regression test for Codex P1 follow-up: loopCut walked both + // sides of the start edge independently, so a quad+tri adjacency + // would produce a one-sided cut on the quad side and silently + // mutate topology when the user expected a "needs a quad mesh" + // failure. The op must reject upfront when EITHER face adjacent + // to the start edge is non-quad. + EditableMesh em; + EditableSubMesh sub; + sub.materialName = "M"; + auto mkV = [](float x, float y, float z) { + EditableVertex v; + v.position = Ogre::Vector3(x, y, z); + v.normal = Ogre::Vector3::UNIT_Z; v.hasNormal = true; + return v; + }; + // 5 verts, planar in XY: + // 3 -- 2 + // | /| + // | / | + // | / | + // 0 -- 1 -- 4 (4 lifted to make a triangle) + // + // Quad face: [0,1,2,3] (winds CCW around +Z) + // Tri face: [1,4,2] (shares edge (1,2) with the quad) + // Edge (1,2) is the shared boundary — the loop-cut start edge. + sub.vertices = { + mkV(0, 0, 0), + mkV(1, 0, 0), + mkV(1, 1, 0), + mkV(0, 1, 0), + mkV(2, 0, 0), + }; + EditableFace quad; quad.indices = {0, 1, 2, 3}; + EditableFace tri; tri.indices = {1, 4, 2}; + sub.faces = {quad, tri}; + triangulateFaces(sub); + em.subMeshes().push_back(std::move(sub)); + + HalfEdgeMesh he; + ASSERT_TRUE(he.buildFromEditableMesh(em)); + const int sharedEdge = findEdge(he, 1, 2); + ASSERT_GE(sharedEdge, 0); + + auto newVerts = he.loopCut(sharedEdge); + EXPECT_TRUE(newVerts.empty()) + << "loop cut must reject mixed quad/tri adjacency upfront, " + "not produce a half-loop on the quad side"; + EXPECT_TRUE(he.validate()); + + // No partial mutation: the HE mesh keeps the 2 input faces + // (1 quad + 1 triangle) — buildFromEditableMesh respects + // sub.faces directly when it's populated. + int active = 0; + for (size_t f = 0; f < he.faceCount(); ++f) { + if (he.face(static_cast(f)).halfEdge >= 0) ++active; + } + EXPECT_EQ(active, 2); +} + // =========================================================================== // Merge vertices // =========================================================================== diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index ae2413d71..ffdf9a76d 100755 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1063,9 +1063,11 @@ void MainWindow::initToolBar() // uses the first selected edge as the start; multi-edge loop // cuts aren't in scope for the MVP. loopCutButton->setEnabled(mode == 1 && hasEdges); - // Convert to Quads: whole-mesh; disable once the mesh already - // has n-gon canonical faces (no work to do). - convertToQuadsButton->setEnabled(!c->isMeshQuadBased()); + // Convert to Quads: whole-mesh; disable only when EVERY submesh + // is already n-gon canonical. Mixed meshes (some submeshes tri, + // some quad) still qualify — the tri-only submeshes can still + // be merged. (CodeRabbit follow-up on PR #347.) + convertToQuadsButton->setEnabled(c->canConvertToQuads()); vertexPaintButton->setEnabled(true); }; refreshTopoButtons();