Simplify Autofocus's dispose handling, drop idempotency guard - #368
Open
kvvasuu wants to merge 6 commits into
Open
Simplify Autofocus's dispose handling, drop idempotency guard#368kvvasuu wants to merge 6 commits into
kvvasuu wants to merge 6 commits into
Conversation
Passes are now derived from the r3f scene graph and only rebuilt when the resolved node list actually changes, not on every render. Fixes real GPU-resource bugs found along the way: composer-level prop changes (multisampling etc.) could dispose effects still in use by the new composer, discarded EffectPass wrappers leaked their own material and kept a stale change listener on the effect they wrapped, and a user's own EffectPass rendered as a child could be mistaken for one we generated.
Covers every effect whose postprocessing class constructs with zero arguments (Bloom, Noise, Vignette, FXAA, and ~20 others) - live props update the existing instance instead of reconstructing on every change, construction-only options move to explicit args. Also fixes a few bugs these effects had on top of the migration: opacity typing on nine of them, ChromaticAberration's radialModulation/modulationOffset incorrectly required, ColorDepth's bits not resetting on removal.
Outline, SelectiveBloom, ShockWave, GodRays, DepthOfField, SSAO, LUT, and N8AO all need real constructor args (scene/camera/etc.), so they stay hand-built with useMemo, but now apply live props through useLiveDefaults instead of reconstructing on every change. This is where nearly every real runtime bug from review surfaced: a first-apply bug where a still-correct value's setter fired anyway (Outline's multisampling disposing its render target before first use - the actual reason several of these didn't render at all), SSAO's color/fade/minRadiusScale/world* thresholds not resetting on removal, DepthOfField's depthTexture reconstructing instead of using the live setDepthTexture, and GodRays/N8AO not invalidating on live changes under frameloop="demand".
makeDisposeIdempotent guarded against depthPickingPass/copyPass getting disposed twice (once by the composer's own teardown, once by Autofocus's own cleanup) - unnecessary, since postprocessing/three dispose() is confirmed idempotent (event-fire or shallow property disposal, no internal state).
makeDisposeIdempotent guarded against depthPickingPass/copyPass getting disposed twice (once by the composer's own teardown, once by Autofocus's own cleanup) - unnecessary, since postprocessing/three dispose() is confirmed idempotent (event-fire or shallow property disposal, no internal state).
…rocessing into pr5/autofocus
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.
Summary
Fifth and last PR in the stack (base:
pr4/hand-rolled-effects). Small -AutofocuswrapsDepthOfField(PR4) and otherwise needed no changes for the prop-handling redesign itself.makeDisposeIdempotent, a wrapper wired arounddepthPickingPass/copyPassto guard againstdispose()being called twice (once by the composer's own teardown, since they're registered viacomposer.addPass, once by Autofocus's own cleanup effect). Confirmed unnecessary:postprocessing'sPass/Effect.dispose()andthree'sMaterial/Texture/WebGLRenderTarget.dispose()are all either pure event-firing or shallow property disposal with no internal state - none of them guard against being called more than once, so ours didn't need to either.