fix: face identity is IsSame, and the stale half of the shared-face test

.github/workflows/tests.yml

@@ -20,8 +20,9 @@ name: tests
 # macOS-only: the OCCTSwift dependency ships as a macOS xcframework, the same
 # reasoning OCCTSwiftScripts' own tests.yml uses.
 #
-# Baseline at merge: 314 tests across 27 suites, of which 5 fail (OCCTSwiftAIS#46,
-# pre-existing and inherited from before the merge).
+# Baseline at merge was 314 tests across 27 suites, of which 5 failed (OCCTSwiftAIS#46,
+# inherited from before the merge). Green since OCCTSwiftInteraction#1 settled face
+# identity on `IsSame` and rewrote the stale half of that test: 314 tests, 0 failures.
 
 on:
   push:

CLAUDE.md

@@ -70,20 +70,28 @@ OCCTSwiftIO built clean with zero errors while carrying three real breaks in its
 Dependencies resolve against local siblings when present (`../OCCTSwift` and friends), else the
 published URLs. No binary lives in this repo.
 
-**Expected baseline: 314 tests across 27 suites, of which 5 fail.** See below.
+**Expected baseline: 314 tests across 27 suites, all passing.**
 
-## Known failing test, do not "fix" it the easy way
+## Face identity is `IsSame`, and that decision is settled
 
