Skip to content

Commit 30e568e

Browse files
committed
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.
1 parent c30741b commit 30e568e

5 files changed

Lines changed: 88 additions & 31 deletions

File tree

src/effects/Outline.tsx

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import { Object3D } from 'three'
44
import { EffectComposerContext } from '../EffectComposer'
55
import { EMPTY_ARRAY, useDispose, useSelectionSync } from '../util'
66

7-
type ObjectRef = RefObject<Object3D>
7+
type ObjectRef = RefObject<Object3D | null>
88

99
export type OutlineProps = ConstructorParameters<typeof OutlineEffect>[2] &
1010
Partial<{
@@ -18,17 +18,21 @@ export function Outline({
1818
selectionLayer = 10,
1919
blendFunction,
2020
patternTexture,
21+
patternScale,
2122
edgeStrength,
2223
pulseSpeed,
2324
visibleEdgeColor,
2425
hiddenEdgeColor,
26+
multisampling,
27+
resolutionScale,
28+
resolutionX,
29+
resolutionY,
2530
width,
2631
height,
2732
kernelSize,
2833
blur,
2934
xRay,
3035
ref,
31-
...props
3236
}: OutlineProps) {
3337
const { scene, camera } = use(EffectComposerContext)
3438

@@ -37,33 +41,40 @@ export function Outline({
3741
new OutlineEffect(scene, camera, {
3842
blendFunction,
3943
patternTexture,
44+
patternScale,
4045
edgeStrength,
4146
pulseSpeed,
4247
visibleEdgeColor,
4348
hiddenEdgeColor,
49+
multisampling,
50+
resolutionScale,
51+
resolutionX,
52+
resolutionY,
4453
width,
4554
height,
4655
kernelSize,
4756
blur,
4857
xRay,
49-
...props,
5058
}),
51-
// NOTE: `props` is an unstable reference, so we can't memoize it
52-
// eslint-disable-next-line react-hooks/exhaustive-deps
5359
[
5460
blendFunction,
55-
blur,
56-
camera,
57-
edgeStrength,
58-
height,
59-
hiddenEdgeColor,
60-
kernelSize,
6161
patternTexture,
62+
patternScale,
63+
edgeStrength,
6264
pulseSpeed,
63-
scene,
6465
visibleEdgeColor,
66+
hiddenEdgeColor,
67+
multisampling,
68+
resolutionScale,
69+
resolutionX,
70+
resolutionY,
6571
width,
72+
height,
73+
kernelSize,
74+
blur,
6675
xRay,
76+
camera,
77+
scene,
6778
]
6879
)
6980

src/effects/SelectiveBloom.tsx

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { Object3D } from 'three'
66
import { EffectComposerContext } from '../EffectComposer'
77
import { EMPTY_ARRAY, resolveRef, useDispose, useSelectionSync } from '../util'
88

9-
type ObjectRef = RefObject<Object3D>
9+
type ObjectRef = RefObject<Object3D | null>
1010

1111
export type SelectiveBloomProps = BloomEffectOptions &
1212
Partial<{
@@ -29,13 +29,17 @@ export function SelectiveBloom({
2929
ignoreBackground = false,
3030
luminanceThreshold,
3131
luminanceSmoothing,
32+
mipmapBlur,
3233
intensity,
34+
radius,
35+
levels,
36+
kernelSize,
37+
resolutionScale,
3338
width,
3439
height,
35-
kernelSize,
36-
mipmapBlur,
40+
resolutionX,
41+
resolutionY,
3742
ref,
38-
...props
3943
}: SelectiveBloomProps) {
4044
const { scene, camera } = use(EffectComposerContext)
4145

@@ -46,28 +50,35 @@ export function SelectiveBloom({
4650
blendFunction: BlendFunction.ADD,
4751
luminanceThreshold,
4852
luminanceSmoothing,
53+
mipmapBlur,
4954
intensity,
55+
radius,
56+
levels,
57+
kernelSize,
58+
resolutionScale,
5059
width,
5160
height,
52-
kernelSize,
53-
mipmapBlur,
54-
...props,
61+
resolutionX,
62+
resolutionY,
5563
})
5664
instance.inverted = inverted
5765
instance.ignoreBackground = ignoreBackground
5866
return instance
59-
// NOTE: `props` is an unstable reference, so we can't memoize it
60-
// eslint-disable-next-line react-hooks/exhaustive-deps
6167
}, [
6268
scene,
6369
camera,
6470
luminanceThreshold,
6571
luminanceSmoothing,
72+
mipmapBlur,
6673
intensity,
74+
radius,
75+
levels,
76+
kernelSize,
77+
resolutionScale,
6778
width,
6879
height,
69-
kernelSize,
70-
mipmapBlur,
80+
resolutionX,
81+
resolutionY,
7182
inverted,
7283
ignoreBackground,
7384
])
@@ -83,12 +94,17 @@ export function SelectiveBloom({
8394
return
8495
}
8596

86-
lights.forEach((light) => addLight(resolveRef(light), effect))
97+
// Refs may not have attached yet - resolve and drop nullish entries
98+
// rather than crashing addLight/removeLight on a null object.
99+
const resolvedLights = lights.map((light) => resolveRef(light)).filter((light): light is Object3D => light != null)
100+
if (resolvedLights.length === 0) return
101+
102+
resolvedLights.forEach((light) => addLight(light, effect))
87103

88104
invalidate()
89105

90106
return () => {
91-
lights.forEach((light) => removeLight(resolveRef(light), effect))
107+
resolvedLights.forEach((light) => removeLight(light, effect))
92108

93109
invalidate()
94110
}

src/tests/Outline.test.tsx

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { EffectComposer as EffectComposerImpl, OutlineEffect, Selection as PPSelection } from 'postprocessing'
22
import * as React from 'react'
3-
import { Mesh } from 'three'
3+
import { Mesh, Object3D } from 'three'
44
import { afterEach, describe, expect, it, vi } from 'vitest'
55
import { EffectComposer } from '../EffectComposer'
66
import { Outline } from '../effects/Outline'
@@ -71,4 +71,19 @@ describe('Outline', () => {
7171

7272
expect(Array.from(effectRef.current!.selection)).not.toContain(meshRef.current)
7373
})
74+
75+
it('does not throw when a selection ref has not attached yet', async () => {
76+
const composerRef = React.createRef<EffectComposerImpl>()
77+
const unattachedRef = React.createRef<Object3D>()
78+
79+
await React.act(async () =>
80+
root.render(
81+
<EffectComposer ref={composerRef}>
82+
<Outline selection={[unattachedRef]} />
83+
</EffectComposer>
84+
)
85+
)
86+
await waitForComposer(composerRef)
87+
await expect(flush()).resolves.not.toThrow()
88+
})
7489
})

src/tests/SelectiveBloom.test.tsx

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { EffectComposer as EffectComposerImpl, SelectiveBloomEffect } from 'postprocessing'
22
import * as React from 'react'
3-
import { Mesh, PointLight } from 'three'
3+
import { Mesh, Object3D, PointLight } from 'three'
44
import { afterEach, describe, expect, it } from 'vitest'
55
import { EffectComposer } from '../EffectComposer'
66
import { SelectiveBloom } from '../effects/SelectiveBloom'
@@ -100,4 +100,19 @@ describe('SelectiveBloom', () => {
100100
expect(onLayer(15)).toBe(true)
101101
expect(onLayer(10)).toBe(false)
102102
})
103+
104+
it('does not throw when a lights ref has not attached yet', async () => {
105+
const composerRef = React.createRef<EffectComposerImpl>()
106+
const unattachedRef = React.createRef<Object3D>()
107+
108+
await React.act(async () =>
109+
root.render(
110+
<EffectComposer ref={composerRef}>
111+
<SelectiveBloom lights={[unattachedRef]} />
112+
</EffectComposer>
113+
)
114+
)
115+
await waitForComposer(composerRef)
116+
await expect(flush()).resolves.not.toThrow()
117+
})
103118
})

src/util.tsx

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ export const resolveRef = <T,>(ref: T | RefObject<T>) =>
2121
*/
2222
export function useSelectionSync(
2323
effect: { selection: PPSelection },
24-
selection: Object3D | Object3D[] | RefObject<Object3D> | RefObject<Object3D>[],
24+
selection: Object3D | Object3D[] | RefObject<Object3D | null> | RefObject<Object3D | null>[],
2525
selectionLayer: number
2626
): void {
2727
const invalidate = useThree((state) => state.invalidate)
@@ -34,9 +34,9 @@ export function useSelectionSync(
3434

3535
useEffect(() => {
3636
if (api) return
37-
const resolved: Object3D[] = Array.isArray(selection)
38-
? selection.map((o) => resolveRef(o))
39-
: [resolveRef(selection)]
37+
const resolved = (Array.isArray(selection) ? selection.map((o) => resolveRef(o)) : [resolveRef(selection)]).filter(
38+
(o): o is Object3D => o != null
39+
)
4040
if (!resolved.length) return
4141

4242
effect.selection.set(resolved)

0 commit comments

Comments
 (0)