refactor(controllers): Refactored how Controllers interact w/ three (#294)

This a big refactor of controller and how they interact with three counterparts. The flow of events was very confusing, to say the least. This PR makes this flow much direct and does some other notable changes.

---------

Co-authored-by: Cody Bennett <[email protected]>

Author: saitonakamura

package.json

@@ -37,6 +37,7 @@
   "devDependencies": {
     "@react-three/drei": "^9.13.2",
     "@react-three/fiber": "^8.0.27",
+    "@react-three/test-renderer": "^8.2.0",
     "@types/react": "^18.0.14",
     "@types/react-dom": "^18.0.5",
     "@types/react-test-renderer": "^18.0.0",

src/Controllers.test.tsx

@@ -0,0 +1,144 @@
+import * as React from 'react'
+import { describe, it, expect, vi } from 'vitest'
+import { createStoreMock, createStoreProvider } from './mocks/storeMock'
+import { render } from './testUtils/testUtilsThree'
+import { Controllers } from './Controllers'
+import { XRControllerMock } from './mocks/XRControllerMock'
+import { XRControllerModel } from './XRControllerModel'
+import { XRControllerModelFactoryMock } from './mocks/XRControllerModelFactoryMock'
+import { XRInputSourceMock } from './mocks/XRInputSourceMock'
+import { act } from '@react-three/test-renderer'
+
+vi.mock('./XRControllerModelFactory', async () => {
+  const { XRControllerModelFactoryMock } = await vi.importActual<typeof import('./mocks/XRControllerModelFactoryMock')>(
+    './mocks/XRControllerModelFactoryMock'
+  )
+  return { XRControllerModelFactory: XRControllerModelFactoryMock }
+})
+
+describe('Controllers', () => {
+  it('should not render anything if controllers in state are empty', async () => {
+    const store = createStoreMock()
+    const xrControllerMock = new XRControllerMock(0)
+    store.setState({ controllers: [] })
+
+    const { renderer } = await render(<Controllers />, { wrapper: createStoreProvider(store) })
+
+    // We aren't rendering anything as a direct children, only in portals
+    const graph = renderer.toGraph()
+    expect(graph).toHaveLength(0)
+    // Checking portals
+    expect(xrControllerMock.grip.children).toHaveLength(0)
+    expect(xrControllerMock.controller.children).toHaveLength(0)
+  })
+
+  it('should render one xr controller model and one ray given one controller in state', async () => {
+    const store = createStoreMock()
+    const xrControllerMock = new XRControllerMock(0)
+    store.setState({ controllers: [xrControllerMock] })
+
+    await render(<Controllers />, { wrapper: createStoreProvider(store) })
+
+    // Checking portals
+    expect(xrControllerMock.grip.children).toHaveLength(1)
+    expect(xrControllerMock.grip.children[0]).toBeInstanceOf(XRControllerModel)
+    expect(xrControllerMock.controller.children).toHaveLength(1)
+    expect(xrControllerMock.controller.children[0].type).toBe('Line')
+  })
+
+  it('should render two xr controller models and two rays given one controller in state', async () => {
+    const store = createStoreMock()
+    const xrControllerMockLeft = new XRControllerMock(0)
+    const xrControllerMockRight = new XRControllerMock(1)
+    store.setState({ controllers: [xrControllerMockLeft, xrControllerMockRight] })
+
+    await render(<Controllers />, { wrapper: createStoreProvider(store) })
+
+    // Checking portals
+    // left
+    expect(xrControllerMockLeft.grip.children).toHaveLength(1)
+    expect(xrControllerMockLeft.grip.children[0]).toBeInstanceOf(XRControllerModel)
+    expect(xrControllerMockLeft.controller.children).toHaveLength(1)
+    expect(xrControllerMockLeft.controller.children[0].type).toBe('Line')
+    // right
+    expect(xrControllerMockRight.grip.children).toHaveLength(1)
+    expect(xrControllerMockRight.grip.children[0]).toBeInstanceOf(XRControllerModel)
+    expect(xrControllerMockRight.controller.children).toHaveLength(1)
+    expect(xrControllerMockRight.controller.children[0].type).toBe('Line')
+  })
+
+  it('should remove xr controller model when controller is removed from state', async () => {
+    const store = createStoreMock()
+    const xrControllerMock = new XRControllerMock(0)
+    xrControllerMock.inputSource = new XRInputSourceMock()
+    store.setState({ controllers: [xrControllerMock] })
+
+    const { renderer } = await render(<Controllers />, { wrapper: createStoreProvider(store) })
+
+    await act(async () => {
+      store.setState({ controllers: [] })
+    })
+
+    // We aren't rendering anything as a direct children, only in portals
+    const graph = renderer.toGraph()
+    expect(graph).toHaveLength(0)
+    // Checking portals
+    expect(xrControllerMock.grip.children).toHaveLength(0)
+    expect(xrControllerMock.controller.children).toHaveLength(0)
+  })
+
+  it('should handle xr controller model given one controller in state', async () => {
+    const store = createStoreMock()
+    const xrControllerMock = new XRControllerMock(0)
+    xrControllerMock.inputSource = new XRInputSourceMock()
+    store.setState({ controllers: [xrControllerMock] })
+
+    await render(<Controllers />, { wrapper: createStoreProvider(store) })
+
+    const xrControllerModelFactory = XRControllerModelFactoryMock.instance
+    expect(xrControllerModelFactory).toBeDefined()
+    expect(xrControllerMock.xrControllerModel).toBeInstanceOf(XRControllerModel)
+    expect(xrControllerModelFactory?.initializeControllerModel).toBeCalled()
+  })
+
+  it('should handle xr controller model when controller is removed from state', async () => {
+    const store = createStoreMock()
+    const xrControllerMock = new XRControllerMock(0)
+    xrControllerMock.inputSource = new XRInputSourceMock()
+    store.setState({ controllers: [xrControllerMock] })
+
+    await render(<Controllers />, { wrapper: createStoreProvider(store) })
+
+    const xrControllerModel = xrControllerMock.xrControllerModel
+    const disconnectSpy = vi.spyOn(xrControllerModel!, 'disconnect')
+
+    await act(async () => {
+      store.setState({ controllers: [] })
+    })
+
+    const xrControllerModelFactory = XRControllerModelFactoryMock.instance
+    expect(xrControllerModelFactory).toBeDefined()
+    expect(xrControllerMock.xrControllerModel).toBeNull()
+    expect(disconnectSpy).toBeCalled()
+  })
+
+  it('should not reconnect when component is rerendered', async () => {
+    const store = createStoreMock()
+    const xrControllerMock = new XRControllerMock(0)
+    xrControllerMock.inputSource = new XRInputSourceMock()
+    store.setState({ controllers: [xrControllerMock] })
+
+    const { rerender } = await render(<Controllers />, { wrapper: createStoreProvider(store) })
+
+    const xrControllerModel = xrControllerMock.xrControllerModel
+    const disconnectSpy = vi.spyOn(xrControllerModel!, 'disconnect')
+
+    await rerender(<Controllers />)
+
+    const xrControllerModelFactory = XRControllerModelFactoryMock.instance
+    expect(xrControllerModelFactory).toBeDefined()
+    expect(xrControllerMock.xrControllerModel).not.toBeNull()
+    expect(disconnectSpy).not.toBeCalled()
+    expect(xrControllerModelFactory?.initializeControllerModel).toBeCalledTimes(1)
+  })
+})

src/Controllers.tsx

@@ -3,16 +3,17 @@ import * as THREE from 'three'
 import { useFrame, Object3DNode, extend, createPortal } from '@react-three/fiber'
 import { useXR } from './XR'
 import { XRController } from './XRController'
-import { useIsomorphicLayoutEffect } from './utils'
-import { XRControllerModel, XRControllerModelFactory } from './XRControllerModelFactory'
-import { XRControllerEvent } from './XREvents'
+import { XRControllerModelFactory } from './XRControllerModelFactory'
+import { useCallback } from 'react'
+import { XRControllerModel } from './XRControllerModel'
 
 export interface RayProps extends Partial<JSX.IntrinsicElements['object3D']> {
   /** The XRController to attach the ray to */
   target: XRController
   /** Whether to hide the ray on controller blur. Default is `false` */
   hideOnBlur?: boolean
 }
+
 export const Ray = React.forwardRef<THREE.Line, RayProps>(function Ray({ target, hideOnBlur = false, ...props }, forwardedRef) {
   const hoverState = useXR((state) => state.hoverState)
   const ray = React.useRef<THREE.Line>(null!)
@@ -24,6 +25,10 @@ export const Ray = React.forwardRef<THREE.Line, RayProps>(function Ray({ target,
 
   // Show ray line when hovering objects
   useFrame(() => {
+    if (!target.inputSource) {
+      return
+    }
+
     let rayLength = 1
 
     const intersection: THREE.Intersection = hoverState[target.inputSource.handedness].values().next().value
@@ -46,47 +51,10 @@ export const Ray = React.forwardRef<THREE.Line, RayProps>(function Ray({ target,
 
 const modelFactory = new XRControllerModelFactory()
 
-class ControllerModel extends THREE.Group {
-  readonly target: XRController
-  readonly xrControllerModel: XRControllerModel
-
-  constructor(target: XRController) {
-    super()
-    this.xrControllerModel = new XRControllerModel()
-    this.target = target
-    this.add(this.xrControllerModel)
-
-    this._onConnected = this._onConnected.bind(this)
-    this._onDisconnected = this._onDisconnected.bind(this)
-
-    this.target.controller.addEventListener('connected', this._onConnected)
-    this.target.controller.addEventListener('disconnected', this._onDisconnected)
-  }
-
-  private _onConnected(event: XRControllerEvent) {
-    if (event.data?.hand) {
-      return
-    }
-    modelFactory.initializeControllerModel(this.xrControllerModel, event)
-  }
-
-  private _onDisconnected(event: XRControllerEvent) {
-    if (event.data?.hand) {
-      return
-    }
-    this.xrControllerModel.disconnect()
-  }
-
-  dispose() {
-    this.target.controller.removeEventListener('connected', this._onConnected)
-    this.target.controller.removeEventListener('disconnected', this._onDisconnected)
-  }
-}
-
 declare global {
   namespace JSX {
     interface IntrinsicElements {
-      controllerModel: Object3DNode<ControllerModel, typeof ControllerModel>
+      xRControllerModel: Object3DNode<XRControllerModel, typeof XRControllerModel>
     }
   }
 }
@@ -97,6 +65,34 @@ export interface ControllersProps {
   /** Whether to hide controllers' rays on blur. Default is `false` */
   hideRaysOnBlur?: boolean
 }
+
+const ControllerModel = ({ target }: { target: XRController }) => {
+  const handleControllerModel = useCallback(
+    (xrControllerModel: XRControllerModel | null) => {
+      if (xrControllerModel) {
+        target.xrControllerModel = xrControllerModel
+        if (target.inputSource?.hand) {
+          return
+        }
+        if (target.inputSource) {
+          modelFactory.initializeControllerModel(xrControllerModel, target.inputSource)
+        } else {
+          console.warn('no input source on XRController when handleControllerModel')
+        }
+      } else {
+        if (target.inputSource?.hand) {
+          return
+        }
+        target.xrControllerModel?.disconnect()
+        target.xrControllerModel = null
+      }
+    },
+    [target]
+  )
+
+  return <xRControllerModel ref={handleControllerModel} />
+}
+
 export function Controllers({ rayMaterial = {}, hideRaysOnBlur = false }: ControllersProps) {
   const controllers = useXR((state) => state.controllers)
   const isHandTracking = useXR((state) => state.isHandTracking)
@@ -111,20 +107,13 @@ export function Controllers({ rayMaterial = {}, hideRaysOnBlur = false }: Contro
       ),
     [JSON.stringify(rayMaterial)] // eslint-disable-line react-hooks/exhaustive-deps
   )
-  React.useMemo(() => extend({ ControllerModel }), [])
-
-  // Send fake connected event (no-op) so models start loading
-  useIsomorphicLayoutEffect(() => {
-    for (const target of controllers) {
-      target.controller.dispatchEvent({ type: 'connected', data: target.inputSource, fake: true })
-    }
-  }, [controllers])
+  React.useMemo(() => extend({ XRControllerModel }), [])
 
   return (
     <>
       {controllers.map((target, i) => (
         <React.Fragment key={i}>
-          {createPortal(<controllerModel args={[target]} />, target.grip)}
+          {createPortal(<ControllerModel target={target} />, target.grip)}
           {createPortal(
             <Ray visible={!isHandTracking} hideOnBlur={hideRaysOnBlur} target={target} {...rayMaterialProps} />,
             target.controller

src/Interactions.test.tsx

@@ -0,0 +1,60 @@
+import { describe, it, expect, vi } from 'vitest'
+import { render } from './testUtils/testUtilsThree'
+import * as React from 'react'
+import { createStoreMock, createStoreProvider } from './mocks/storeMock'
+import { InteractionManager, Interactive } from './Interactions'
+import { XRControllerMock } from './mocks/XRControllerMock'
+import { act } from '@react-three/test-renderer'
+import { XRInputSourceMock } from './mocks/XRInputSourceMock'
+import { Intersection } from '@react-three/fiber'
+import { Vector3 } from 'three'
+
+describe('Interactions', () => {
+  it('should call onSelect when select event is dispatched', async () => {
+    const store = createStoreMock()
+    const xrControllerMock = new XRControllerMock(0)
+    const xrInputSourceMock = new XRInputSourceMock({ handedness: 'right' })
+    xrControllerMock.inputSource = xrInputSourceMock
+    const rightHoverState = new Map()
+    store.setState({
+      controllers: [xrControllerMock],
+      hoverState: {
+        none: new Map(),
+        left: new Map(),
+        right: rightHoverState
+      }
+    })
+
+    const selectSpy = vi.fn()
+    const { renderer } = await render(
+      <InteractionManager>
+        <Interactive onSelect={selectSpy}>
+          <mesh position={[0, 0, -1]}>
+            <planeGeometry args={[1, 1]} />
+          </mesh>
+        </Interactive>
+      </InteractionManager>,
+      { wrapper: createStoreProvider(store) }
+    )
+
+    const mesh = renderer.scene.findByType('Mesh').instance
+    const interactiveGroup = renderer.scene.findByType('Group').instance
+    expect(mesh).toBeDefined()
+    expect(interactiveGroup).toBeDefined()
+    const intersection: Intersection = {
+      eventObject: mesh,
+      distance: 1,
+      point: new Vector3(0, 0, 0),
+      object: mesh
+    }
+
+    rightHoverState.set(mesh, intersection)
+    rightHoverState.set(interactiveGroup, intersection)
+
+    await act(async () => {
+      xrControllerMock.controller.dispatchEvent({ type: 'select', data: {} })
+    })
+
+    expect(selectSpy).toBeCalled()
+  })
+})

src/Interactions.tsx

@@ -56,6 +56,9 @@ export function InteractionManager({ children }: { children: React.ReactNode })
     if (interactions.size === 0) return
 
     for (const target of controllers) {
+      if (!target.inputSource?.handedness) {
+        return
+      }
       const hovering = hoverState[target.inputSource.handedness]
       const hits = new Set()
       let intersections = intersect(target.controller)
@@ -109,6 +112,9 @@ export function InteractionManager({ children }: { children: React.ReactNode })
 
   const triggerEvent = React.useCallback(
     (interaction: XRInteractionType) => (e: XREvent<XRControllerEvent>) => {
+      if (!e.target.inputSource?.handedness) {
+        return
+      }
       const hovering = hoverState[e.target.inputSource.handedness]
       const intersections = Array.from(new Set(hovering.values()))
 

src/Teleportation.tsx

@@ -73,7 +73,7 @@ export const TeleportationPlane = React.forwardRef<THREE.Group, TeleportationPla
 
   const isInteractive = React.useCallback(
     (e: XRInteractionEvent): boolean => {
-      const { handedness } = e.target.inputSource
+      const handedness = e.target.inputSource?.handedness
       return !!((handedness !== 'left' || leftHand) && (handedness !== 'right' || rightHand))
     },
     [leftHand, rightHand]

src/XR.test.tsx

@@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
 import { XRButton } from './XR'
 import * as React from 'react'
 import { XRSystemMock } from './mocks/XRSystemMock'
-import { render } from './testUtils'
+import { render } from './testUtils/testUtilsDom'
 
 describe('XR', () => {
   let xrSystemMock = new XRSystemMock()

src/XR.tsx

@@ -476,7 +476,7 @@ export function useXR<T = XRState>(
 export function useController(handedness: XRHandedness) {
   const controllers = useXR((state) => state.controllers)
   const controller = React.useMemo(
-    () => controllers.find(({ inputSource }) => inputSource.handedness === handedness),
+    () => controllers.find(({ inputSource }) => inputSource?.handedness && inputSource.handedness === handedness),
     [handedness, controllers]
   )