-`InteractiveContextMutationTests` fails 5 assertions
-([OCCTSwiftInteraction#1](https://github.com/SecondMouseAU/OCCTSwiftInteraction/issues/1),
-formerly OCCTSwiftAIS#46). Inherited from before the merge.
+Phase 0 of ecosystem#43, decided in
+[OCCTSwiftInteraction#1](https://github.com/SecondMouseAU/OCCTSwiftInteraction/issues/1). Do not
+re-open it; encode it.
 
-It asserts the raw-versus-deduplicated `Shape.faces()` split that OCCTSwift **2.0.0** erased. Not a
-v3.0.0 regression: `Shape.faces()` is byte-identical between the v2.0.0 and v3.0.0 tags.
+Face identity keys on OCCT's `TopoDS_Shape::IsSame`: same `TShape`, same `Location`, orientation may
+differ. Consequences, all settled:
 
-**Do not relax the assertions.** They are the regression test OCCTSwiftAIS#31 exists to keep. The
-real fix is deciding whether face identity keys on `faces()` or `orientedFaces()`, which is also
-phase 0 of ecosystem#43 and blocks the rest of that work.
+- `faces()`, deduplicated through `TopTools_IndexedMapOfShape`, is the correct enumeration for
+  `FaceIdentityTable`. `orientedFaces()` is the occurrence enumeration and is not an identity.
+- A face shared between two shells is **one** identity, not two. That it bounds two solids is a fact
+  about the model, not two selectable things.
+- A caller needing to know *which* use of a shared face was picked reads orientation off the
+  returned shape, which is OCCT's own answer, not a second enumeration.
+
+The mesher still walks face **occurrences**, so a shared face is tessellated once per owning shell
+and both triangulations carry the one deduplicated ordinal. `InteractiveContextMutationTests`
+holds that down: it asserts both shells' copies reach the mesh and that picks into either resolve to
+the same durable uid. If you change the tessellation or identity path, that test is the tripwire.
 
 ## Active known duplication
 

README.md

@@ -59,10 +59,11 @@ Your `import` lines do not change. See [docs/MIGRATION.md](docs/MIGRATION.md).
 - Historical changelogs: [OCCTSwiftTools](docs/CHANGELOG-OCCTSwiftTools.md),
   [OCCTSwiftAIS](docs/CHANGELOG-OCCTSwiftAIS.md)
 
-## Known issues
-
-`InteractiveContextMutationTests` fails 5 assertions, carried over from before the merge and tracked
-as [OCCTSwiftAIS#46](https://github.com/SecondMouseAU/OCCTSwiftAIS/issues/46). The test asserts a
-`Shape.faces()` enumeration split that OCCTSwift 2.0.0 erased. Fixing it requires deciding whether
-face identity keys on `faces()` or `orientedFaces()`, which is also the first step of the picking
-consolidation.
+## Face identity
+
+Face identity keys on OCCT's `TopoDS_Shape::IsSame`: same `TShape`, same `Location`, orientation may
+differ ([#1](https://github.com/SecondMouseAU/OCCTSwiftInteraction/issues/1)). So `faces()`, the
+enumeration deduplicated through `TopTools_IndexedMapOfShape`, is what `FaceIdentityTable` is built
+from, and a face shared between two shells is one selectable thing rather than two. A caller that
+needs to know which use of a shared face was picked reads orientation off the returned shape, which
+is how OCCT answers the same question.

Sources/OCCTSwiftTools/FaceIdentityTable.swift

@@ -9,6 +9,30 @@ import OCCTSwift
 /// `CADBodyMetadata.faceIndices`) back to the `Shape` (and, when available, the durable
 /// `GraphUID`) it was tessellated from.
 ///
+/// ## What identity means here (OCCTSwiftInteraction#1, settled)
+///
+/// Face identity keys on OCCT's `TopoDS_Shape::IsSame`: same `TShape`, same `Location`,
+/// orientation may differ. That is a comparison semantic, not a choice of function, and the
+/// enumeration follows from it: `Shape.faces()`, deduplicated through
+/// `TopTools_IndexedMapOfShape`, is `IsSame` and is what this table is built from.
+/// `orientedFaces()` is occurrence-based (`IsEqual`) and is deliberately NOT an identity here.
+///
+/// So a face shared between two shells is **one** entry in `shapes`, not two. That it bounds two
+/// solids is a fact about the model rather than two selectable things. A caller needing to know
+/// which use of a shared face was picked reads orientation off the returned `Shape`, which is
+/// OCCT's own answer (`StdSelect_BRepOwner` carries the shape, never an ordinal) rather than a
+/// second enumeration.
+///
+/// The mesher still walks face occurrences, so a shared face is tessellated once per owning shell,
+/// each wound for its own outside, and **both** triangulations carry the one deduplicated ordinal.
+/// Two triangles with the same ordinal can therefore be different geometry belonging to different
+/// shells, and they resolve to one identity. That is the intended behaviour, not a collision.
+///
+/// This table is also the reason the ordinal is durable at all. OCCT attaches the `TopoDS_Shape`
+/// to the sensitive entity when selection is computed, because its selection data is a CPU-side
+/// structure; ours is a GPU buffer of triangles, so the attachment has to happen at tessellation
+/// time instead. That is what this type is, rather than an index-caching optimisation.
+///
 /// Consumers have historically resolved a triangle's face ordinal via
 /// `shape.subShapes(ofType: .face)[ordinal]`. Before OCCTSwift v2.0.0, that assumed the
 /// render-path ordinal, which walked faces via the same raw, non-deduplicating

Tests/OCCTSwiftAISTests/InteractiveContextMutationTests.swift

@@ -90,8 +90,15 @@ struct InteractiveContextMutationTests {
     /// them) but drives it through `InteractiveContext.display` + real synthesized
     /// picks rather than calling `FaceIdentityTable` directly: this is the
     /// consumer-side regression #31 calls for: every face pick on a multi-shell
-    /// solid with a shared face must resolve to the correct sub-shape, not a
-    /// neighbour shifted by the shared face's index collapse.
+    /// solid with a shared face must resolve to the correct sub-shape.
+    ///
+    /// Face identity keys on OCCT's `IsSame` (OCCTSwiftInteraction#1): same TShape,
+    /// same Location, orientation may differ. A face shared between two shells is
+    /// therefore ONE identity, not two, and `faces()`, deduplicated through
+    /// `TopTools_IndexedMapOfShape`, is the enumeration that encodes it. The
+    /// contract under test is unchanged by that decision: picks landing on either
+    /// shell's copy of the shared face must resolve to the same durable identity,
+    /// and neither shell's copy may go missing on the way to the GPU.
     @Test func t_sharedFaceBetweenShells_everyPickResolvesToCorrectSubShape() throws {
         let box = try #require(Shape.box(width: 10, height: 10, depth: 10))
         let boxFaces = box.subShapes(ofType: .face)
@@ -103,37 +110,118 @@ struct InteractiveContextMutationTests {
             Shape.shellFromFaces([sharedFace, boxFaces[3], boxFaces[4], boxFaces[5]]))
         let compound = try #require(Shape.compound([shellA, shellB]))
 
-        // Raw render-path traversal counts the shared face once per shell (7);
-        // `subShapes(ofType:)` collapses it to 6: the exact divergence #42/#31
-        // are about. FaceIdentityTable.shapes is built from the former.
-        #expect(compound.faces().count == 7)
+        // Under `IsSame` the shared face is one face, so both deduplicated
+        // enumerations report 6 (they are one traversal now: OCCTSwift #541/#613
+        // routed `faces()` through the same `TopTools_IndexedMapOfShape` that
+        // backs `subShapes(ofType:)`). The occurrence count lives on
+        // `orientedFaces()` instead: 7, one entry per (face, owning shell) pair.
+        // Both numbers are right and answer different questions.
+        // `FaceIdentityTable` is built from the 6, per the decision.
+        #expect(compound.faces().count == 6)
         #expect(compound.subShapes(ofType: .face).count == 6)
+        #expect(compound.orientedFaces().count == 7)
 
         let ctx = makeContext()
         ctx.selectionMode = [.face]
         let obj = ctx.display(compound)
         let identity = try #require(ctx.faceIdentityTable(for: obj))
-        #expect(identity.shapes.count == 7)
-
-        // Ordinal 0 (shellA's copy) and ordinal 3 (shellB's copy, right after
-        // shellA's 3 faces) must carry the SAME durable uid.
-        let uid0 = try #require(identity.uid(forOrdinal: 0))
-        let uid3 = try #require(identity.uid(forOrdinal: 3))
-        #expect(uid0 == uid3)
-        // Sanity: the equality above isn't vacuous, an unshared ordinal differs.
-        let uid1 = try #require(identity.uid(forOrdinal: 1))
-        #expect(uid1 != uid0)
-
-        // Drive it through the real pick path for EVERY ordinal: each must
-        // resolve to a genuine face, and the two shared-face ordinals must
-        // resolve to the same uid via `handlePick`, not just via the table.
+        #expect(identity.shapes.count == 6)
+
+        // Locate the shared face by identity, not by position: an ordinal is an
+        // artifact of enumeration order, `isSame` is the decision's own primitive
+        // and is what makes this assertion survive a reordering of the fixture.
+        let sharedOrdinal = try #require(identity.shapes.firstIndex { $0.isSame(as: sharedFace) })
+        let sharedUID = try #require(identity.uid(forOrdinal: sharedOrdinal))
+        // Sanity: everything below is non-vacuous, no OTHER face carries that uid.
+        for ordinal in identity.shapes.indices where ordinal != sharedOrdinal {
+            #expect(identity.uid(forOrdinal: ordinal) != sharedUID)
+        }
+
         let body = try #require(ctx.sourceBody(for: obj))
         #expect(!body.faceIndices.isEmpty)
+        #expect(body.faceIndices.count == body.indices.count / 3)
+
+        // `vertexData` is interleaved [px, py, pz, nx, ny, nz] with stride 6.
+        func meshVertices(of triangle: Int) -> [UInt32] {
+            Array(body.indices[(triangle * 3)..<(triangle * 3 + 3)])
+        }
+        func centroid(of triangle: Int) -> SIMD3<Float> {
+            let corners = meshVertices(of: triangle).map { vertex -> SIMD3<Float> in
+                let base = Int(vertex) * 6
+                return SIMD3(
+                    body.vertexData[base], body.vertexData[base + 1], body.vertexData[base + 2])
+            }
+            return (corners[0] + corners[1] + corners[2]) / 3
+        }
+        /// Split `triangles` into connected components by shared mesh vertex.
+        /// Each owning shell tessellates the shared face into its own triangles
+        /// over its own vertices, so one component per shell's copy.
+        func copiesByVertexConnectivity(_ triangles: [Int]) -> [[Int]] {
+            var pending = triangles
+            var components: [[Int]] = []
+            while let seed = pending.popLast() {
+                var component = [seed]
+                var pool = Set(meshVertices(of: seed))
+                var grew = true
+                while grew {
+                    grew = false
+                    for (offset, triangle) in pending.enumerated().reversed()
+                    where !pool.isDisjoint(with: meshVertices(of: triangle)) {
+                        component.append(triangle)
+                        pool.formUnion(meshVertices(of: triangle))
+                        pending.remove(at: offset)
+                        grew = true
+                    }
+                }
+                components.append(component.sorted())
+            }
+            return components.sorted { ($0.first ?? 0) < ($1.first ?? 0) }
+        }
+
+        // THE regression. Under dedup, "both ordinals agree" is true by
+        // construction and so proves nothing; what can still silently break is a
+        // shell's copy of the shared face never reaching the mesh at all. The
+        // mesher walks face OCCURRENCES and stamps each with the deduplicated
+        // index, so the shared ordinal must own two independent triangulations.
+        let sharedTriangles = body.faceIndices.indices.filter {
+            Int(body.faceIndices[$0]) == sharedOrdinal
+        }
+        let copies = copiesByVertexConnectivity(sharedTriangles)
+        #expect(
+            copies.count == 2,
+            "both shells' copies of the shared face should be tessellated, got \(copies.count)")
+        if copies.count == 2 {
+            // And the second copy is the shared face again rather than a
+            // neighbour mis-stamped with its ordinal: same triangles, same place.
+            // This reads mesh geometry, not the identity table, so it is not
+            // circular with the uid assertions below.
+            let placesA = copies[0].map(centroid).sorted { ($0.x, $0.y, $0.z) < ($1.x, $1.y, $1.z) }
+            let placesB = copies[1].map(centroid).sorted { ($0.x, $0.y, $0.z) < ($1.x, $1.y, $1.z) }
+            #expect(placesA.count == placesB.count)
+            for (a, b) in zip(placesA, placesB) {
+                #expect(
+                    simd_distance(a, b) < 1e-4,
+                    "the two copies should cover the same face, got \(a) versus \(b)")
+            }
+        }
+        var copyByTriangle: [Int: Int] = [:]
+        for (copyIdx, copy) in copies.enumerated() {
+            for triangle in copy { copyByTriangle[triangle] = copyIdx }
+        }
+
+        // Drive it through the real pick path: every ordinal must resolve to a
+        // genuine face, and EVERY triangle of the shared face, from both shells'
+        // copies, must resolve to the one shared uid via `handlePick` rather than
+        // only via the table.
         var seenOrdinals: Set<Int> = []
-        var uidByOrdinal: [Int: BRepGraph.GraphUID] = [:]
+        var uidsByCopy: [[BRepGraph.GraphUID?]] = Array(repeating: [], count: copies.count)
         for (triIdx, faceOrdinalRaw) in body.faceIndices.enumerated() {
             let ordinal = Int(faceOrdinalRaw)
-            guard ordinal >= 0, !seenOrdinals.contains(ordinal) else { continue }
+            guard ordinal >= 0 else { continue }
+            let isShared = ordinal == sharedOrdinal
+            // Every triangle of the shared face; one per ordinal otherwise, which
+            // is all the reachability coverage the unshared faces need.
+            guard isShared || !seenOrdinals.contains(ordinal) else { continue }
             seenOrdinals.insert(ordinal)
 
             let raw = UInt32(triIdx) << 16
@@ -148,14 +236,21 @@ struct InteractiveContextMutationTests {
             #expect(pickedObj == obj)
             let resolvedFace: Face? = OCCTSwift.Face(ref.shape)
             #expect(resolvedFace != nil, "ordinal \(ordinal) should resolve to a genuine face")
-            uidByOrdinal[ordinal] = ref.uid
+            guard isShared else { continue }
+            #expect(
+                ref.shape.isSame(as: sharedFace),
+                "triangle \(triIdx) carries the shared ordinal so it should resolve to that face")
+            if let copyIdx = copyByTriangle[triIdx] { uidsByCopy[copyIdx].append(ref.uid) }
         }
         #expect(
-            seenOrdinals.count == 7,
-            "every one of the 7 render-path ordinals should be reachable by a pick")
-        #expect(
-            uidByOrdinal[0] != nil && uidByOrdinal[0] == uidByOrdinal[3],
-            "picks landing on either shell's copy of the shared face must resolve to the same durable identity"
-        )
+            seenOrdinals.count == 6,
+            "every one of the 6 distinct faces should be reachable by a pick")
+        for (copyIdx, uids) in uidsByCopy.enumerated() {
+            #expect(!uids.isEmpty, "copy \(copyIdx) of the shared face should have been picked")
+            #expect(
+                uids.allSatisfy { $0 == sharedUID },
+                "picks landing on either shell's copy of the shared face must resolve to the same durable identity"
+            )
+        }
     }
 }

okf/index.md

@@ -67,9 +67,10 @@ See [`references/`](references/index.md).
 - **Picking is resolved in more than one place** across these targets and across the wider fleet
   (ecosystem#43). Four implementations exist, and two have already diverged. Read that issue before
   writing anything that maps a `PickResult` to topology.
-- **`InteractiveContextMutationTests` fails 5 assertions**
-  ([#1](https://github.com/SecondMouseAU/OCCTSwiftInteraction/issues/1)), inherited from before the
-  merge. Do not fix it by relaxing the assertions.
+- **Face identity is decided and settled**: OCCT's `IsSame` semantics, so `faces()` (deduplicated)
+  is the enumeration behind `FaceIdentityTable` and a face shared between two shells is one identity
+  ([#1](https://github.com/SecondMouseAU/OCCTSwiftInteraction/issues/1), phase 0 of ecosystem#43).
+  Encode it rather than re-deciding it.
 
 ## Policies