Fix/composer recreate dispose

src/EffectComposer.tsx

@@ -34,9 +34,7 @@ export const EffectComposerContext = /* @__PURE__ */ createContext<{
   camera: Camera
   scene: Scene
   resolutionScale?: number
-  // A child's local state update doesn't re-render its parent - descendants
-  // that add/remove a bare Pass of their own (e.g. EffectGroup) call this
-  // to make the tree walk below notice.
+  // For descendants whose own Pass changes without re-rendering us (e.g. EffectGroup).
   requestRebuild: () => void
   autoClear: boolean
 }>(null!)
@@ -73,8 +71,7 @@ const isConvolution = (effect: Effect): boolean =>
 // mainUv isn't a bitflag - detect it the same way postprocessing itself does.
 const hasMainUv = (effect: Effect): boolean => /mainUv/.test(effect.getFragmentShader() ?? '')
 
-// autoClear/toneMapping get force-set and never restored. Ref-counted per
-// (renderer, property) since composers can share a renderer.
+// Restores autoClear/toneMapping once the last composer on a renderer is gone.
 function createRendererPropertyGuard<K extends 'autoClear' | 'toneMapping'>(property: K) {
   const refs = new WeakMap<
     WebGLRenderer,
@@ -108,28 +105,19 @@ function createRendererPropertyGuard<K extends 'autoClear' | 'toneMapping'>(prop
 const autoClearGuard = /* @__PURE__ */ createRendererPropertyGuard('autoClear')
 const toneMappingGuard = /* @__PURE__ */ createRendererPropertyGuard('toneMapping')
 
-// Scratch vector for gl.getSize() below - written then read synchronously
-// within the same call, never held onto across a render/frame, so sharing
-// one instance across every <EffectComposer> is safe (same reasoning as
-// DEFAULT_SCREEN_RES in LensFlare.tsx: safe because nothing ever aliases it).
+// Shared scratch vector, only ever used synchronously.
 const glSize = /* @__PURE__ */ new Vector2()
 
-// Stable identity for the `renderPass` default - it's in the creation
-// effect's dependency array below (changing it recreates the composer,
-// same as depthBuffer/multisampling), so a fresh arrow function every
-// render here would recreate the composer on every render too.
+// Stable default - renderPass is a composer creation dep.
 const defaultRenderPass = (scene: Scene, camera: Camera): Pass => new RenderPass(scene, camera)
 
-// Only passes buildPasses itself constructs - not a user's own bare Pass,
-// which owns its own lifecycle.
+// Passes built by buildPasses - user passes own their lifecycle.
 const generatedPasses = /* @__PURE__ */ new WeakSet<Pass>()
 
-// EffectGroup registers its pass here (see EffectGroup.tsx) so the rebuild
-// effect below knows to add a shared trailing CopyPass.
+// Passes that need the shared trailing CopyPass (EffectGroup, N8AO).
 export const groupPasses = /* @__PURE__ */ new WeakSet<Pass>()
 
-// Not pass.dispose() - EffectPass.dispose() also disposes the effects it
-// wraps, which have their own lifecycle. setEffects([]) detaches them first.
+// EffectPass.dispose() would also dispose its effects, which have their own lifecycle.
 export function disposePassWithoutEffects(pass: Pass): void {
   if (pass instanceof EffectPass) {
     ;(pass as unknown as { setEffects(effects: never[]): void }).setEffects([])
@@ -142,10 +130,8 @@ function disposeGeneratedPass(pass: Pass): void {
   disposePassWithoutEffects(pass)
 }
 
-// 'auto' (default): consecutive Effects share one EffectPass, keeping
-// convolution and mainUv Effects apart since postprocessing throws if they
-// mix. 'all' merges through those limits too, no guardrails, same as
-// EffectGroup. 'none' gives every Effect its own EffectPass.
+// 'auto' merges consecutive Effects but keeps convolution/mainUv apart (postprocessing
+// throws if they mix), 'all' merges everything, 'none' never merges.
 function buildPasses(nodes: Array<Effect | Pass>, camera: Camera, mergeMode: 'auto' | 'all' | 'none'): Pass[] {
   const passes: Pass[] = []
 
@@ -243,18 +229,21 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({
 
     effectComposer.setSize(glSize.width, glSize.height)
 
+    const ownPasses = new Set(effectComposer.passes)
+
     setComposerState({ composer: effectComposer, normalPass, downSamplingPass })
 
     return () => {
-      // The rebuild effect below may not have detached its passes yet
-      // (composerState only updates next render) - without this, dispose()
-      // would kill effects the new composer is about to reuse.
-      for (const pass of effectComposer.passes) disposeGeneratedPass(pass)
+      // Detach passes not created here, dispose() would kill ones the new composer reuses.
+      for (const pass of [...effectComposer.passes]) {
+        if (ownPasses.has(pass)) continue
+        effectComposer.removePass(pass)
+        disposeGeneratedPass(pass)
+      }
       effectComposer.dispose()
       autoClearGuard.release(gl)
     }
-    // `glSize` intentionally excluded: it's applied via the composer.setSize
-    // effect below, and shouldn't tear down/recreate the whole composer.
+    // glSize is applied in useFrame, it shouldn't recreate the composer.
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [
     camera,
@@ -270,11 +259,7 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({
     resolutionScale,
   ])
 
-  // Last size actually applied to the composer, so the check below is a
-  // cheap no-op on frames where nothing changed. The pixel ratio is part of
-  // it: setDpr, AdaptiveDpr or moving to another display change the drawing
-  // buffer without changing gl.getSize(), and composer.setSize() sizes its
-  // buffers and passes from the drawing buffer.
+  // Pixel ratio included - setDpr changes the drawing buffer but not gl.getSize().
   const appliedSizeRef = useRef({ width: -1, height: -1, pixelRatio: -1 })
 
   useFrame(
@@ -301,14 +286,12 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({
     enabled ? renderPriority : 0
   )
 
-  // Derived from the r3f scene graph (not tracked incrementally) so order
-  // always matches JSX, even through wrapper components or a reorder.
+  // Read from the r3f scene graph so order always matches JSX, even through wrappers.
   const group = useRef<Group>(null!)
   const nodesRef = useRef<Array<Effect | Pass>>([])
   const [nodesVersion, setNodesVersion] = useState(0)
 
-  // Runs every render, but only bumps nodesVersion (triggering the rebuild
-  // below) when the resolved node list actually changed.
+  // Runs every render, bumps nodesVersion only when the node list changed.
   useLayoutEffect(() => {
     if (!composerState) return
     const nodes = readGroupChildren(
@@ -318,18 +301,14 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({
     if (updateIfChanged(nodesRef, nodes)) setNodesVersion((v) => v + 1)
   })
 
-  // Only re-runs when nodesVersion/composerState/camera change - React's
-  // own dependency bailout, so create/cleanup pairing stays correct.
   useLayoutEffect(() => {
     if (!composerState) return
     const { composer, normalPass, downSamplingPass } = composerState
 
     const passes = buildPasses(nodesRef.current, camera, mergeMode)
 
-    // A toggleable pass (EffectGroup) sitting last would leave nothing on
-    // screen once disabled - postprocessing assigns renderToScreen by
-    // structural position, not `.enabled`. A shared trailing CopyPass
-    // (always enabled, always added last) fixes that for the whole chain.
+    // renderToScreen goes by position, not `.enabled` - without this, a disabled
+    // group at the end would leave the screen empty.
     if (passes.some((pass) => groupPasses.has(pass))) {
       const trailingCopyPass = new CopyPass()
       generatedPasses.add(trailingCopyPass)
@@ -383,8 +362,7 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({
   // Expose the composer
   useImperativeHandle(ref, () => composerState?.composer as EffectComposerImpl, [composerState])
 
-  // Wait until the composer exists before mounting children so they always
-  // see a valid composer instance via context.
+  // Children mount only once the composer exists, so context is never empty.
   if (!state) return null
 
   return (

src/EffectGroup.tsx

@@ -11,23 +11,16 @@ export type EffectGroupProps = {
   ref?: Ref<EffectPass>
 }
 
-// Groups its children into one EffectPass instead of relying on
-// EffectComposer's automatic consecutive-effect merging. Renders children
-// into a hidden inner <group> (invisible to EffectComposer's own top-level
-// walk) and renders the resulting pass back out via <primitive> at this
-// component's own JSX position, so EffectComposer's existing bare-Pass
-// passthrough places it correctly relative to sibling effects.
+// Children go into a hidden <group> that EffectComposer's walk skips, and the
+// combined pass is rendered back at this component's JSX position.
 export function EffectGroup({ enabled = true, children, ref }: EffectGroupProps) {
   const { camera, scene, requestRebuild } = use(EffectComposerContext)
 
   const group = useRef<Group>(null!)
   const effectsRef = useRef<Effect[]>([])
   const [pass, setPass] = useState<EffectPass | null>(null)
 
-  // Mirrors EffectComposer's own buildPasses: always constructs a fresh
-  // EffectPass when the effect list changes, rather than updating one in
-  // place - so composer.addPass() runs its normal setSize/initialize dance
-  // for every effect, with nothing to replicate by hand here.
+  // Fresh pass on every effect list change (like buildPasses), so addPass() does all the setup.
   useLayoutEffect(() => {
     const effects = readGroupChildren(group.current, (object): object is Effect => object instanceof Effect)
     if (!updateIfChanged(effectsRef, effects)) return
@@ -42,31 +35,24 @@ export function EffectGroup({ enabled = true, children, ref }: EffectGroupProps)
     setPass(newPass)
   })
 
-  // The pass snapshots camera near/far (and forwards mainCamera/mainScene to
-  // its effects) only at construction. EffectComposer's own passes are rebuilt
-  // when the composer camera or scene changes; this one is kept, so depth
-  // effects inside the group would keep reading the previous camera.
+  // EffectComposer rebuilds its own passes on camera/scene change, this one is kept, so sync it here.
   useLayoutEffect(() => {
     if (!pass) return
     pass.mainCamera = camera
     pass.mainScene = scene
   }, [pass, camera, scene])
 
-  // useLayoutEffect (not useEffect) so `enabled` is applied before
-  // EffectComposer's own effect adds this pass to the composer.
+  // Layout effect so `enabled` is set before EffectComposer adds the pass.
   useLayoutEffect(() => {
     if (pass) pass.enabled = enabled
   }, [pass, enabled])
 
-  // A local state update above doesn't re-render EffectComposer, so its own
-  // tree walk would never notice this pass appearing/disappearing.
+  // Local state changes don't re-render EffectComposer, so ask it to re-walk.
   useLayoutEffect(() => {
     requestRebuild()
   }, [pass, requestRebuild])
 
-  // Disposes whichever pass this replaces (or the last one, on unmount) -
-  // the only place a pass gets disposed, so there's no double-dispose risk
-  // from also doing it inline above where `pass` is replaced.
+  // Disposes the replaced pass, or the last one on unmount.
   useLayoutEffect(() => {
     return () => {
       if (pass) disposePassWithoutEffects(pass)

src/passes/N8AO.tsx

@@ -45,11 +45,7 @@ export function N8AO({
   const invalidate = useThree((state) => state.invalidate)
   const effect = useMemo(() => {
     const instance = new N8AOPostPass(scene, camera)
-    // N8AOPostPass isn't a postprocessing Effect, so it's not mergeable via
-    // EffectGroup - it's already a standalone Pass, hence its own `enabled`
-    // prop below instead. Registering it here still gets it the shared
-    // trailing CopyPass safety net (see EffectComposer.tsx) so disabling it
-    // while it's the last pass in the chain doesn't blank the canvas.
+    // Gets the trailing CopyPass, so disabling it as the last pass doesn't blank the canvas.
     groupPasses.add(instance)
     return instance
   }, [camera, scene])
@@ -76,8 +72,7 @@ export function N8AO({
       halfRes,
       depthAwareUpsampling,
     })
-    // effect.configuration is a plain object, never r3f-managed - applyProps'
-    // own invalidate (gated behind object.__r3f) never fires for it.
+    // configuration isn't r3f-managed, so applyProps won't invalidate on its own.
     invalidate()
   }, [
     screenSpaceRadius,

src/tests/EffectComposer.test.tsx

@@ -13,6 +13,7 @@ import * as React from 'react'
 import * as THREE from 'three'
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 import { EffectComposer } from '../EffectComposer'
+import { EffectGroup } from '../EffectGroup'
 import { ColorAverage } from '../effects/ColorAverage'
 import { wrapEffect } from '../wrapEffect'
 import { EFFECT_SHADER, flush, root, strict, waitForComposer, waitForEffects, waitForNewComposer } from './test-utils'
@@ -900,6 +901,64 @@ describe('EffectComposer', () => {
       effectDisposeSpy.mockRestore()
     })
 
+    it('does not dispose a user-provided pass or its effects when a composer-level prop recreates the composer', async () => {
+      const ref = React.createRef<EffectComposerImpl>()
+      const userEffect = new EffectC()
+      const userPass = new EffectPass(new THREE.PerspectiveCamera(), userEffect)
+      const passDisposeSpy = vi.spyOn(userPass, 'dispose')
+      const effectDisposeSpy = vi.spyOn(userEffect, 'dispose')
+
+      const render = (multisampling: number) =>
+        root.render(
+          <EffectComposer ref={ref} multisampling={multisampling}>
+            <primitive object={userPass} />
+          </EffectComposer>
+        )
+
+      await React.act(async () => render(0))
+      const firstComposer = await waitForComposer(ref)
+      await flush()
+      expect(firstComposer.passes).toContain(userPass)
+
+      await React.act(async () => render(4))
+      const secondComposer = await waitForNewComposer(ref, firstComposer)
+      await flush()
+
+      expect(secondComposer.passes).toContain(userPass)
+      expect(passDisposeSpy).not.toHaveBeenCalled()
+      expect(effectDisposeSpy).not.toHaveBeenCalled()
+    })
+
+    it("keeps an EffectGroup's effects alive and still wired to its pass when a composer-level prop recreates the composer", async () => {
+      const ref = React.createRef<EffectComposerImpl>()
+      const groupRef = React.createRef<EffectPass>()
+      const effect = new EffectA()
+      const effectDisposeSpy = vi.spyOn(effect, 'dispose')
+
+      const render = (multisampling: number) =>
+        root.render(
+          <EffectComposer ref={ref} multisampling={multisampling}>
+            <EffectGroup ref={groupRef}>
+              <primitive object={effect} />
+            </EffectGroup>
+          </EffectComposer>
+        )
+
+      await React.act(async () => render(0))
+      const firstComposer = await waitForComposer(ref)
+      await waitForEffects(ref, 1)
+      const pass = groupRef.current!
+
+      await React.act(async () => render(4))
+      const secondComposer = await waitForNewComposer(ref, firstComposer)
+      await flush()
+
+      expect(secondComposer.passes).toContain(pass)
+      expect(effectDisposeSpy).not.toHaveBeenCalled()
+      // @ts-expect-error - `listener` isn't part of the public EffectPass typing
+      expect(effect.hasEventListener('change', pass.listener)).toBe(true)
+    })
+
     it('disposes a hand-constructed effect exactly once on unmount', async () => {
       const disposeSpy = vi.spyOn(ColorAverageEffect.prototype, 'dispose')
       const ref = React.createRef<EffectComposerImpl>()