chore(tests): add coverage for EffectComposer merging behavior (#307)

Author: CodyJasonBennett

.github/workflows/main.yml

@@ -11,6 +11,15 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-node@v3
         with:
-          cache: "yarn"
-      - name: "install deps and build"
+          cache: 'yarn'
+      - name: Install Dependencies
         run: yarn install --frozen-lockfile
+
+      - name: Check build health
+        run: yarn build
+
+      - name: Check for regressions
+        run: yarn eslint:ci
+
+      - name: Run tests
+        run: yarn test

package.json

@@ -39,7 +39,7 @@
     "prepare": "yarn build",
     "eslint": "eslint . --fix --ext=js,ts,jsx,tsx",
     "eslint:ci": "eslint . --ext=js,ts,jsx,tsx",
-    "test": "echo no tests yet",
+    "test": "vitest run",
     "typecheck": "tsc --noEmit false --strict --jsx react",
     "release": "semantic-release",
     "storybook": "storybook dev -p 6006",
@@ -85,7 +85,8 @@
     "storybook": "^7.0.10",
     "three": "^0.151.3",
     "typescript": "^5.0.4",
-    "vite": "^4.3.5"
+    "vite": "^4.3.5",
+    "vitest": "^2.1.8"
   },
   "peerDependencies": {
     "@react-three/fiber": ">=8.0",

src/EffectComposer.test.tsx

@@ -0,0 +1,129 @@
+import * as React from 'react'
+import * as THREE from 'three'
+import { vi, describe, it, expect } from 'vitest'
+import { extend, createRoot, act } from '@react-three/fiber'
+import { EffectComposer } from './EffectComposer'
+import { EffectComposer as EffectComposerImpl, RenderPass, Pass, Effect, EffectPass } from 'postprocessing'
+
+// Let React know that we'll be testing effectful components
+declare global {
+  var IS_REACT_ACT_ENVIRONMENT: boolean
+}
+global.IS_REACT_ACT_ENVIRONMENT = true
+
+// Mock scheduler to test React features
+vi.mock('scheduler', () => require('scheduler/unstable_mock'))
+
+// Create virtual R3F root for testing
+extend(THREE)
+const root = createRoot({
+  style: {} as CSSStyleDeclaration,
+  addEventListener: (() => {}) as any,
+  removeEventListener: (() => {}) as any,
+  width: 1280,
+  height: 800,
+  clientWidth: 1280,
+  clientHeight: 800,
+  getContext: (() =>
+    new Proxy(
+      {},
+      {
+        get(_target, prop) {
+          switch (prop) {
+            case 'getParameter':
+              return () => 'WebGL 2' // GL_VERSION
+            case 'getExtension':
+              return () => ({}) // EXT_blend_minmax
+            case 'getContextAttributes':
+              return () => ({ alpha: true })
+            case 'getShaderPrecisionFormat':
+              return () => ({ rangeMin: 1, rangeMax: 1, precision: 1 })
+            default:
+              return () => {}
+          }
+        },
+      }
+    )) as any,
+} satisfies Partial<HTMLCanvasElement> as HTMLCanvasElement)
+root.configure({ frameloop: 'never' })
+
+const EFFECT_SHADER = 'mainImage() {}'
+
+describe('EffectComposer', () => {
+  it('should merge effects together', async () => {
+    const composerRef = React.createRef<EffectComposerImpl>()
+
+    const effectA = new Effect('A', EFFECT_SHADER)
+    const effectB = new Effect('B', EFFECT_SHADER)
+    const effectC = new Effect('C', EFFECT_SHADER)
+    const passA = new Pass()
+    const passB = new Pass()
+
+    // Forward order
+    await act(async () =>
+      root.render(
+        <EffectComposer ref={composerRef}>
+          {/* EffectPass(effectA, effectB) */}
+          <primitive object={effectA} />
+          <primitive object={effectB} />
+          {/* PassA */}
+          <primitive object={passA} />
+          {/* EffectPass(effectC) */}
+          <primitive object={effectC} />
+          {/* PassB */}
+          <primitive object={passB} />
+        </EffectComposer>
+      )
+    )
+    expect(composerRef.current!.passes.map((p) => p.constructor)).toStrictEqual([
+      RenderPass,
+      EffectPass,
+      Pass,
+      EffectPass,
+      Pass,
+    ])
+    // @ts-expect-error
+    expect((composerRef.current!.passes[1] as EffectPass).effects).toStrictEqual([effectA, effectB])
+    expect(composerRef.current!.passes[2]).toBe(passA)
+    // @ts-expect-error
+    expect((composerRef.current!.passes[3] as EffectPass).effects).toStrictEqual([effectC])
+    expect(composerRef.current!.passes[4]).toBe(passB)
+
+    // NOTE: instance children ordering is unstable until R3F v9, so we remount from scratch
+    await act(async () => root.render(null))
+
+    // Reverse order
+    await act(async () =>
+      root.render(
+        <EffectComposer ref={composerRef}>
+          {/* PassB */}
+          <primitive object={passB} />
+          {/* EffectPass(effectC) */}
+          <primitive object={effectC} />
+          {/* PassA */}
+          <primitive object={passA} />
+          {/* EffectPass(effectB, effectA) */}
+          <primitive object={effectB} />
+          <primitive object={effectA} />
+        </EffectComposer>
+      )
+    )
+    expect(composerRef.current!.passes.map((p) => p.constructor)).toStrictEqual([
+      RenderPass,
+      Pass,
+      EffectPass,
+      Pass,
+      EffectPass,
+    ])
+    expect(composerRef.current!.passes[1]).toBe(passB)
+    // @ts-expect-error
+    expect((composerRef.current!.passes[2] as EffectPass).effects).toStrictEqual([effectC])
+    expect(composerRef.current!.passes[3]).toBe(passA)
+    // @ts-expect-error
+    expect((composerRef.current!.passes[4] as EffectPass).effects).toStrictEqual([effectB, effectA])
+  })
+
+  it.skip('should split convolution effects', async () => {
+    await act(async () => root.render(null))
+  })
+})

src/EffectComposer.tsx

@@ -9,7 +9,7 @@ import React, {
   useRef,
   useImperativeHandle,
 } from 'react'
-import { useThree, useFrame, useInstanceHandle } from '@react-three/fiber'
+import { useThree, useFrame } from '@react-three/fiber'
 import {
   EffectComposer as EffectComposerImpl,
   RenderPass,
@@ -32,7 +32,7 @@ export const EffectComposerContext = createContext<{
   resolutionScale?: number
 }>(null!)
 
-export type EffectComposerProps = {  
+export type EffectComposerProps = {
   enabled?: boolean
   children: JSX.Element | JSX.Element[]
   depthBuffer?: boolean
@@ -52,7 +52,7 @@ const isConvolution = (effect: Effect): boolean =>
   (effect.getAttributes() & EffectAttribute.CONVOLUTION) === EffectAttribute.CONVOLUTION
 
 export const EffectComposer = React.memo(
-  forwardRef(
+  forwardRef<EffectComposerImpl, EffectComposerProps>(
     (
       {
         children,
@@ -67,7 +67,7 @@ export const EffectComposer = React.memo(
         stencilBuffer,
         multisampling = 8,
         frameBufferType = HalfFloatType,
-      }: EffectComposerProps,
+      },
       ref
     ) => {
       const { gl, scene: defaultScene, camera: defaultCamera, size } = useThree()
@@ -129,12 +129,14 @@ export const EffectComposer = React.memo(
       )
 
       const group = useRef(null)
-      const instance = useInstanceHandle(group)
       useLayoutEffect(() => {
         const passes: Pass[] = []
 
-        if (group.current && instance.current && composer) {
-          const children = instance.current.objects as unknown[]
+        // TODO: rewrite all of this with R3F v9
+        const groupInstance = (group.current as any)?.__r3f as { objects: unknown[] }
+
+        if (groupInstance && composer) {
+          const children = groupInstance.objects
 
           for (let i = 0; i < children.length; i++) {
             const child = children[i]
@@ -169,7 +171,7 @@ export const EffectComposer = React.memo(
           if (normalPass) normalPass.enabled = false
           if (downSamplingPass) downSamplingPass.enabled = false
         }
-      }, [composer, children, camera, normalPass, downSamplingPass, instance])
+      }, [composer, children, camera, normalPass, downSamplingPass])
 
       // Disable tone mapping because threejs disallows tonemapping on render targets
       useEffect(() => {
@@ -178,7 +180,7 @@ export const EffectComposer = React.memo(
         return () => {
           gl.toneMapping = currentTonemapping
         }
-      }, [])
+      }, [gl])
 
       // Memoize state, otherwise it would trigger all consumers on every render
       const state = useMemo(

src/effects/Autofocus.tsx

@@ -10,7 +10,7 @@ import React, {
   RefObject,
   useMemo,
 } from 'react'
-import { useThree, useFrame, createPortal, Vector3 } from '@react-three/fiber'
+import { useThree, useFrame, createPortal, type Vector3 } from '@react-three/fiber'
 import { CopyPass, DepthPickingPass, DepthOfFieldEffect } from 'postprocessing'
 import { easing } from 'maath'
 

src/effects/Grid.tsx

@@ -16,6 +16,6 @@ export const Grid = forwardRef(function Grid({ size, ...props }: GridProps, ref:
   useLayoutEffect(() => {
     if (size) effect.setSize(size.width, size.height)
     invalidate()
-  }, [effect, size])
+  }, [effect, size, invalidate])
   return <primitive ref={ref} object={effect} dispose={null} />
 })

src/effects/N8AO/index.tsx

@@ -40,7 +40,9 @@ export const N8AO = forwardRef<N8AOPostPass, N8AOProps>(
     ref: Ref<N8AOPostPass>
   ) => {
     const { camera, scene } = useThree()
-    const effect = useMemo(() => new N8AOPostPass(scene, camera), [])
+    const effect = useMemo(() => new N8AOPostPass(scene, camera), [camera, scene])
+
+    // TODO: implement dispose upstream; this effect has memory leaks without
     useLayoutEffect(() => {
       applyProps(effect.configuration, {
         color,
@@ -67,10 +69,13 @@ export const N8AO = forwardRef<N8AOPostPass, N8AOProps>(
       renderMode,
       halfRes,
       depthAwareUpsampling,
+      effect,
     ])
+
     useLayoutEffect(() => {
       if (quality) effect.setQualityMode(quality.charAt(0).toUpperCase() + quality.slice(1))
-    }, [quality])
+    }, [effect, quality])
+
     return <primitive ref={ref} object={effect} />
   }
 )

src/effects/Outline.tsx

@@ -52,6 +52,8 @@ export const Outline = forwardRef(function Outline(
         xRay,
         ...props,
       }),
+    // NOTE: `props` is an unstable reference, so we can't memoize it
+    // eslint-disable-next-line react-hooks/exhaustive-deps
     [
       blendFunction,
       blur,

src/effects/SSAO.tsx

@@ -31,6 +31,8 @@ export const SSAO = forwardRef<SSAOEffect, SSAOProps>(function SSAO(props: SSAOP
       depthAwareUpsampling: true,
       ...props,
     })
-  }, [camera, normalPass, props])
+    // NOTE: `props` is an unstable reference, so we can't memoize it
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [camera, downSamplingPass, normalPass, resolutionScale])
   return <primitive ref={ref} object={effect} dispose={null} />
 })

src/effects/SSR/index.tsx

@@ -64,7 +64,7 @@ export const SSR = forwardRef<SSREffect, SSRProps>(function SSR(
 
   ref: Ref<SSREffect>
 ) {
-  const { invalidate } = useThree()
+  const invalidate = useThree((s) => s.invalidate)
   const { scene, camera } = useContext(EffectComposerContext)
   const effect = useMemo(
     () => new SSREffect(scene, camera, { ENABLE_BLUR, USE_MRT, ...props }),
@@ -83,7 +83,7 @@ export const SSR = forwardRef<SSREffect, SSRProps>(function SSR(
         }
       }
     }
-  }, [api])
+  }, [api, effect, invalidate])
 
   return <primitive ref={ref} object={effect} {...props} />
 })