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
base: master
Are you sure you want to change the base?
Conversation
lineWidthUnits?: number; | ||
}; | ||
|
||
export default { |
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.
Nit: avoid default exports in new code?
|
||
export default { | ||
name: 'scatterplot', | ||
vs: uniformBlock, |
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 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
, oruniforms
field to shader modules, - etc.
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 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 = { |
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.
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.
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.
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) { |
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.
Thought: Maybe should be part of the layer test utils at some point?
Follow up to #8782, trying out moving
ScatterplotLayer
to use UBO.Change List
ShaderModule
for bindingscatterplot
uniformsshaderInputs
rather thanuniforms
indraw()