Make GSplat scene parameters tree-shakeable - #9328
Conversation
Build size reportThis PR changes the size of the minified bundles.
|
mvaligursky
left a comment
mvaligursky
left a comment
There was a problem hiding this comment.
🤖 Automated PR review — posted on my behalf by Claude Code (Opus 4.8). Not a human review. The points below are suggestions to weigh as possible improvements, not changes that necessarily need to be addressed.
The injection is thorough and the lifecycle holds up. Things I checked and found clean:
- No unguarded consumer left behind. I swept every
scene.gsplatreference insrc/at this head — the only remaining ones are JSDoc text and deprecation strings. All live reads are either guarded with?.or converted to the injectedthis.gsplat. - Teardown ordering is safe.
GSplatComponentSystem.destroy()touchesapp.sceneandapp.renderer, andAppBase.destroyrunssystems.destroy()(1905) well beforerenderer.destroy()(1948) andscene.destroy()(1961), so neither is null yet.Renderer.destroy()also destroys the director, but both paths null the field and use?., so there's no double-destroy in either order. - The tree-shaking claim holds and the strengthened filter (now covering
/scene/gsplat/,/scene/gsplat-unified/,/chunks/gsplat/and/gsplat-chunks-) is a real improvement over the four-file list it replaces. 84 tests pass, lint clean.
One point on the API change worth weighing before this lands, plus two minor notes — all inline.
Minor, and not inline-able since the file isn't in this diff: Renderer.destroy() (renderer.js:293-294) also destroys and nulls the director. That line is now dead in every normal teardown, since systems.destroy() runs first and nulls the field, so the ?. always short-circuits. Harmless, but the director's ownership is now expressed in two places - worth either dropping it (the system is the sole creator, so the sole owner) or noting that it's a backstop for a renderer torn down without its systems, so the next reader knows which is authoritative.
| * @type {GSplatParams} | ||
| * Returns null when the application does not include {@link GSplatComponentSystem}. | ||
| * | ||
| * @type {GSplatParams|null} |
There was a problem hiding this comment.
Worth weighing whether the declared type needs to become nullable, separately from the runtime value.
The migration example in the description shows a read (app.scene.gsplat?.renderer), but the dominant use of this getter is configuration, and optional chaining is a SyntaxError on an assignment target — app.scene.gsplat?.splatBudget = 4e6 does not parse. So a downstream user with strictNullChecks on (the default under strict, and the norm for new TS projects) can't reach for ?. at all; every write has to be restructured into a guard or a non-null assertion. In this repo's own examples that's billions (12 sites), downtown (13), clipping (8), benchmark (5), and so on across roughly twenty gaussian-splatting examples.
And the guard is noise for all of them: an app touching these params has necessarily included GSplatComponentSystem, which is exactly the condition under which the value is never null. So the nullability is unobservable to every legitimate consumer while being mandatory in their types.
An alternative that keeps the tree-shaking win — scene.js still doesn't import GSplatParams, which is the whole mechanism — is to leave the declared type as GSplatParams and put the honesty in the getter instead:
get gsplat() {
Debug.assert(this._gsplatParams, 'Scene#gsplat requires GSplatComponentSystem to be included in the app.');
return this._gsplatParams;
}Zero consumer churn, TS users unaffected, and an app that wrongly reaches for it gets a clear message instead of a Cannot read properties of null. The cost is that the type is optimistic for apps without the system — which have no reason to read it.
If you'd rather keep | null as the honest signature, the description's migration section is worth extending to cover the write case, since that's where essentially all the churn is.
(For what it's worth this won't fail CI as it stands: examples/tsconfig.examples.json sets checkJs: false and the base config doesn't enable strictNullChecks, so the un-updated examples aren't checked. That also means the repo has no signal that its own sample code now models a pattern strict-mode consumers can't copy.)
| // Own the scene-wide parameters here so apps that omit this system also tree-shake the | ||
| // GSplat formats, varyings and shader chunks imported by GSplatParams. | ||
| const gsplatParams = new GSplatParams(app.graphicsDevice); | ||
| app.scene._gsplatParams = gsplatParams; |
There was a problem hiding this comment.
This writes a _-prefixed private of another class from a different module, and Scene exposes no setter for it — so the field name is now part of a cross-module contract while still reading as private. A rename inside Scene would break this silently, and there's nothing at the Scene end pointing at the fact that a component system owns the value.
An @ignored internal setter (or a small _setGsplatParams(params)) would make the coupling declared at both ends and give the ownership comment somewhere to live next to the field.
|
Tree-shaking size comparison using the unchanged Method: bundle from
Combined result from before #9322 to this PR:
Exact parent-to-head minified reductions were 14,287 B in #9322, 13,511 B in #9327, and 30,223 B in this PR. |
1 participant
Move scene-wide GSplat parameters behind
GSplatComponentSystemso applications that omit the system can fully tree-shake the GSplat implementation.Changes:
GSplatComponentSystem.API Changes:
Scene#gsplatchanges fromGSplatParamstoGSplatParams | null.GSplatComponentSystemis included and is otherwisenull.Before:
For applications without
GSplatComponentSystem:Performance:
GSplatComponentSystemno longer retain GSplat formats, varyings, resource implementations, or GLSL/WGSL shader chunks.