More centering fixes (#200)

* More centering fixes

* Fix types

* Break out functions and add tests

* Clean up and lint

* Add missing import

* Add added/removed boundingsphere defaults

* Update darwin e2e snapshots

* Update linux e2e snapshots

* Update names

* Better error message

* Update e2e darwin

* Update e2e linux

* Consistency tweak

* Consistency tweak

src/components/diff/CadDiff.tsx

@@ -1,54 +1,106 @@
 import React, { useEffect, useState } from 'react'
 import '@react-three/fiber'
-import { Box, useTheme, Text, TabNav, StyledOcticon } from '@primer/react'
+import { Box, Text, useTheme, TabNav, StyledOcticon } from '@primer/react'
 import { FileDiff } from '../../chrome/types'
 import { Viewer3D } from './Viewer3D'
-import { OBJLoader } from 'three/examples/jsm/loaders/OBJLoader'
-import { BufferAttribute, BufferGeometry, Mesh } from 'three'
+import { BufferGeometry, Sphere } from 'three'
 import { WireframeColors, WireframeModel } from './WireframeModel'
-import { Buffer } from 'buffer'
 import { useRef } from 'react'
-import { UnifiedModel } from './UnifiedModel'
+import { CombinedModel } from './CombinedModel'
 import { BeakerIcon } from '@primer/octicons-react'
 import { LegendBox, LegendLabel } from './Legend'
+import { getCommonSphere, loadGeometry } from '../../utils/three'
 
-function loadGeometry(file: string, checkUV = false): BufferGeometry {
-    const loader = new OBJLoader()
-    const buffer = Buffer.from(file, 'base64').toString()
-    const group = loader.parse(buffer)
-    console.log(`Model ${group.id} loaded`)
-    const geometry = (group.children[0] as Mesh)?.geometry
-    if (checkUV && !geometry.attributes.uv) {
-        // UV is needed for @react-three/csg
-        // see: github.com/KittyCAD/diff-viewer-extension/issues/73
-        geometry.setAttribute(
-            'uv',
-            new BufferAttribute(new Float32Array([]), 1)
-        )
+function Viewer3D2Up({
+    beforeGeometry,
+    afterGeometry,
+    boundingSphere,
+}: {
+    beforeGeometry?: BufferGeometry
+    afterGeometry?: BufferGeometry
+    boundingSphere?: Sphere
+}) {
+    const cameraRef = useRef<any>()
+    const { theme } = useTheme()
+    const beforeColors: WireframeColors = {
+        face: theme?.colors.fg.default,
+        edge: theme?.colors.danger.muted,
+        dashEdge: theme?.colors.danger.subtle,
     }
-    geometry.computeBoundingSphere() // will be used for auto-centering
-    return geometry
+    const afterColors: WireframeColors = {
+        face: theme?.colors.fg.default,
+        edge: theme?.colors.success.muted,
+        dashEdge: theme?.colors.success.subtle,
+    }
+    return (
+        <>
+            {beforeGeometry && (
+                <Box flexGrow={1} minWidth={0} backgroundColor="danger.subtle">
+                    <Viewer3D
+                        cameraRef={cameraRef}
+                        geometry={beforeGeometry}
+                        boundingSphere={boundingSphere}
+                    >
+                        <WireframeModel
+                            geometry={beforeGeometry}
+                            boundingSphere={boundingSphere}
+                            cameraRef={cameraRef}
+                            colors={beforeColors}
+                        />
+                    </Viewer3D>
+                </Box>
+            )}
+            {afterGeometry && (
+                <Box
+                    flexGrow={1}
+                    minWidth={0}
+                    backgroundColor="success.subtle"
+                    borderLeftWidth={1}
+                    borderLeftColor="border.default"
+                    borderLeftStyle="solid"
+                >
+                    <Viewer3D
+                        cameraRef={cameraRef}
+                        geometry={afterGeometry}
+                        boundingSphere={boundingSphere}
+                    >
+                        <WireframeModel
+                            geometry={afterGeometry}
+                            boundingSphere={boundingSphere}
+                            cameraRef={cameraRef}
+                            colors={afterColors}
+                        />
+                    </Viewer3D>
+                </Box>
+            )}
+        </>
+    )
 }
 
-function Loader3DUnified({ before, after }: { before: string; after: string }) {
+function Viewer3DCombined({
+    beforeGeometry,
+    afterGeometry,
+    boundingSphere,
+}: {
+    beforeGeometry: BufferGeometry
+    afterGeometry: BufferGeometry
+    boundingSphere: Sphere
+}) {
+    const cameraRef = useRef<any>()
     const [showUnchanged, setShowUnchanged] = useState(true)
     const [showAdditions, setShowAdditions] = useState(true)
     const [showDeletions, setShowDeletions] = useState(true)
-    const cameraRef = useRef<any>()
-    const [beforeGeometry, setBeforeGeometry] = useState<BufferGeometry>()
-    const [afterGeometry, setAfterGeometry] = useState<BufferGeometry>()
-    useEffect(() => {
-        setBeforeGeometry(loadGeometry(before, true))
-    }, [before])
-    useEffect(() => {
-        setAfterGeometry(loadGeometry(after, true))
-    }, [after])
-    return beforeGeometry && afterGeometry ? (
+    return (
         <>
-            <Viewer3D cameraRef={cameraRef} geometry={beforeGeometry}>
-                <UnifiedModel
+            <Viewer3D
+                cameraRef={cameraRef}
+                geometry={beforeGeometry}
+                boundingSphere={boundingSphere}
+            >
+                <CombinedModel
                     beforeGeometry={beforeGeometry}
                     afterGeometry={afterGeometry}
+                    boundingSphere={boundingSphere}
                     cameraRef={cameraRef}
                     showUnchanged={showUnchanged}
                     showAdditions={showAdditions}
@@ -76,87 +128,67 @@ function Loader3DUnified({ before, after }: { before: string; after: string }) {
                 />
             </LegendBox>
         </>
-    ) : (
-        <Box p={3}>
-            <Text>Sorry, the rich diff can't be displayed for this file.</Text>
-        </Box>
-    )
-}
-
-function Loader3D({ file, colors }: { file: string; colors: WireframeColors }) {
-    const cameraRef = useRef<any>()
-    const [geometry, setGeometry] = useState<BufferGeometry>()
-    useEffect(() => {
-        setGeometry(loadGeometry(file))
-    }, [file])
-    return geometry ? (
-        <Viewer3D cameraRef={cameraRef} geometry={geometry}>
-            <WireframeModel
-                geometry={geometry}
-                cameraRef={cameraRef}
-                colors={colors}
-            />
-        </Viewer3D>
-    ) : (
-        <Box p={3}>
-            <Text>Sorry, the rich diff can't be displayed for this file.</Text>
-        </Box>
     )
 }
 
 export function CadDiff({ before, after }: FileDiff): React.ReactElement {
-    const canShowUnified = before && after
-    let [showUnified, setShowUnified] = useState(false)
-    const { theme } = useTheme()
-    const beforeColors: WireframeColors = {
-        face: theme?.colors.fg.default,
-        edge: theme?.colors.danger.muted,
-        dashEdge: theme?.colors.danger.subtle,
-    }
-    const afterColors: WireframeColors = {
-        face: theme?.colors.fg.default,
-        edge: theme?.colors.success.muted,
-        dashEdge: theme?.colors.success.subtle,
-    }
+    let [showCombined, setShowCombined] = useState(false)
+    const [beforeGeometry, setBeforeGeometry] = useState<BufferGeometry>()
+    const [afterGeometry, setAfterGeometry] = useState<BufferGeometry>()
+    const [boundingSphere, setBoundingSphere] = useState<Sphere>()
+    useEffect(() => {
+        let beforeGeometry: BufferGeometry | undefined = undefined
+        let afterGeometry: BufferGeometry | undefined = undefined
+        if (before) {
+            beforeGeometry = loadGeometry(before)
+            setBeforeGeometry(beforeGeometry)
+        }
+        if (after) {
+            afterGeometry = loadGeometry(after)
+            setAfterGeometry(afterGeometry)
+        }
+        if (beforeGeometry && afterGeometry) {
+            const boundingSphere = getCommonSphere(
+                beforeGeometry,
+                afterGeometry
+            )
+            setBoundingSphere(boundingSphere)
+        } else if (beforeGeometry && beforeGeometry.boundingSphere) {
+            setBoundingSphere(beforeGeometry.boundingSphere)
+        } else if (afterGeometry && afterGeometry.boundingSphere) {
+            setBoundingSphere(afterGeometry.boundingSphere)
+        }
+    }, [before, after])
     return (
         <>
-            <Box
-                display="flex"
-                height={300}
-                overflow="hidden"
-                minWidth={0}
-                position="relative"
-            >
-                {canShowUnified && showUnified && (
-                    <Loader3DUnified before={before} after={after} />
-                )}
-                {!showUnified && (
-                    <>
-                        {before && (
-                            <Box
-                                flexGrow={1}
-                                minWidth={0}
-                                backgroundColor="danger.subtle"
-                            >
-                                <Loader3D file={before} colors={beforeColors} />
-                            </Box>
+            {(beforeGeometry || afterGeometry) && (
+                <Box
+                    display="flex"
+                    height={300}
+                    overflow="hidden"
+                    minWidth={0}
+                    position="relative"
+                >
+                    {beforeGeometry &&
+                        afterGeometry &&
+                        boundingSphere &&
+                        showCombined && (
+                            <Viewer3DCombined
+                                beforeGeometry={beforeGeometry}
+                                afterGeometry={afterGeometry}
+                                boundingSphere={boundingSphere}
+                            />
                         )}
-                        {after && (
-                            <Box
-                                flexGrow={1}
-                                minWidth={0}
-                                backgroundColor="success.subtle"
-                                borderLeftWidth={1}
-                                borderLeftColor="border.default"
-                                borderLeftStyle="solid"
-                            >
-                                <Loader3D file={after} colors={afterColors} />
-                            </Box>
-                        )}
-                    </>
-                )}
-            </Box>
-            {canShowUnified && (
+                    {!showCombined && (
+                        <Viewer3D2Up
+                            beforeGeometry={beforeGeometry}
+                            afterGeometry={afterGeometry}
+                            boundingSphere={boundingSphere}
+                        />
+                    )}
+                </Box>
+            )}
+            {beforeGeometry && afterGeometry && boundingSphere && (
                 <Box
                     pt={2}
                     backgroundColor="canvas.default"
@@ -177,18 +209,18 @@ export function CadDiff({ before, after }: FileDiff): React.ReactElement {
                         }}
                     >
                         <TabNav.Link
-                            selected={!showUnified}
-                            onClick={() => setShowUnified(false)}
+                            selected={!showCombined}
+                            onClick={() => setShowCombined(false)}
                             sx={{ cursor: 'pointer' }}
                         >
-                            Side-by-side
+                            2-up
                         </TabNav.Link>
                         <TabNav.Link
-                            selected={showUnified}
-                            onClick={() => setShowUnified(true)}
+                            selected={showCombined}
+                            onClick={() => setShowCombined(true)}
                             sx={{ cursor: 'pointer' }}
                         >
-                            Unified
+                            Combined
                             <StyledOcticon
                                 icon={BeakerIcon}
                                 color="fg.muted"
@@ -199,6 +231,13 @@ export function CadDiff({ before, after }: FileDiff): React.ReactElement {
                     </TabNav>
                 </Box>
             )}
+            {!beforeGeometry && !afterGeometry && (
+                <Box p={3}>
+                    <Text>
+                        Sorry, the rich diff can't be displayed for this file.
+                    </Text>
+                </Box>
+            )}
         </>
     )
 }

src/components/diff/Camera.tsx

@@ -1,19 +1,18 @@
 import { OrthographicCamera } from '@react-three/drei'
 import { useThree } from '@react-three/fiber'
 import { useEffect, useLayoutEffect, useRef, useState } from 'react'
-import { BufferGeometry } from 'three'
+import { Sphere } from 'three'
 
-function CameraLighting({ geometry }: Props) {
+function CameraLighting({ boundingSphere }: { boundingSphere?: Sphere }) {
     const ref1 = useRef<any>()
     const ref2 = useRef<any>()
     useEffect(() => {
-        if (geometry && ref1.current) {
-            geometry.computeBoundingSphere()
-            const { radius } = geometry.boundingSphere || { radius: 1 }
+        if (ref1.current) {
+            const { radius } = boundingSphere || { radius: 1 }
             // move spot light away relative to the object's size
             ref1.current.position.setLength(radius * 15)
         }
-    }, [geometry])
+    }, [boundingSphere])
     return (
         <>
             <spotLight
@@ -23,7 +22,7 @@ function CameraLighting({ geometry }: Props) {
                 intensity={4}
                 castShadow
                 shadow-mapSize={[1024, 1024]}
-                shadowCameraNear={1}
+                shadow-cameraNear={1}
             />
             <spotLight
                 ref={ref2}
@@ -43,11 +42,7 @@ export function calculateFovFactor(fov: number, canvasHeight: number): number {
     return pixelsFromCenterToTop / Math.tan(halfFovRadians)
 }
 
-type Props = {
-    geometry: BufferGeometry
-}
-
-export function Camera({ geometry }: Props) {
+export function Camera({ boundingSphere }: { boundingSphere?: Sphere }) {
     const fov = 15
     const persRef = useRef<any>(null)
     const orthoRef = useRef<any>(null)
@@ -71,7 +66,7 @@ export function Camera({ geometry }: Props) {
     return (
         <>
             <OrthographicCamera ref={orthoRef} makeDefault>
-                <CameraLighting geometry={geometry} />
+                <CameraLighting boundingSphere={boundingSphere} />
             </OrthographicCamera>
         </>
     )

src/components/diff/CameraControls.tsx

@@ -1,16 +1,19 @@
 import { OrbitControls } from '@react-three/drei'
 import { useThree } from '@react-three/fiber'
+import { Vector3 } from 'three'
 
 type Props = {
     cameraRef: any
+    target?: Vector3
 }
 
-export function CameraControls({ cameraRef }: Props) {
+export function CameraControls({ cameraRef, target }: Props) {
     const camera = useThree(s => s.camera)
     const gl = useThree(s => s.gl)
     return (
         <OrbitControls
             makeDefault
+            target={target}
             ref={cameraRef}
             args={[camera, gl.domElement]}
             enablePan={false}

src/components/diff/CombinedModel.tsx

@@ -1,58 +1,35 @@
 import type { MutableRefObject } from 'react'
 import { useTheme } from '@primer/react'
-import {
-    Box3,
-    BufferGeometry,
-    Group,
-    Mesh,
-    MeshBasicMaterial,
-    Sphere,
-    Vector3,
-} from 'three'
+import { BufferGeometry, Sphere } from 'three'
 import { Geometry, Base, Subtraction, Intersection } from '@react-three/csg'
 import { BaseModel } from './BaseModel'
 
-type UnifiedModelProps = {
+type CombinedModelProps = {
     beforeGeometry: BufferGeometry
     afterGeometry: BufferGeometry
+    boundingSphere: Sphere
     cameraRef: MutableRefObject<any>
     showUnchanged: boolean
     showAdditions: boolean
     showDeletions: boolean
 }
 
-function getCommonSphere(
-    beforeGeometry: BufferGeometry,
-    afterGeometry: BufferGeometry
-) {
-    const group = new Group()
-    const dummyMaterial = new MeshBasicMaterial()
-    group.add(new Mesh(beforeGeometry, dummyMaterial))
-    group.add(new Mesh(afterGeometry, dummyMaterial))
-    const boundingBox = new Box3().setFromObject(group)
-    const center = new Vector3()
-    boundingBox.getCenter(center)
-    return boundingBox.getBoundingSphere(new Sphere(center))
-}
-
-export function UnifiedModel({
+export function CombinedModel({
     beforeGeometry,
     afterGeometry,
+    boundingSphere,
     cameraRef,
     showUnchanged,
     showAdditions,
     showDeletions,
-}: UnifiedModelProps) {
+}: CombinedModelProps) {
     const { theme } = useTheme()
     const commonColor = theme?.colors.fg.muted
     const additionsColor = theme?.colors.success.muted
     const deletionsColor = theme?.colors.danger.muted
 
     return (
-        <BaseModel
-            boundingSphere={getCommonSphere(beforeGeometry, afterGeometry)}
-            cameraRef={cameraRef}
-        >
+        <BaseModel boundingSphere={boundingSphere} cameraRef={cameraRef}>
             {/* Unchanged */}
             <mesh>
                 <meshPhongMaterial

src/components/diff/Viewer3D.tsx

@@ -4,22 +4,28 @@ import { BufferGeometry } from 'three'
 import { Canvas } from '@react-three/fiber'
 import { Camera } from './Camera'
 import { CameraControls } from './CameraControls'
+import { Sphere } from 'three'
 
 type Viewer3DProps = {
     cameraRef: MutableRefObject<any>
     geometry: BufferGeometry
+    boundingSphere?: Sphere
 }
 
 export function Viewer3D({
     cameraRef,
     geometry,
+    boundingSphere,
     children,
 }: PropsWithChildren<Viewer3DProps>) {
     return (
         <Canvas dpr={[1, 2]} shadows>
             {children}
-            <CameraControls cameraRef={cameraRef} />
-            {geometry && <Camera geometry={geometry} />}
+            <CameraControls
+                cameraRef={cameraRef}
+                target={boundingSphere?.center}
+            />
+            {geometry && <Camera boundingSphere={boundingSphere} />}
         </Canvas>
     )
 }

src/components/diff/WireframeModel.tsx

@@ -1,7 +1,7 @@
 import type { MutableRefObject } from 'react'
 import { useMemo, useRef } from 'react'
 import { BufferGeometry, DoubleSide } from 'three'
-import { EdgesGeometry } from 'three'
+import { EdgesGeometry, Sphere } from 'three'
 import { BaseModel } from './BaseModel'
 
 export type WireframeColors = {
@@ -14,18 +14,27 @@ type Props = {
     cameraRef: MutableRefObject<any>
     geometry: BufferGeometry
     colors: WireframeColors
+    boundingSphere?: Sphere
 }
 
-export function WireframeModel({ geometry, cameraRef, colors }: Props) {
+export function WireframeModel({
+    geometry,
+    boundingSphere,
+    cameraRef,
+    colors,
+}: Props) {
     const groupRef = useRef<any>()
     const edgeThresholdAngle = 10
     const edges = useMemo(
-        () => new EdgesGeometry(geometry.center(), edgeThresholdAngle),
+        () => new EdgesGeometry(geometry, edgeThresholdAngle),
         [geometry]
     )
 
     return (
-        <BaseModel boundingSphere={geometry.boundingSphere} cameraRef={cameraRef}>
+        <BaseModel
+            boundingSphere={boundingSphere || geometry.boundingSphere}
+            cameraRef={cameraRef}
+        >
             <group ref={groupRef}>
                 <mesh
                     castShadow={true}

src/utils/three.test.ts

@@ -0,0 +1,84 @@
+import { BoxGeometry } from 'three'
+import { getCommonSphere, loadGeometry } from './three'
+
+describe('Function loadGeometry', () => {
+    it('loads a three geometry from a 10x10mm box', () => {
+        const box_10 = Buffer.from(
+            `
+v 0.000000 0.000000 0.000000
+v 10.000000 0.000000 0.000000
+v 0.000000 0.000000 10.000000
+v 10.000000 0.000000 10.000000
+v 10.000000 10.000000 0.000000
+v 10.000000 10.000000 10.000000
+v 0.000000 10.000000 0.000000
+v 0.000000 10.000000 10.000000
+vn 0.000000 -1.000000 0.000000
+vn 0.000000 -1.000000 0.000000
+vn 1.000000 0.000000 0.000000
+vn 1.000000 -0.000000 0.000000
+vn 0.000000 1.000000 -0.000000
+vn 0.000000 1.000000 0.000000
+vn -1.000000 0.000000 0.000000
+vn -1.000000 -0.000000 0.000000
+vn 0.000000 0.000000 -1.000000
+vn 0.000000 0.000000 -1.000000
+vn 0.000000 0.000000 1.000000
+vn 0.000000 0.000000 1.000000
+f 1//1 2//1 3//1
+f 3//2 2//2 4//2
+f 2//3 5//3 4//3
+f 4//4 5//4 6//4
+f 5//5 7//5 6//5
+f 6//6 7//6 8//6
+f 7//7 1//7 8//7
+f 8//8 1//8 3//8
+f 1//9 5//9 2//9
+f 7//10 5//10 1//10
+f 6//11 3//11 4//11
+f 6//12 8//12 3//12
+`
+        ).toString('base64')
+        const geometry = loadGeometry(box_10)
+        expect(geometry?.attributes.position.count).toEqual(6 * 2 * 3)
+        expect(geometry?.attributes.normal.count).toEqual(6 * 2 * 3)
+        expect(geometry?.attributes.uv).toBeDefined()
+        expect(geometry?.boundingSphere?.center.x).toEqual(5)
+        expect(geometry?.boundingSphere?.center.y).toEqual(5)
+        expect(geometry?.boundingSphere?.center.z).toEqual(5)
+        const diagonal = 10 * Math.sqrt(3)
+        expect(geometry?.boundingSphere?.radius).toBeCloseTo(diagonal / 2)
+    })
+
+    it('fails with an empty or invalid input', () => {
+        expect(loadGeometry('invalid')).toBeUndefined()
+    })
+})
+
+describe('Function getCommonSphere', () => {
+    // 1mm cube with "top-right" corner at origin
+    const box_1_geom = new BoxGeometry(1, 1, 1)
+    box_1_geom.translate(-1 / 2, -1 / 2, -1 / 2)
+    // 2mm cube with "bottom left" corner at origin
+    const box_2_geom = new BoxGeometry(2, 2, 2)
+    box_2_geom.translate(2 / 2, 2 / 2, 2 / 2)
+
+    it('gets the common bounding sphere between two geometries', () => {
+        const sphere = getCommonSphere(box_1_geom, box_2_geom)
+        expect(sphere.center.x).toEqual((2 - 1) / 2)
+        expect(sphere.center.y).toEqual((2 - 1) / 2)
+        expect(sphere.center.z).toEqual((2 - 1) / 2)
+        const bothDiagonals = (1 + 2) * Math.sqrt(3)
+        expect(sphere.radius).toBeCloseTo(bothDiagonals / 2)
+    })
+
+    it('gets the same bounding sphere from two identical geometries', () => {
+        box_1_geom.computeBoundingSphere()
+        const sphere_1 = getCommonSphere(box_1_geom, box_1_geom)
+        expect(sphere_1.equals(box_1_geom.boundingSphere!)).toBeTruthy()
+
+        box_2_geom.computeBoundingSphere()
+        const sphere_2 = getCommonSphere(box_2_geom, box_2_geom)
+        expect(sphere_2.equals(box_2_geom.boundingSphere!)).toBeTruthy()
+    })
+})

src/utils/three.ts

@@ -0,0 +1,46 @@
+import {
+    Box3,
+    BufferAttribute,
+    BufferGeometry,
+    Group,
+    Mesh,
+    MeshBasicMaterial,
+    Sphere,
+    Vector3,
+} from 'three'
+import { OBJLoader } from 'three/examples/jsm/loaders/OBJLoader'
+import { Buffer } from 'buffer'
+
+export function loadGeometry(file: string): BufferGeometry | undefined {
+    const loader = new OBJLoader()
+    const buffer = Buffer.from(file, 'base64').toString()
+    const group = loader.parse(buffer)
+    console.log(`Model ${group.id} loaded`)
+    const geometry = (group.children[0] as Mesh)?.geometry
+    if (geometry) {
+        if (!geometry.attributes.uv) {
+            // UV is needed for @react-three/csg
+            // see: github.com/KittyCAD/diff-viewer-extension/issues/73
+            geometry.setAttribute(
+                'uv',
+                new BufferAttribute(new Float32Array([]), 1)
+            )
+        }
+        geometry.computeBoundingSphere() // will be used for auto-centering
+    }
+    return geometry
+}
+
+export function getCommonSphere(
+    beforeGeometry: BufferGeometry,
+    afterGeometry: BufferGeometry
+) {
+    const group = new Group()
+    const dummyMaterial = new MeshBasicMaterial()
+    group.add(new Mesh(beforeGeometry, dummyMaterial))
+    group.add(new Mesh(afterGeometry, dummyMaterial))
+    const boundingBox = new Box3().setFromObject(group)
+    const center = new Vector3()
+    boundingBox.getCenter(center)
+    return boundingBox.getBoundingSphere(new Sphere(center))
+}