Skip to content

Dispose geometry and material in SparkRenderer.dispose() - #410

Open
ArisLiWind wants to merge 2 commits into
sparkjsdev:mainfrom
ArisLiWind:codex/spark-dispose-geometry
Open

Dispose geometry and material in SparkRenderer.dispose()#410
ArisLiWind wants to merge 2 commits into
sparkjsdev:mainfrom
ArisLiWind:codex/spark-dispose-geometry

Conversation

@ArisLiWind

Copy link
Copy Markdown

Summary

Fixes a GPU resource leak in SparkRenderer.dispose() by releasing the renderer-owned SplatGeometry created in the constructor.

Root cause

SparkRenderer constructs its own SplatGeometry:

const geometry = new SplatGeometry();
super(geometry, material);

That geometry is owned by the renderer instance, but dispose() previously released render targets, textures, workers, accumulators, and pager state without disposing the geometry itself.

Because SplatGeometry allocates both a position attribute and an index buffer, each create/dispose lifecycle could leave two WebGL buffers behind.

Fix

Store the constructor-created geometry as renderer-owned state and dispose it during SparkRenderer.dispose().

Regression coverage

Added tests to verify that:

  • SparkRenderer.dispose() releases its internal geometry
  • disposing a renderer does not dispose geometry assigned from outside
  • dispose() is safe to call multiple times

Validation

  • npm test
  • npm run lint -- --reporter=summary

@mrxz

mrxz commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Store the constructor-created geometry as renderer-owned state and dispose it during SparkRenderer.dispose().

There is no need to specifically keep track of the constructor-created geometry. Replacing the geometry of the SparkRenderer is not supported. You can simplify the dispose logic to just this.geometry.dispose(). For completion sake could you also dispose the material?

Also no need to include the unit tests in this PR. While ultimately we should have extensive tests covering things like this, the required bootstrapping isn't ideal.

@asundqui

asundqui commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@mrxz this will already be covered by #413 right? If so should we close this PR?

@oscarlorentzon

Copy link
Copy Markdown
Collaborator

This is independent from #413 which disposes the Object3D, and as @mrxz mentioned we should remove the tests for now and simplify this to:

this.geometry.dispose();
this.material.dispose();

@asundqui

asundqui commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

I see, got it! @ArisLiWind how would you like to make these changes so we can merge the PR? We can also take over this PR and apply these finishing touches.

@ArisLiWind

Copy link
Copy Markdown
Author

Thanks! Feel free to take over the PR and apply the remaining changes needed for the merge.@asundqui

@oscarlorentzon
oscarlorentzon force-pushed the codex/spark-dispose-geometry branch from 2eb7ee0 to 6030f83 Compare September 9, 2026 22:39
@oscarlorentzon oscarlorentzon changed the title fix: dispose renderer-owned geometry in SparkRenderer Dispose geometry and material in SparkRenderer.dispose() Sep 9, 2026
@oscarlorentzon

Copy link
Copy Markdown
Collaborator

Took this over as discussed, thanks @ArisLiWind.

Simplified according to the discussion above and rebased on main to resolve the conflict with #413.

@oscarlorentzon
oscarlorentzon marked this pull request as ready for review September 9, 2026 22:42
@oscarlorentzon

Copy link
Copy Markdown
Collaborator

Fixes #408

@oscarlorentzon

Copy link
Copy Markdown
Collaborator

@dmarcos This should be ready to merge.

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.

4 participants