quads follow-up: knife works on quad meshes; fill produces n-gons

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<unsigned int, Ogre::String> m_savedMaterials; ///< SubEntity index → original material name

src/EditModeController_test.cpp

@@ -27,6 +27,26 @@ The MIT License
 #include <array>
 #include <map>
 
+// ===========================================================================
+// 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)
 // ===========================================================================

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<unsigned short>(b);
+                if (skeletonForBoneHandles) {
+                    const std::string boneName = bone->mName.C_Str();
+                    if (!skeletonForBoneHandles->hasBone(boneName)) continue;
+                    handle = static_cast<unsigned short>(
+                        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<unsigned short>(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;
 }

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

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.

src/HalfEdgeMesh.cpp

@@ -5325,22 +5325,72 @@
             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. 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<int>& 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<int> 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
+    //    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(winding[0], winding[1], winding[2], subIdx);
+        created = 1;
+    } else {
+        const int fIdx = appendFace(winding, 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