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

feat(layers): ScatterplotLayer uniform buffer #8132

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Sep 23, 2023

For #

Background

  • Start transition to uniform buffers

Change List

  • Collects scatterplot-specific uniforms in a uniform buffer
  • Uses loaders.gl UniformStore class to generate the uniform buffer
  • Set the uniform buffer using model bindings property
  • Types ScatterplotLayer state so that we can benefit from uniformStore.setUniforms() type checking.

@ibgreen ibgreen marked this pull request as ready for review September 23, 2023 17:59
varying vec4 vFillColor;
varying vec4 vLineColor;
varying vec2 unitPosition;
varying float innerUnitRadius;
varying float outerRadiusPixels;

// Needs to be identical to fragment shader uniforms
Copy link
Collaborator

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?

Copy link
Collaborator Author

@ibgreen ibgreen Sep 25, 2023

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...

Copy link
Collaborator

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

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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

@ibgreen ibgreen force-pushed the ib/scatterplot-uniform-buffer branch from c467da8 to 730e9ca Compare September 25, 2023 14:32
@ibgreen ibgreen force-pushed the ib/scatterplot-uniform-buffer branch from 730e9ca to 3bfeadc Compare September 25, 2023 15:05
@ibgreen ibgreen changed the base branch from 9.0-no-legacy to master September 26, 2023 11:08
filled: boolean;
antialiasing: boolean;
billboard: boolean;
radiusUnits: number;
Copy link
Collaborator

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>} = {
Copy link
Collaborator

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?

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.

None yet

4 participants