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: Avoid vector reuse in luminance() input #29540

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

donmccurdy
Copy link
Collaborator

Since the luminance() function could be called multiple times with different coefficients, and I'm not sure whether the vector is stored internally by vec3(vector), perhaps it's better not to reuse a local variable here.

/cc @sunag does this sound correct?

@donmccurdy donmccurdy requested a review from sunag October 2, 2024 00:42
Copy link

github-actions bot commented Oct 2, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 688.13
170.5
688.13
170.5
+0 B
+0 B
WebGPU 807.26
217.48
807.25
217.46
-9 B
-18 B
WebGPU Nodes 806.77
217.34
806.76
217.32
-9 B
-19 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 462.85
111.78
462.85
111.78
+0 B
+0 B
WebGPU 536.06
144.73
536.06
144.73
+0 B
+0 B
WebGPU Nodes 492.14
134.46
492.14
134.46
+0 B
+0 B

@sunag
Copy link
Collaborator

sunag commented Oct 2, 2024

Yes, sounds correct to me too. I think we can remove the spread syntax (...) too.

@sunag sunag added this to the r170 milestone Oct 2, 2024
@donmccurdy donmccurdy merged commit 612cca4 into mrdoob:dev Oct 2, 2024
12 checks passed
@donmccurdy donmccurdy deleted the fix/coloradjustment-new-vector branch October 2, 2024 20:32
@donmccurdy donmccurdy changed the title Nodes: Avoid vector reuse in luminance() input TSL: Avoid vector reuse in luminance() input Oct 27, 2024
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