Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: WIP fix for memory leak #273

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

davcri
Copy link
Contributor

@davcri davcri commented Mar 28, 2024

WIP fix for #270

The EffectComposer implementation in this PR still leaks memory, so I marked the PR as a draft. I'm sharing this in order to investigate a better solution.

What I tried is to use refs for the composer, normalPass and downsamplePass. I create/destroy instances whenever there is a change.

useEffect(() => {
const webGL2Available = isWebGL2Available()
// Initialize composer
composer.current = new EffectComposerImpl(gl, {
depthBuffer,
stencilBuffer,
multisampling: multisampling > 0 && webGL2Available ? multisampling : 0,
frameBufferType,
})
// Add render pass
composer.current.addPass(new RenderPass(scene, camera))
// Create normal pass
if (enableNormalPass) {
normalPass.current = new NormalPass(scene, camera)
normalPass.current.enabled = false
composer.current.addPass(normalPass.current)
if (resolutionScale !== undefined && webGL2Available) {
downSamplingPass.current = new DepthDownsamplingPass({
normalBuffer: normalPass.current.texture,
resolutionScale,
})
downSamplingPass.current.enabled = false
composer.current.addPass(downSamplingPass.current)
}
}
const passes: Pass[] = []
if (group.current && instance.current && composer.current) {
const children = instance.current.objects as unknown[]
for (let i = 0; i < children.length; i++) {
const child = children[i]
if (child instanceof Effect) {
const effects: Effect[] = [child]
if (!isConvolution(child)) {
let next: unknown = null
while ((next = children[i + 1]) instanceof Effect) {
if (isConvolution(next)) break
effects.push(next)
i++
}
}
const pass = new EffectPass(camera, ...effects)
passes.push(pass)
} else if (child instanceof Pass) {
passes.push(child)
}
}
for (const pass of passes) composer.current?.addPass(pass)
if (normalPass.current) normalPass.current.enabled = true
if (downSamplingPass.current) downSamplingPass.current.enabled = true
}
return () => {
for (const pass of passes) composer.current?.removePass(pass)
if (normalPass.current) normalPass.current.enabled = false
if (downSamplingPass.current) downSamplingPass.current.enabled = false
normalPass.current?.dispose()
downSamplingPass.current?.dispose()
composer.current?.dispose()
composer.current = undefined
normalPass.current = undefined
downSamplingPass.current = undefined
}
}, [
camera,
gl,
depthBuffer,
stencilBuffer,
multisampling,
frameBufferType,
scene,
enableNormalPass,
resolutionScale,
instance,
])

Probably a better approach would be to instantiate only on mount and dispose them on unmount, handling any effect change by updating the composer instance instead of recreating it.

I also added a couple of stories for easier debugging:

  • In "MemoryLeak With Postprocessing" story you can press the c key and notice how the memory usage grows constantly as the camera switches (as reported in Memory leak when switching cameras #270).
  • In "MemoryLeak Without Postprocessing" story I used the same scene but without postprocessing and the memory leak is not reproducible (this suggests that something related to postprocessing is causing the leak)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant