From da8fe4951b605effd5c8c7a438926f128c23b7d4 Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Tue, 20 Oct 2026 18:43:51 +0200 Subject: [PATCH 1/2] fix(EffectComposer): don't dispose reused passes when recreating the composer Old composer's dispose() killed EffectGroup/N8AO/user passes and their effects still in use by the new one. --- src/EffectComposer.tsx | 10 ++++-- src/tests/EffectComposer.test.tsx | 59 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/src/EffectComposer.tsx b/src/EffectComposer.tsx index d012257..6e80ee9 100644 --- a/src/EffectComposer.tsx +++ b/src/EffectComposer.tsx @@ -243,13 +243,19 @@ 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) + // would kill passes and effects the new composer is about to reuse. + for (const pass of [...effectComposer.passes]) { + if (ownPasses.has(pass)) continue + effectComposer.removePass(pass) + disposeGeneratedPass(pass) + } effectComposer.dispose() autoClearGuard.release(gl) } diff --git a/src/tests/EffectComposer.test.tsx b/src/tests/EffectComposer.test.tsx index 0370152..ca1ffdb 100644 --- a/src/tests/EffectComposer.test.tsx +++ b/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() + 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( + + + + ) + + 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() + const groupRef = React.createRef() + const effect = new EffectA() + const effectDisposeSpy = vi.spyOn(effect, 'dispose') + + const render = (multisampling: number) => + root.render( + + + + + + ) + + 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() From 5ed9f44293a509f76d9b0b5afb7df0353a68efc5 Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Tue, 20 Oct 2026 18:46:38 +0200 Subject: [PATCH 2/2] chore: shorten comments in EffectComposer, EffectGroup and N8AO --- src/EffectComposer.tsx | 62 ++++++++++++------------------------------ src/EffectGroup.tsx | 28 +++++-------------- src/passes/N8AO.tsx | 9 ++---- 3 files changed, 26 insertions(+), 73 deletions(-) diff --git a/src/EffectComposer.tsx b/src/EffectComposer.tsx index 6e80ee9..f38d7cf 100644 --- a/src/EffectComposer.tsx +++ b/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(property: K) { const refs = new WeakMap< WebGLRenderer, @@ -108,28 +105,19 @@ function createRendererPropertyGuard(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 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() -// 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() -// 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, camera: Camera, mergeMode: 'auto' | 'all' | 'none'): Pass[] { const passes: Pass[] = [] @@ -248,9 +234,7 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({ 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 passes and effects the new composer is about to reuse. + // 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) @@ -259,8 +243,7 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({ 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, @@ -276,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( @@ -307,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(null!) const nodesRef = useRef>([]) 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( @@ -324,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) @@ -389,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 ( diff --git a/src/EffectGroup.tsx b/src/EffectGroup.tsx index 10e29f5..01b989d 100644 --- a/src/EffectGroup.tsx +++ b/src/EffectGroup.tsx @@ -11,12 +11,8 @@ export type EffectGroupProps = { ref?: Ref } -// Groups its children into one EffectPass instead of relying on -// EffectComposer's automatic consecutive-effect merging. Renders children -// into a hidden inner (invisible to EffectComposer's own top-level -// walk) and renders the resulting pass back out via 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 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) @@ -24,10 +20,7 @@ export function EffectGroup({ enabled = true, children, ref }: EffectGroupProps) const effectsRef = useRef([]) const [pass, setPass] = useState(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) diff --git a/src/passes/N8AO.tsx b/src/passes/N8AO.tsx index 7e38cd2..91c2452 100644 --- a/src/passes/N8AO.tsx +++ b/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,