From f09e755d55f47bf38dcbe38b8e1893445d4eab94 Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 19:46:15 -0400 Subject: [PATCH 1/9] fix(quads): preserve transforms + bone handles in n-gon re-import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/EditModeController.cpp | 32 +++++++++++++++- src/EditableMesh.cpp | 63 +++++++++++++++++++++++-------- src/EditableMesh.h | 25 ++++++++++++- src/EditableMesh_test.cpp | 72 ++++++++++++++++++++++++++++++++++++ src/MeshImporterExporter.cpp | 12 ++++++ 5 files changed, 186 insertions(+), 18 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index 6a41141a7..3c5f8df68 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -318,8 +318,38 @@ bool EditModeController::enterEditMode() convertLH = Ogre::any_cast(lhAny); } catch (const Ogre::Exception&) {} } + // The original importer also caches the source up- + // axis (1 = Y-up, 2 = Z-up). MeshProcessor bakes a + // +90°-around-X rotation into the rendered buffers + // for Z-up assets; we pass `isZup` so the editable + // representation stays in the same basis. Without + // this, FBX/glTF assets that declare Z-up would + // surface vertex overlays rotated 90° relative to + // the on-screen geometry, and a commit would write + // the rotated positions back, silently rotating + // the entity. + bool isZup = false; + const Ogre::Any& upAny = + bindings.getUserAny("qtme.source_up_axis"); + if (upAny.has_value()) { + try { + isZup = (Ogre::any_cast(upAny) == 2); + } catch (const Ogre::Exception&) {} + } + // Resolve aiBone names against the live skeleton so + // the n-gon path emits Ogre bone HANDLES (matching + // MeshProcessor) instead of mesh-local aiBone + // indices. Without this, a topology op on a skinned + // mesh re-emits VertexBoneAssignments with wild + // handles and skinning rebinds vertices to wrong + // bones. + const Ogre::Skeleton* skel = nullptr; + if (meshPtr->hasSkeleton()) { + skel = meshPtr->getSkeleton().get(); + } if (!sourcePath.empty() - && m_editableMesh->loadFromAssimpFile(sourcePath, convertLH)) { + && m_editableMesh->loadFromAssimpFile( + sourcePath, convertLH, isZup, skel)) { loaded = true; SentryReporter::addBreadcrumb("edit_mode", "Edit Mode entered via n-gon import path"); diff --git a/src/EditableMesh.cpp b/src/EditableMesh.cpp index 611c692dd..5a482ab40 100644 --- a/src/EditableMesh.cpp +++ b/src/EditableMesh.cpp @@ -147,7 +147,9 @@ bool EditableMesh::loadFromMesh(const Ogre::MeshPtr& meshPtr) } bool EditableMesh::loadFromAssimpFile(const std::string& path, - bool convertToLeftHanded) + bool convertToLeftHanded, + bool isZup, + const Ogre::Skeleton* skeletonForBoneHandles) { if (path.empty()) return false; @@ -189,6 +191,18 @@ bool EditableMesh::loadFromAssimpFile(const std::string& path, m_subMeshes.clear(); m_subMeshes.reserve(scene->mNumMeshes); + // Mirror MeshProcessor's Z-up bake: when the source asset declares + // Z-up (FBX UpAxis = 2), the GUI importer rotates every position / + // normal / tangent +90° around X so the Ogre scene-graph stays Y-up + // without needing a node rotation. We must apply the SAME rotation + // here so the editable representation lives in the same basis as + // the rendered Ogre buffers; otherwise the vertex/edge/face overlays + // appear rotated 90° (and on commit, the mismatched positions get + // written back, silently rotating the geometry). + const Ogre::Quaternion zUpBake = isZup + ? Ogre::Quaternion(Ogre::Degree(90), Ogre::Vector3::UNIT_X) + : Ogre::Quaternion::IDENTITY; + for (unsigned m = 0; m < scene->mNumMeshes; ++m) { const aiMesh* aim = scene->mMeshes[m]; if (!aim || aim->mNumVertices == 0) continue; @@ -210,10 +224,10 @@ bool EditableMesh::loadFromAssimpFile(const std::string& path, for (unsigned i = 0; i < aim->mNumVertices; ++i) { EditableVertex& ev = sub.vertices[i]; const aiVector3D& p = aim->mVertices[i]; - ev.position = Ogre::Vector3(p.x, p.y, p.z); + ev.position = zUpBake * Ogre::Vector3(p.x, p.y, p.z); if (hasNormals) { const aiVector3D& n = aim->mNormals[i]; - ev.normal = Ogre::Vector3(n.x, n.y, n.z); + ev.normal = zUpBake * Ogre::Vector3(n.x, n.y, n.z); ev.hasNormal = true; } if (hasUVs) { @@ -227,33 +241,48 @@ bool EditableMesh::loadFromAssimpFile(const std::string& path, ev.hasColor = true; } if (hasTangents) { - const aiVector3D& t = aim->mTangents[i]; // RTSS expects FLOAT4 tangents with handedness in w. - // Compute parity from the input bitangent: if cross - // (normal × tangent) aligns with bitangent, parity = - // +1, else -1. Same convention MeshProcessor / - // applyNormalMapsToEntity use. - const aiVector3D& bt = aim->mBitangents[i]; - Ogre::Vector3 normalV = ev.hasNormal ? ev.normal : Ogre::Vector3::UNIT_Z; - Ogre::Vector3 tangentV(t.x, t.y, t.z); - Ogre::Vector3 expectedBT = normalV.crossProduct(tangentV); - float parity = expectedBT.dotProduct( - Ogre::Vector3(bt.x, bt.y, bt.z)) >= 0.0f ? 1.0f : -1.0f; - ev.tangent = Ogre::Vector4(t.x, t.y, t.z, parity); + // Apply the same Z-up bake to T/B/N before computing + // parity so the convention matches MeshProcessor. + Ogre::Vector3 T = zUpBake * Ogre::Vector3( + aim->mTangents[i].x, aim->mTangents[i].y, aim->mTangents[i].z); + Ogre::Vector3 B = zUpBake * Ogre::Vector3( + aim->mBitangents[i].x, aim->mBitangents[i].y, aim->mBitangents[i].z); + Ogre::Vector3 N = ev.hasNormal ? ev.normal : Ogre::Vector3::UNIT_Z; + float parity = N.crossProduct(T).dotProduct(B) >= 0.0f ? 1.0f : -1.0f; + ev.tangent = Ogre::Vector4(T.x, T.y, T.z, parity); ev.hasTangent = true; } } // Bone weights, if any. + // + // CRITICAL: assignments must use the same bone-handle convention + // the rendered Ogre mesh uses. MeshProcessor (the GUI import + // path) resolves `aiBone->mName` against the loaded skeleton and + // stores `Ogre::Bone::getHandle()`. If we instead stored aiBone + // mesh-local indices here, then on the next topology op (which + // re-emits VertexBoneAssignments through resizeEntityBuffers) + // those indices would be interpreted as handles — vertices would + // bind to whichever bone happens to live at that handle slot, + // not the bone the source file actually weighted them to. + // Skip bones that don't resolve so we never emit wild handles. if (aim->mNumBones > 0) { for (unsigned b = 0; b < aim->mNumBones; ++b) { const aiBone* bone = aim->mBones[b]; if (!bone) continue; + unsigned short handle = static_cast(b); + if (skeletonForBoneHandles) { + const std::string boneName = bone->mName.C_Str(); + if (!skeletonForBoneHandles->hasBone(boneName)) continue; + handle = static_cast( + skeletonForBoneHandles->getBone(boneName)->getHandle()); + } for (unsigned w = 0; w < bone->mNumWeights; ++w) { const aiVertexWeight& vw = bone->mWeights[w]; if (vw.mVertexId >= sub.vertices.size()) continue; EditableBoneAssignment eba; - eba.boneIndex = static_cast(b); + eba.boneIndex = handle; eba.weight = vw.mWeight; sub.vertices[vw.mVertexId].boneAssignments.push_back(eba); } @@ -487,6 +516,7 @@ bool EditableMesh::commitToEntity(Ogre::Entity* entity) // discarded by an n-gon re-import. (Quad migration #326, chunk 4.) mesh->getUserObjectBindings().eraseUserAny("qtme.source_path"); mesh->getUserObjectBindings().eraseUserAny("qtme.source_convert_lh"); + mesh->getUserObjectBindings().eraseUserAny("qtme.source_up_axis"); return true; } @@ -682,6 +712,7 @@ bool EditableMesh::resizeEntityBuffers(Ogre::Entity* entity) // Topology has changed — same rationale as commitToEntity above. mesh->getUserObjectBindings().eraseUserAny("qtme.source_path"); mesh->getUserObjectBindings().eraseUserAny("qtme.source_convert_lh"); + mesh->getUserObjectBindings().eraseUserAny("qtme.source_up_axis"); return true; } diff --git a/src/EditableMesh.h b/src/EditableMesh.h index 579a443be..b88c39334 100644 --- a/src/EditableMesh.h +++ b/src/EditableMesh.h @@ -304,8 +304,31 @@ class EditableMesh * @return true on success; false if the file is missing, can't be * parsed, or contains no mesh data. */ + /** + * @param isZup If true, applies the same +90°-around-X rotation that + * `MeshProcessor` bakes into the rendered Ogre buffers + * when the source asset declares a Z-up coordinate + * system (FBX `UpAxis = 2`). Without this, the editable + * vertices live in the source's pre-bake basis while the + * rendered buffers are post-bake, and selection overlays + * appear rotated 90° relative to the rendered geometry. + * The original importer caches its choice at + * `getUserAny("qtme.source_up_axis")` (an int — 1 = Y-up, + * 2 = Z-up). + * @param skeletonForBoneHandles If non-null, `aiBone->mName` is + * resolved against this skeleton via `getBone(name)` and + * the resulting `Ogre::Bone::getHandle()` is stored as + * `EditableBoneAssignment::boneIndex`. This matches the + * handle convention `MeshProcessor` writes into the live + * Ogre mesh; without it, this path emits mesh-local + * aiBone indices that re-bind vertices to the wrong + * bones after a topology op. Pass nullptr only for + * unskinned meshes. + */ bool loadFromAssimpFile(const std::string& path, - bool convertToLeftHanded = true); + bool convertToLeftHanded = true, + bool isZup = false, + const Ogre::Skeleton* skeletonForBoneHandles = nullptr); /** * @brief Merge vertices at (approximately) coincident positions within diff --git a/src/EditableMesh_test.cpp b/src/EditableMesh_test.cpp index 5c1c5b9ca..2ec09e6c0 100644 --- a/src/EditableMesh_test.cpp +++ b/src/EditableMesh_test.cpp @@ -1024,6 +1024,78 @@ TEST(EditableMeshStandalone, LoadFromAssimpFileMixedTriAndQuadKeepsBoth) { EXPECT_EQ(sub.triangles.size(), 3u); } +TEST(EditableMeshStandalone, LoadFromAssimpFileAppliesZUpBakeWhenRequested) { + // Regression for the quads follow-up: when the source asset was + // declared Z-up (FBX UpAxis = 2), MeshProcessor bakes a +90° X + // rotation into the rendered Ogre buffers. loadFromAssimpFile must + // apply the SAME rotation when isZup=true so the editable mesh + // lives in the same basis. Without this, vertex overlays would + // appear rotated 90° on Z-up assets. + // + // OBJ ignores UpAxis metadata, so passing isZup=true here triggers + // the rotation deterministically regardless of file format. + const QString path = writeObj("editmesh_zup", + "v 0 0 0\nv 1 0 0\nv 0 1 0\n", // y=1 vertex at index 2 + "f 1 2 3\n"); + ASSERT_FALSE(path.isEmpty()); + + EditableMesh ymesh; + ASSERT_TRUE(ymesh.loadFromAssimpFile( + path.toStdString(), /*convertLH=*/false, /*isZup=*/false)); + EditableMesh zmesh; + ASSERT_TRUE(zmesh.loadFromAssimpFile( + path.toStdString(), /*convertLH=*/false, /*isZup=*/true)); + QFile::remove(path); + + ASSERT_EQ(ymesh.subMeshes()[0].vertices.size(), 3u); + ASSERT_EQ(zmesh.subMeshes()[0].vertices.size(), 3u); + + // The vertex at OBJ index 3 is (0,1,0) — Y-up. After +90° X bake + // (R_x90 * (0,1,0) = (0,0,1)) it should land on the Z axis. + const auto& y = ymesh.subMeshes()[0].vertices[2].position; + const auto& z = zmesh.subMeshes()[0].vertices[2].position; + EXPECT_NEAR(y.x, 0.0f, 1e-5f); + EXPECT_NEAR(y.y, 1.0f, 1e-5f); + EXPECT_NEAR(y.z, 0.0f, 1e-5f); + EXPECT_NEAR(z.x, 0.0f, 1e-5f); + EXPECT_NEAR(z.y, 0.0f, 1e-5f); + EXPECT_NEAR(z.z, 1.0f, 1e-5f); +} + +TEST(EditableMeshStandalone, LoadFromAssimpFileSkipsBonesNotInSkeleton) { + // Regression for the quads follow-up bone-handle bug. Without a + // skeleton lookup, this path emitted aiBone mesh-local indices as + // if they were Ogre bone handles. With skeletonForBoneHandles=null + // the legacy behaviour (mesh-local indices) is preserved for + // unskinned meshes; with a non-null skeleton, only resolvable + // bones contribute assignments. This standalone test exercises + // the unskinned-mesh path (OBJ has no bones), confirming the new + // signature compiles and behaves identically when there are no + // bones — the skinned-mesh skeleton-lookup case is exercised in + // the EditModeController integration tests where a real skeleton + // is available. + const QString path = writeObj("editmesh_nobone", + "v 0 0 0\nv 1 0 0\nv 0 1 0\n", + "f 1 2 3\n"); + ASSERT_FALSE(path.isEmpty()); + + EditableMesh m1, m2; + ASSERT_TRUE(m1.loadFromAssimpFile(path.toStdString())); + ASSERT_TRUE(m2.loadFromAssimpFile( + path.toStdString(), /*convertLH=*/true, /*isZup=*/false, + /*skeletonForBoneHandles=*/nullptr)); + QFile::remove(path); + + // Both should produce identical output for an unskinned mesh. + ASSERT_EQ(m1.subMeshCount(), 1u); + ASSERT_EQ(m2.subMeshCount(), 1u); + EXPECT_EQ(m1.subMeshes()[0].vertices.size(), + m2.subMeshes()[0].vertices.size()); + for (const auto& v : m1.subMeshes()[0].vertices) { + EXPECT_TRUE(v.boneAssignments.empty()); + } +} + TEST(EditableMeshStandalone, LoadFromAssimpFileReplacesPreviousContents) { // Loading into a non-empty EditableMesh should replace the // existing submeshes — like buildFromEditableMesh does. diff --git a/src/MeshImporterExporter.cpp b/src/MeshImporterExporter.cpp index 8e398df67..a2e8bbb16 100755 --- a/src/MeshImporterExporter.cpp +++ b/src/MeshImporterExporter.cpp @@ -1023,6 +1023,18 @@ void MeshImporterExporter::importer(const QStringList &_uriList, unsigned int ad // every non-.x asset. (Chunk 4.) mesh->getUserObjectBindings().setUserAny( "qtme.source_convert_lh", Ogre::Any(convertLH)); + // Cache the source up-axis (1 = Y-up, 2 = Z-up) so + // EditModeController can apply MeshProcessor's + // +90°-around-X bake when re-importing the asset + // through the n-gon-aware path. Without this the + // editable representation lives in pre-bake space + // while the rendered buffers are post-bake — the + // overlays appear rotated 90° on FBX/glTF Z-up + // assets, and a commit would write the rotated + // positions back. (Quad migration follow-up.) + mesh->getUserObjectBindings().setUserAny( + "qtme.source_up_axis", + Ogre::Any(importer.getSceneUpAxis())); } if (!mesh) { // Animation-only file: skeleton/animations were loaded, but there is no mesh. From 49f5a2b589c004d5c23370296a4f70ead4bee8d2 Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 20:30:10 -0400 Subject: [PATCH 2/9] fix(quads): preserve bump map + per-pixel lighting after topology ops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- src/EditModeController.cpp | 253 ++++++++++++----------------- src/EditModeController.h | 13 ++ src/EditModeController_test.cpp | 20 +++ src/commands/TransformCommands.cpp | 7 +- 4 files changed, 144 insertions(+), 149 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index 3c5f8df68..45ec5f89f 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -1733,24 +1733,9 @@ bool EditModeController::extrudeSelection() if (!m_editableMesh->resizeEntityBuffers(m_editEntity)) return false; - // Force Entity to rebuild its SubEntity list from the updated Mesh. - // Without this, Ogre's skeletal skinning pipeline may use stale - // animation blend buffers that still reference the old vertex layout. - m_editEntity->_deinitialise(); - m_editEntity->_initialise(true); - - // Invalidate RTSS shaders for the entity's materials so they regenerate - // against the new vertex declaration (with possibly new tangent / blend - // indices elements). Without this, bump maps / skinning may render wrong. - auto* shaderGen = Ogre::RTShader::ShaderGenerator::getSingletonPtr(); - if (shaderGen) { - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) { - const std::string& matName = m_editEntity->getSubEntity(i)->getMaterialName(); - if (!matName.empty()) - shaderGen->invalidateMaterial( - Ogre::RTShader::ShaderGenerator::DEFAULT_SCHEME_NAME, matName); - } - } + // Refresh Entity caches + RTSS state (incl. tangent rebuild for + // bump-mapped materials). See `rewriteEntityAfterTopologyChange`. + rewriteEntityAfterTopologyChange(m_editEntity); // Select the new (offset) vertices by position — offset ensures uniqueness m_selectedVertices.clear(); @@ -1874,34 +1859,10 @@ bool EditModeController::applyBevelTopology( if (!m_editableMesh->resizeEntityBuffers(m_editEntity)) return false; - // _deinitialise/_initialise rebuilds SubEntities and resets their - // material to the SubMesh default. In edit mode the SubEntity holds - // the wireframe variant and MaterialEditor writes go to the SubEntity - // (not the SubMesh), so both must be preserved across the rebuild. - std::vector preMats; - preMats.reserve(m_editEntity->getNumSubEntities()); - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) { - preMats.push_back(m_editEntity->getSubEntity(i)->getMaterialName()); - } - - m_editEntity->_deinitialise(); - m_editEntity->_initialise(true); - - for (unsigned int i = 0; - i < m_editEntity->getNumSubEntities() && i < preMats.size(); ++i) { - if (!preMats[i].empty()) - m_editEntity->getSubEntity(i)->setMaterialName(preMats[i]); - } - - auto* shaderGen = Ogre::RTShader::ShaderGenerator::getSingletonPtr(); - if (shaderGen) { - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) { - const std::string& matName = m_editEntity->getSubEntity(i)->getMaterialName(); - if (!matName.empty()) - shaderGen->invalidateMaterial( - Ogre::RTShader::ShaderGenerator::DEFAULT_SCHEME_NAME, matName); - } - } + // Refresh Entity caches + RTSS state (incl. tangent rebuild for + // bump-mapped materials), preserving per-subentity material + // overrides (wireframe variant, MaterialEditor writes). + rewriteEntityAfterTopologyChange(m_editEntity); m_selectedVertices.clear(); m_selectedEdges.clear(); @@ -1999,29 +1960,7 @@ bool EditModeController::applyBevelVertexTopology( if (!m_editableMesh->resizeEntityBuffers(m_editEntity)) return false; - std::vector preMats; - preMats.reserve(m_editEntity->getNumSubEntities()); - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) - preMats.push_back(m_editEntity->getSubEntity(i)->getMaterialName()); - - m_editEntity->_deinitialise(); - m_editEntity->_initialise(true); - - for (unsigned int i = 0; - i < m_editEntity->getNumSubEntities() && i < preMats.size(); ++i) { - if (!preMats[i].empty()) - m_editEntity->getSubEntity(i)->setMaterialName(preMats[i]); - } - - auto* shaderGen = Ogre::RTShader::ShaderGenerator::getSingletonPtr(); - if (shaderGen) { - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) { - const std::string& matName = m_editEntity->getSubEntity(i)->getMaterialName(); - if (!matName.empty()) - shaderGen->invalidateMaterial( - Ogre::RTShader::ShaderGenerator::DEFAULT_SCHEME_NAME, matName); - } - } + rewriteEntityAfterTopologyChange(m_editEntity); m_selectedVertices.clear(); m_selectedEdges.clear(); @@ -2431,35 +2370,10 @@ void EditModeController::cancelBevel() m_selectedEdges = std::move(m_bevelSession.origSelectedEdges); m_selectedFaces = std::move(m_bevelSession.origSelectedFaces); - // Re-sync the entity with the restored mesh. Snapshot SubEntity - // materials before _deinitialise/_initialise (Ogre resets them to - // the SubMesh default) so wireframe / material-editor overrides - // survive the Esc path, matching the commit path. + // Re-sync the entity with the restored mesh — preserves wireframe / + // material-editor SubEntity overrides through the Esc path. m_editableMesh->resizeEntityBuffers(m_editEntity); - if (m_editEntity) { - std::vector preMats; - preMats.reserve(m_editEntity->getNumSubEntities()); - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) - preMats.push_back(m_editEntity->getSubEntity(i)->getMaterialName()); - - m_editEntity->_deinitialise(); - m_editEntity->_initialise(true); - - for (unsigned int i = 0; - i < m_editEntity->getNumSubEntities() && i < preMats.size(); ++i) { - if (!preMats[i].empty()) - m_editEntity->getSubEntity(i)->setMaterialName(preMats[i]); - } - } - auto* shaderGen = Ogre::RTShader::ShaderGenerator::getSingletonPtr(); - if (shaderGen && m_editEntity) { - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) { - const std::string& matName = m_editEntity->getSubEntity(i)->getMaterialName(); - if (!matName.empty()) - shaderGen->invalidateMaterial( - Ogre::RTShader::ShaderGenerator::DEFAULT_SCHEME_NAME, matName); - } - } + rewriteEntityAfterTopologyChange(m_editEntity); m_bevelSession = {}; if (m_bevelGizmo) m_bevelGizmo->setVisible(false); @@ -2644,29 +2558,7 @@ bool EditModeController::commitKnife() m_editableMesh->subMeshes() = std::move(updated.subMeshes()); m_editableMesh->resizeEntityBuffers(m_editEntity); - std::vector preMats; - preMats.reserve(m_editEntity->getNumSubEntities()); - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) { - preMats.push_back(m_editEntity->getSubEntity(i)->getMaterialName()); - } - - m_editEntity->_deinitialise(); - m_editEntity->_initialise(true); - - for (unsigned int i = 0; - i < m_editEntity->getNumSubEntities() && i < preMats.size(); ++i) { - if (!preMats[i].empty()) - m_editEntity->getSubEntity(i)->setMaterialName(preMats[i]); - } - - if (auto* shaderGen = Ogre::RTShader::ShaderGenerator::getSingletonPtr()) { - for (unsigned int i = 0; i < m_editEntity->getNumSubEntities(); ++i) { - const std::string& matName = m_editEntity->getSubEntity(i)->getMaterialName(); - if (!matName.empty()) - shaderGen->invalidateMaterial( - Ogre::RTShader::ShaderGenerator::DEFAULT_SCHEME_NAME, matName); - } - } + rewriteEntityAfterTopologyChange(m_editEntity); // Clear edge/face selection — pre-cut IDs refer to retired topology // slots and the walk added new vertices that aren't in any existing @@ -2703,17 +2595,88 @@ bool EditModeController::commitKnife() // the HE primitive (centroid / first / last / per-cluster centroid for // by-distance). `applyMergeOp` factors that boilerplate. // --------------------------------------------------------------------------- -namespace { -// Shared post-mesh-mutation hook used by knife + merge: rewrite the entity's -// Ogre buffers, save / restore per-subentity material overrides, and tell -// RTSS to re-link shaders against the new vertex layout. Captures locally -// to avoid a public helper just for this. -inline void rewriteEntityAfterTopologyChange(Ogre::Entity* ent) { +// Shared post-mesh-mutation hook: rewrite the entity's Ogre buffers, +// save / restore per-subentity material overrides, and tell RTSS to +// re-link shaders against the new vertex layout. Used by every Edit- +// Mode topology op AND by EditMeshTopologyCommand::applyMeshState so +// undo/redo preserves bump map / per-pixel lighting state. +// +// Bump-map / per-pixel lighting handling: after a topology op the new +// vertices written by EditableMesh::buildSubMeshBuffers have zero-valued +// tangents (EditableVertex defaults), and the declaration may even drop +// the VES_TANGENT slot entirely if `vertices[0].hasTangent == false` +// (which is the case on the n-gon import path that omits +// aiProcess_CalcTangentSpace). Either way RTSS's SRS_NORMALMAP TBN math +// collapses, dropping bump mapping and (depending on the shader path) +// basic per-pixel lighting. Fix: force `Mesh::buildTangentVectors` +// BEFORE `_deinitialise/_initialise` so the SubEntity vertex-decl cache +// reads the corrected layout, then re-run `applyNormalMapsToEntity` so +// RTSS re-attaches SRS_NORMALMAP against the fresh tangents. +void EditModeController::rewriteEntityAfterTopologyChange(Ogre::Entity* ent) { + if (!ent) return; std::vector preMats; preMats.reserve(ent->getNumSubEntities()); for (unsigned int i = 0; i < ent->getNumSubEntities(); ++i) preMats.push_back(ent->getSubEntity(i)->getMaterialName()); + // Rebuild tangents BEFORE `_deinitialise/_initialise` so the + // SubEntity vertex-decl cache captured during `_initialise` already + // sees the VES_TANGENT element. Calling `buildTangentVectors` AFTER + // `_initialise` is too late: the SubEntity has already linked + // against the old (no-tangent) declaration and the next render + // compiles RTSS shaders against that stale layout, so the + // SRS_NORMALMAP code path collapses to a no-op even though the + // tangents physically exist in the buffer. + // + // We need tangents whenever any subentity is bump-mapped (has a + // `normal_map`/`NormalMap` TUS). Two cases trigger that: + // 1. The mesh declaration ALREADY has VES_TANGENT but the new + // vertices written by `EditableMesh::buildSubMeshBuffers` are + // zero-valued (default-constructed `EditableVertex::tangent`). + // 2. The declaration LOST VES_TANGENT during the buffer rewrite — + // this happens when the n-gon import path (which doesn't + // request `aiProcess_CalcTangentSpace` because it forces + // triangulation) didn't have tangents in the source file, so + // `EditableVertex::hasTangent == false` for every vertex and + // the rebuilt declaration drops the slot entirely. + // Either way `Mesh::buildTangentVectors` is the fix: it adds the + // VES_TANGENT element if missing and (re)computes values from + // positions/normals/UVs. + if (auto mesh = ent->getMesh()) { + bool wantsTangents = false; + for (unsigned int i = 0; i < ent->getNumSubEntities(); ++i) { + auto mat = ent->getSubEntity(i)->getMaterial(); + if (!mat) continue; + if (!mat->isLoaded()) { + try { mat->load(); } catch (...) {} + } + if (mat->getNumTechniques() == 0) continue; + auto* pass = mat->getTechnique(0)->getPass(0); + if (!pass) continue; + for (unsigned short t = 0; t < pass->getNumTextureUnitStates(); ++t) { + const auto& tusName = + pass->getTextureUnitState(t)->getName(); + if (tusName == "normal_map" || tusName == "NormalMap") { + wantsTangents = true; + break; + } + } + if (wantsTangents) break; + } + if (wantsTangents) { + try { + // storeParityInW=true → VET_FLOAT4, matching what + // RTShaderHelper::applyNormalMap and the import path use. + mesh->buildTangentVectors( + Ogre::VES_TANGENT, 0, 0, false, false, true); + } catch (const Ogre::Exception& e) { + Ogre::LogManager::getSingleton().logMessage( + "rewriteEntityAfterTopologyChange: buildTangentVectors " + "failed for '" + mesh->getName() + "': " + e.getDescription()); + } + } + } + // Re-init the entity so its SubEntity caches (vertex / index // counts, skeleton anim buffers) sync to the resized mesh. // Without this the next render uses stale draw-call params and @@ -2727,24 +2690,23 @@ inline void rewriteEntityAfterTopologyChange(Ogre::Entity* ent) { ent->getSubEntity(i)->setMaterialName(preMats[i]); } - // Invalidate the cached RTSS technique so the next render - // regenerates it against the new vertex declaration. Lazy regen - // via SchemeResolverListener::handleSchemeNotFound runs on the - // next render. (Same pattern every other topology op uses.) - // - // KNOWN ISSUE post-chunk-4: bump-mapped meshes loaded through - // the n-gon import path lose their bump map (and sometimes basic - // per-pixel lighting) after a topology op. The cause is that - // `EditableMesh::loadFromAssimpFile` deliberately skips - // aiProcess_CalcTangentSpace (it forces triangulation) and the - // post-edit GPU upload path doesn't always reliably trigger an - // Ogre `buildTangentVectors` rebuild. Various RTSS - // re-attach permutations (sync / deferred / via - // applyNormalMapsToEntity / invalidate-only) all reproduce the - // failure on the same model. Tracked for a follow-up fix-PR - // with proper shader-pipeline instrumentation rather than - // continued blind permutation. Triangle-only assets and - // procedural primitives are unaffected. + // Re-attach the RTSS SRS_NORMALMAP sub-render-state per material that + // has a normal-map TUS. `invalidateMaterial` alone only drops the + // cached shader programs; the renderState's template list is + // preserved so the regenerated shader would normally inherit + // SRS_NORMALMAP — except `applyNormalMap` uses + // `removeShaderBasedTechnique + createShaderBasedTechnique` to start + // from a clean slate (some tangent-related state in the render state + // doesn't survive a buffer-format change), so we must run it again + // to guarantee the SRS_NORMALMAP is re-bound against the new + // tangents. Materials without a normal-map TUS are no-ops here. + MeshImporterExporter::applyNormalMapsToEntity(ent); + + // Final invalidate so any per-material cache that survived the + // applyNormalMap path drops too. validateMaterial inside + // applyNormalMap already kicks shader regen, but explicit + // invalidate keeps behaviour identical to other topology ops for + // materials that aren't bump-mapped. if (auto* sg = Ogre::RTShader::ShaderGenerator::getSingletonPtr()) { for (unsigned int i = 0; i < ent->getNumSubEntities(); ++i) { const std::string& m = ent->getSubEntity(i)->getMaterialName(); @@ -2754,7 +2716,6 @@ inline void rewriteEntityAfterTopologyChange(Ogre::Entity* ent) { } } } -} // namespace // Find global vertex indices of the survivor positions after toEditableMesh // re-packed the per-submesh arrays. Each survivor is the unique vertex at @@ -3031,7 +2992,7 @@ int applyTopologyMutationNoSurvivor( editableMesh->recalculateNormalsFlat(); editableMesh->resizeEntityBuffers(editEntity); - rewriteEntityAfterTopologyChange(editEntity); + EditModeController::rewriteEntityAfterTopologyChange(editEntity); selVerts.clear(); selEdges.clear(); diff --git a/src/EditModeController.h b/src/EditModeController.h index de551da0c..7dec3d4f3 100644 --- a/src/EditModeController.h +++ b/src/EditModeController.h @@ -885,6 +885,19 @@ private slots: void applyWireframeMaterials(); void removeWireframeMaterials(); +public: + /// Refresh an entity after a topology mutation: rebuild tangents + /// (when any material is bump-mapped), `_deinitialise/_initialise` + /// the entity, restore per-subentity material overrides, re-attach + /// RTSS SRS_NORMALMAP, and invalidate cached shader programs. Used + /// by every Edit-Mode topology op (subdivide, extrude, bevel, …) + /// AND by `EditMeshTopologyCommand::applyMeshState` so undo/redo + /// preserves bump map / per-pixel lighting state. Static so + /// command code can invoke it without a controller instance. + static void rewriteEntityAfterTopologyChange(Ogre::Entity* ent); + +private: + // Wireframe mode bool m_wireframeEnabled = false; std::map m_savedMaterials; ///< SubEntity index → original material name diff --git a/src/EditModeController_test.cpp b/src/EditModeController_test.cpp index 650c8cd5a..9f4a81ce6 100644 --- a/src/EditModeController_test.cpp +++ b/src/EditModeController_test.cpp @@ -27,6 +27,26 @@ The MIT License #include #include +// =========================================================================== +// rewriteEntityAfterTopologyChange — null + no-bump-map shape (chunk: quads +// follow-up). Full bump-map / RTSS exercise needs a GL context, which the +// test infra doesn't provide on macOS; the integration coverage lives in +// hand smoke tests on the bump-mapped Mixamo asset. Here we lock down the +// public-API contract: static, callable from outside the class (notably +// from EditMeshTopologyCommand::applyMeshState in the undo/redo path), +// null-tolerant, and a no-op when no entity material requests tangents. +// =========================================================================== + +TEST(EditModeControllerStandalone, RewriteEntityAfterTopologyChangeNullIsNoOp) { + // Regression for chunk 4b → quads follow-up: the static helper is + // reachable from command code in TransformCommands.cpp via a class- + // qualified call, and accepts a null entity without crashing. If + // someone refactors it back to a free function in an anonymous + // namespace, undo/redo will silently lose its lighting hook again. + EditModeController::rewriteEntityAfterTopologyChange(nullptr); + SUCCEED(); +} + // =========================================================================== // Pure geometry tests (no Ogre needed) // =========================================================================== diff --git a/src/commands/TransformCommands.cpp b/src/commands/TransformCommands.cpp index 458cc1495..e3830c36b 100644 --- a/src/commands/TransformCommands.cpp +++ b/src/commands/TransformCommands.cpp @@ -664,9 +664,10 @@ void EditMeshTopologyCommand::applyMeshState( // and skeletal skinning are preserved through undo/redo. ctrl->currentMesh()->resizeEntityBuffers(ctrl->editEntity()); - // Refresh Entity's SubEntity caches for the new vertex layout - ctrl->editEntity()->_deinitialise(); - ctrl->editEntity()->_initialise(true); + // Refresh Entity caches + RTSS state — same hook every topology op + // uses, so undo/redo preserves bump map / per-pixel lighting on + // bump-mapped assets through the n-gon import path. + EditModeController::rewriteEntityAfterTopologyChange(ctrl->editEntity()); // Restore full selection state (vertices, edges, and faces) ctrl->deselectAll(); From 60a118dd5fdec12817beab7185eb52a56854ea2b Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 21:08:42 -0400 Subject: [PATCH 3/9] fix(quads): fill produces a single n-gon; knife works on quad meshes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/EditModeController.cpp | 18 +++++++++++++-- src/HalfEdgeMesh.cpp | 30 +++++++++++++++++-------- src/HalfEdgeMesh_test.cpp | 45 ++++++++++++++++++++++++++++++-------- 3 files changed, 73 insertions(+), 20 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index 6a41141a7..447313bb0 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -2559,8 +2559,22 @@ bool EditModeController::commitKnife() // consecutive endpoints, so the visible preview becomes a real chain // of mesh edges. OnFace/OnVertex clicks aren't yet in scope — the // commit skips them and proceeds on the OnEdge subset. + // + // splitEdge — the primitive cutPath uses — is a triangle-only MVP + // (n-gon support requires ear-clip-aware rewiring). On a quad- + // imported mesh, every face is an n-gon and splitEdge bails + // immediately, so knife silently fails. Workaround until a proper + // n-gon splitEdge lands: build the HE from a triangle-mode COPY + // (clear `.faces` so buildFromEditableMesh falls back to the + // fan-triangulated `.triangles` mirror). The user gets a working + // knife at the cost of materialising the fan diagonals on every + // submesh the cut touches. Tracked as a follow-up. + EditableMesh triOnly; + triOnly.subMeshes() = m_editableMesh->subMeshes(); + for (auto& sub : triOnly.subMeshes()) + sub.faces.clear(); HalfEdgeMesh hm; - if (!hm.buildFromEditableMesh(*m_editableMesh)) { + if (!hm.buildFromEditableMesh(triOnly)) { cancelKnife(); return false; } @@ -3481,7 +3495,7 @@ int EditModeController::fillSelection() validateMesh(); SentryReporter::addBreadcrumb("edit_mode", - QString("Fill (verts=%1, tris=%2)") + QString("Fill (verts=%1, faces=%2)") .arg(targetVerts.size()).arg(created)); updateSelectionOverlay(); diff --git a/src/HalfEdgeMesh.cpp b/src/HalfEdgeMesh.cpp index 40653c393..1d9385a16 100644 --- a/src/HalfEdgeMesh.cpp +++ b/src/HalfEdgeMesh.cpp @@ -5325,22 +5325,34 @@ int HalfEdgeMesh::fillSelection(const std::vector& vertexIndices) return 0; } - // 3. Fan-triangulate from vertexIndices[0]. For n=3 emits one triangle; - // for n=4 emits two; for general N emits N-2. - int triCount = 0; - for (int i = 1; i + 1 < n; ++i) { - appendTriangle(vertexIndices[0], vertexIndices[i], - vertexIndices[i + 1], subIdx); - ++triCount; + // 3. Build the new face. For n=3 emit a single triangle (matches + // the existing fan output); for n>=4 emit a single n-gon HEFace + // so the result round-trips back through toEditableMesh as one + // EditableFace with N indices, not N-2 fan triangles. Without + // this branch, filling a 4-vertex selection would create two + // triangles whose shared diagonal then surfaces as a fan-edge + // in Edit Mode (and Standard Subdivide treats them as + // triangles, not as a quad). Returns the number of polygons + // created (always 1 here — kept as `int` to preserve the + // function signature; callers use the return only as a + // success/fail flag). + int created; + if (n == 3) { + appendTriangle(vertexIndices[0], vertexIndices[1], + vertexIndices[2], subIdx); + created = 1; + } else { + const int fIdx = appendFace(vertexIndices, subIdx); + if (fIdx < 0) return 0; + created = 1; } - if (triCount == 0) return 0; rebuildEdgesAndTwins(); compactBoundaryHalfEdges(); buildBoundaryHalfEdges(); fixVertexHalfEdges(); - return triCount; + return created; } bool HalfEdgeMesh::validate() const diff --git a/src/HalfEdgeMesh_test.cpp b/src/HalfEdgeMesh_test.cpp index 9bf64eac9..3599a4e84 100644 --- a/src/HalfEdgeMesh_test.cpp +++ b/src/HalfEdgeMesh_test.cpp @@ -4293,19 +4293,35 @@ TEST(HalfEdgeMeshStandalone, FillSelectionFourVerticesEmitsTwoTriangles) { ASSERT_TRUE(he.buildFromEditableMesh(mesh)); ASSERT_EQ(activeFaceCount(he), 1); - // Fan-triangulate the (3, 4, 5, 6) quad from vertex 3. Produces - // (3, 4, 5) and (3, 5, 6) — a watertight 4-vertex fill. - EXPECT_EQ(he.fillSelection({3, 4, 5, 6}), 2); - EXPECT_EQ(activeFaceCount(he), 3); + // Fill (3, 4, 5, 6) as a single quad face — NOT a fan-triangulation. + // Pre-quads-followup this returned 2 (triangle count); now it + // returns 1 (face count) and the result round-trips back through + // toEditableMesh as one 4-index EditableFace. + EXPECT_EQ(he.fillSelection({3, 4, 5, 6}), 1); + EXPECT_EQ(activeFaceCount(he), 2); EXPECT_TRUE(he.validate()); EditableMesh back; ASSERT_TRUE(he.toEditableMesh(back)); EXPECT_TRUE(isManifold(back)); + + // The new face must round-trip as a quad EditableFace, not 2 tris. + ASSERT_EQ(back.subMeshes().size(), 1u); + const auto& subBack = back.subMeshes()[0]; + ASSERT_FALSE(subBack.faces.empty()) + << "n>=4 fill must populate EditableSubMesh::faces"; + bool sawQuad = false; + for (const auto& f : subBack.faces) { + if (f.indices.size() == 4) { sawQuad = true; break; } + } + EXPECT_TRUE(sawQuad) + << "fillSelection of 4 verts must produce a single quad face"; } -TEST(HalfEdgeMeshStandalone, FillSelectionFiveVerticesFanTriangulatesIntoThree) { - // Pentagon fan-triangulated from vertexIndices[0]: 5 vertices → 3 tris. +TEST(HalfEdgeMeshStandalone, FillSelectionFiveVerticesProducesSinglePentagon) { + // Pentagon fill: 5 vertices → ONE 5-vertex EditableFace, not 3 fan + // triangles. (Quads-followup: a single n-gon HEFace round-trips as + // a single n-gon EditableFace through toEditableMesh.) EditableMesh mesh; EditableSubMesh sub; sub.materialName = "FillMat"; @@ -4325,8 +4341,19 @@ TEST(HalfEdgeMeshStandalone, FillSelectionFiveVerticesFanTriangulatesIntoThree) ASSERT_TRUE(he.buildFromEditableMesh(mesh)); // Vertices 0..4 form a convex pentagon. Fill its loop in order. - EXPECT_EQ(he.fillSelection({0, 1, 2, 3, 4}), 3); + EXPECT_EQ(he.fillSelection({0, 1, 2, 3, 4}), 1); EXPECT_TRUE(he.validate()); + + // Confirm round-trip preserves the 5-gon. + EditableMesh back; + ASSERT_TRUE(he.toEditableMesh(back)); + ASSERT_EQ(back.subMeshes().size(), 1u); + bool sawPentagon = false; + for (const auto& f : back.subMeshes()[0].faces) { + if (f.indices.size() == 5) { sawPentagon = true; break; } + } + EXPECT_TRUE(sawPentagon) + << "fillSelection of 5 verts must produce a single pentagon face"; } // =========================================================================== @@ -4537,8 +4564,8 @@ TEST(HalfEdgeMeshStandalone, FillSelectionAcceptsOrphanedVertices) { HalfEdgeMesh he; ASSERT_TRUE(he.buildFromEditableMesh(mesh)); - // Fill with all 4 orphans → 2 new triangles (fan from vert 0). - EXPECT_EQ(he.fillSelection({0, 1, 3, 2}), 2); + // Fill with all 4 orphans → ONE new quad face (quads-followup). + EXPECT_EQ(he.fillSelection({0, 1, 3, 2}), 1); EXPECT_TRUE(he.validate()); } From 1aaff0d48b8cb67b20672e8435fd2413d415c644 Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 21:10:56 -0400 Subject: [PATCH 4/9] fix(quads): guard normal-map mat->load against broken resources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/MeshImporterExporter.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/MeshImporterExporter.cpp b/src/MeshImporterExporter.cpp index a2e8bbb16..00a858195 100755 --- a/src/MeshImporterExporter.cpp +++ b/src/MeshImporterExporter.cpp @@ -904,7 +904,19 @@ void MeshImporterExporter::applyNormalMapsToEntity(const Ogre::Entity* en) auto mat = subEnt->getMaterial(); if (!mat) continue; // Ensure the material is fully loaded so TUS names are populated. - if (!mat->isLoaded()) mat->load(); + // `load()` can throw on broken/unresolvable resources; this hook + // runs on every topology mutation AND undo/redo, so a single + // bad material shouldn't abort the edit op. Skip the offending + // sub-entity and let the rest of the entity refresh. + if (!mat->isLoaded()) { + try { + mat->load(); + } catch (const Ogre::Exception& e) { + log.logMessage("applyNormalMapsToEntity: skipping mat '" + + mat->getName() + "' — load failed: " + e.getDescription()); + continue; + } + } if (mat->getNumTechniques() == 0) continue; auto* pass = mat->getTechnique(0)->getPass(0); if (!pass) continue; From 4de903ac37a2af9b89c2e28d1fe9b12570d66ab4 Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 21:48:54 -0400 Subject: [PATCH 5/9] fix(quads): fill orients to surrounding mesh; knife uses same HE in hit-test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/EditModeController.cpp | 12 +++++++++- src/HalfEdgeMesh.cpp | 46 ++++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index 8cea81224..de55fc379 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -3553,8 +3553,18 @@ bool EditModeController::knifeHitTest(const QPoint& screenPos, OgreWidget* widge rD = worldToLocal.linear() * rD; } + // Build the HE from a triangle-mode COPY of the editable mesh, + // mirroring what the knife commit path does. Otherwise the HE + // edge indices we record on the click here (n-gon HE: one edge + // per polygon side) wouldn't match the indices `commitKnife` + // resolves against (triangle HE: one edge per fan side), and + // every cut would land on the wrong edge — manifesting as a + // knife that "does nothing" on quad-imported assets. + EditableMesh triOnly; + triOnly.subMeshes() = m_editableMesh->subMeshes(); + for (auto& sub : triOnly.subMeshes()) sub.faces.clear(); HalfEdgeMesh tmp; - if (tmp.buildFromEditableMesh(*m_editableMesh)) { + if (tmp.buildFromEditableMesh(triOnly)) { const Ogre::Vector3 camPosWorld = camera->getDerivedPosition(); constexpr float kPixelRadius = 10.0f; float bestDepth = std::numeric_limits::infinity(); diff --git a/src/HalfEdgeMesh.cpp b/src/HalfEdgeMesh.cpp index 1d9385a16..22e490c1f 100644 --- a/src/HalfEdgeMesh.cpp +++ b/src/HalfEdgeMesh.cpp @@ -5325,7 +5325,46 @@ int HalfEdgeMesh::fillSelection(const std::vector& vertexIndices) return 0; } - // 3. Build the new face. For n=3 emit a single triangle (matches + // 3. Pick a winding that faces the same way as the surrounding mesh. + // Without this, a fill on a hole's boundary loop produces a face + // whose normal points INTO the volume (the user's selection order + // is whatever std::set / the loop walker produced; nothing + // guarantees it matches the existing topology). + // Heuristic: compare the candidate face's Newell normal to the + // average 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. + auto computeNewellNormal = + [this](const std::vector& verts) -> Ogre::Vector3 { + Ogre::Vector3 normal = Ogre::Vector3::ZERO; + const size_t N = verts.size(); + for (size_t i = 0; i < N; ++i) { + const auto& a = m_vertices[verts[i]].position; + const auto& b = m_vertices[verts[(i + 1) % N]].position; + normal.x += (a.y - b.y) * (a.z + b.z); + normal.y += (a.z - b.z) * (a.x + b.x); + normal.z += (a.x - b.x) * (a.y + b.y); + } + return normal; + }; + + Ogre::Vector3 candidateNormal = computeNewellNormal(vertexIndices); + Ogre::Vector3 referenceNormal = Ogre::Vector3::ZERO; + for (int v : vertexIndices) { + for (int f : facesAroundVertex(v)) { + const auto fv = faceVertices(f); + if (fv.size() < 3) continue; + referenceNormal += computeNewellNormal(fv); + } + } + std::vector winding = vertexIndices; + if (referenceNormal.squaredLength() > 1e-12f + && candidateNormal.squaredLength() > 1e-12f + && candidateNormal.dotProduct(referenceNormal) < 0.0f) { + std::reverse(winding.begin(), winding.end()); + } + + // 4. Build the new face. For n=3 emit a single triangle (matches // the existing fan output); for n>=4 emit a single n-gon HEFace // so the result round-trips back through toEditableMesh as one // EditableFace with N indices, not N-2 fan triangles. Without @@ -5338,11 +5377,10 @@ int HalfEdgeMesh::fillSelection(const std::vector& vertexIndices) // success/fail flag). int created; if (n == 3) { - appendTriangle(vertexIndices[0], vertexIndices[1], - vertexIndices[2], subIdx); + appendTriangle(winding[0], winding[1], winding[2], subIdx); created = 1; } else { - const int fIdx = appendFace(vertexIndices, subIdx); + const int fIdx = appendFace(winding, subIdx); if (fIdx < 0) return 0; created = 1; } From 197a67361047f85fdbff7098c714430115dc4cb8 Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 22:23:11 -0400 Subject: [PATCH 6/9] fix(quads): knife accepts OnVertex clicks on dense imported meshes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- src/EditModeController.cpp | 46 +++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index de55fc379..781710480 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -2523,24 +2523,44 @@ bool EditModeController::commitKnife() return false; } - // Refuse the commit if any confirmed point isn't snapped to an edge. - // OnFace and OnVertex captures exist for the preview, but the MVP - // commit pipeline only knows how to cut along edges; silently - // dropping a face click would produce a mesh that doesn't match the - // line the user just drew, which is worse than failing the commit. + // Build the CutPoint list, translating OnVertex clicks into edge + // clicks: pick any incident edge to the vertex and use t=0 or t=1 + // depending on which endpoint the vertex sits at. `cutPath` only + // accepts edge inputs (its `splitEdge` primitive is edge-keyed), + // and `splitEdge` clamps t away from 0/1 by 1e-4 to avoid sliver + // faces — so the resulting vertex sits a hair off the user's click + // but topology stays clean. OnFace clicks are still rejected: the + // commit can't represent them with the current edge-walk algorithm. + std::vector cpts; + cpts.reserve(m_knifeSession.points.size()); for (const auto& p : m_knifeSession.points) { - if (p.kind != KnifePoint::OnEdge) { + if (p.kind == KnifePoint::OnEdge) { + cpts.push_back({p.edgeIndex, p.edgeT}); + continue; + } + if (p.kind != KnifePoint::OnVertex) { SentryReporter::addBreadcrumb("edit_mode", - "Knife: commit rejected (non-edge point — only edge cuts supported)"); + "Knife: commit rejected (OnFace point — only edge / vertex supported)"); cancelKnife(); return false; } - } - - std::vector cpts; - cpts.reserve(m_knifeSession.points.size()); - for (const auto& p : m_knifeSession.points) { - cpts.push_back({p.edgeIndex, p.edgeT}); + // OnVertex → find any incident edge in the triangle-mode HE + // and pick the t that puts the new split-vertex closest to the + // clicked vertex. + int incidentEdge = -1; + float incidentT = 0.0f; + for (size_t e = 0; e < hm.edgeCount(); ++e) { + const auto [ev0, ev1] = hm.edgeVertices(static_cast(e)); + if (ev0 == p.vertexIndex) { incidentEdge = static_cast(e); incidentT = 0.0f; break; } + if (ev1 == p.vertexIndex) { incidentEdge = static_cast(e); incidentT = 1.0f; break; } + } + if (incidentEdge < 0) { + SentryReporter::addBreadcrumb("edit_mode", + "Knife: commit rejected (OnVertex point — no incident edge)"); + cancelKnife(); + return false; + } + cpts.push_back({incidentEdge, incidentT}); } if (cpts.size() < 2) { SentryReporter::addBreadcrumb("edit_mode", From 4c96d0f71d9096f9e4049fcdba2885472ac4c638 Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 22:39:31 -0400 Subject: [PATCH 7/9] fix(quads): knife hit-test culls back-face geometry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/EditModeController.cpp | 80 +++++++++++++++++++++++++++++++++++--- 1 file changed, 75 insertions(+), 5 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index 781710480..2b2156ae9 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -3536,10 +3536,70 @@ bool EditModeController::knifeHitTest(const QPoint& screenPos, OgreWidget* widge const int vw = widget->width(); const int vh = widget->height(); + // Build the front-facing vertex / edge sets once (mirrors the + // chunk-4b behaviour of regular hitTestEdge / face selection): on + // a dense mesh, snapping to back-face geometry that's hidden behind + // the visible surface is impossible to control. Front-facing means + // the polygon's Newell normal points toward the camera. A vertex is + // front-facing if at least one incident polygon faces the camera; + // an edge is front-facing if at least one of its two adjacent + // polygons does. Edge keys use (min,max) global vertex indices so + // they line up with the triangle-mode HE used below. + Ogre::SceneNode* node = m_editEntity->getParentSceneNode(); + const Ogre::Vector3 camPos = camera->getDerivedPosition(); + auto isPolygonFrontFacing = [&](const EditableSubMesh& sub, + const std::vector& corners) -> bool { + if (corners.size() < 3 || !node) return false; + Ogre::Vector3 nrm = Ogre::Vector3::ZERO; + for (size_t i = 0; i < corners.size(); ++i) { + if (corners[i] >= sub.vertices.size()) return false; + const auto& a = sub.vertices[corners[i]].position; + const auto& b = sub.vertices[corners[(i + 1) % corners.size()]].position; + nrm.x += (a.y - b.y) * (a.z + b.z); + nrm.y += (a.z - b.z) * (a.x + b.x); + nrm.z += (a.x - b.x) * (a.y + b.y); + } + const Ogre::Vector3 worldNrm = node->_getDerivedOrientation() * nrm; + const Ogre::Vector3 anyCornerWorld = + node->convertLocalToWorldPosition(sub.vertices[corners[0]].position); + return worldNrm.dotProduct(camPos - anyCornerWorld) > 0.0f; + }; + + std::set frontVerts; + std::set> frontEdges; + for (size_t si = 0; si < m_editableMesh->subMeshes().size(); ++si) { + const auto& sub = m_editableMesh->subMeshes()[si]; + const int vertOffset = localToGlobal(static_cast(si), 0); + auto recordFrontPoly = [&](const std::vector& corners) { + for (size_t i = 0; i < corners.size(); ++i) { + const int g0 = vertOffset + static_cast(corners[i]); + const int g1 = vertOffset + static_cast(corners[(i + 1) % corners.size()]); + frontVerts.insert(g0); + frontEdges.emplace(std::min(g0, g1), std::max(g0, g1)); + } + }; + if (!sub.faces.empty()) { + for (const auto& face : sub.faces) { + if (face.indices.size() < 3) continue; + if (!isPolygonFrontFacing(sub, face.indices)) continue; + recordFrontPoly(face.indices); + } + } else { + for (const auto& tri : sub.triangles) { + std::vector corners = { + tri.indices[0], tri.indices[1], tri.indices[2] }; + if (!isPolygonFrontFacing(sub, corners)) continue; + recordFrontPoly(corners); + } + } + } + // Priority 1: vertex snap. Uses the same radius as regular edit-mode - // vertex picking so the user's eye can predict the snap. + // vertex picking so the user's eye can predict the snap. Skip + // back-face vertices (not in `frontVerts`) so dense meshes don't + // pull clicks to vertices the user can't see. const int snapVert = hitTestVertex(screenPos, camera, vw, vh, 10.0f); - if (snapVert >= 0) { + if (snapVert >= 0 && frontVerts.count(snapVert)) { auto [subIdx, localIdx] = globalToLocal(snapVert); if (subIdx < m_editableMesh->subMeshes().size() && localIdx < m_editableMesh->subMeshes()[subIdx].vertices.size()) { @@ -3563,7 +3623,6 @@ bool EditModeController::knifeHitTest(const QPoint& screenPos, OgreWidget* widge const Ogre::Real nx = static_cast(screenPos.x()) / vw; const Ogre::Real ny = static_cast(screenPos.y()) / vh; const Ogre::Ray ray = camera->getCameraToViewportRay(nx, ny); - Ogre::SceneNode* node = m_editEntity->getParentSceneNode(); Ogre::Vector3 rO = ray.getOrigin(); Ogre::Vector3 rD = ray.getDirection(); @@ -3585,7 +3644,6 @@ bool EditModeController::knifeHitTest(const QPoint& screenPos, OgreWidget* widge for (auto& sub : triOnly.subMeshes()) sub.faces.clear(); HalfEdgeMesh tmp; if (tmp.buildFromEditableMesh(triOnly)) { - const Ogre::Vector3 camPosWorld = camera->getDerivedPosition(); constexpr float kPixelRadius = 10.0f; float bestDepth = std::numeric_limits::infinity(); int bestEdgeIdx = -1; @@ -3595,6 +3653,18 @@ bool EditModeController::knifeHitTest(const QPoint& screenPos, OgreWidget* widge for (size_t e = 0; e < tmp.edgeCount(); ++e) { auto [gv0, gv1] = tmp.edgeVertices(static_cast(e)); if (gv0 < 0 || gv1 < 0) continue; + // Skip back-face edges. `frontEdges` was computed from + // the n-gon `m_editableMesh`, keyed by global vertex + // pair; the triangle-mode `tmp` indices use the same + // global numbering, so the lookup is direct. Note that + // `frontEdges` only contains polygon-perimeter edges: + // a fan-triangulation diagonal between two front-facing + // verts is rejected here, which is correct — the user + // shouldn't be able to click on a fake interior edge. + { + const std::pair key{std::min(gv0, gv1), std::max(gv0, gv1)}; + if (!frontEdges.count(key)) continue; + } auto [sub0, loc0] = globalToLocal(gv0); auto [sub1, loc1] = globalToLocal(gv1); if (sub0 >= m_editableMesh->subMeshes().size()) continue; @@ -3633,7 +3703,7 @@ bool EditModeController::knifeHitTest(const QPoint& screenPos, OgreWidget* widge } const Ogre::Vector3 local = p0 + d * t; const Ogre::Vector3 world = node->convertLocalToWorldPosition(local); - const float depth = (world - camPosWorld).dotProduct(camera->getDerivedDirection()); + const float depth = (world - camPos).dotProduct(camera->getDerivedDirection()); if (depth <= 0.0f) continue; // behind camera if (depth < bestDepth) { bestDepth = depth; From 2b0a1ccda050cf9de13db4cdf82edde39e8a37eb Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 22:42:44 -0400 Subject: [PATCH 8/9] refactor(quads): extract helpers to satisfy Sonar complexity gate 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. --- src/EditModeController.cpp | 303 +++++++++++++++++-------------------- 1 file changed, 141 insertions(+), 162 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index 2b2156ae9..d48c82d3e 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -256,6 +256,55 @@ void EditModeController::toggleEditMode() enterEditMode(); } +// Try the n-gon-aware re-import path: when the entity's mesh has the +// `qtme.source_path` user-binding (cached by MeshImporterExporter at +// import time AND not yet wiped by a topology mutation), re-read the +// source asset through Assimp with aiProcess_Triangulate disabled so +// source quads survive into EditableSubMesh::faces. The cached +// `qtme.source_convert_lh` and `qtme.source_up_axis` keys make the +// editable mesh share basis with the rendered buffers; passing the +// live skeleton makes bone handles match MeshProcessor's convention. +// Returns true on success (caller uses the editable mesh as-is), +// false to signal that the legacy `loadFromEntity` path should run. +static bool tryLoadEditableMeshNGonPath( + Ogre::Entity* entity, EditableMesh* editableMesh) +{ + if (!entity || !editableMesh) return false; + const Ogre::MeshPtr meshPtr = entity->getMesh(); + if (!meshPtr) return false; + + const auto& bindings = meshPtr->getUserObjectBindings(); + const Ogre::Any& any = bindings.getUserAny("qtme.source_path"); + if (!any.has_value()) return false; + + std::string sourcePath; + try { + sourcePath = Ogre::any_cast(any); + } catch (const Ogre::Exception&) { + return false; // Any held the wrong type — fall back defensively. + } + if (sourcePath.empty()) return false; + + // Default: convert-LH=true matches AssimpToOgreImporter's behaviour + // for unknown origins; up-axis=Y-up is the safe default if the + // import didn't cache one. + bool convertLH = true; + const Ogre::Any& lhAny = bindings.getUserAny("qtme.source_convert_lh"); + if (lhAny.has_value()) { + try { convertLH = Ogre::any_cast(lhAny); } + catch (const Ogre::Exception&) {} + } + bool isZup = false; + const Ogre::Any& upAny = bindings.getUserAny("qtme.source_up_axis"); + if (upAny.has_value()) { + try { isZup = (Ogre::any_cast(upAny) == 2); } + catch (const Ogre::Exception&) {} + } + const Ogre::Skeleton* skel = + meshPtr->hasSkeleton() ? meshPtr->getSkeleton().get() : nullptr; + return editableMesh->loadFromAssimpFile(sourcePath, convertLH, isZup, skel); +} + bool EditModeController::enterEditMode() { if (m_editModeActive) @@ -270,97 +319,20 @@ bool EditModeController::enterEditMode() QList entities = sel->getResolvedEntities(); m_editEntity = entities.first(); - // Decompose mesh into editable data. Two paths: - // - // - n-gon path: when MeshImporterExporter has cached the source - // file path (qtme.source_path) on the Ogre::Mesh AND no edit - // has been committed since import (commitToEntity / resize- - // EntityBuffers wipe the cache on mutation), re-import the - // asset through Assimp with aiProcess_Triangulate disabled so - // source quads survive into EditableSubMesh::faces. - // - // - legacy path: read the live Ogre buffers via loadFromEntity. - // This is what every prior chunk used, and what every code - // path that doesn't have a source path (procedural primitives, - // .scene.glb sub-entities, post-edit re-entries) falls back to. - // - // The n-gon path enables Catmull-Clark subdivision, loop cut, and - // any future quad-aware op to act on real source quads instead of - // the diagonal triangulation Assimp emits by default. - // (Quad migration #326, chunk 4.) + // Decompose mesh into editable data. Prefer the n-gon path (re- + // import via Assimp with aiProcess_Triangulate off) when the mesh + // still carries qtme.source_path. Fall back to the legacy + // loadFromEntity path for procedural primitives, .scene.glb sub- + // entities, and post-edit re-entries (where commitToEntity / + // resizeEntityBuffers wipe the cache on mutation). The n-gon path + // is what enables Catmull-Clark subdivide / loop cut / future + // quad-aware ops to act on real source quads. (Quad migration + // #326, chunk 4.) m_editableMesh = std::make_unique(); - - bool loaded = false; - { - const Ogre::MeshPtr meshPtr = m_editEntity->getMesh(); - if (meshPtr) { - const auto& bindings = meshPtr->getUserObjectBindings(); - const Ogre::Any& any = bindings.getUserAny("qtme.source_path"); - if (any.has_value()) { - try { - const std::string sourcePath = - Ogre::any_cast(any); - // The original importer cached its - // convert-to-left-handed choice alongside the path. - // Apply the SAME flag here so the editable mesh - // stays in the same coordinate system as the - // rendered Ogre buffers — without this, on every - // non-.x asset the vertex / edge / face overlays - // would draw mirrored (X flipped) relative to the - // on-screen geometry. Defaults to true to match - // AssimpToOgreImporter's behaviour for unknown - // origins. - bool convertLH = true; - const Ogre::Any& lhAny = - bindings.getUserAny("qtme.source_convert_lh"); - if (lhAny.has_value()) { - try { - convertLH = Ogre::any_cast(lhAny); - } catch (const Ogre::Exception&) {} - } - // The original importer also caches the source up- - // axis (1 = Y-up, 2 = Z-up). MeshProcessor bakes a - // +90°-around-X rotation into the rendered buffers - // for Z-up assets; we pass `isZup` so the editable - // representation stays in the same basis. Without - // this, FBX/glTF assets that declare Z-up would - // surface vertex overlays rotated 90° relative to - // the on-screen geometry, and a commit would write - // the rotated positions back, silently rotating - // the entity. - bool isZup = false; - const Ogre::Any& upAny = - bindings.getUserAny("qtme.source_up_axis"); - if (upAny.has_value()) { - try { - isZup = (Ogre::any_cast(upAny) == 2); - } catch (const Ogre::Exception&) {} - } - // Resolve aiBone names against the live skeleton so - // the n-gon path emits Ogre bone HANDLES (matching - // MeshProcessor) instead of mesh-local aiBone - // indices. Without this, a topology op on a skinned - // mesh re-emits VertexBoneAssignments with wild - // handles and skinning rebinds vertices to wrong - // bones. - const Ogre::Skeleton* skel = nullptr; - if (meshPtr->hasSkeleton()) { - skel = meshPtr->getSkeleton().get(); - } - if (!sourcePath.empty() - && m_editableMesh->loadFromAssimpFile( - sourcePath, convertLH, isZup, skel)) { - loaded = true; - SentryReporter::addBreadcrumb("edit_mode", - "Edit Mode entered via n-gon import path"); - } - } catch (const Ogre::Exception&) { - // The Any contained something other than a string — - // shouldn't happen but fall through to the legacy - // path defensively. - } - } - } + bool loaded = tryLoadEditableMeshNGonPath(m_editEntity, m_editableMesh.get()); + if (loaded) { + SentryReporter::addBreadcrumb("edit_mode", + "Edit Mode entered via n-gon import path"); } if (!loaded && !m_editableMesh->loadFromEntity(m_editEntity)) { SentryReporter::addBreadcrumb("edit_mode", "Failed to load mesh data for Edit Mode"); @@ -2646,8 +2618,70 @@ bool EditModeController::commitKnife() // BEFORE `_deinitialise/_initialise` so the SubEntity vertex-decl cache // reads the corrected layout, then re-run `applyNormalMapsToEntity` so // RTSS re-attaches SRS_NORMALMAP against the fresh tangents. +namespace { +// Returns true if any sub-entity of `ent` has a normal-map TUS — the +// signal we use to decide whether RTSS will need tangents on the next +// render. Materials that fail to load are skipped so a single broken +// resource doesn't abort the topology op. +bool entityWantsTangents(Ogre::Entity* ent) { + for (unsigned int i = 0; i < ent->getNumSubEntities(); ++i) { + auto mat = ent->getSubEntity(i)->getMaterial(); + if (!mat) continue; + if (!mat->isLoaded()) { + try { mat->load(); } + catch (const Ogre::Exception&) { continue; } + } + if (mat->getNumTechniques() == 0) continue; + auto* pass = mat->getTechnique(0)->getPass(0); + if (!pass) continue; + for (unsigned short t = 0; t < pass->getNumTextureUnitStates(); ++t) { + const auto& tusName = pass->getTextureUnitState(t)->getName(); + if (tusName == "normal_map" || tusName == "NormalMap") return true; + } + } + return false; +} + +// Force-rebuild tangent vectors on `mesh`. Logs (rather than throws) on +// failure since the caller runs from inside an entity-refresh hook. +void rebuildMeshTangents(const Ogre::MeshPtr& mesh) { + if (!mesh) return; + try { + // storeParityInW=true → VET_FLOAT4, matching what + // RTShaderHelper::applyNormalMap and the import path use. + mesh->buildTangentVectors(/*sourceTexCoordSet=*/0, + /*splitMirrored=*/false, + /*splitRotated=*/false, + /*storeParityInW=*/true); + } catch (const Ogre::Exception& e) { + Ogre::LogManager::getSingleton().logMessage( + "rewriteEntityAfterTopologyChange: buildTangentVectors " + "failed for '" + mesh->getName() + "': " + e.getDescription()); + } +} + +// Drop the cached RTSS shader programs for every sub-entity material +// so the next render regenerates them against the post-topology-op +// vertex declaration. No-op when the ShaderGenerator is absent. +void invalidateEntityRtssMaterials(Ogre::Entity* ent) { + auto* sg = Ogre::RTShader::ShaderGenerator::getSingletonPtr(); + if (!sg) return; + for (unsigned int i = 0; i < ent->getNumSubEntities(); ++i) { + const std::string& m = ent->getSubEntity(i)->getMaterialName(); + if (!m.empty()) + sg->invalidateMaterial( + Ogre::RTShader::ShaderGenerator::DEFAULT_SCHEME_NAME, m); + } +} +} // namespace + void EditModeController::rewriteEntityAfterTopologyChange(Ogre::Entity* ent) { if (!ent) return; + + // Snapshot per-subentity material overrides — _deinitialise/_initialise + // resets them to the SubMesh default, but the wireframe overlay and + // MaterialEditor write to the SubEntity, not the SubMesh, so both + // must survive the rebuild. std::vector preMats; preMats.reserve(ent->getNumSubEntities()); for (unsigned int i = 0; i < ent->getNumSubEntities(); ++i) @@ -2662,54 +2696,15 @@ void EditModeController::rewriteEntityAfterTopologyChange(Ogre::Entity* ent) { // SRS_NORMALMAP code path collapses to a no-op even though the // tangents physically exist in the buffer. // - // We need tangents whenever any subentity is bump-mapped (has a - // `normal_map`/`NormalMap` TUS). Two cases trigger that: - // 1. The mesh declaration ALREADY has VES_TANGENT but the new - // vertices written by `EditableMesh::buildSubMeshBuffers` are - // zero-valued (default-constructed `EditableVertex::tangent`). - // 2. The declaration LOST VES_TANGENT during the buffer rewrite — - // this happens when the n-gon import path (which doesn't - // request `aiProcess_CalcTangentSpace` because it forces - // triangulation) didn't have tangents in the source file, so - // `EditableVertex::hasTangent == false` for every vertex and - // the rebuilt declaration drops the slot entirely. - // Either way `Mesh::buildTangentVectors` is the fix: it adds the - // VES_TANGENT element if missing and (re)computes values from - // positions/normals/UVs. - if (auto mesh = ent->getMesh()) { - bool wantsTangents = false; - for (unsigned int i = 0; i < ent->getNumSubEntities(); ++i) { - auto mat = ent->getSubEntity(i)->getMaterial(); - if (!mat) continue; - if (!mat->isLoaded()) { - try { mat->load(); } catch (...) {} - } - if (mat->getNumTechniques() == 0) continue; - auto* pass = mat->getTechnique(0)->getPass(0); - if (!pass) continue; - for (unsigned short t = 0; t < pass->getNumTextureUnitStates(); ++t) { - const auto& tusName = - pass->getTextureUnitState(t)->getName(); - if (tusName == "normal_map" || tusName == "NormalMap") { - wantsTangents = true; - break; - } - } - if (wantsTangents) break; - } - if (wantsTangents) { - try { - // storeParityInW=true → VET_FLOAT4, matching what - // RTShaderHelper::applyNormalMap and the import path use. - mesh->buildTangentVectors( - Ogre::VES_TANGENT, 0, 0, false, false, true); - } catch (const Ogre::Exception& e) { - Ogre::LogManager::getSingleton().logMessage( - "rewriteEntityAfterTopologyChange: buildTangentVectors " - "failed for '" + mesh->getName() + "': " + e.getDescription()); - } - } - } + // We rebuild whenever any subentity is bump-mapped: new vertices + // written by `EditableMesh::buildSubMeshBuffers` start with zero- + // valued tangents (EditableVertex defaults), and on the n-gon + // import path — which omits `aiProcess_CalcTangentSpace` because + // that flag forces triangulation — the declaration drops VES_TANGENT + // entirely. `Mesh::buildTangentVectors` re-adds the element if + // missing and recomputes values from positions/normals/UVs. + if (entityWantsTangents(ent)) + rebuildMeshTangents(ent->getMesh()); // Re-init the entity so its SubEntity caches (vertex / index // counts, skeleton anim buffers) sync to the resized mesh. @@ -2724,31 +2719,15 @@ void EditModeController::rewriteEntityAfterTopologyChange(Ogre::Entity* ent) { ent->getSubEntity(i)->setMaterialName(preMats[i]); } - // Re-attach the RTSS SRS_NORMALMAP sub-render-state per material that - // has a normal-map TUS. `invalidateMaterial` alone only drops the - // cached shader programs; the renderState's template list is - // preserved so the regenerated shader would normally inherit - // SRS_NORMALMAP — except `applyNormalMap` uses - // `removeShaderBasedTechnique + createShaderBasedTechnique` to start - // from a clean slate (some tangent-related state in the render state - // doesn't survive a buffer-format change), so we must run it again - // to guarantee the SRS_NORMALMAP is re-bound against the new - // tangents. Materials without a normal-map TUS are no-ops here. + // Re-attach the RTSS SRS_NORMALMAP sub-render-state per material + // that has a normal-map TUS, then drop any cached shader programs + // so the next render regenerates against the new vertex layout. + // `applyNormalMapsToEntity` uses `removeShaderBasedTechnique + + // createShaderBasedTechnique` to start from a clean slate (some + // tangent-related state doesn't survive a buffer-format change), + // so it must run AFTER the tangent rebuild + _initialise. MeshImporterExporter::applyNormalMapsToEntity(ent); - - // Final invalidate so any per-material cache that survived the - // applyNormalMap path drops too. validateMaterial inside - // applyNormalMap already kicks shader regen, but explicit - // invalidate keeps behaviour identical to other topology ops for - // materials that aren't bump-mapped. - if (auto* sg = Ogre::RTShader::ShaderGenerator::getSingletonPtr()) { - for (unsigned int i = 0; i < ent->getNumSubEntities(); ++i) { - const std::string& m = ent->getSubEntity(i)->getMaterialName(); - if (!m.empty()) - sg->invalidateMaterial( - Ogre::RTShader::ShaderGenerator::DEFAULT_SCHEME_NAME, m); - } - } + invalidateEntityRtssMaterials(ent); } // Find global vertex indices of the survivor positions after toEditableMesh From c7da02ca6a40f766009da3d7828d374de59dcc44 Mon Sep 17 00:00:00 2001 From: Fernando Date: Tue, 28 Apr 2026 23:14:37 -0400 Subject: [PATCH 9/9] fix(quads): knife restores n-gon faces on untouched submeshes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/EditModeController.cpp | 40 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/src/EditModeController.cpp b/src/EditModeController.cpp index d48c82d3e..b13a2ec50 100644 --- a/src/EditModeController.cpp +++ b/src/EditModeController.cpp @@ -2487,8 +2487,17 @@ bool EditModeController::commitKnife() // submesh the cut touches. Tracked as a follow-up. EditableMesh triOnly; triOnly.subMeshes() = m_editableMesh->subMeshes(); - for (auto& sub : triOnly.subMeshes()) + // Remember which submeshes were originally n-gon-canonical + // (`!faces.empty()`) so we can restore them post-write-back. Without + // this, a knife on submesh 0 would convert every untouched submesh + // to fan triangles globally (since toEditableMesh writes back from + // the all-triangulated HE). + std::vector wasNGonSub; + wasNGonSub.reserve(triOnly.subMeshes().size()); + for (auto& sub : triOnly.subMeshes()) { + wasNGonSub.push_back(!sub.faces.empty()); sub.faces.clear(); + } HalfEdgeMesh hm; if (!hm.buildFromEditableMesh(triOnly)) { cancelKnife(); @@ -2561,7 +2570,34 @@ bool EditModeController::commitKnife() cancelKnife(); return false; } - m_editableMesh->subMeshes() = std::move(updated.subMeshes()); + + // Determine which submeshes the cut actually touched. A cut creates + // new vertices either at edge clicks or at interior crossings; each + // new vertex lives on faces in the submesh(es) it pierced. + // toEditableMesh writes EVERY submesh as triangle-only (since the + // HE was triangulated up-front), so without restoring untouched + // n-gon submeshes back from the original snapshot, a single knife + // op would silently convert unrelated submeshes to fan triangles. + // (Codex P1 review on this PR.) + std::set touchedSubs; + for (int v : cutVerts) { + for (int f : hm.facesAroundVertex(v)) { + touchedSubs.insert(hm.face(f).subMeshIndex); + } + } + auto& outSubs = updated.subMeshes(); + for (size_t s = 0; + s < outSubs.size() && s < originalSubMeshes.size() && s < wasNGonSub.size(); + ++s) { + if (touchedSubs.count(static_cast(s))) continue; + if (!wasNGonSub[s]) continue; // already triangle-only — nothing to restore + // Untouched + originally n-gon → restore the original submesh + // verbatim. resizeEntityBuffers consumes both `triangles` and + // bone assignments, so we want the full pre-cut state. + outSubs[s] = originalSubMeshes[s]; + } + + m_editableMesh->subMeshes() = std::move(outSubs); m_editableMesh->resizeEntityBuffers(m_editEntity); rewriteEntityAfterTopologyChange(m_editEntity);