From c4e85e9826706d4ce6813694c8314f6b33d4042b Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Sat, 1 Aug 2026 20:31:26 +0200 Subject: [PATCH 1/2] fix(Outline,SelectiveBloom): stop unnecessary effect reconstruction and re-runs Unstable `[]` prop defaults and `props` in a useMemo dep array caused SelectiveBloomEffect to reconstruct (and both effects' declarative selection to re-set) on every unrelated render. Extracted the shared selection-sync logic into `useSelectionSync`, and fixed an effect ordering bug found along the way (lights were assigned to a stale selection layer). --- src/effects/Outline.tsx | 47 ++------------ src/effects/SelectiveBloom.tsx | 74 +++++++-------------- src/tests/Outline.test.tsx | 74 +++++++++++++++++++++ src/tests/SelectiveBloom.test.tsx | 103 ++++++++++++++++++++++++++++++ src/util.tsx | 60 ++++++++++++++++- 5 files changed, 263 insertions(+), 95 deletions(-) create mode 100644 src/tests/Outline.test.tsx create mode 100644 src/tests/SelectiveBloom.test.tsx diff --git a/src/effects/Outline.tsx b/src/effects/Outline.tsx index 16b7bde7..5c31fc80 100644 --- a/src/effects/Outline.tsx +++ b/src/effects/Outline.tsx @@ -1,10 +1,8 @@ -import { useThree } from '@react-three/fiber' import { OutlineEffect } from 'postprocessing' -import { Ref, RefObject, useContext, useEffect, useMemo } from 'react' +import { Ref, RefObject, use, useMemo } from 'react' import { Object3D } from 'three' import { EffectComposerContext } from '../EffectComposer' -import { selectionContext } from '../Selection' -import { resolveRef, useDispose } from '../util' +import { EMPTY_ARRAY, useDispose, useSelectionSync } from '../util' type ObjectRef = RefObject @@ -16,7 +14,7 @@ export type OutlineProps = ConstructorParameters[2] & }> export function Outline({ - selection = [], + selection = EMPTY_ARRAY, selectionLayer = 10, blendFunction, patternTexture, @@ -32,8 +30,7 @@ export function Outline({ ref, ...props }: OutlineProps) { - const invalidate = useThree((state) => state.invalidate) - const { scene, camera } = useContext(EffectComposerContext) + const { scene, camera } = use(EffectComposerContext) const effect = useMemo( () => @@ -70,41 +67,7 @@ export function Outline({ ] ) - const api = useContext(selectionContext) - - useEffect(() => { - // Do not allow array selection if declarative selection is active - // TODO: array selection should probably be deprecated altogether - if (!api && selection) { - effect.selection.set( - Array.isArray(selection) ? (selection as Object3D[]).map(resolveRef) : [resolveRef(selection) as Object3D] - ) - invalidate() - return () => { - effect.selection.clear() - invalidate() - } - } - }, [effect, selection, api, invalidate]) - - useEffect(() => { - effect.selectionLayer = selectionLayer - invalidate() - }, [effect, invalidate, selectionLayer]) - - useEffect(() => { - if (api && api.enabled) { - if (api.selected?.length) { - effect.selection.set(api.selected) - invalidate() - return () => { - effect.selection.clear() - invalidate() - } - } - } - }, [api, effect.selection, invalidate]) - + useSelectionSync(effect, selection, selectionLayer) useDispose(effect) return diff --git a/src/effects/SelectiveBloom.tsx b/src/effects/SelectiveBloom.tsx index 7088ad73..ab553932 100644 --- a/src/effects/SelectiveBloom.tsx +++ b/src/effects/SelectiveBloom.tsx @@ -1,11 +1,10 @@ import { useThree } from '@react-three/fiber' import type { BloomEffectOptions } from 'postprocessing' import { BlendFunction, SelectiveBloomEffect } from 'postprocessing' -import { Ref, RefObject, useContext, useEffect, useMemo } from 'react' +import { Ref, RefObject, use, useEffect, useMemo } from 'react' import { Object3D } from 'three' import { EffectComposerContext } from '../EffectComposer' -import { selectionContext } from '../Selection' -import { resolveRef, useDispose } from '../util' +import { EMPTY_ARRAY, resolveRef, useDispose, useSelectionSync } from '../util' type ObjectRef = RefObject @@ -23,9 +22,9 @@ const addLight = (light: Object3D, effect: SelectiveBloomEffect) => light.layers const removeLight = (light: Object3D, effect: SelectiveBloomEffect) => light.layers.disable(effect.selection.layer) export function SelectiveBloom({ - selection = [], + selection = EMPTY_ARRAY, selectionLayer = 10, - lights = [], + lights = EMPTY_ARRAY, inverted = false, ignoreBackground = false, luminanceThreshold, @@ -38,14 +37,12 @@ export function SelectiveBloom({ ref, ...props }: SelectiveBloomProps) { - if (lights.length === 0) { - console.warn('SelectiveBloom requires lights to work.') - } + const { scene, camera } = use(EffectComposerContext) const invalidate = useThree((state) => state.invalidate) - const { scene, camera } = useContext(EffectComposerContext) + const effect = useMemo(() => { - const effect = new SelectiveBloomEffect(scene, camera, { + const instance = new SelectiveBloomEffect(scene, camera, { blendFunction: BlendFunction.ADD, luminanceThreshold, luminanceSmoothing, @@ -56,9 +53,11 @@ export function SelectiveBloom({ mipmapBlur, ...props, }) - effect.inverted = inverted - effect.ignoreBackground = ignoreBackground - return effect + instance.inverted = inverted + instance.ignoreBackground = ignoreBackground + return instance + // NOTE: `props` is an unstable reference, so we can't memoize it + // eslint-disable-next-line react-hooks/exhaustive-deps }, [ scene, camera, @@ -71,55 +70,30 @@ export function SelectiveBloom({ mipmapBlur, inverted, ignoreBackground, - props, ]) - const api = useContext(selectionContext) + // Must run before the lights effect below: addLight/removeLight read + // effect.selection.layer live, so it needs to already reflect the + // latest selectionLayer by the time lights get (re-)assigned to it. + useSelectionSync(effect, selection, selectionLayer) useEffect(() => { - // Do not allow array selection if declarative selection is active - // TODO: array selection should probably be deprecated altogether - if (!api && selection) { - effect.selection.set( - Array.isArray(selection) ? (selection as Object3D[]).map(resolveRef) : [resolveRef(selection) as Object3D] - ) - invalidate() - return () => { - effect.selection.clear() - invalidate() - } + if (lights.length === 0) { + console.warn('SelectiveBloom requires lights to work.') + return } - }, [effect, selection, api, invalidate]) - useEffect(() => { - effect.selection.layer = selectionLayer + lights.forEach((light) => addLight(resolveRef(light), effect)) + invalidate() - }, [effect, invalidate, selectionLayer]) - useEffect(() => { - if (lights && lights.length > 0) { - lights.forEach((light) => addLight(resolveRef(light), effect)) + return () => { + lights.forEach((light) => removeLight(resolveRef(light), effect)) + invalidate() - return () => { - lights.forEach((light) => removeLight(resolveRef(light), effect)) - invalidate() - } } }, [effect, invalidate, lights, selectionLayer]) - useEffect(() => { - if (api && api.enabled) { - if (api.selected?.length) { - effect.selection.set(api.selected) - invalidate() - return () => { - effect.selection.clear() - invalidate() - } - } - } - }, [api, effect.selection, invalidate]) - useDispose(effect) return diff --git a/src/tests/Outline.test.tsx b/src/tests/Outline.test.tsx new file mode 100644 index 00000000..ece65c9e --- /dev/null +++ b/src/tests/Outline.test.tsx @@ -0,0 +1,74 @@ +import { EffectComposer as EffectComposerImpl, OutlineEffect, Selection as PPSelection } from 'postprocessing' +import * as React from 'react' +import { Mesh } from 'three' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { EffectComposer } from '../EffectComposer' +import { Outline } from '../effects/Outline' +import { Select, Selection } from '../Selection' +import { flush, root, waitForComposer } from './test-utils' + +afterEach(async () => { + await React.act(async () => { + root.render(null) + }) +}) + +describe('Outline', () => { + it('does not re-set its (empty, declarative-mode) selection on unrelated re-renders', async () => { + const setSpy = vi.spyOn(PPSelection.prototype, 'set') + const composerRef = React.createRef() + + const render = (tick: number) => + root.render( + + + + + ) + + await React.act(async () => render(0)) + await waitForComposer(composerRef) + await flush() + setSpy.mockClear() + + for (let t = 1; t <= 5; t++) { + await React.act(async () => render(t)) + await flush() + } + + expect(setSpy).not.toHaveBeenCalled() + setSpy.mockRestore() + }) + + it('sets its selection from the Selection/Select API and clears it when the object deselects', async () => { + const effectRef = React.createRef() + const meshRef = React.createRef() + + const render = (enabled: boolean) => + root.render( + + + + + + + ) + + await React.act(async () => render(true)) + await flush() + await flush() + + expect(Array.from(effectRef.current!.selection)).toContain(meshRef.current) + + await React.act(async () => render(false)) + await flush() + await flush() + + expect(Array.from(effectRef.current!.selection)).not.toContain(meshRef.current) + }) +}) diff --git a/src/tests/SelectiveBloom.test.tsx b/src/tests/SelectiveBloom.test.tsx new file mode 100644 index 00000000..075b21cb --- /dev/null +++ b/src/tests/SelectiveBloom.test.tsx @@ -0,0 +1,103 @@ +import { EffectComposer as EffectComposerImpl, SelectiveBloomEffect } from 'postprocessing' +import * as React from 'react' +import { Mesh, PointLight } from 'three' +import { afterEach, describe, expect, it } from 'vitest' +import { EffectComposer } from '../EffectComposer' +import { SelectiveBloom } from '../effects/SelectiveBloom' +import { Select, Selection } from '../Selection' +import { flush, root, waitForComposer } from './test-utils' + +afterEach(async () => { + await React.act(async () => { + root.render(null) + }) +}) + +describe('SelectiveBloom', () => { + it('does not reconstruct the effect (with its GPU resources) on unrelated re-renders', async () => { + const composerRef = React.createRef() + const effectRef = React.createRef() + const light = new PointLight() + + const render = (tick: number) => + root.render( + + + + + ) + + await React.act(async () => render(0)) + await waitForComposer(composerRef) + await flush() + const first = effectRef.current + expect(first).toBeTruthy() + + for (let t = 1; t <= 5; t++) { + await React.act(async () => render(t)) + await flush() + } + + expect(effectRef.current).toBe(first) + }) + + it('sets its selection from the Selection/Select API and clears it when the object deselects', async () => { + const effectRef = React.createRef() + const meshRef = React.createRef() + const light = new PointLight() + + const render = (enabled: boolean) => + root.render( + + + + + + + ) + + await React.act(async () => render(true)) + await flush() + await flush() + + expect(Array.from(effectRef.current!.selection)).toContain(meshRef.current) + + await React.act(async () => render(false)) + await flush() + await flush() + + expect(Array.from(effectRef.current!.selection)).not.toContain(meshRef.current) + }) + + it('moves lights to the new render layer when selectionLayer changes', async () => { + const composerRef = React.createRef() + const effectRef = React.createRef() + const light = new PointLight() + const onLayer = (n: number) => light.layers.test({ mask: 1 << n } as never) + + const render = (layer: number) => + root.render( + + + + ) + + await React.act(async () => render(10)) + await waitForComposer(composerRef) + await flush() + await flush() + expect(onLayer(10)).toBe(true) + + await React.act(async () => render(15)) + await flush() + await flush() + + expect(onLayer(15)).toBe(true) + expect(onLayer(10)).toBe(false) + }) +}) diff --git a/src/util.tsx b/src/util.tsx index 21d5d5b7..d2da2cdb 100644 --- a/src/util.tsx +++ b/src/util.tsx @@ -1,10 +1,64 @@ -import type { ReactThreeFiber } from '@react-three/fiber' -import { useEffect, useMemo, useRef, type RefObject } from 'react' -import { Vector2, type Vector2Tuple } from 'three' +import { useThree, type ReactThreeFiber } from '@react-three/fiber' +import type { Selection as PPSelection } from 'postprocessing' +import { use, useEffect, useMemo, useRef, type RefObject } from 'react' +import { Object3D, Vector2, type Vector2Tuple } from 'three' +import { selectionContext } from './Selection' + +// Stable reference for array-typed props defaulting to "nothing" - `= []` +// as a default parameter allocates a new array on every call, which is +// enough to retrigger any effect that depends on it. +export const EMPTY_ARRAY: never[] = [] export const resolveRef = (ref: T | RefObject) => typeof ref === 'object' && ref != null && 'current' in ref ? ref.current : ref +/** + * Keeps a postprocessing effect's `selection` (and its render layer) in + * sync with either mode effects support: the manual + * `selection` prop (used only when there's no enclosing ), or + * the declarative Selection/Select API. The two are mutually exclusive - + * context wins when both are present. + */ +export function useSelectionSync( + effect: { selection: PPSelection }, + selection: Object3D | Object3D[] | RefObject | RefObject[], + selectionLayer: number +): void { + const invalidate = useThree((state) => state.invalidate) + const api = use(selectionContext) + + useEffect(() => { + effect.selection.layer = selectionLayer + invalidate() + }, [effect, invalidate, selectionLayer]) + + useEffect(() => { + if (api) return + const resolved: Object3D[] = Array.isArray(selection) + ? selection.map((o) => resolveRef(o)) + : [resolveRef(selection)] + if (!resolved.length) return + + effect.selection.set(resolved) + invalidate() + return () => { + effect.selection.clear() + invalidate() + } + }, [effect, selection, api, invalidate]) + + useEffect(() => { + if (api && api.enabled && api.selected?.length) { + effect.selection.set(api.selected) + invalidate() + return () => { + effect.selection.clear() + invalidate() + } + } + }, [api, effect.selection, invalidate]) +} + /** * r3f never disposes objects (their state may be owned outside * React), so effects rendered that way must dispose themselves. Guards From 04bc2895fbe4c5c76e948b1c21ca9f8704c01964 Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Sat, 1 Aug 2026 21:06:08 +0200 Subject: [PATCH 2/2] fix(Outline,SelectiveBloom): guard against unattached refs, make all effect options reactive Copilot review: resolveRef can return null before a ref attaches, which crashed effect.selection.set()/addLight. Filtered nulls, and fixed ObjectRef's own type to admit null so tsc catches this going forward. Also enumerated the remaining BloomEffectOptions/OutlineEffect options explicitly instead of an unstable ...props spread, so they update reactively instead of only applying on first mount. --- src/effects/Outline.tsx | 35 +++++++++++++++++--------- src/effects/SelectiveBloom.tsx | 42 +++++++++++++++++++++---------- src/tests/Outline.test.tsx | 17 ++++++++++++- src/tests/SelectiveBloom.test.tsx | 17 ++++++++++++- src/util.tsx | 8 +++--- 5 files changed, 88 insertions(+), 31 deletions(-) diff --git a/src/effects/Outline.tsx b/src/effects/Outline.tsx index 5c31fc80..8f8e99a2 100644 --- a/src/effects/Outline.tsx +++ b/src/effects/Outline.tsx @@ -4,7 +4,7 @@ import { Object3D } from 'three' import { EffectComposerContext } from '../EffectComposer' import { EMPTY_ARRAY, useDispose, useSelectionSync } from '../util' -type ObjectRef = RefObject +type ObjectRef = RefObject export type OutlineProps = ConstructorParameters[2] & Partial<{ @@ -18,17 +18,21 @@ export function Outline({ selectionLayer = 10, blendFunction, patternTexture, + patternScale, edgeStrength, pulseSpeed, visibleEdgeColor, hiddenEdgeColor, + multisampling, + resolutionScale, + resolutionX, + resolutionY, width, height, kernelSize, blur, xRay, ref, - ...props }: OutlineProps) { const { scene, camera } = use(EffectComposerContext) @@ -37,33 +41,40 @@ export function Outline({ new OutlineEffect(scene, camera, { blendFunction, patternTexture, + patternScale, edgeStrength, pulseSpeed, visibleEdgeColor, hiddenEdgeColor, + multisampling, + resolutionScale, + resolutionX, + resolutionY, width, height, kernelSize, blur, xRay, - ...props, }), - // NOTE: `props` is an unstable reference, so we can't memoize it - // eslint-disable-next-line react-hooks/exhaustive-deps [ blendFunction, - blur, - camera, - edgeStrength, - height, - hiddenEdgeColor, - kernelSize, patternTexture, + patternScale, + edgeStrength, pulseSpeed, - scene, visibleEdgeColor, + hiddenEdgeColor, + multisampling, + resolutionScale, + resolutionX, + resolutionY, width, + height, + kernelSize, + blur, xRay, + camera, + scene, ] ) diff --git a/src/effects/SelectiveBloom.tsx b/src/effects/SelectiveBloom.tsx index ab553932..7007dddc 100644 --- a/src/effects/SelectiveBloom.tsx +++ b/src/effects/SelectiveBloom.tsx @@ -6,7 +6,7 @@ import { Object3D } from 'three' import { EffectComposerContext } from '../EffectComposer' import { EMPTY_ARRAY, resolveRef, useDispose, useSelectionSync } from '../util' -type ObjectRef = RefObject +type ObjectRef = RefObject export type SelectiveBloomProps = BloomEffectOptions & Partial<{ @@ -29,13 +29,17 @@ export function SelectiveBloom({ ignoreBackground = false, luminanceThreshold, luminanceSmoothing, + mipmapBlur, intensity, + radius, + levels, + kernelSize, + resolutionScale, width, height, - kernelSize, - mipmapBlur, + resolutionX, + resolutionY, ref, - ...props }: SelectiveBloomProps) { const { scene, camera } = use(EffectComposerContext) @@ -46,28 +50,35 @@ export function SelectiveBloom({ blendFunction: BlendFunction.ADD, luminanceThreshold, luminanceSmoothing, + mipmapBlur, intensity, + radius, + levels, + kernelSize, + resolutionScale, width, height, - kernelSize, - mipmapBlur, - ...props, + resolutionX, + resolutionY, }) instance.inverted = inverted instance.ignoreBackground = ignoreBackground return instance - // NOTE: `props` is an unstable reference, so we can't memoize it - // eslint-disable-next-line react-hooks/exhaustive-deps }, [ scene, camera, luminanceThreshold, luminanceSmoothing, + mipmapBlur, intensity, + radius, + levels, + kernelSize, + resolutionScale, width, height, - kernelSize, - mipmapBlur, + resolutionX, + resolutionY, inverted, ignoreBackground, ]) @@ -83,12 +94,17 @@ export function SelectiveBloom({ return } - lights.forEach((light) => addLight(resolveRef(light), effect)) + // Refs may not have attached yet - resolve and drop nullish entries + // rather than crashing addLight/removeLight on a null object. + const resolvedLights = lights.map((light) => resolveRef(light)).filter((light): light is Object3D => light != null) + if (resolvedLights.length === 0) return + + resolvedLights.forEach((light) => addLight(light, effect)) invalidate() return () => { - lights.forEach((light) => removeLight(resolveRef(light), effect)) + resolvedLights.forEach((light) => removeLight(light, effect)) invalidate() } diff --git a/src/tests/Outline.test.tsx b/src/tests/Outline.test.tsx index ece65c9e..e0e62062 100644 --- a/src/tests/Outline.test.tsx +++ b/src/tests/Outline.test.tsx @@ -1,6 +1,6 @@ import { EffectComposer as EffectComposerImpl, OutlineEffect, Selection as PPSelection } from 'postprocessing' import * as React from 'react' -import { Mesh } from 'three' +import { Mesh, Object3D } from 'three' import { afterEach, describe, expect, it, vi } from 'vitest' import { EffectComposer } from '../EffectComposer' import { Outline } from '../effects/Outline' @@ -71,4 +71,19 @@ describe('Outline', () => { expect(Array.from(effectRef.current!.selection)).not.toContain(meshRef.current) }) + + it('does not throw when a selection ref has not attached yet', async () => { + const composerRef = React.createRef() + const unattachedRef = React.createRef() + + await React.act(async () => + root.render( + + + + ) + ) + await waitForComposer(composerRef) + await expect(flush()).resolves.not.toThrow() + }) }) diff --git a/src/tests/SelectiveBloom.test.tsx b/src/tests/SelectiveBloom.test.tsx index 075b21cb..d7bff48e 100644 --- a/src/tests/SelectiveBloom.test.tsx +++ b/src/tests/SelectiveBloom.test.tsx @@ -1,6 +1,6 @@ import { EffectComposer as EffectComposerImpl, SelectiveBloomEffect } from 'postprocessing' import * as React from 'react' -import { Mesh, PointLight } from 'three' +import { Mesh, Object3D, PointLight } from 'three' import { afterEach, describe, expect, it } from 'vitest' import { EffectComposer } from '../EffectComposer' import { SelectiveBloom } from '../effects/SelectiveBloom' @@ -100,4 +100,19 @@ describe('SelectiveBloom', () => { expect(onLayer(15)).toBe(true) expect(onLayer(10)).toBe(false) }) + + it('does not throw when a lights ref has not attached yet', async () => { + const composerRef = React.createRef() + const unattachedRef = React.createRef() + + await React.act(async () => + root.render( + + + + ) + ) + await waitForComposer(composerRef) + await expect(flush()).resolves.not.toThrow() + }) }) diff --git a/src/util.tsx b/src/util.tsx index d2da2cdb..e45d9e6d 100644 --- a/src/util.tsx +++ b/src/util.tsx @@ -21,7 +21,7 @@ export const resolveRef = (ref: T | RefObject) => */ export function useSelectionSync( effect: { selection: PPSelection }, - selection: Object3D | Object3D[] | RefObject | RefObject[], + selection: Object3D | Object3D[] | RefObject | RefObject[], selectionLayer: number ): void { const invalidate = useThree((state) => state.invalidate) @@ -34,9 +34,9 @@ export function useSelectionSync( useEffect(() => { if (api) return - const resolved: Object3D[] = Array.isArray(selection) - ? selection.map((o) => resolveRef(o)) - : [resolveRef(selection)] + const resolved = (Array.isArray(selection) ? selection.map((o) => resolveRef(o)) : [resolveRef(selection)]).filter( + (o): o is Object3D => o != null + ) if (!resolved.length) return effect.selection.set(resolved)