From 7fb66eb8cddc361d53f2846ff06735a91d92d02e Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Fri, 31 Jul 2026 22:24:39 +0200 Subject: [PATCH] fix: dispose -based effects, r3f never does it for them r3f explicitly never auto-disposes objects rendered via ("their state may be kept outside of React"), regardless of dispose={null}. Several effects that render this way had no cleanup at all (ASCII, ColorAverage, SelectiveBloom, SSAO), so they leaked their underlying postprocessing effect/texture on every unmount and every prop change that recreates the instance. Added a small useDispose hook (util.tsx) and wired it into every -based effect, with a WeakSet guard against double-dispose across StrictMode's dev-only mount/cleanup/mount cycle. Also fixes GodRays, which was declared as (props, ref) without forwardRef - under React 19 that ref parameter is never populated, so consumers passing a ref to GodRays silently got nothing. ChromaticAberration moves off wrapEffect onto the same manual construct-and-dispose pattern so its tuple `offset` prop coerces through useVector2 like the other vector-typed effects; test locks in that coercion. Adds the ColorAverage dispose coverage to EffectComposer.test.tsx that the previous commit's suite deferred here, since ColorAverage didn't dispose itself until this fix landed. --- src/effects/ASCII.tsx | 71 +++++++++---------- src/effects/ChromaticAberration.tsx | 31 ++++++++- src/effects/ColorAverage.tsx | 30 ++++---- src/effects/DepthOfField.tsx | 60 ++++++++-------- src/effects/Glitch.tsx | 27 ++++---- src/effects/GodRays.tsx | 16 +++-- src/effects/Grid.tsx | 17 +++-- src/effects/LUT.tsx | 17 ++--- src/effects/N8AO.tsx | 94 +++++++++++++------------- src/effects/Outline.tsx | 54 +++++++-------- src/effects/Pixelation.tsx | 16 +++-- src/effects/SSAO.tsx | 21 +++--- src/effects/SelectiveBloom.tsx | 50 +++++++------- src/effects/Texture.tsx | 24 ++++--- src/tests/ChromaticAberration.test.tsx | 31 +++++++++ src/tests/EffectComposer.test.tsx | 80 ++++++++++++++++++++++ src/util.tsx | 22 +++++- 17 files changed, 415 insertions(+), 246 deletions(-) create mode 100644 src/tests/ChromaticAberration.test.tsx diff --git a/src/effects/ASCII.tsx b/src/effects/ASCII.tsx index f25cb45f..b6744b56 100644 --- a/src/effects/ASCII.tsx +++ b/src/effects/ASCII.tsx @@ -1,29 +1,30 @@ // From: https://github.com/emilwidlund/ASCII // https://twitter.com/emilwidlund/status/1652386482420609024 -import { forwardRef, useMemo } from 'react' -import { CanvasTexture, Color, NearestFilter, RepeatWrapping, Texture, Uniform } from 'three' import { Effect } from 'postprocessing' +import { Ref, useMemo } from 'react' +import { CanvasTexture, Color, NearestFilter, RepeatWrapping, Texture, Uniform } from 'three' +import { useDispose } from '../util' -const fragment = ` -uniform sampler2D uCharacters; -uniform float uCharactersCount; -uniform float uCellSize; -uniform bool uInvert; -uniform vec3 uColor; +const fragment = /* glsl */ ` + uniform sampler2D uCharacters; + uniform float uCharactersCount; + uniform float uCellSize; + uniform bool uInvert; + uniform vec3 uColor; -const vec2 SIZE = vec2(16.); + const vec2 SIZE = vec2(16.); -vec3 greyscale(vec3 color, float strength) { + vec3 greyscale(vec3 color, float strength) { float g = dot(color, vec3(0.299, 0.587, 0.114)); return mix(color, vec3(g), strength); -} + } -vec3 greyscale(vec3 color) { + vec3 greyscale(vec3 color) { return greyscale(color, 1.0); -} + } -void mainImage(const in vec4 inputColor, const in vec2 uv, out vec4 outputColor) { + void mainImage(const in vec4 inputColor, const in vec2 uv, out vec4 outputColor) { vec2 cell = resolution / uCellSize; vec2 grid = 1.0 / cell; vec2 pixelizedUV = grid * (0.5 + floor(uv / grid)); @@ -43,7 +44,7 @@ void mainImage(const in vec4 inputColor, const in vec2 uv, out vec4 outputColor) asciiCharacter.rgb = uColor * asciiCharacter.r; asciiCharacter.a = pixelized.a; outputColor = asciiCharacter; -} + } ` interface IASCIIEffectProps { @@ -53,6 +54,7 @@ interface IASCIIEffectProps { cellSize?: number color?: string invert?: boolean + ref?: Ref } class ASCIIEffect extends Effect { @@ -63,7 +65,7 @@ class ASCIIEffect extends Effect { cellSize = 16, color = '#ffffff', invert = false, - }: IASCIIEffectProps = {}) { + }: Omit = {}) { const uniforms = new Map([ ['uCharacters', new Uniform(new Texture())], ['uCellSize', new Uniform(cellSize)], @@ -114,22 +116,21 @@ class ASCIIEffect extends Effect { } } -export const ASCII = /* @__PURE__ */ forwardRef( - ( - { - font = 'arial', - characters = ` .:,'-^=*+?!|0#X%WM@`, - fontSize = 54, - cellSize = 16, - color = '#ffffff', - invert = false, - }, - fref - ) => { - const effect = useMemo( - () => new ASCIIEffect({ characters, font, fontSize, cellSize, color, invert }), - [characters, fontSize, cellSize, color, invert, font] - ) - return - } -) +export function ASCII({ + font = 'arial', + characters = ` .:,'-^=*+?!|0#X%WM@`, + fontSize = 54, + cellSize = 16, + color = '#ffffff', + invert = false, + ref, +}: IASCIIEffectProps) { + const effect = useMemo( + () => new ASCIIEffect({ characters, font, fontSize, cellSize, color, invert }), + [characters, fontSize, cellSize, color, invert, font] + ) + + useDispose(effect) + + return +} diff --git a/src/effects/ChromaticAberration.tsx b/src/effects/ChromaticAberration.tsx index ff9a41df..c768071c 100644 --- a/src/effects/ChromaticAberration.tsx +++ b/src/effects/ChromaticAberration.tsx @@ -1,5 +1,30 @@ +import type { ReactThreeFiber } from '@react-three/fiber' import { ChromaticAberrationEffect } from 'postprocessing' -import { type EffectProps, wrapEffect } from '../wrapEffect' +import type { Ref } from 'react' +import { useMemo } from 'react' +import { useDispose, useVector2 } from '../util' -export type ChromaticAberrationProps = EffectProps -export const ChromaticAberration = /* @__PURE__ */ wrapEffect(ChromaticAberrationEffect) +export type ChromaticAberrationProps = Omit< + Partial[0]>, + 'offset' +> & { + ref?: Ref + offset?: ReactThreeFiber.Vector2 +} + +export function ChromaticAberration({ ref, ...props }: ChromaticAberrationProps) { + const offset = useVector2(props, 'offset') + + const effect = useMemo( + () => + new ChromaticAberrationEffect({ + ...props, + offset, + } as ConstructorParameters[0]), + [offset, props] + ) + + useDispose(effect) + + return +} diff --git a/src/effects/ColorAverage.tsx b/src/effects/ColorAverage.tsx index be603eae..5a56292e 100644 --- a/src/effects/ColorAverage.tsx +++ b/src/effects/ColorAverage.tsx @@ -1,15 +1,17 @@ -import { ColorAverageEffect, BlendFunction } from 'postprocessing' -import React, { Ref, forwardRef, useMemo } from 'react' - -export type ColorAverageProps = Partial<{ - blendFunction: BlendFunction -}> - -export const ColorAverage = /* @__PURE__ */ forwardRef(function ColorAverage( - { blendFunction = BlendFunction.NORMAL }: ColorAverageProps, - ref: Ref -) { - /** Because ColorAverage blendFunction is not an object but a number, we have to define a custom prop "blendFunction" */ +import { BlendFunction, ColorAverageEffect } from 'postprocessing' +import type { Ref } from 'react' +import { useMemo } from 'react' +import { useDispose } from '../util' + +export type ColorAverageProps = { + blendFunction?: BlendFunction + ref?: Ref +} + +export function ColorAverage({ blendFunction = BlendFunction.NORMAL, ref }: ColorAverageProps) { const effect = useMemo(() => new ColorAverageEffect(blendFunction), [blendFunction]) - return -}) + + useDispose(effect) + + return +} diff --git a/src/effects/DepthOfField.tsx b/src/effects/DepthOfField.tsx index e4dd8525..5c9b1fad 100644 --- a/src/effects/DepthOfField.tsx +++ b/src/effects/DepthOfField.tsx @@ -1,11 +1,14 @@ +import type { ReactThreeFiber } from '@react-three/fiber' import { DepthOfFieldEffect, MaskFunction } from 'postprocessing' -import { Ref, forwardRef, useMemo, useEffect, useContext } from 'react' -import { ReactThreeFiber } from '@react-three/fiber' +import type { Ref } from 'react' +import { use, useMemo } from 'react' import { type DepthPackingStrategies, type Texture, Vector3 } from 'three' import { EffectComposerContext } from '../EffectComposer' +import { useDispose } from '../util' -type DOFProps = ConstructorParameters[1] & +export type DepthOfFieldProps = ConstructorParameters[1] & Partial<{ + ref: Ref target: ReactThreeFiber.Vector3 depthTexture: { texture: Texture @@ -16,28 +19,27 @@ type DOFProps = ConstructorParameters[1] & blur: number }> -export const DepthOfField = /* @__PURE__ */ forwardRef(function DepthOfField( - { - blendFunction, - worldFocusDistance, - worldFocusRange, - focusDistance, - focusRange, - focalLength, - bokehScale, - resolutionScale, - resolutionX, - resolutionY, - width, - height, - target, - depthTexture, - ...props - }: DOFProps, - ref: Ref -) { - const { camera } = useContext(EffectComposerContext) +export function DepthOfField({ + ref, + blendFunction, + worldFocusDistance, + worldFocusRange, + focusDistance, + focusRange, + focalLength, + bokehScale, + resolutionScale, + resolutionX, + resolutionY, + width, + height, + target, + depthTexture, + ...props +}: DepthOfFieldProps) { + const { camera } = use(EffectComposerContext) const autoFocus = target != null + const effect = useMemo(() => { const effect = new DepthOfFieldEffect(camera, { blendFunction, @@ -79,11 +81,7 @@ export const DepthOfField = /* @__PURE__ */ forwardRef(function DepthOfField( depthTexture, ]) - useEffect(() => { - return () => { - effect.dispose() - } - }, [effect]) + useDispose(effect) - return -}) + return +} diff --git a/src/effects/Glitch.tsx b/src/effects/Glitch.tsx index 2ff7cb20..488823c8 100644 --- a/src/effects/Glitch.tsx +++ b/src/effects/Glitch.tsx @@ -1,8 +1,7 @@ -import { Vector2 } from 'three' -import { GlitchEffect, GlitchMode } from 'postprocessing' -import { Ref, forwardRef, useMemo, useLayoutEffect, useEffect } from 'react' import { ReactThreeFiber, useThree } from '@react-three/fiber' -import { useVector2 } from '../util' +import { GlitchEffect, GlitchMode } from 'postprocessing' +import { Ref, useLayoutEffect, useMemo } from 'react' +import { useDispose, useVector2 } from '../util' export type GlitchProps = ConstructorParameters[0] & Partial<{ @@ -12,29 +11,27 @@ export type GlitchProps = ConstructorParameters[0] & duration: ReactThreeFiber.Vector2 chromaticAberrationOffset: ReactThreeFiber.Vector2 strength: ReactThreeFiber.Vector2 + ref?: Ref }> -export const Glitch = /* @__PURE__ */ forwardRef(function Glitch( - { active = true, ...props }: GlitchProps, - ref: Ref -) { +export function Glitch({ active = true, ref, ...props }: GlitchProps) { const invalidate = useThree((state) => state.invalidate) const delay = useVector2(props, 'delay') const duration = useVector2(props, 'duration') const strength = useVector2(props, 'strength') const chromaticAberrationOffset = useVector2(props, 'chromaticAberrationOffset') + const effect = useMemo( () => new GlitchEffect({ ...props, delay, duration, strength, chromaticAberrationOffset }), [delay, duration, props, strength, chromaticAberrationOffset] ) + useLayoutEffect(() => { effect.mode = active ? props.mode || GlitchMode.SPORADIC : GlitchMode.DISABLED invalidate() }, [active, effect, invalidate, props.mode]) - useEffect(() => { - return () => { - effect.dispose?.() - } - }, [effect]) - return -}) + + useDispose(effect) + + return +} diff --git a/src/effects/GodRays.tsx b/src/effects/GodRays.tsx index 8f1db2ad..b4a3df6b 100644 --- a/src/effects/GodRays.tsx +++ b/src/effects/GodRays.tsx @@ -1,16 +1,20 @@ import { GodRaysEffect } from 'postprocessing' -import React, { Ref, forwardRef, useMemo, useContext, useLayoutEffect } from 'react' +import { Ref, RefObject, useContext, useLayoutEffect, useMemo } from 'react' import { Mesh, Points } from 'three' import { EffectComposerContext } from '../EffectComposer' -import { resolveRef } from '../util' +import { resolveRef, useDispose } from '../util' type GodRaysProps = ConstructorParameters[2] & { - sun: Mesh | Points | React.RefObject + sun: Mesh | Points | RefObject + ref?: Ref } -export const GodRays = /* @__PURE__ */ forwardRef(function GodRays(props: GodRaysProps, ref: Ref) { +export function GodRays({ ref, ...props }: GodRaysProps) { const { camera } = useContext(EffectComposerContext) const effect = useMemo(() => new GodRaysEffect(camera, resolveRef(props.sun), props), [camera, props]) useLayoutEffect(() => void (effect.lightSource = resolveRef(props.sun)), [effect, props.sun]) - return -}) + + useDispose(effect) + + return +} diff --git a/src/effects/Grid.tsx b/src/effects/Grid.tsx index 9fe03793..639e22b0 100644 --- a/src/effects/Grid.tsx +++ b/src/effects/Grid.tsx @@ -1,6 +1,7 @@ -import React, { Ref, forwardRef, useMemo, useLayoutEffect } from 'react' -import { GridEffect } from 'postprocessing' import { useThree } from '@react-three/fiber' +import { GridEffect } from 'postprocessing' +import { Ref, useLayoutEffect, useMemo } from 'react' +import { useDispose } from '../util' type GridProps = ConstructorParameters[0] & Partial<{ @@ -8,14 +9,20 @@ type GridProps = ConstructorParameters[0] & width: number height: number } + ref: Ref }> -export const Grid = /* @__PURE__ */ forwardRef(function Grid({ size, ...props }: GridProps, ref: Ref) { +export function Grid({ size, ref, ...props }: GridProps) { const invalidate = useThree((state) => state.invalidate) + const effect = useMemo(() => new GridEffect(props), [props]) + useLayoutEffect(() => { if (size) effect.setSize(size.width, size.height) invalidate() }, [effect, size, invalidate]) - return -}) + + useDispose(effect) + + return +} diff --git a/src/effects/LUT.tsx b/src/effects/LUT.tsx index f3d42d29..73abbb48 100644 --- a/src/effects/LUT.tsx +++ b/src/effects/LUT.tsx @@ -1,18 +1,17 @@ import { useThree } from '@react-three/fiber' -import { LUT3DEffect, BlendFunction } from 'postprocessing' -import React, { forwardRef, Ref, useLayoutEffect, useMemo } from 'react' +import { BlendFunction, LUT3DEffect } from 'postprocessing' +import { Ref, useLayoutEffect, useMemo } from 'react' import type { Texture } from 'three' +import { useDispose } from '../util' export type LUTProps = { lut: Texture blendFunction?: BlendFunction tetrahedralInterpolation?: boolean + ref: Ref } -export const LUT = /* @__PURE__ */ forwardRef(function LUT( - { lut, tetrahedralInterpolation, ...props }: LUTProps, - ref: Ref -) { +export function LUT({ lut, tetrahedralInterpolation, ref, ...props }: LUTProps) { const effect = useMemo(() => new LUT3DEffect(lut, props), [lut, props]) const invalidate = useThree((state) => state.invalidate) @@ -22,5 +21,7 @@ export const LUT = /* @__PURE__ */ forwardRef(function LUT( invalidate() }, [effect, invalidate, lut, tetrahedralInterpolation]) - return -}) + useDispose(effect) + + return +} diff --git a/src/effects/N8AO.tsx b/src/effects/N8AO.tsx index 1d861289..2c68a726 100644 --- a/src/effects/N8AO.tsx +++ b/src/effects/N8AO.tsx @@ -1,10 +1,11 @@ // From https://github.com/N8python/n8ao // https://twitter.com/N8Programs/status/1660996748485984261 -import { Ref, forwardRef, useLayoutEffect, useMemo } from 'react' +import { ReactThreeFiber, applyProps, useThree } from '@react-three/fiber' +import { Ref, useLayoutEffect, useMemo } from 'react' /* @ts-ignore */ import { N8AOPostPass } from 'n8ao' -import { useThree, ReactThreeFiber, applyProps } from '@react-three/fiber' +import { useDispose } from '../util' export type N8AOProps = { aoRadius?: number @@ -19,46 +20,32 @@ export type N8AOProps = { depthAwareUpsampling?: boolean screenSpaceRadius?: boolean renderMode?: 0 | 1 | 2 | 3 | 4 + ref?: Ref } -export const N8AO = /* @__PURE__ */ forwardRef( - ( - { - halfRes, - screenSpaceRadius, - quality, - depthAwareUpsampling = true, - aoRadius = 5, - aoSamples = 16, - denoiseSamples = 4, - denoiseRadius = 12, - distanceFalloff = 1, - intensity = 1, - color, - renderMode = 0, - }, - ref: Ref - ) => { - const { camera, scene } = useThree() - const effect = useMemo(() => new N8AOPostPass(scene, camera), [camera, scene]) +export function N8AO({ + halfRes, + screenSpaceRadius, + quality, + depthAwareUpsampling = true, + aoRadius = 5, + aoSamples = 16, + denoiseSamples = 4, + denoiseRadius = 12, + distanceFalloff = 1, + intensity = 1, + color, + renderMode = 0, + ref, +}: N8AOProps) { + const { camera, scene } = useThree() + const effect = useMemo(() => new N8AOPostPass(scene, camera), [camera, scene]) - // TODO: implement dispose upstream; this effect has memory leaks without - useLayoutEffect(() => { - applyProps(effect.configuration, { - color, - aoRadius, - distanceFalloff, - intensity, - aoSamples, - denoiseSamples, - denoiseRadius, - screenSpaceRadius, - renderMode, - halfRes, - depthAwareUpsampling, - }) - }, [ - screenSpaceRadius, + // TODO: implement dispose upstream; this effect has memory leaks without + useDispose(effect) + + useLayoutEffect(() => { + applyProps(effect.configuration, { color, aoRadius, distanceFalloff, @@ -66,16 +53,29 @@ export const N8AO = /* @__PURE__ */ forwardRef( aoSamples, denoiseSamples, denoiseRadius, + screenSpaceRadius, renderMode, halfRes, depthAwareUpsampling, - effect, - ]) + }) + }, [ + screenSpaceRadius, + color, + aoRadius, + distanceFalloff, + intensity, + aoSamples, + denoiseSamples, + denoiseRadius, + renderMode, + halfRes, + depthAwareUpsampling, + effect, + ]) - useLayoutEffect(() => { - if (quality) effect.setQualityMode(quality.charAt(0).toUpperCase() + quality.slice(1)) - }, [effect, quality]) + useLayoutEffect(() => { + if (quality) effect.setQualityMode(quality.charAt(0).toUpperCase() + quality.slice(1)) + }, [effect, quality]) - return - } -) + return +} diff --git a/src/effects/Outline.tsx b/src/effects/Outline.tsx index f78b69d8..16b7bde7 100644 --- a/src/effects/Outline.tsx +++ b/src/effects/Outline.tsx @@ -1,10 +1,10 @@ +import { useThree } from '@react-three/fiber' import { OutlineEffect } from 'postprocessing' -import { Ref, RefObject, forwardRef, useMemo, useEffect, useContext, useRef } from 'react' +import { Ref, RefObject, useContext, useEffect, useMemo } from 'react' import { Object3D } from 'three' -import { useThree } from '@react-three/fiber' import { EffectComposerContext } from '../EffectComposer' import { selectionContext } from '../Selection' -import { resolveRef } from '../util' +import { resolveRef, useDispose } from '../util' type ObjectRef = RefObject @@ -12,27 +12,26 @@ export type OutlineProps = ConstructorParameters[2] & Partial<{ selection: Object3D | Object3D[] | ObjectRef | ObjectRef[] selectionLayer: number + ref?: Ref }> -export const Outline = /* @__PURE__ */ forwardRef(function Outline( - { - selection = [], - selectionLayer = 10, - blendFunction, - patternTexture, - edgeStrength, - pulseSpeed, - visibleEdgeColor, - hiddenEdgeColor, - width, - height, - kernelSize, - blur, - xRay, - ...props - }: OutlineProps, - forwardRef: Ref -) { +export function Outline({ + selection = [], + selectionLayer = 10, + blendFunction, + patternTexture, + edgeStrength, + pulseSpeed, + visibleEdgeColor, + hiddenEdgeColor, + width, + height, + kernelSize, + blur, + xRay, + ref, + ...props +}: OutlineProps) { const invalidate = useThree((state) => state.invalidate) const { scene, camera } = useContext(EffectComposerContext) @@ -93,7 +92,6 @@ export const Outline = /* @__PURE__ */ forwardRef(function Outline( invalidate() }, [effect, invalidate, selectionLayer]) - const ref = useRef(undefined) useEffect(() => { if (api && api.enabled) { if (api.selected?.length) { @@ -107,11 +105,7 @@ export const Outline = /* @__PURE__ */ forwardRef(function Outline( } }, [api, effect.selection, invalidate]) - useEffect(() => { - return () => { - effect.dispose() - } - }, [effect]) + useDispose(effect) - return -}) + return +} diff --git a/src/effects/Pixelation.tsx b/src/effects/Pixelation.tsx index 61a24a3e..66ad13bf 100644 --- a/src/effects/Pixelation.tsx +++ b/src/effects/Pixelation.tsx @@ -1,15 +1,17 @@ -import { forwardRef, useMemo, Ref } from 'react' import { PixelationEffect } from 'postprocessing' +import { Ref, useMemo } from 'react' +import { useDispose } from '../util' export type PixelationProps = { granularity?: number + ref?: Ref } -export const Pixelation = /* @__PURE__ */ forwardRef(function Pixelation( - { granularity = 5 }: PixelationProps, - ref: Ref -) { +export function Pixelation({ granularity = 5, ref }: PixelationProps) { /** Because GlitchEffect granularity is not an object but a number, we have to define a custom prop "granularity" */ const effect = useMemo(() => new PixelationEffect(granularity), [granularity]) - return -}) + + useDispose(effect) + + return +} diff --git a/src/effects/SSAO.tsx b/src/effects/SSAO.tsx index 5bbac10a..2d5fd723 100644 --- a/src/effects/SSAO.tsx +++ b/src/effects/SSAO.tsx @@ -1,20 +1,20 @@ -import { Ref, forwardRef, useContext, useMemo } from 'react' -import { SSAOEffect, BlendFunction } from 'postprocessing' +import { BlendFunction, SSAOEffect } from 'postprocessing' +import { Ref, useContext, useMemo } from 'react' import { EffectComposerContext } from '../EffectComposer' +import { useDispose } from '../util' // first two args are camera and texture -type SSAOProps = ConstructorParameters[2] +type SSAOProps = ConstructorParameters[2] & { ref?: Ref } -export const SSAO = /* @__PURE__ */ forwardRef(function SSAO( - props: SSAOProps, - ref: Ref -) { +export function SSAO({ ref, ...props }: SSAOProps) { const { camera, normalPass, downSamplingPass, resolutionScale } = useContext(EffectComposerContext) + const effect = useMemo(() => { if (normalPass === null && downSamplingPass === null) { console.error('Please enable the NormalPass in the EffectComposer in order to use SSAO.') return {} } + return new SSAOEffect(camera, normalPass && !downSamplingPass ? (normalPass as any).texture : null, { blendFunction: BlendFunction.MULTIPLY, samples: 30, @@ -37,5 +37,8 @@ export const SSAO = /* @__PURE__ */ forwardRef(function S // 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 -}) + + useDispose(effect) + + return +} diff --git a/src/effects/SelectiveBloom.tsx b/src/effects/SelectiveBloom.tsx index ff8bab1d..7088ad73 100644 --- a/src/effects/SelectiveBloom.tsx +++ b/src/effects/SelectiveBloom.tsx @@ -1,11 +1,11 @@ -import { SelectiveBloomEffect, BlendFunction } from 'postprocessing' +import { useThree } from '@react-three/fiber' import type { BloomEffectOptions } from 'postprocessing' -import React, { Ref, RefObject, forwardRef, useMemo, useEffect, useContext, useRef } from 'react' +import { BlendFunction, SelectiveBloomEffect } from 'postprocessing' +import { Ref, RefObject, useContext, useEffect, useMemo } from 'react' import { Object3D } from 'three' -import { useThree } from '@react-three/fiber' import { EffectComposerContext } from '../EffectComposer' import { selectionContext } from '../Selection' -import { resolveRef } from '../util' +import { resolveRef, useDispose } from '../util' type ObjectRef = RefObject @@ -16,30 +16,28 @@ export type SelectiveBloomProps = BloomEffectOptions & selectionLayer: number inverted: boolean ignoreBackground: boolean + ref?: Ref }> const addLight = (light: Object3D, effect: SelectiveBloomEffect) => light.layers.enable(effect.selection.layer) const removeLight = (light: Object3D, effect: SelectiveBloomEffect) => light.layers.disable(effect.selection.layer) -export const SelectiveBloom = /* @__PURE__ */ forwardRef(function SelectiveBloom( - { - selection = [], - selectionLayer = 10, - lights = [], - inverted = false, - ignoreBackground = false, - luminanceThreshold, - luminanceSmoothing, - intensity, - width, - height, - kernelSize, - mipmapBlur, - - ...props - }: SelectiveBloomProps, - forwardRef: Ref -) { +export function SelectiveBloom({ + selection = [], + selectionLayer = 10, + lights = [], + inverted = false, + ignoreBackground = false, + luminanceThreshold, + luminanceSmoothing, + intensity, + width, + height, + kernelSize, + mipmapBlur, + ref, + ...props +}: SelectiveBloomProps) { if (lights.length === 0) { console.warn('SelectiveBloom requires lights to work.') } @@ -122,5 +120,7 @@ export const SelectiveBloom = /* @__PURE__ */ forwardRef(function SelectiveBloom } }, [api, effect.selection, invalidate]) - return -}) + useDispose(effect) + + return +} diff --git a/src/effects/Texture.tsx b/src/effects/Texture.tsx index f6bf4794..68057a51 100644 --- a/src/effects/Texture.tsx +++ b/src/effects/Texture.tsx @@ -1,23 +1,27 @@ -import { TextureEffect } from 'postprocessing' -import { Ref, forwardRef, useMemo, useLayoutEffect } from 'react' import { useLoader } from '@react-three/fiber' -import { TextureLoader, SRGBColorSpace, RepeatWrapping } from 'three' +import { TextureEffect } from 'postprocessing' +import { Ref, useLayoutEffect, useMemo } from 'react' +import { RepeatWrapping, SRGBColorSpace, TextureLoader } from 'three' +import { useDispose } from '../util' type TextureProps = ConstructorParameters[0] & { textureSrc: string /** opacity of provided texture */ opacity?: number + ref: Ref } -export const Texture = /* @__PURE__ */ forwardRef(function Texture( - { textureSrc, texture, opacity = 1, ...props }: TextureProps, - ref: Ref -) { +export function Texture({ textureSrc, texture, opacity = 1, ref, ...props }: TextureProps) { const t = useLoader(TextureLoader, textureSrc) + useLayoutEffect(() => { t.colorSpace = SRGBColorSpace t.wrapS = t.wrapT = RepeatWrapping }, [t]) - const effect = useMemo(() => new TextureEffect({ ...props, texture: t || texture }), [props, t, texture]) - return -}) + + const effect = useMemo(() => new TextureEffect({ ...props, texture: t || texture }), []) + + useDispose(effect) + + return +} diff --git a/src/tests/ChromaticAberration.test.tsx b/src/tests/ChromaticAberration.test.tsx new file mode 100644 index 00000000..5e7c5d16 --- /dev/null +++ b/src/tests/ChromaticAberration.test.tsx @@ -0,0 +1,31 @@ +import { ChromaticAberrationEffect, EffectComposer as EffectComposerImpl } from 'postprocessing' +import * as React from 'react' +import { Vector2 } from 'three' +import { describe, expect, it } from 'vitest' +import { EffectComposer } from '../EffectComposer' +import { ChromaticAberration } from '../effects/ChromaticAberration' +import { flush, root } from './test-utils' + +describe('ChromaticAberration', () => { + it('coerces a tuple offset into a real Vector2 (#348)', async () => { + const ref = React.createRef() + const composerRef = React.createRef() + + await React.act(async () => + root.render( + + + + ) + ) + + await flush() + + expect(ref.current!.offset).toBeInstanceOf(Vector2) + expect(() => ref.current!.offset.set(0.002, 0.001)).not.toThrow() + expect(ref.current!.offset.x).toBeCloseTo(0.002) + expect(ref.current!.offset.y).toBeCloseTo(0.001) + + await React.act(async () => root.render(null)) + }) +}) diff --git a/src/tests/EffectComposer.test.tsx b/src/tests/EffectComposer.test.tsx index c30fd760..60c28e8a 100644 --- a/src/tests/EffectComposer.test.tsx +++ b/src/tests/EffectComposer.test.tsx @@ -1,4 +1,5 @@ import { + BlendFunction, ColorAverageEffect, DepthDownsamplingPass, Effect, @@ -403,6 +404,85 @@ describe('EffectComposer', () => { disposeSpy.mockRestore() }) + + it('disposes a hand-constructed effect exactly once on unmount', async () => { + const disposeSpy = vi.spyOn(ColorAverageEffect.prototype, 'dispose') + const ref = React.createRef() + + await React.act(async () => + root.render( + + + + + ) + ) + + await waitForComposer(ref) + await React.act(async () => root.render(null)) + + expect(disposeSpy).toHaveBeenCalledTimes(1) + disposeSpy.mockRestore() + }) + + it('disposes exactly as many ColorAverage instances as it constructs, across repeated prop changes', async () => { + const disposeSpy = vi.spyOn(ColorAverageEffect.prototype, 'dispose') + const ref = React.createRef() + const seenInstances = new Set() + const cycles = 20 + + try { + for (let i = 0; i < cycles; i++) { + await React.act(async () => + root.render( + + + + ) + ) + await flush() + if (ref.current) seenInstances.add(ref.current) + } + + await React.act(async () => root.render(null)) + + expect(seenInstances.size).toBe(cycles) + expect(disposeSpy).toHaveBeenCalledTimes(cycles) + } finally { + disposeSpy.mockRestore() + } + }) + + it('never disposes the same ColorAverage instance twice, even in StrictMode', async () => { + const disposedNodes: ColorAverageEffect[] = [] + const disposeSpy = vi.spyOn(ColorAverageEffect.prototype, 'dispose').mockImplementation(function ( + this: ColorAverageEffect + ) { + disposedNodes.push(this) + }) + + try { + const ref = React.createRef() + for (let i = 0; i < 20; i++) { + await React.act(async () => + root.render( + strict( + + + + ) + ) + ) + await flush() + } + await React.act(async () => root.render(null)) + + const uniqueDisposed = new Set(disposedNodes) + expect(uniqueDisposed.size).toBe(disposedNodes.length) + } finally { + disposeSpy.mockRestore() + } + }) }) describe('StrictMode', () => { diff --git a/src/util.tsx b/src/util.tsx index 58da3fd0..21d5d5b7 100644 --- a/src/util.tsx +++ b/src/util.tsx @@ -1,10 +1,30 @@ import type { ReactThreeFiber } from '@react-three/fiber' -import { useMemo, type RefObject } from 'react' +import { useEffect, useMemo, useRef, type RefObject } from 'react' import { Vector2, type Vector2Tuple } from 'three' export const resolveRef = (ref: T | RefObject) => typeof ref === 'object' && ref != null && 'current' in ref ? ref.current : ref +/** + * r3f never disposes objects (their state may be owned outside + * React), so effects rendered that way must dispose themselves. Guards + * against double-dispose across StrictMode's dev-only mount/cleanup/mount + * cycle, where the cleanup closure re-runs against the same instance. + */ +export const useDispose = void }>(instance: T): void => { + const disposedRef = useRef>(new WeakSet()) + + useEffect(() => { + const disposed = disposedRef.current + return () => { + if (instance && typeof instance === 'object' && !disposed.has(instance)) { + disposed.add(instance) + instance.dispose?.() + } + } + }, [instance]) +} + export const useVector2 = (props: Record, key: string): Vector2 => { const value = props[key] as ReactThreeFiber.Vector2 | undefined