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

RFC: Move ScatterplotLayer to UBOs #8875

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

Conversation

felixpalmer
Copy link
Collaborator

Follow up to #8782, trying out moving ScatterplotLayer to use UBO.

Change List

  • Add ShaderModule for binding scatterplot uniforms
  • Update shaders
  • Set shaderInputs rather than uniforms in draw()

@felixpalmer felixpalmer changed the title WIP: Move ScatterplotLayer to UBOs RFC: Move ScatterplotLayer to UBOs May 7, 2024
@coveralls
Copy link

Coverage Status

coverage: 89.826% (+0.005%) from 89.821%
when pulling ed9343e on felix/layer-ubo
into ff9246e on master.

lineWidthUnits?: number;
};

export default {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: avoid default exports in new code?


export default {
name: 'scatterplot',
vs: uniformBlock,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something we need to sort now:

  • I get the concern about typing twice and getting errors, but this extra module and file for just uniforms is just too much plumbing in my opinion that fragments code and adds as much inefficiency as it solves

There are other possible approaches we should consider,

  • e.g. autogenerating the blocks from the shader module props.
  • adding a common, or uniforms field to shader modules,
  • etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to agree on a pattern that everyone is on board with, to avoid doing this work twice. Could you elaborate on how you'd expect the auto-generation to work? Are you saying we take the value of uniformTypes and produce the uniformBlock?

@@ -254,8 +256,8 @@ export default class ScatterplotLayer<DataT = any, ExtraPropsT extends {} = {}>
const model = this.state.model!;

model.setUniforms(uniforms);
model.setUniforms({
stroked: stroked ? 1 : 0,
const scatterplot: ScatterplotSettings = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should standardize on Settings vs Props for uniform inputs. I have gravitated towards ...Props in recent shadertools refactorings, but the question could still be considered open.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how close it is to the ScatterplotLayerProps, it could be cleaner to just have the ShaderModule accept those and do the conversion in getUniforms

@@ -6,6 +6,11 @@ import * as FIXTURES from 'deck.gl-test/data';

const SIZE = 1;

function getUniforms(layer: Layer) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: Maybe should be part of the layer test utils at some point?

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

3 participants