-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: line layer and project module port to WGSL, test app for WebGPU line #9509
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
Conversation
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.
Very cool - nice to see how the majority of the changes can be made outside the existing code.
I tried running the example apps but got errors unfortunately. Is this the correct approach?
cd deck.gl
rm -rf node_modules
yarn bootstrap
cd test/apps/webgpu-scatterplot
yarn
yarn start-local
port: 8080 | ||
}, | ||
optimizeDeps: { | ||
esbuildOptions: {target: 'es2020'} |
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.
Is this needed for this PR? Seems like a potentially risky unrelated change if not
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.
At the moment deck.gl async loading (props.data = url) doesn't work, I have to load the data first and doing it using global await is most convenient.
Not making this change would require more work in the PR. I think we could land and revert it if problematic, the revert would only break the webgpu test apps.
// ----------------------------------------------------------------------------- | ||
// Returns an adjustment factor for commonUnitsPerMeter | ||
fn _project_size_at_latitude(lat: f32) -> f32 { |
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.
Why use _underscores in the function 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.
The intention is to clearly mark that these are internal helper functions and not part of the public API of the shader module.
Since the module is injected in full source form into the shader, all of these functions are available to the layer shader.
transform: mat3x3<f32>, | ||
}; | ||
fn project_needs_rotation(commonPosition: vec3<f32>) -> RotationResult { |
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.
Perhaps cleaner to just return a boolean and then call project_get_orientation_matrix()
when needed to avoid the struct and identity matrix creation?
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.
Perhaps cleaner to just return a boolean and then call project_get_orientation_matrix() when needed to avoid the struct and identity matrix creation?
Good idea. I think we need to audit this entire API, there are a bunch of new function names to handle the overloads and they most likely can be named better.
I think that audit can be a separate PR as we port a few more layers and get a good sense for what the best API will look like.
fn project_position_vec4_f64(position: vec4<f32>, position64Low: vec3<f32>) -> vec4<f32> { | ||
var position_world = project.modelMatrix * position; | ||
// Work around for a Mac+NVIDIA bug: |
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.
Interesting question is whether we keep these workarounds? There is a good chance they are no longer needed given that the code paths taken by the drivers will likely be very different
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.
Interesting question is whether we keep these workarounds? There is a good chance they are no longer needed given that the code paths taken by the drivers will likely be very different
Yes good point. Again, I would prefer to remove those in iterative follow up PRs rather than further raise the bar on this "enabler PR"
// Copyright (c) vis.gl contributors | ||
|
||
export const shaderWGSL = /* wgsl */ `\ | ||
// TODO(ibgreen): Hack for Layer uniforms (move to new "color" module?) |
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.
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.
Yes, but I didn't implement the layer module at this stage. One needs to stop descending into the rabbithole at some point.
fs: uniformBlock, | ||
vs: glslUniformBlock, | ||
fs: glslUniformBlock, | ||
source: '', |
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.
Why not declare here like you did with the line-layer?
lineWidthUnits: i32, | ||
}; | ||
struct ConstantAttributeUniforms { |
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.
Not sure what this is doing? Is this some experiment to pass constant values in place of attributes?
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.
Not sure what this is doing? Is this some experiment to pass constant values in place of attributes?
Correct, this is my best idea for how we would handle constant attributes.
@@ -0,0 +1,132 @@ | |||
// deck.gl |
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.
If these test apps are identical to the existing WebGL ones I think it's better to just add an option to the existing apps to run using webgpu (e.g. check for #webgpu
in the URL, or add a toggle switch)
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.
If these test apps are identical to the existing WebGL ones I think it's better to just add an option to the existing apps to run using webgpu (e.g. check for
#webgpu
in the URL, or add a toggle switch)
Yes, it will be very desirable to get to that point. But right now we would at least need to fix the async data loading bug in WebGPU and possible more issues. Again suitable for a fast follow in my opinion.
This PR only contains partial changes intended to be easy to review, approve and land. The full PR is #9432 |
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.
Great observations, really good review.
My assumption was that as long as we don't break or degrade the WebGL code, we can land something imperfect for WebGPU and iterate on it in follow-ups.
@felixpalmer added a note to the WebGPU tracker that we should address your findings here. |
@ibgreen @felixpalmer We tried to update to deck-gl
However, I haven't looked more into it yet. Maybe the issue is on our side. |
@ibgreen could it be the file extension that is breaking some bundlers? |
Fine to change it but probably not, as far as I can tell we are already using similar .glsl.ts extension for glsl files. We could always change to -wgsl instead of .wgsl to be sure. |
@lukasmasuch should be fixed in 9.1.7 |
@felixpalmer Thanks! 9.1.7 resolved the issue. |
Closes #
Background
Change List