-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(layers): ScatterplotLayer uniform buffer #8132
base: master
Are you sure you want to change the base?
Conversation
varying vec4 vFillColor; | ||
varying vec4 vLineColor; | ||
varying vec2 unitPosition; | ||
varying float innerUnitRadius; | ||
varying float outerRadiusPixels; | ||
|
||
// Needs to be identical to fragment shader uniforms |
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 it needs to be identical, could you define in one place? Derive it from scatterplot.uniformTypes
?
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, though there are some tradeoffs to it. So far it seems the shader linker complains loudly if they are different so it does not seem to be a subtle source of errors.
I tried breaking the uniforms into a separate string and injecting it. A minor problem with that is that I use the vscode GLSL syntax highlighter to make these glsl
tagged strings readable, and it breaks if I add template string arguments.
I actually added code for generating uniform declarations from uniform types (in the @luma.gl/shadertools
module), but I haven't started using that yet.
The high-level question for the TSC is perhaps: how much shader code should be autogenerated? Once one starts down that dark path...
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.
Do all the uniforms need to be typed out in both the vertex and fragment shader? It seems unnecessary to have to include uniforms in the vertex shader that only the fragment shader reads and vice-versa
I do agree there is a tradeoff between saving time and having shaders that are actually readable in the 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.
Do all the uniforms need to be typed out in both the vertex and fragment shader? It seems unnecessary to have to include uniforms in the vertex shader that only the fragment shader reads and vice-versa
Yes I believe so, though more experience may show otherwise. Also we need to see how this works in both GLSL and WGSL.
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.
Agree with you there. Auto-generation does add a layer of complexity, making it all the more important to have solid debugging tools. I'm all for it, just think we really need to keep an eye on the debugging side of things to make sure everything runs smoothly.
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.
OK, in the spirit of keeping it simple & landing v9 I'm for just typing out the uniforms in full at this stage
c467da8
to
730e9ca
Compare
730e9ca
to
3bfeadc
Compare
filled: boolean; | ||
antialiasing: boolean; | ||
billboard: boolean; | ||
radiusUnits: number; |
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.
More restrictive type? 0 | 1 | 2
lineWidthUnits: number; | ||
}; | ||
|
||
export const scatterplot: {uniformTypes: Record<string, ShaderUniformType>} = { |
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.
It's a little confusing to see this constant referenced in the layer code. Maybe name it ScatterplotUniformLayout
?
0a083b6
to
eb85da3
Compare
For #
Background
Change List
UniformStore
class to generate the uniform bufferuniformStore.setUniforms()
type checking.