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

update build setup to the simplest migration to WebGPU #29827

Closed
wants to merge 1 commit into from

Conversation

trusktr
Copy link
Contributor

@trusktr trusktr commented Nov 6, 2024

Related issue:

Description

Updates the build setup

  • export both WebGL and WebGPU renderers from 'three' module so that existing projects can update without needing to configure their existing builds
  • provide separate optional WebGL-only and WebGPU-only entry points (and builds) by adding a new Three.WebGL.js entry point (and build) that allows people who really need the WebGL-only entry point (or build) to choose that option, while not requiring everyone else to do more work
  • expose the WebGPU.Nodes build in package.json exports for consistency with other builds
  • rename /src/renderers/extras/PMREMGenerator to PMREMGenerator2 to avoid confusion with src/extras/PMREMGenerator (it also caused a build error when trying to export two items of the same name)
  • ensure that all entry points have the same lines, either commented or uncommented, for sanity checking how each entry point differs (some lines were missing in one or the other, making it less easy to tell the difference)
  • I left the three/tsl path alone, which is redundant, but it could easily be deleted.

The new mental model for anyone importing would be

  • three for the whole lib (same as before),
  • or three/* for slimmer versions.

In my opinion this is a consistent way to do it, besides also being easier to consume.

This solution makes migration to WebGPU the simplest because it requires no build configuration updates in downstream projects. All they need to do is update the three version number, and then import new WebGPURenderer and related APIs (unless there's something I overlooked, if so please let me know).

This contribution is funded by my time :)

Copy link

github-actions bot commented Nov 6, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 692.22
171.45
1.15
298.44
+462.07 kB
+126.99 kB
WebGPU 823.66
222.44
823.66
222.44
+1 B
+1 B
WebGPU Nodes 822.78
222.25
822.78
222.25
+1 B
+2 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 464.58
112.26
553.78
137.03
+89.2 kB
+24.76 kB
WebGPU 543.88
147.3
543.88
147.3
+0 B
+0 B
WebGPU Nodes 499.82
137.03
499.82
137.03
+0 B
+0 B

@trusktr trusktr force-pushed the export-WebGPURenderer-from-three branch from e49ce84 to b4266d8 Compare November 6, 2024 21:47
@trusktr
Copy link
Contributor Author

trusktr commented Nov 6, 2024

  • I think the bundle size analyzer setup needs an update now too. But better to see if this idea is worth it first.

@LukeWood
Copy link

LukeWood commented Nov 6, 2024

Just one data point but I like this a lot. It would make my life easier.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 6, 2024

I think the difficulty in this approach is going to be that top-level await (TLA) is not widely supported across environments and build tools. We use TLA for the WebGPU builds (and can afford to use newer syntax like TLA there), but with this change the 'stable' three imports will contain TLA too, and this may become a wider breaking change for users who have not otherwise opted-in to the experimental WebGPU support.

While I haven't done the deep dive on which exact environments and build tools will break on TLA (this would be welcome information!), my feeling is that #29404 will be the safer path, and should not require build configuration changes in any modern build setup (with support for package.json#exports).

There is also the consideration that the 'default' bundle will be much larger for CDN-based applications, containing both WebGL and WebGPU builds, and they'd want to switch to backend-specific imports instead. But I'm not so worried about that part.

@trusktr
Copy link
Contributor Author

trusktr commented Nov 7, 2024

@donmccurdy Can you please provide a simple reproduction?

I posted the following test repo that proves there are no TLAs in current Threejs source code:

https://github.com/trusktr/esbuild-three-test/tree/1eb16cb2a8775937f83cfe75454f868d2c8eff3c

Specifically, the comment on this line explains:

https://github.com/trusktr/esbuild-three-test/blob/1eb16cb2a8775937f83cfe75454f868d2c8eff3c/src/index.jsx#L3

@trusktr trusktr force-pushed the export-WebGPURenderer-from-three branch from b4266d8 to b5af57d Compare November 7, 2024 01:22
@trusktr
Copy link
Contributor Author

trusktr commented Nov 7, 2024

The difference between Three.WebGPU and Three.WebGPU.Nodes is these two lines:

this.library = new StandardNodeLibrary();

and

this.library = new BasicNodeLibrary();

To make this tree shakeable by any build tool (not only Three's build setup, and even if importing both WebGPURenderer and BasicNodeLibrary from three), the value would need to start as not defined like this,

 this.library = null;

and the user would be required to choose which library to use:

import {WebGPURenderer} from 'three'
import {WhateverNodeLibrary} from 'anywhere'

const renderer = new WebGPURenderer()
renderer.library = WhateverNodeLibrary
// or
const renderer = new WebGPURenderer({library: WhateverNodeLibrary})

With this approach, three could include everything (and still be fully tree shakeable regarding NodeLibs), while paths like three/webgpu, three/webgpu.std, three/webgpu.basic, or similar, would provide optional slimmed bundles.

- export both WebGL and WebGPU renderers from 'three' module so that existing projects can update without needing to configure their existing builds
- provide separate optional WebGL-only and WebGPU-only entry points and builds
- expose the WebGPU.Nodes builds in package exports for consistency with other builds
- rename /src/renderers/extras/PMREMGenerator to PMREMGenerator2 to avoid confusion with src/extras/PMREMGenerator
- ensure that all entry points have the same lines, but commented or uncommented, for sanity checking
@trusktr trusktr force-pushed the export-WebGPURenderer-from-three branch from b5af57d to 23b905e Compare November 7, 2024 01:30
@trusktr
Copy link
Contributor Author

trusktr commented Nov 7, 2024

my feeling is that #29404 will be the safer path, and should not require build configuration changes in any modern build setup (with support for package.json#exports).

It does require at the very least an alias update for build tools (in projects importing from 'three' everywhere). For custom and bespoke build scripts it can be more complicated (happy to DM more details about this).

@mrdoob
Copy link
Owner

mrdoob commented Nov 7, 2024

Closing in favour of #29404

@mrdoob mrdoob closed this Nov 7, 2024
@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 8, 2024

It does require at the very least an alias update for build tools (in projects importing from 'three' everywhere)

I don't think this seems correct for build tools supporting package.json#exports, which is fair to require for WebGPU. You can mix and match imports, without defining an alias, for the common pieces like THREE.Scene. No build config should be required to do this; importing Scene from three or three/webgpu is fine:

import { Scene } from 'three';
import { Scene as Scene2, WebGPURenderer } from 'three/webgpu';

console.log(Scene === Scene2); // → true

Tested with Vite v5.4, and the three.js build from #29404. Importantly, the test above fails on the current r170 release, without Cody's changes, and prints the troublesome "multiple versions of three.js" warning.


The difference between Three.WebGPU and Three.WebGPU.Nodes is these two lines...

Hm, I do find that a bit confusing. If that's a major difference in tree-shaking size then I wouldn't be opposed to alternatives, but I think that should probably be considered downstream of the decisions about build artifacts.


I posted the following test repo that proves there are no TLAs in current Threejs source...

Ah, thanks! Very possibly this is not a problem any more, I had missed #29218.


I do vote to continue with #29404, as @mrdoob suggests, I think that approach balances various concerns.

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