fix: composer leak, effect ordering, and several primitive/wrapEffect bugs - #350
Open
kvvasuu wants to merge 18 commits into
Open
fix: composer leak, effect ordering, and several primitive/wrapEffect bugs#350kvvasuu wants to merge 18 commits into
kvvasuu wants to merge 18 commits into
Conversation
Pulls wrapEffect out of the general util grab-bag into its own module, and lets generated effect components accept ref as a regular prop (React 19) instead of having no ref support at all — previously none of the wrapEffect-based effects (Bloom, Vignette, HueSaturation, etc.) could be given a ref.
…reorder bug EffectComposerImpl was created inside useMemo, which doesn't guarantee its cleanup runs before React discards a memoized value — this leaked the composer (and its WebGL resources) under StrictMode. It's now created and disposed inside a useState/useEffect lifecycle instead. Pass collection walks the real r3f instance tree (group.current.__r3f .children) and rebuilds the pass list from scratch whenever it or the composer changes, so the order always matches current JSX — including through wrapper components — after a reorder or a remount. While testing the reorder case, found a genuine bug in r3f's host config: when React moves (not remounts) an existing child — e.g. a key-preserving reorder of a keyed effect list — insertBefore/ appendChild splice the moved instance into its new slot without detaching it from the old one first, leaving a stale duplicate in Instance.children. Worth reporting upstream. Added a cheap dedupe (keep each object's last occurrence, sorted by that position) that recovers the correct order either way and is a no-op when the bug isn't present.
r3f explicitly never auto-disposes objects rendered via <primitive
object={...}> ("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 <primitive>-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.
Named imports instead of namespace imports, plus LensFlare's wrapEffect import path (moved to its own module in an earlier commit here).
Covers pass registration/composition order — including reorder with identity preserved, and instance recreation via a prop-driven arg change — plus composer StrictMode disposal and wrapEffect's dispose timing. Replaces the old root-level EffectComposer.test.tsx (superseded, predates this suite and used a different mock-root setup).
The args useMemo used JSON.stringify(props) as a change-fingerprint, which throws "Converting circular structure to JSON" whenever a prop holds an object with a back-reference (textures, refs, and other three.js objects commonly do). Two independent reports hit this on ordinary usage (a texture on LensFlare, animating via ref on Scanline). Swapped in a stringify that breaks cycles with a placeholder instead of throwing — it's only ever used as a memo key, not real serialization, so that's sufficient.
Already fixed as of the useVector2 adoption a few commits back — this just locks it in so it can't regress silently.
JSON.stringify calls a value's own toJSON before our replacer ever sees it, so the circular-safe stringify from the last commit still paid for it: THREE.Texture.toJSON() re-encodes the whole image to a base64 data URL, on every single render, just to compute a fingerprint that's immediately discarded. Measured ~39ms for a 512x512 texture — enough to reliably drop a frame, exactly the kind of stutter #334 (LensFlare + a dirt texture, "circular structure... on every resize") would have produced even before the crash. Fingerprinting now walks own properties directly instead of going through JSON.stringify/toJSON, with typed arrays/buffers collapsed to an identity token — Object.keys() on one returns every numeric index, so walking a texture's backing buffer element-by-element measured ~340ms, worse than the toJSON it replaces. Identity is enough: same buffer means unchanged, a new one means recreate. Verified both the toJSON call is gone and identity-based recreate/reuse still behaves correctly.
The old wrapEffect never destructured ref out of props, so it landed in ...props and got included in the args fingerprint. Once mounted, ref.current pointed at the effect instance itself, so every render after the first fingerprinted a different value (null, then the instance) and recreated the instance — a permanent, silent rebuild loop, not just an occasional crash. Confirmed by temporarily reproducing the old (non-destructured) signature: this test fails, not with a thrown exception in this environment, but by asserting the instance never stabilizes across re-renders. Current wrapEffect destructures ref explicitly, so it never reaches the fingerprint.
children was typed as JSX.Element | JSX.Element[], which rejects the extremely common `condition && <Effect/>` idiom (that expression is `false | Element`, and `false` isn't assignable to either half of the union) — forced callers into a cast to render an effect conditionally. The runtime already tolerates arbitrary children gracefully (the tree-walk in EffectComposer just filters for `instanceof Effect || instanceof Pass` and ignores anything else), so the stricter type wasn't buying any actual safety, just rejecting a valid pattern. Pre-existing in master, unrelated to this branch's other changes. Confirmed via an isolated repro that this only typechecks against the new signature and fails against the old one. Note: this project's tsconfig excludes *.test.* from typecheck entirely (vitest transforms via esbuild, which strips types without checking them), so the test added here verifies the *runtime* behavior only — it is not a compile-time regression guard for this specific fix.
EffectComposerImpl.dispose() disposes every pass it holds, including these two added via composer.addPass — if Autofocus unmounts alongside its ancestor EffectComposer, its own cleanup disposed them a second time.
Mounts each effect with minimal props inside a real EffectComposer and checks it doesn't crash and disposes correctly; a coverage test fails CI if a new file under src/effects isn't added to the manifest or the exclusion list.
This was referenced Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A set of independently-verified bugfixes on top of the existing architecture. No changes to the public component API.
Dependencies / build
postprocessingmoved from a direct dependency to a peer dependency (^6.36.0).@react-three/fiberpeer range raised to>=9.5.0threepeer range raised to>=0.182.0.Fixed
EffectComposerImplwas created inuseMemo, which doesn't guarantee its cleanup runs before React discards a memoized value. Now created/disposed viauseState+useEffect. As a side effect, this surfaced a double-dispose inAutofocus(its own cleanup and the composer's new, correct teardown could both dispose the same passes) - guarded against that too.wrapEffectcrashed with "Converting circular structure to JSON" when a prop held an object with a back-reference (textures, or any ref - the old code never destructuredrefout of props). Fixed the crash, and a follow-up perf issue in the same code path: the fix still triggeredTexture.toJSON()(full image -> base64 re-encode) on every render - measured ~39ms for a 512x512 texture, now ~0.2ms.ChromaticAberration's tupleoffsetwasn't coerced to a realVector2, so.set()threw.dispose()wired up across<primitive>-based effects that had none at all in master and leaked on every unmount/prop-driven recreation (ASCII, ColorAverage, ChromaticAberration, Grid, LUT, N8AO, Pixelation, SelectiveBloom, SSAO, Texture, GodRays), and consolidated onto the same StrictMode-safe hook for a few that already disposed on normal unmount but could double-dispose under StrictMode (DepthOfField, Glitch, Outline). Fixes SelectiveBloom allocates huge GPU memory even with empty selection #344EffectComposer'schildrenprop was typed asJSX.Element | JSX.Element[], rejectingcondition && <Effect/>and forcing a cast. Widened toReactNode.wrapEffect's generated prop types resolved to effectivelyanyin master (ConstructorParameters<T>[0]degrades badly for optional/no-arg constructors) - no autocomplete, no compile-time checks.EffectProps<T>now derives real per-effect types from each constructor's actual signature.Tests
EffectComposer, and checks it mounts, unmounts, and disposes without throwing.src/effects/*.tsxat run time and fails if a file isn't in the smoke-test manifest or an explicit, documented exclusion list (2 effects need a real browser -document/image decoding - so they're excluded with a reason rather than silently skipped) - so a newly added effect can't ship without at least this baseline check.Test plan
yarn test- 63 testsyarn typecheck/yarn eslint:ciclean