Skip to content

Tree shaking improvements - #417

Open
mrxz wants to merge 4 commits into
sparkjsdev:mainfrom
mrxz:tree-shaking-improvements
Open

Tree shaking improvements#417
mrxz wants to merge 4 commits into
sparkjsdev:mainfrom
mrxz:tree-shaking-improvements

Conversation

@mrxz

@mrxz mrxz commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

This PR improves the tree-shakability of Spark by focussing on classes that users might not need or use. See #321

  • Ensures the SplatSkinning class is only included when used, including its shader chunks.
  • Ensures SplatEdit and SplatEditSdf are tree-shaken, unless imported.
    • This is done by adopting the three.js approach of having isType = true properties on the classes for type discrimination. Since these can't easily be used in TypeScript, however, helper type guard functions are introduced (isSplatEdit and isSplatEditSdf).
    • NOTE: The SplatEdits class is still included at all times as SplatMesh is capable of constructing this: SplatMesh.ts#975
  • Ensures SparkXr and related code is tree-shaken.
    • There was a lot of duplication between SparkXr.ts and hands.ts, but now the former imports from the latter
    • Annoyingly a simple .length prevented tree-shaking, so resorted to a reduce function which can be statically analyzed for elimination (alternatively an IIFE could be used for this as well)

Comment thread src/hands.ts
t2: 0.0115,
t0: 0.015,
t1: 0.012,
t2: 0.0105,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Curious about these changes? Do they match another part of the code or a previous source? I forget but I think I may have edited them from the original data I used. Don't feel strongly either way but just curious!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There were two almost identical lists, one in hands.ts and one in SparkXr.ts, the latter seemed more recent so those values are now used. They seemed close enough that either one would probably be fine.

One alternative could be to retrieve the joint radii from the WebXR API (https://www.w3.org/TR/webxr-hand-input-1/#dom-xrframe-filljointradii), as that could in theory align better with the user's actual hands (bar spec mandated anonymization)

@asundqui

asundqui commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

This is great, anything that reduces bundle size and opts-in code sounds good to me! @oscarlorentzon @dmarcos we should merge this yes?

Comment thread src/hands.ts
p3 = "pinky-finger-phalanx-distal",
p4 = "pinky-finger-tip",
}
export const JointEnum = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

JointEnum is exported from index.ts, so this seems to break compatibility for anyone currently using let j: JointEnum. Do we need handle that for minor version releases?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Frankly I doubt people use the JointEnum type, as it's effectively just the WebXR Hand Input Module joint names. The interaction with the WebXR API is handled by the SparkXr/hands.ts code and from the user's code everything is keyed on JointId.

Mentioning the change in the release notes should suffice IMHO.

@oscarlorentzon

Copy link
Copy Markdown
Collaborator

This is great, anything that reduces bundle size and opts-in code sounds good to me! @oscarlorentzon @dmarcos we should merge this yes?

Looks good. It would be good to clarify the potential API compatibility issue before we 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.

3 participants