-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
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
Codesplit WebGL/WebGPU entrypoints: Fix WebGPU Addons #29644
Codesplit WebGL/WebGPU entrypoints: Fix WebGPU Addons #29644
Conversation
@@ -22,6 +22,7 @@ | |||
"imports": { | |||
"three": "../build/three.webgpu.js", | |||
"three/tsl": "../build/three.webgpu.js", | |||
"three/webgpu": "../build/three.webgpu.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in #29404 I would favor just one import path, not two. Do you mind using three/tsl
for now? Otherwise I would suggest we migrate from three/tsl
to three/webgpu
in all examples (and then ditch three/tsl
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we should replace three/tsl
to three/webgpu
in all examples and completely remove three
from the webgpu examples to align with the new build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After investigating a bit the refactor I would suggest that first we merge this as it is to fix the current build, then we can refactor in a second time most of the webgpu addons that uses three
and three/tsl
internally before being able to remove "three/tsl": "../build/three.webgpu.js",
and dropping "three": "../..."
.
For example:
import { FileLoader, Loader, TextureLoader, RepeatWrapping } from 'three';
import { MeshBasicNodeMaterial, MeshPhysicalNodeMaterial } from 'three/webgpu';
import {
float, bool, int, vec2, vec3, vec4, color, texture,
positionLocal, positionWorld, uv, vertexColor,
normalLocal, normalWorld, tangentLocal, tangentWorld,
add, sub, mul, div, mod, abs, sign, floor, ceil, round, pow, sin, cos, tan,
asin, acos, atan2, sqrt, exp, clamp, min, max, normalize, length, dot, cross, normalMap,
remap, smoothstep, luminance, mx_rgbtohsv, mx_hsvtorgb,
mix, split,
mx_ramplr, mx_ramptb, mx_splitlr, mx_splittb,
mx_fractal_noise_float, mx_noise_float, mx_cell_noise_float, mx_worley_noise_float,
mx_transform_uv,
mx_safepower, mx_contrast,
mx_srgb_texture_to_lin_rec709,
saturation,
timerLocal, frameId
} from 'three/tsl';
in jsm/loaders/MaterialXLoader.js
would need to become:
import { FileLoader, Loader, TextureLoader, RepeatWrapping, MeshBasicNodeMaterial, MeshPhysicalNodeMaterial } from 'three/webgpu';
import {
float, bool, int, vec2, vec3, vec4, color, texture,
positionLocal, positionWorld, uv, vertexColor,
normalLocal, normalWorld, tangentLocal, tangentWorld,
add, sub, mul, div, mod, abs, sign, floor, ceil, round, pow, sin, cos, tan,
asin, acos, atan2, sqrt, exp, clamp, min, max, normalize, length, dot, cross, normalMap,
remap, smoothstep, luminance, mx_rgbtohsv, mx_hsvtorgb,
mix, split,
mx_ramplr, mx_ramptb, mx_splitlr, mx_splittb,
mx_fractal_noise_float, mx_noise_float, mx_cell_noise_float, mx_worley_noise_float,
mx_transform_uv,
mx_safepower, mx_contrast,
mx_srgb_texture_to_lin_rec709,
saturation,
timerLocal, frameId
} from 'three/webgpu';
this involve updating the imports for all the webgpu addons code (mostly in #29295)
/cc @sunag (as I want to make sure if we drop three/tsl
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the way I realized we might need to keep the alias "three": "../build/three.webgpu.js",
in the example for addons such as GLTFLoader which is used by both renderers... @Mugen87
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be nice too:
import { TSL } from './three.webgpu.js';
const { vec2, vec3 } = TSL;
But I think you said that this approach was not tree-shakeable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think you said that this approach was not tree-shakeable?
So I ended up figuring out another way to do this once we have a new file.
If we have a three.tsl.js
file like you suggested but for the alias this will work with three-shaking.
I posted a link to the working example #29644 (comment)
Instead of const { vec2, vec3 } = TSL;
we would use import { vec3 } from './three.tsl.js';
The alias file would look like this:
import { TSL } from './three.webgpu.js';
const vec2 = TSL.vec2;
const vec3 = TSL.vec3;
// ...
export { vec3, vec2 };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we're in alignment then.
I think this approach is the most readable:
import * as THREE from './three.webgpu.js';
import { vec2, vec3 } from './three.tsl.js';
Should we create ./src/Three.TSL.js
or ./src/TSL.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create ./src/Three.TSL.js or ./src/TSL.js?
Whatever you prefer, you usually choose the best names :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha! Ok, lets to ./src/TSL.js
👍
mrdoob/three.js#29644 moves TSL functions into `three/tsl` The users have to modify the import map or bundler config because of this change See: #1548 (comment)
Related PR: #29404
Description
Following #29404, fixes WebGPU Addons resolution with bundlers that depends on WebGPURenderer related code which now bundled under
three/webgpu
instead ofthree
.Related comment: #29404 (comment)
This contribution is funded by Utsubo