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

WebGPURenderer: Tree-shaking 1/2 #29187

Merged
merged 29 commits into from
Aug 27, 2024
Merged

WebGPURenderer: Tree-shaking 1/2 #29187

merged 29 commits into from
Aug 27, 2024

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Aug 20, 2024

Related issue: #28328

Description

This PR prepares the architecture for three.webgpu.nodes.js which will be done in another PR, some chaning methods have been removed in favor of tree-shaking and should be imported now.

We have StandardNodeLibrary all the Three.js shader inclusions from Materials to ToneMappings, this static part is only assigned to three.webgpu.js which will not have full support for three-shaking.

This is a big change and may impact some existing projects, but I think it will be the last big change until release.

  • Move NodeMaterials out of Nodes
  • Create NodeLibrary and StandardNodeLibrary support
  • Added support to NodeLibrary
  • Move NodeLoader out of Nodes
  • Create TSLBase.js
  • Revisions 1/2
  • Create TSL.js
  • Revision Nodes
  • Revision Examples
  • Revision NodeLoader
  • Revisions 2/2

Copy link

github-actions bot commented Aug 20, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
685.4 kB (169.7 kB) 685.4 kB (169.7 kB) +0 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
462 kB (111.4 kB) 462 kB (111.4 kB) +0 B

@RenaudRohlinger
Copy link
Collaborator

Related: #29156

@sunag sunag marked this pull request as ready for review August 27, 2024 02:05
@sunag sunag merged commit e13d012 into mrdoob:dev Aug 27, 2024
12 checks passed
@sunag sunag deleted the dev-tsl-tree-shaking branch August 27, 2024 02:12
@sunag sunag changed the title WebGPURenderer: Tree-shaking 1/2 - WIP WebGPURenderer: Tree-shaking 1/2 Aug 27, 2024
@Mugen87 Mugen87 added this to the r168 milestone Aug 27, 2024
@WestLangley
Copy link
Collaborator

some chaining methods have been removed in favor of tree-shaking

Some TSL operators can now be chained and others cannot. IMO, this results in a confusing API.

Users will be annoyed that they can't remember which ones can, and which ones cannot, be chained.

Can we solve this -- even if it hurts tree-shaking somewhat?

@sunag
Copy link
Collaborator Author

sunag commented Aug 27, 2024

Some TSL operators can now be chained and others cannot. IMO, this results in a confusing API.

@WestLangley Can you list the TSL operators you are currently using that are no longer available? This might help me get a second understanding of the issue.

@WestLangley
Copy link
Collaborator

@sunag I rewrote my code to accommodate this PR, but I believe this was the original set.

bloom
sepia
posterize
dof
saturation
vignette

@RenaudRohlinger
Copy link
Collaborator

RenaudRohlinger commented Aug 27, 2024

There’s also more here: #29238.

Also I would say that all of these feel quite niche and case-specific, making them more suitable as third-party nodes.

And if a developer wants to use them as chainabable it is still possible in user-land through addMethodChaining.

@mrdoob
Copy link
Owner

mrdoob commented Aug 28, 2024

@sunag I rewrote my code to accommodate this PR, but I believe this was the original set.

bloom
sepia
posterize
dof
saturation
vignette

Maybe these nodes should be addons instead of being in core? Maybe that'll help to understand what's chainable and whats not.

@sunag
Copy link
Collaborator Author

sunag commented Aug 28, 2024

Maybe these nodes should be addons instead of being in core?

Would we then have two imports? Or would it be something else?
Some nodes that are in the core are not chainable like texture, uniform, vec*, ...

import { texture, uniform } from 'three/tsl';
import { saturation, posterize } from 'three/addons/nodes/TSLAddons.js';

@mrdoob
Copy link
Owner

mrdoob commented Aug 28, 2024

Or something like this...

import { texture, uniform } from 'three/tsl';
import { posterize } from 'three/addons/tsl/display/posterize.js';
import { saturation } from 'three/addons/tsl/display/saturation.js';

@sunag
Copy link
Collaborator Author

sunag commented Aug 29, 2024

I think I need more one day to review some items in this PR 😇

@sunag
Copy link
Collaborator Author

sunag commented Aug 29, 2024

I think I need more one day to review some items in this PR 😇

@mrdoob Done! :)

0b5vr added a commit to pixiv/three-vrm that referenced this pull request Aug 30, 2024
`normalMap` is no longer a node element, we use this as an individual node instead
This should work also in r167
See: mrdoob/three.js#29187
@sunag sunag mentioned this pull request Sep 1, 2024
5 tasks
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.

5 participants