From 96ce39bf0273c245bae8d01d5a43fb22a88a3491 Mon Sep 17 00:00:00 2001 From: gsdali <51393997+gsdali@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:59:13 +1000 Subject: [PATCH] Frame the camera on every body, not just the first (OCCTSwiftCADKit#19) Closes the camera half of the OCCTSwift#302 multibody ripple, tracked as ecosystem#18. Before OCCTSwift v1.11.3 a multibody STL/BREP came back as one lumped shape, so "the first shape" genuinely was the whole model. Since v1.11.3 the importers return one body per solid, and the framing code kept asking for the first one. Two paths were affected, and only one of them was reported: - `loadFile(from:progress:)` (deprecated) sets `legacyLoadedShape` to `result.shapes.first`, so `focusOnLoadedShape()` zoomed to body 0. This is what OCCTSwiftCADKit#19 and OCCTSwiftUX#12 describe. - `focus(on:)` framed via `shape(id:)`, which returns only an entity's FIRST body. The multi-entity `loadFile(from:id:progress:)` registers a multibody file as ONE entity owning N bodies, and its own documentation tells callers to "call focus(on:) once you've loaded what should be visible". So the documented way to frame a multibody import was the broken one. The issues did not mention this path. The same first-body-only assumption was fixed once before in `applySideBySide`, whose regression test records it as "fine for the roughly-frame-the-camera use in focus(on:)". It was not fine, and that comment is why this stayed open: the earlier fix stopped at the boundary of the reported symptom. Both paths now go through `combinedBounds(ofEntities:)`, which unions over `Entity.bodyIDs` rather than `shape(id:)`. That also collapses the two copies of the centre/max-dimension arithmetic that had drifted into `focus(on:)` and `focusOnLoadedShape()` into one `frameCamera(on:)`. `focusOnLoadedShape()` keeps a `currentSingleShape` fallback for any path that sets `legacyLoadedShape` without installing identity. Nothing does that today; the fallback is there so it stays a non-event rather than an unframed camera. Tested by asserting the union directly rather than the camera: `focus(on:)` animates over 0.3s, so `cameraState` read straight afterwards is mid interpolation and cannot say what was framed. Verified discriminating by reverting the union to `bodyIDs.prefix(1)`, which fails it at 1.0 against the expected 21.0. The single-body companion test compares with a tolerance rather than for exact equality: `load` tessellates the shape, and the bounding box read afterwards is computed over the triangulation, so it lands within a rounding step of the pre-tessellation one rather than on it. 362 tests pass; swift-format --strict and swiftlint --strict are clean. --- .../OCCTSwiftCADKit/CADViewportService.swift | 86 +++++++++++---- Tests/OCCTSwiftCADKitTests/SmokeTests.swift | 103 ++++++++++++++++++ 2 files changed, 168 insertions(+), 21 deletions(-) diff --git a/Sources/OCCTSwiftCADKit/CADViewportService.swift b/Sources/OCCTSwiftCADKit/CADViewportService.swift index ae40f5d..ce5d571 100644 --- a/Sources/OCCTSwiftCADKit/CADViewportService.swift +++ b/Sources/OCCTSwiftCADKit/CADViewportService.swift @@ -819,23 +819,56 @@ public final class CADViewportService { /// No-op if none of `ids` are currently loaded, or if none of the loaded ones has a /// bounding box. public func focus(on ids: [String]) { - let shapes = ids.compactMap { shape(id: $0) } - guard !shapes.isEmpty else { return } - + guard let box = combinedBounds(ofEntities: ids) else { return } + frameCamera(on: box) + } + + /// The union of every body's bounding box across the named entities, or `nil` if none of + /// them is loaded or none has bounds. + /// + /// **Unions over `Entity.bodyIDs`, not `shape(id:)`.** `shape(id:)` returns only an + /// entity's *first* body, and a multibody file loaded through + /// `loadFile(from:id:progress:)` is one entity owning N bodies. Framing off `shape(id:)` + /// therefore zoomed to body 0 and left the rest of the assembly off screen, which is + /// OCCTSwiftUX#12 / OCCTSwiftCADKit#19, the camera half of the OCCTSwift#302 multibody + /// ripple. Before OCCTSwift v1.11.3 a multibody file came back as one lumped shape, so + /// "the first shape" genuinely was the whole model and this read correctly. + /// + /// The same first-body-only assumption was already fixed once in `applySideBySide`, whose + /// regression test records it as "fine for the roughly-frame-the-camera use in + /// `focus(on:)`". It was not fine; that is what this method exists to correct. + /// + /// Internal so tests can assert the union directly. The camera itself animates toward the + /// framing over 0.3s, so `cameraState` right after a `focus(on:)` is mid-interpolation and + /// cannot answer what was framed. + func combinedBounds(ofEntities ids: [String]) + -> (min: SIMD3, max: SIMD3)? + { var minPt = SIMD3(repeating: .infinity) var maxPt = SIMD3(repeating: -.infinity) - for s in shapes { - guard let b = s.bounds else { continue } - minPt = SIMD3(min(minPt.x, b.min.x), min(minPt.y, b.min.y), min(minPt.z, b.min.z)) - maxPt = SIMD3(max(maxPt.x, b.max.x), max(maxPt.y, b.max.y), max(maxPt.z, b.max.z)) + for id in ids { + guard let entity = entities[id] else { continue } + for bodyID in entity.bodyIDs { + guard let b = bodyShapes[bodyID]?.bounds else { continue } + minPt = SIMD3(min(minPt.x, b.min.x), min(minPt.y, b.min.y), min(minPt.z, b.min.z)) + maxPt = SIMD3(max(maxPt.x, b.max.x), max(maxPt.y, b.max.y), max(maxPt.z, b.max.z)) + } } - guard minPt.x.isFinite else { return } + guard minPt.x.isFinite else { return nil } + return (minPt, maxPt) + } + + /// Points the camera at the centre of `box`, far enough back to hold its largest dimension. + /// + /// The one place that turns a bounding box into a camera move. + private func frameCamera(on box: (min: SIMD3, max: SIMD3)) { let center = SIMD3( - Float((minPt.x + maxPt.x) / 2), - Float((minPt.y + maxPt.y) / 2), - Float((minPt.z + maxPt.z) / 2) + Float((box.min.x + box.max.x) / 2), + Float((box.min.y + box.max.y) / 2), + Float((box.min.z + box.max.z) / 2) ) - let maxDim = Float(max(maxPt.x - minPt.x, max(maxPt.y - minPt.y, maxPt.z - minPt.z))) + let maxDim = Float( + max(box.max.x - box.min.x, max(box.max.y - box.min.y, box.max.z - box.min.z))) controller.focusOn(point: center, distance: maxDim * 2.5) } @@ -1668,17 +1701,28 @@ public final class CADViewportService { return SIMD3(world.x, world.y, world.z) } - /// No-op if nothing is loaded, or if the loaded shape has no bounding box: leaving the - /// camera where it is beats aiming it at the world origin. + /// Frames everything the deprecated single-shape loaders just put on screen. + /// + /// Both callers (`loadFile(from:progress:)` and `loadShape(_:id:)`) call + /// `resetAllModelState()` first, so `entities` holds exactly their own load, and framing + /// all of it is the same thing as framing what they loaded. + /// + /// It frames **every** body rather than `currentSingleShape`, which is the fix for + /// OCCTSwiftUX#12 / OCCTSwiftCADKit#19: `loadFile(from:progress:)` sets + /// `legacyLoadedShape` to `result.shapes.first`, so on a multibody file the camera used to + /// zoom to body 0 while the other bodies rendered off screen. + /// + /// No-op if nothing is loaded, or if nothing loaded has a bounding box: leaving the camera + /// where it is beats aiming it at the world origin. The `currentSingleShape` fallback + /// preserves the old behaviour for any loader path that populates `legacyLoadedShape` + /// without installing identity, which nothing does today. private func focusOnLoadedShape() { + if let box = combinedBounds(ofEntities: Array(entities.keys)) { + frameCamera(on: box) + return + } guard let shape = currentSingleShape, let b = shape.bounds else { return } - let center = SIMD3( - Float((b.min.x + b.max.x) / 2), - Float((b.min.y + b.max.y) / 2), - Float((b.min.z + b.max.z) / 2) - ) - let maxDim = Float(max(b.max.x - b.min.x, max(b.max.y - b.min.y, b.max.z - b.min.z))) - controller.focusOn(point: center, distance: maxDim * 2.5) + frameCamera(on: b) } // MARK: - Shape Info diff --git a/Tests/OCCTSwiftCADKitTests/SmokeTests.swift b/Tests/OCCTSwiftCADKitTests/SmokeTests.swift index 01ad66b..eb117f2 100644 --- a/Tests/OCCTSwiftCADKitTests/SmokeTests.swift +++ b/Tests/OCCTSwiftCADKitTests/SmokeTests.swift @@ -2857,4 +2857,107 @@ struct SmokeTests { let responseB = await taskB.value #expect(responseB == .chose(candidateID: "yes"), "B must resolve with its own real answer") } + + /// Regression for OCCTSwiftUX#12 / OCCTSwiftCADKit#19 (ecosystem#18), the camera half of + /// the OCCTSwift#302 multibody ripple. + /// + /// `loadFile(from:id:progress:)` registers a multibody file as ONE entity owning N bodies, + /// and framing used to run off `shape(id:)`, which returns only that entity's first body. + /// So the camera zoomed to body 0 and the rest of the assembly rendered off screen. + /// + /// Asserts the union rather than the camera because `focus(on:)` animates toward the + /// framing over 0.3s, so `cameraState` read straight afterwards is mid-interpolation. + /// + /// Seeds the two-body entity through the internal `entities`/`installIdentity` seams: this + /// package ships no multi-body file on disk. + @MainActor + @Test("Framing spans every body of a multi-body entity, not just the first") + func framingSpansEveryBodyOfAMultiBodyEntity() { + guard let near = Shape.box(width: 2, height: 2, depth: 2), + let farUnplaced = Shape.box(width: 2, height: 2, depth: 2) + else { + Issue.record("Shape.box returned nil") + return + } + guard + let far = farUnplaced.transformed(matrix: [ + 1, 0, 0, + 0, 1, 0, + 0, 0, 1, + 20, 0, 0, + ]) + else { + Issue.record("Shape.transformed returned nil") + return + } + guard let nearBounds = near.bounds, let farBounds = far.bounds else { + Issue.record("expected both boxes to have bounds") + return + } + + let service = CADViewportService() + service.modelBodies.append( + _ViewportBody( + id: "asm-0", vertexData: [0, 0, 0, 0, 0, 1], indices: [0], edges: [], + color: SIMD4(0.7, 0.7, 0.75, 1.0))) + service.modelBodies.append( + _ViewportBody( + id: "asm-1", vertexData: [20, 0, 0, 0, 0, 1], indices: [0], edges: [], + color: SIMD4(0.7, 0.7, 0.75, 1.0))) + service.installIdentity([ + "asm-0": ShapeIdentity(shape: near), + "asm-1": ShapeIdentity(shape: far), + ]) + service.entities["asm"] = CADViewportService.Entity(bodyIDs: ["asm-0", "asm-1"]) + + guard let box = service.combinedBounds(ofEntities: ["asm"]) else { + Issue.record("expected a two-body entity to produce bounds") + return + } + + #expect(box.min.x == min(nearBounds.min.x, farBounds.min.x)) + #expect(box.max.x == max(nearBounds.max.x, farBounds.max.x)) + + // The discriminating pair: framing body 0 alone would stop at its own far face, well + // short of the second body 20 units away. Both must hold, or the union collapsed back + // to one body. + #expect( + box.max.x > nearBounds.max.x, + "the union must reach past the first body, or the camera frames body 0 only") + #expect( + box.max.x - box.min.x > 20, + "the span must cover both bodies and the gap between them") + } + + /// The single-body case is unchanged: one entity with one body frames exactly that body. + /// + /// Guards against the multi-body fix widening the common case. + @MainActor + @Test("Framing a single-body entity still matches that body's own bounds") + func framingSingleBodyEntityIsUnchanged() { + guard let box = Shape.box(width: 4, height: 4, depth: 4), let expected = box.bounds else { + Issue.record("Shape.box returned nil, or had no bounds") + return + } + let service = CADViewportService() + service.load(box, id: "solo") + + guard let actual = service.combinedBounds(ofEntities: ["solo"]) else { + Issue.record("expected a loaded single body to produce bounds") + return + } + // Compared with a tolerance, not for exact equality: `load` tessellates the shape, and + // the bounding box read afterwards is computed over the triangulation, so it lands + // within a rounding step of the pre-tessellation one rather than on it. The point of + // this test is that the single-body case still frames that one body, not that OCCT + // returns bit-identical doubles either side of a mesh. + for i in 0..<3 { + #expect(abs(actual.min[i] - expected.min[i]) < 1e-9) + #expect(abs(actual.max[i] - expected.max[i]) < 1e-9) + } + + #expect( + service.combinedBounds(ofEntities: ["never-loaded"]) == nil, + "an unloaded id contributes nothing") + } }