Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 2 additions & 20 deletions src/effects/Autofocus.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,6 @@ import { Mesh, Vector3 } from 'three'
import { EffectComposerContext } from '../EffectComposer'
import { DepthOfField } from './DepthOfField'

// EffectComposerImpl.dispose() disposes every pass it currently holds —
// including these two, since they're added via composer.addPass below.
// When Autofocus unmounts alongside its ancestor EffectComposer (e.g. a
// full tree unmount), both the composer's own teardown AND this
// component's cleanup effect would dispose the same instances. Wrapping
// dispose here makes it safe no matter which caller gets there first.
function makeDisposeIdempotent<T extends { dispose: () => void }>(instance: T): T {
let disposed = false
const dispose = instance.dispose.bind(instance)
instance.dispose = () => {
if (disposed) return
disposed = true
dispose()
}
return instance
}

export type AutofocusProps = ComponentProps<typeof DepthOfField> & {
target?: R3FVector3
/** should the target follow the pointer */
Expand Down Expand Up @@ -71,9 +54,8 @@ export function Autofocus({
const pointer = useThree(({ pointer }) => pointer)
const { composer, camera } = useContext(EffectComposerContext)

// see: https://codesandbox.io/s/depthpickingpass-x130hg
const [depthPickingPass] = useState(() => makeDisposeIdempotent(new DepthPickingPass()))
const [copyPass] = useState(() => makeDisposeIdempotent(new CopyPass()))
const [depthPickingPass] = useState(() => new DepthPickingPass())
const [copyPass] = useState(() => new CopyPass())
useEffect(() => {
composer.addPass(depthPickingPass)
composer.addPass(copyPass)
Expand Down
45 changes: 14 additions & 31 deletions src/tests/effects.smoke.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -165,32 +165,15 @@ describe('effect smoke tests', () => {
}
})

// Tracks dispose() calls per instance rather than per class — EffectComposerImpl
// constructs its own internal CopyPass (this.copyPass, for compositing) and
// disposes it as part of its own teardown, unrelated to any CopyPass an effect
// constructs. A class-wide spy would conflate the two into a false "double
// dispose"; this only flags it if the *same* instance is disposed twice.
function trackDisposePerInstance(Ctor: { prototype: { dispose: (...args: unknown[]) => unknown } }) {
const counts = new Map<object, number>()
const original = Ctor.prototype.dispose
const spy = vi.spyOn(Ctor.prototype, 'dispose').mockImplementation(function (this: object, ...args: unknown[]) {
counts.set(this, (counts.get(this) ?? 0) + 1)
return original.apply(this, args)
})
return {
restore: () => spy.mockRestore(),
maxCallsForAnySingleInstance: () => Math.max(0, ...counts.values()),
}
}

// Autofocus's ref resolves to { dofRef, hitpoint, update } (its own
// imperative API), not an effect instance — the generic dispose check
// above silently no-ops for it. It actually owns three disposables
// (depthPickingPass, copyPass, and the DepthOfField effect it renders
// internally), verified explicitly here instead.
it('Autofocus disposes depthPickingPass, copyPass, and the nested DepthOfField effect exactly once each', async () => {
const depthPickingTracker = trackDisposePerInstance(DepthPickingPass)
const copyPassTracker = trackDisposePerInstance(CopyPass)
// Autofocus's ref resolves to { dofRef, hitpoint, update }, not an effect
// instance - the generic dispose check above no-ops for it. It owns three
// disposables (depthPickingPass, copyPass, the nested DepthOfField effect),
// verified here. Both the composer's teardown and Autofocus's own cleanup
// end up disposing depthPickingPass/copyPass - that's fine, dispose() is
// idempotent (just event-firing / shallow property disposal, no state).
it('Autofocus disposes depthPickingPass, copyPass, and the nested DepthOfField effect', async () => {
const depthPickingDisposeSpy = vi.spyOn(DepthPickingPass.prototype, 'dispose')
const copyPassDisposeSpy = vi.spyOn(CopyPass.prototype, 'dispose')
// AutofocusProps' `ref` type is broken (ComponentProps<typeof DepthOfField>
// drags in DepthOfField's own `ref: Ref<DepthOfFieldEffect>`, which then
// intersects with `Ref<AutofocusApi>` — separate pre-existing issue,
Expand All @@ -214,12 +197,12 @@ describe('effect smoke tests', () => {
await React.act(async () => root.render(null))
await flush()

expect(depthPickingTracker.maxCallsForAnySingleInstance()).toBeLessThanOrEqual(1)
expect(copyPassTracker.maxCallsForAnySingleInstance()).toBeLessThanOrEqual(1)
expect(dofDisposeSpy).toHaveBeenCalledTimes(1)
expect(depthPickingDisposeSpy).toHaveBeenCalled()
expect(copyPassDisposeSpy).toHaveBeenCalled()
expect(dofDisposeSpy).toHaveBeenCalled()

depthPickingTracker.restore()
copyPassTracker.restore()
depthPickingDisposeSpy.mockRestore()
copyPassDisposeSpy.mockRestore()
})

it('covers every file in src/effects (or documents why it is excluded)', () => {
Expand Down