Merge pull request #358 from pmndrs/fix/07-renderer-state-restoration

fix(EffectComposer): restore renderer.autoClear/toneMapping on unmount

Author: kvvasuu

src/EffectComposer.tsx

@@ -21,7 +21,7 @@ import {
   type ReactNode,
   type Ref,
 } from 'react'
-import type { Camera, Group, Scene, TextureDataType } from 'three'
+import type { Camera, Group, Scene, TextureDataType, WebGLRenderer } from 'three'
 import { HalfFloatType, NoToneMapping } from 'three'
 
 export const EffectComposerContext = /* @__PURE__ */ createContext<{
@@ -59,6 +59,44 @@ type ComposerState = {
 const isConvolution = (effect: Effect): boolean =>
   (effect.getAttributes() & EffectAttribute.CONVOLUTION) === EffectAttribute.CONVOLUTION
 
+/**
+ * autoClear/toneMapping get force-set and never restored by whoever sets
+ * them. Ref-counted per (renderer, property) since composers can share a
+ * renderer; skips restoring if the value already changed since acquire.
+ */
+function createRendererPropertyGuard<K extends 'autoClear' | 'toneMapping'>(property: K) {
+  const refs = new WeakMap<
+    WebGLRenderer,
+    { count: number; original: WebGLRenderer[K]; forcedValue: WebGLRenderer[K] }
+  >()
+
+  return {
+    acquire(gl: WebGLRenderer, forcedValue: WebGLRenderer[K]): void {
+      const existing = refs.get(gl)
+      if (existing) {
+        existing.count++
+        existing.forcedValue = forcedValue
+      } else {
+        refs.set(gl, { count: 1, original: gl[property], forcedValue })
+      }
+    },
+    release(gl: WebGLRenderer): void {
+      const entry = refs.get(gl)
+      if (!entry) return
+
+      if (--entry.count <= 0) {
+        if (gl[property] === entry.forcedValue) {
+          gl[property] = entry.original
+        }
+        refs.delete(gl)
+      }
+    },
+  }
+}
+
+const autoClearGuard = /* @__PURE__ */ createRendererPropertyGuard('autoClear')
+const toneMappingGuard = /* @__PURE__ */ createRendererPropertyGuard('toneMapping')
+
 /**
  * Groups a flat, ordered list of Effect/Pass instances into actual composer
  * passes, merging consecutive non-convolution Effects into a single
@@ -116,6 +154,8 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({
   const [composerState, setComposerState] = useState<ComposerState | null>(null)
 
   useEffect(() => {
+    autoClearGuard.acquire(gl, false)
+
     const effectComposer = new EffectComposerImpl(gl, { depthBuffer, stencilBuffer, multisampling, frameBufferType })
     effectComposer.addPass(new RenderPass(scene, camera))
 
@@ -140,6 +180,7 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({
 
     return () => {
       effectComposer.dispose()
+      autoClearGuard.release(gl)
     }
     // `size` intentionally excluded: it's applied via the composer.setSize
     // effect below, and shouldn't tear down/recreate the whole composer.
@@ -199,10 +240,10 @@ export const EffectComposer = /* @__PURE__ */ memo(function EffectComposer({
 
   // Disable tone mapping because threejs disallows tonemapping on render targets
   useEffect(() => {
-    const currentTonemapping = gl.toneMapping
+    toneMappingGuard.acquire(gl, NoToneMapping)
     gl.toneMapping = NoToneMapping
     return () => {
-      gl.toneMapping = currentTonemapping
+      toneMappingGuard.release(gl)
     }
   }, [gl])
 

src/tests/EffectComposer.test.tsx

@@ -485,6 +485,197 @@ describe('EffectComposer', () => {
     })
   })
 
+  describe('renderer state restoration', () => {
+    it('restores autoClear to its previous value once the composer is fully unmounted', async () => {
+      const gl = root.render(null).getState().gl
+      gl.autoClear = true // known baseline, independent of whatever earlier tests in this file left behind
+
+      const ref = React.createRef<EffectComposerImpl>()
+
+      await React.act(async () =>
+        root.render(
+          <EffectComposer ref={ref}>
+            <WrappedEffectA />
+          </EffectComposer>
+        )
+      )
+
+      await waitForComposer(ref)
+      expect(gl.autoClear).toBe(false)
+
+      await React.act(async () => root.render(null))
+
+      expect(gl.autoClear).toBe(true)
+    })
+
+    it('keeps autoClear disabled while a sibling composer on the same renderer is still mounted, regardless of unmount order', async () => {
+      const gl = root.render(null).getState().gl
+      gl.autoClear = true
+
+      const refA = React.createRef<EffectComposerImpl>()
+      const refB = React.createRef<EffectComposerImpl>()
+
+      await React.act(async () =>
+        root.render(
+          <>
+            <EffectComposer key="a" ref={refA}>
+              <WrappedEffectA />
+            </EffectComposer>
+            <EffectComposer key="b" ref={refB}>
+              <WrappedEffectB />
+            </EffectComposer>
+          </>
+        )
+      )
+
+      await waitForComposer(refA)
+      await waitForComposer(refB)
+      expect(gl.autoClear).toBe(false)
+
+      // Unmount the first composer only (key "a" drops out of the tree) -
+      // the second is still relying on autoClear staying off, so this must
+      // not restore it yet. Keying both is essential here: without it,
+      // React would match by position and reuse "a"'s instance in place
+      // (just updating its props to "b"'s), unmounting the wrong one.
+      await React.act(async () =>
+        root.render(
+          <EffectComposer key="b" ref={refB}>
+            <WrappedEffectB />
+          </EffectComposer>
+        )
+      )
+
+      expect(gl.autoClear).toBe(false)
+
+      // Now the last one goes too - only now should it actually restore.
+      await React.act(async () => root.render(null))
+
+      expect(gl.autoClear).toBe(true)
+    })
+
+    it('restores toneMapping to its previous value once the composer is fully unmounted', async () => {
+      const gl = root.render(null).getState().gl
+      gl.toneMapping = THREE.ACESFilmicToneMapping // known baseline, distinct from NoToneMapping
+
+      const ref = React.createRef<EffectComposerImpl>()
+
+      await React.act(async () =>
+        root.render(
+          <EffectComposer ref={ref}>
+            <WrappedEffectA />
+          </EffectComposer>
+        )
+      )
+
+      await waitForComposer(ref)
+      expect(gl.toneMapping).toBe(THREE.NoToneMapping)
+
+      await React.act(async () => root.render(null))
+
+      expect(gl.toneMapping).toBe(THREE.ACESFilmicToneMapping)
+    })
+
+    it('keeps toneMapping disabled while a sibling composer on the same renderer is still mounted, regardless of unmount order', async () => {
+      const gl = root.render(null).getState().gl
+      gl.toneMapping = THREE.ACESFilmicToneMapping
+
+      const refA = React.createRef<EffectComposerImpl>()
+      const refB = React.createRef<EffectComposerImpl>()
+
+      await React.act(async () =>
+        root.render(
+          <>
+            <EffectComposer key="a" ref={refA}>
+              <WrappedEffectA />
+            </EffectComposer>
+            <EffectComposer key="b" ref={refB}>
+              <WrappedEffectB />
+            </EffectComposer>
+          </>
+        )
+      )
+
+      await waitForComposer(refA)
+      await waitForComposer(refB)
+      expect(gl.toneMapping).toBe(THREE.NoToneMapping)
+
+      // Unmount the first composer only - the second still relies on
+      // toneMapping staying off, so this must not restore it yet.
+      await React.act(async () =>
+        root.render(
+          <EffectComposer key="b" ref={refB}>
+            <WrappedEffectB />
+          </EffectComposer>
+        )
+      )
+
+      expect(gl.toneMapping).toBe(THREE.NoToneMapping)
+
+      // Now the last one goes too - only now should it actually restore.
+      await React.act(async () => root.render(null))
+
+      expect(gl.toneMapping).toBe(THREE.ACESFilmicToneMapping)
+    })
+
+    it('does not clobber a manual autoClear change made while the composer was mounted', async () => {
+      const gl = root.render(null).getState().gl
+      // Pre-mount baseline is false (not the usual true) specifically so
+      // it differs from the manual override below - autoClear only has two
+      // states, so this is the only way to make an unconditional restore-
+      // to-original and a "preserve the manual change" outcome observably
+      // different from each other.
+      gl.autoClear = false
+
+      const ref = React.createRef<EffectComposerImpl>()
+
+      await React.act(async () =>
+        root.render(
+          <EffectComposer ref={ref}>
+            <WrappedEffectA />
+          </EffectComposer>
+        )
+      )
+
+      await waitForComposer(ref)
+      expect(gl.autoClear).toBe(false)
+
+      // Something outside this component takes manual control while the
+      // composer happens to still be mounted - a deliberate, more current
+      // intent than whatever we captured before mount.
+      gl.autoClear = true
+
+      await React.act(async () => root.render(null))
+
+      // Must stay what the manual override set it to, not get silently
+      // reset back to the pre-mount value we originally captured (false).
+      expect(gl.autoClear).toBe(true)
+    })
+
+    it('does not clobber a manual toneMapping change made while the composer was mounted', async () => {
+      const gl = root.render(null).getState().gl
+      gl.toneMapping = THREE.ACESFilmicToneMapping
+
+      const ref = React.createRef<EffectComposerImpl>()
+
+      await React.act(async () =>
+        root.render(
+          <EffectComposer ref={ref}>
+            <WrappedEffectA />
+          </EffectComposer>
+        )
+      )
+
+      await waitForComposer(ref)
+      expect(gl.toneMapping).toBe(THREE.NoToneMapping)
+
+      gl.toneMapping = THREE.ReinhardToneMapping
+
+      await React.act(async () => root.render(null))
+
+      expect(gl.toneMapping).toBe(THREE.ReinhardToneMapping)
+    })
+  })
+
   describe('StrictMode', () => {
     it('preserves effects after the StrictMode fake-unmount/remount cycle', async () => {
       const ref = React.createRef<EffectComposerImpl>()