Skip to content

Refine GSplatRenderer API: rename methods for clarity#8522

Merged
mvaligursky merged 1 commit intomainfrom
mv-gsplat-renderer-api-rename
Mar 11, 2026
Merged

Refine GSplatRenderer API: rename methods for clarity#8522
mvaligursky merged 1 commit intomainfrom
mv-gsplat-renderer-api-rename

Conversation

@mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Mar 11, 2026

Refine the internal GSplatRenderer API to use clearer, more generic method names that better describe the rendering mode being configured rather than the implementation mechanism.

Changes:

  • Rename configureMaterial() to onWorkBufferFormatChanged() to reflect its generic purpose
  • Merge setIndirectDraw() + updateIndirect() into a single setGpuSortedRendering() method
  • Rename disableIndirectDraw() to setCpuSortedRendering()
  • Rename internal disableIndirectDraw parameter/variable to useCpuSort

Rename configureMaterial() to onWorkBufferFormatChanged(), merge
setIndirectDraw()+updateIndirect() into setGpuSortedRendering(), and
rename disableIndirectDraw() to setCpuSortedRendering().

Made-with: Cursor
@mvaligursky mvaligursky self-assigned this Mar 11, 2026
@mvaligursky mvaligursky added the area: graphics Graphics related issue label Mar 11, 2026
@mvaligursky mvaligursky merged commit e76e37e into main Mar 11, 2026
8 checks passed
@mvaligursky mvaligursky deleted the mv-gsplat-renderer-api-rename branch March 11, 2026 14:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the unified GSplat rendering interface to better distinguish CPU-sorted vs GPU-driven/indirect rendering, and to centralize handling of work-buffer format changes.

Changes:

  • Renames/reshapes renderer lifecycle hooks (configureMaterialonWorkBufferFormatChanged) and rendering-mode APIs (setIndirectDraw/updateIndirect/disableIndirectDrawsetGpuSortedRendering/setCpuSortedRendering).
  • Updates GSplatQuadRenderer to implement the new hooks and consolidate indirect/GPU-driven setup.
  • Updates GSplatManager to call the new renderer APIs during work-buffer rebuilds and sorting/compaction transitions.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/scene/gsplat-unified/gsplat-renderer.js Updates the base renderer API to new hook/mode method names.
src/scene/gsplat-unified/gsplat-quad-renderer.js Implements onWorkBufferFormatChanged and consolidates GPU-driven rendering configuration.
src/scene/gsplat-unified/gsplat-manager.js Switches manager orchestration to the new renderer hook/mode methods.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +454 to +458
// Switch renderer to CPU mode once, before destroying both compaction systems.
const useCpuSort = false;
this.renderer.setCpuSortedRendering();
this.destroyIntervalCompaction(useCpuSort);
this.destroyCompaction(useCpuSort);
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GSplatManager.destroy() calls renderer.destroy() before destroyGpuSorting(), but destroyGpuSorting() now calls renderer.setCpuSortedRendering(). If destroyGpuSorting() runs after the renderer has been destroyed, this will touch destroyed MeshInstance/Material state and can throw. Either call destroyGpuSorting() before renderer.destroy(), or guard/skip the renderer mode switch when the manager is being destroyed (e.g. based on this._destroyed / renderer existence).

Copilot uses AI. Check for mistakes.
Comment on lines +455 to +458
const useCpuSort = false;
this.renderer.setCpuSortedRendering();
this.destroyIntervalCompaction(useCpuSort);
this.destroyCompaction(useCpuSort);
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The local useCpuSort constant is set to false but is named as if it indicates switching to CPU sorting; it's only used to suppress additional mode switches in the helper teardown calls. Consider renaming it to something like switchRendererToCpuSort / alreadySwitchedToCpuSort (or just pass false directly) to avoid confusion with the useCpuSort parameter name in the teardown methods.

Suggested change
const useCpuSort = false;
this.renderer.setCpuSortedRendering();
this.destroyIntervalCompaction(useCpuSort);
this.destroyCompaction(useCpuSort);
this.renderer.setCpuSortedRendering();
this.destroyIntervalCompaction(false);
this.destroyCompaction(false);

Copilot uses AI. Check for mistakes.
return this._material;
}

onWorkBufferFormatChanged() {
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onWorkBufferFormatChanged() triggers a full material reconfigure, but it doesn't update the inherited _workBufferFormatVersion cache to the new workBuffer.format.extraStreamsVersion. After a workBuffer swap, this makes _syncWithWorkBufferFormat() think the format changed again and perform another sync/update pass on the next frame. Consider updating _workBufferFormatVersion here (or calling super.onWorkBufferFormatChanged() if the base is updated to do it) to keep the cache consistent and avoid redundant work.

Suggested change
onWorkBufferFormatChanged() {
onWorkBufferFormatChanged() {
super.onWorkBufferFormatChanged();

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: graphics Graphics related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants