Skip to content

Commit c4e85e9

Browse files
kvvasuukvvasuu
committed
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).
1 parent 9377839 commit c4e85e9

5 files changed

Lines changed: 263 additions & 95 deletions

File tree

‎src/effects/Outline.tsx‎

Lines changed: 5 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
1-
import { useThree } from '@react-three/fiber'
21
import { OutlineEffect } from 'postprocessing'
3-
import { Ref, RefObject, useContext, useEffect, useMemo } from 'react'
2+
import { Ref, RefObject, use, useMemo } from 'react'
43
import { Object3D } from 'three'
54
import { EffectComposerContext } from '../EffectComposer'
6-
import { selectionContext } from '../Selection'
7-
import { resolveRef, useDispose } from '../util'
5+
import { EMPTY_ARRAY, useDispose, useSelectionSync } from '../util'
86

97
type ObjectRef = RefObject<Object3D>
108

@@ -16,7 +14,7 @@ export type OutlineProps = ConstructorParameters<typeof OutlineEffect>[2] &
1614
}>
1715

1816
export function Outline({
19-
selection = [],
17+
selection = EMPTY_ARRAY,
2018
selectionLayer = 10,
2119
blendFunction,
2220
patternTexture,
@@ -32,8 +30,7 @@ export function Outline({
3230
ref,
3331
...props
3432
}: OutlineProps) {
35-
const invalidate = useThree((state) => state.invalidate)
36-
const { scene, camera } = useContext(EffectComposerContext)
33+
const { scene, camera } = use(EffectComposerContext)
3734

3835
const effect = useMemo(
3936
() =>
@@ -70,41 +67,7 @@ export function Outline({
7067
]
7168
)
7269

73-
const api = useContext(selectionContext)
74-
75-
useEffect(() => {
76-
// Do not allow array selection if declarative selection is active
77-
// TODO: array selection should probably be deprecated altogether
78-
if (!api && selection) {
79-
effect.selection.set(
80-
Array.isArray(selection) ? (selection as Object3D[]).map(resolveRef) : [resolveRef(selection) as Object3D]
81-
)
82-
invalidate()
83-
return () => {
84-
effect.selection.clear()
85-
invalidate()
86-
}
87-
}
88-
}, [effect, selection, api, invalidate])
89-
90-
useEffect(() => {
91-
effect.selectionLayer = selectionLayer
92-
invalidate()
93-
}, [effect, invalidate, selectionLayer])
94-
95-
useEffect(() => {
96-
if (api && api.enabled) {
97-
if (api.selected?.length) {
98-
effect.selection.set(api.selected)
99-
invalidate()
100-
return () => {
101-
effect.selection.clear()
102-
invalidate()
103-
}
104-
}
105-
}
106-
}, [api, effect.selection, invalidate])
107-
70+
useSelectionSync(effect, selection, selectionLayer)
10871
useDispose(effect)
10972

11073
return <primitive ref={ref} object={effect} />

‎src/effects/SelectiveBloom.tsx‎

Lines changed: 24 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
import { useThree } from '@react-three/fiber'
22
import type { BloomEffectOptions } from 'postprocessing'
33
import { BlendFunction, SelectiveBloomEffect } from 'postprocessing'
4-
import { Ref, RefObject, useContext, useEffect, useMemo } from 'react'
4+
import { Ref, RefObject, use, useEffect, useMemo } from 'react'
55
import { Object3D } from 'three'
66
import { EffectComposerContext } from '../EffectComposer'
7-
import { selectionContext } from '../Selection'
8-
import { resolveRef, useDispose } from '../util'
7+
import { EMPTY_ARRAY, resolveRef, useDispose, useSelectionSync } from '../util'
98

109
type ObjectRef = RefObject<Object3D>
1110

@@ -23,9 +22,9 @@ const addLight = (light: Object3D, effect: SelectiveBloomEffect) => light.layers
2322
const removeLight = (light: Object3D, effect: SelectiveBloomEffect) => light.layers.disable(effect.selection.layer)
2423

2524
export function SelectiveBloom({
26-
selection = [],
25+
selection = EMPTY_ARRAY,
2726
selectionLayer = 10,
28-
lights = [],
27+
lights = EMPTY_ARRAY,
2928
inverted = false,
3029
ignoreBackground = false,
3130
luminanceThreshold,
@@ -38,14 +37,12 @@ export function SelectiveBloom({
3837
ref,
3938
...props
4039
}: SelectiveBloomProps) {
41-
if (lights.length === 0) {
42-
console.warn('SelectiveBloom requires lights to work.')
43-
}
40+
const { scene, camera } = use(EffectComposerContext)
4441

4542
const invalidate = useThree((state) => state.invalidate)
46-
const { scene, camera } = useContext(EffectComposerContext)
43+
4744
const effect = useMemo(() => {
48-
const effect = new SelectiveBloomEffect(scene, camera, {
45+
const instance = new SelectiveBloomEffect(scene, camera, {
4946
blendFunction: BlendFunction.ADD,
5047
luminanceThreshold,
5148
luminanceSmoothing,
@@ -56,9 +53,11 @@ export function SelectiveBloom({
5653
mipmapBlur,
5754
...props,
5855
})
59-
effect.inverted = inverted
60-
effect.ignoreBackground = ignoreBackground
61-
return effect
56+
instance.inverted = inverted
57+
instance.ignoreBackground = ignoreBackground
58+
return instance
59+
// NOTE: `props` is an unstable reference, so we can't memoize it
60+
// eslint-disable-next-line react-hooks/exhaustive-deps
6261
}, [
6362
scene,
6463
camera,
@@ -71,55 +70,30 @@ export function SelectiveBloom({
7170
mipmapBlur,
7271
inverted,
7372
ignoreBackground,
74-
props,
7573
])
7674

77-
const api = useContext(selectionContext)
75+
// Must run before the lights effect below: addLight/removeLight read
76+
// effect.selection.layer live, so it needs to already reflect the
77+
// latest selectionLayer by the time lights get (re-)assigned to it.
78+
useSelectionSync(effect, selection, selectionLayer)
7879

7980
useEffect(() => {
80-
// Do not allow array selection if declarative selection is active
81-
// TODO: array selection should probably be deprecated altogether
82-
if (!api && selection) {
83-
effect.selection.set(
84-
Array.isArray(selection) ? (selection as Object3D[]).map(resolveRef) : [resolveRef(selection) as Object3D]
85-
)
86-
invalidate()
87-
return () => {
88-
effect.selection.clear()
89-
invalidate()
90-
}
81+
if (lights.length === 0) {
82+
console.warn('SelectiveBloom requires lights to work.')
83+
return
9184
}
92-
}, [effect, selection, api, invalidate])
9385

94-
useEffect(() => {
95-
effect.selection.layer = selectionLayer
86+
lights.forEach((light) => addLight(resolveRef(light), effect))
87+
9688
invalidate()
97-
}, [effect, invalidate, selectionLayer])
9889

99-
useEffect(() => {
100-
if (lights && lights.length > 0) {
101-
lights.forEach((light) => addLight(resolveRef(light), effect))
90+
return () => {
91+
lights.forEach((light) => removeLight(resolveRef(light), effect))
92+
10293
invalidate()
103-
return () => {
104-
lights.forEach((light) => removeLight(resolveRef(light), effect))
105-
invalidate()
106-
}
10794
}
10895
}, [effect, invalidate, lights, selectionLayer])
10996

110-
useEffect(() => {
111-
if (api && api.enabled) {
112-
if (api.selected?.length) {
113-
effect.selection.set(api.selected)
114-
invalidate()
115-
return () => {
116-
effect.selection.clear()
117-
invalidate()
118-
}
119-
}
120-
}
121-
}, [api, effect.selection, invalidate])
122-
12397
useDispose(effect)
12498

12599
return <primitive ref={ref} object={effect} />

‎src/tests/Outline.test.tsx‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
import { EffectComposer as EffectComposerImpl, OutlineEffect, Selection as PPSelection } from 'postprocessing'
2+
import * as React from 'react'
3+
import { Mesh } from 'three'
4+
import { afterEach, describe, expect, it, vi } from 'vitest'
5+
import { EffectComposer } from '../EffectComposer'
6+
import { Outline } from '../effects/Outline'
7+
import { Select, Selection } from '../Selection'
8+
import { flush, root, waitForComposer } from './test-utils'
9+
10+
afterEach(async () => {
11+
await React.act(async () => {
12+
root.render(null)
13+
})
14+
})
15+
16+
describe('Outline', () => {
17+
it('does not re-set its (empty, declarative-mode) selection on unrelated re-renders', async () => {
18+
const setSpy = vi.spyOn(PPSelection.prototype, 'set')
19+
const composerRef = React.createRef<EffectComposerImpl>()
20+
21+
const render = (tick: number) =>
22+
root.render(
23+
<EffectComposer ref={composerRef}>
24+
<Outline />
25+
<group name={`tick-${tick}`} />
26+
</EffectComposer>
27+
)
28+
29+
await React.act(async () => render(0))
30+
await waitForComposer(composerRef)
31+
await flush()
32+
setSpy.mockClear()
33+
34+
for (let t = 1; t <= 5; t++) {
35+
await React.act(async () => render(t))
36+
await flush()
37+
}
38+
39+
expect(setSpy).not.toHaveBeenCalled()
40+
setSpy.mockRestore()
41+
})
42+
43+
it('sets its selection from the Selection/Select API and clears it when the object deselects', async () => {
44+
const effectRef = React.createRef<OutlineEffect>()
45+
const meshRef = React.createRef<Mesh>()
46+
47+
const render = (enabled: boolean) =>
48+
root.render(
49+
<EffectComposer>
50+
<Selection>
51+
<Select enabled={enabled}>
52+
<mesh ref={meshRef}>
53+
<boxGeometry />
54+
<meshBasicMaterial />
55+
</mesh>
56+
</Select>
57+
<Outline ref={effectRef} />
58+
</Selection>
59+
</EffectComposer>
60+
)
61+
62+
await React.act(async () => render(true))
63+
await flush()
64+
await flush()
65+
66+
expect(Array.from(effectRef.current!.selection)).toContain(meshRef.current)
67+
68+
await React.act(async () => render(false))
69+
await flush()
70+
await flush()
71+
72+
expect(Array.from(effectRef.current!.selection)).not.toContain(meshRef.current)
73+
})
74+
})

‎src/tests/SelectiveBloom.test.tsx‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
import { EffectComposer as EffectComposerImpl, SelectiveBloomEffect } from 'postprocessing'
2+
import * as React from 'react'
3+
import { Mesh, PointLight } from 'three'
4+
import { afterEach, describe, expect, it } from 'vitest'
5+
import { EffectComposer } from '../EffectComposer'
6+
import { SelectiveBloom } from '../effects/SelectiveBloom'
7+
import { Select, Selection } from '../Selection'
8+
import { flush, root, waitForComposer } from './test-utils'
9+
10+
afterEach(async () => {
11+
await React.act(async () => {
12+
root.render(null)
13+
})
14+
})
15+
16+
describe('SelectiveBloom', () => {
17+
it('does not reconstruct the effect (with its GPU resources) on unrelated re-renders', async () => {
18+
const composerRef = React.createRef<EffectComposerImpl>()
19+
const effectRef = React.createRef<SelectiveBloomEffect>()
20+
const light = new PointLight()
21+
22+
const render = (tick: number) =>
23+
root.render(
24+
<EffectComposer ref={composerRef}>
25+
<SelectiveBloom ref={effectRef} lights={[light]} intensity={2} />
26+
<group name={`tick-${tick}`} />
27+
</EffectComposer>
28+
)
29+
30+
await React.act(async () => render(0))
31+
await waitForComposer(composerRef)
32+
await flush()
33+
const first = effectRef.current
34+
expect(first).toBeTruthy()
35+
36+
for (let t = 1; t <= 5; t++) {
37+
await React.act(async () => render(t))
38+
await flush()
39+
}
40+
41+
expect(effectRef.current).toBe(first)
42+
})
43+
44+
it('sets its selection from the Selection/Select API and clears it when the object deselects', async () => {
45+
const effectRef = React.createRef<SelectiveBloomEffect>()
46+
const meshRef = React.createRef<Mesh>()
47+
const light = new PointLight()
48+
49+
const render = (enabled: boolean) =>
50+
root.render(
51+
<EffectComposer>
52+
<Selection>
53+
<Select enabled={enabled}>
54+
<mesh ref={meshRef}>
55+
<boxGeometry />
56+
<meshBasicMaterial />
57+
</mesh>
58+
</Select>
59+
<SelectiveBloom ref={effectRef} lights={[light]} />
60+
</Selection>
61+
</EffectComposer>
62+
)
63+
64+
await React.act(async () => render(true))
65+
await flush()
66+
await flush()
67+
68+
expect(Array.from(effectRef.current!.selection)).toContain(meshRef.current)
69+
70+
await React.act(async () => render(false))
71+
await flush()
72+
await flush()
73+
74+
expect(Array.from(effectRef.current!.selection)).not.toContain(meshRef.current)
75+
})
76+
77+
it('moves lights to the new render layer when selectionLayer changes', async () => {
78+
const composerRef = React.createRef<EffectComposerImpl>()
79+
const effectRef = React.createRef<SelectiveBloomEffect>()
80+
const light = new PointLight()
81+
const onLayer = (n: number) => light.layers.test({ mask: 1 << n } as never)
82+
83+
const render = (layer: number) =>
84+
root.render(
85+
<EffectComposer ref={composerRef}>
86+
<SelectiveBloom ref={effectRef} lights={[light]} selectionLayer={layer} />
87+
</EffectComposer>
88+
)
89+
90+
await React.act(async () => render(10))
91+
await waitForComposer(composerRef)
92+
await flush()
93+
await flush()
94+
expect(onLayer(10)).toBe(true)
95+
96+
await React.act(async () => render(15))
97+
await flush()
98+
await flush()
99+
100+
expect(onLayer(15)).toBe(true)
101+
expect(onLayer(10)).toBe(false)
102+
})
103+
})

0 commit comments

Comments
 (0)