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

TSL: Removing addMethodChaining from Three.TSL.js #30208

Merged
merged 2 commits into from
Dec 26, 2024

Conversation

linbingquan
Copy link
Contributor

Related PR: #30201

Description

Export addMethodsChaining method, users can be simplify the code.

Copy link

github-actions bot commented Dec 26, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.44
79.06
339.44
79.06
+0 B
+0 B
WebGPU 488.27
135.68
488.27
135.68
+0 B
+0 B
WebGPU Nodes 487.74
135.56
487.74
135.56
+0 B
+0 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 465.3
112.11
465.3
112.11
+0 B
+0 B
WebGPU 558.06
151.21
558.06
151.21
+0 B
+0 B
WebGPU Nodes 514.13
140.95
514.13
140.95
+0 B
+0 B

@sunag
Copy link
Collaborator

sunag commented Dec 26, 2024

I think exporting the method chaining is a flaw. I think we can prevent this issue if the user uses tree-shaking in their code. I would suggest removing the current addMethodChaining method instead of adding a new one.

@sunag sunag added this to the r172 milestone Dec 26, 2024
@sunag sunag changed the title TSL: export addMethodsChaining for Three.TSL.js TSL: Removing addMethodChaining from Three.TSL.js Dec 26, 2024
@sunag sunag merged commit 25763c4 into mrdoob:dev Dec 26, 2024
12 checks passed
@linbingquan
Copy link
Contributor Author

I think exporting the method chaining is a flaw.

@sunag Excuse me, What flaw do you mean, I think it is difficult to add custom method to ShaderNode without addMethodChaining in third-party library.

@sunag
Copy link
Collaborator

sunag commented Dec 27, 2024

If the user uses method chaining for example texture().myCustomBlur() without importing it, this loses the reference in the compilation if tree-shaking is used. But if the user imports and uses the function this will import the code for example:

import { myCustomBlur } from ...

myCustomBlur( texture() );

All other features should be available, method chaining should only be useful in inline combination.

@linbingquan
Copy link
Contributor Author

@sunag Thank you for your reply, I tested a very simple method with vite-typescript and after build and minify, it can works.
Maybe I missed something, if not right, please correct.

// main.ts
import { addMethodChaining, float } from 'three/tsl'

function test(node: any, value: number) {
    console.log('test', node.value, value); // just a simple log
    return node
}

addMethodChaining('test', test)

float(1.0).test(1).toGlobal().test(2)

// without `addMethodChaining`, can't chain the method, maybe the readability not good
test(test(float(1.0), 1).toGlobal(), 2)
// vite-env.d.ts
import { ShaderNodeObject } from 'three/tsl'
import { Node } from 'three/webgpu'

declare module 'three/tsl' {
    type CustomNode = Node & {
        test: (value: number) => ShaderNodeObject<CustomNode>
        toGlobal: () => ShaderNodeObject<CustomNode>
    }
    const float: (...params: unknown[]) => ShaderNodeObject<CustomNode>
}

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.

2 participants