Remove withParametersWebGL method in Pass classes #8636

felixpalmer · 2024-03-12T11:26:48Z

For #8582 requires visgl/luma.gl#2046

Blending via layer parameters and Deck parameters is not working. Comparison before/after fix:

Screen.Recording.2024-03-12.at.12.19.42.mov

In general we want to remove withParametersWebGL/setParametersWebGL from the codebase

Change List

Remove setParametersWebGL & withParametersWebGL methods for setting GL parameters
Support colorMask & scissorRect in LayerPass.render()

Pessimistress · 2024-03-12T15:41:02Z

modules/core/src/lib/layer.ts

-          extension.draw.call(this, opts, extension);
-        }
+      // Call subclass lifecycle method
+      const opts: DrawOptions = {


Can you try calling model.setParameters here for all this.getModels()? I think the exception is rare (ScenegraphLayer comes to mind).

Done. It works, however I cannot get picking to work if I remove the withParametersWebGL call below

model.setParameters takes RenderPipelineParameters (v9 string format) and device.withParametersWebGL takes GLParameters (v8 WebGL constant format). Picking pass used the later (blend and blendColor).

Pessimistress · 2024-03-13T01:45:55Z

modules/layers/src/line-layer/line-layer.ts

@@ -182,10 +182,11 @@ export default class LineLayer<DataT = any, ExtraProps extends {} = {}> extends
    }
  }

-  draw({uniforms}): void {
+  draw({parameters, uniforms}): void {


With the current caching mechanism, changing parameters will cause a pipeline to rebuild. Because each layer has a unique polygonOffset to battle z-fighting, calling model.setParameters will practically disable all program reuse. @ibgreen @donmccurdy

Hmm... @ibgreen I assume this is the purpose of the new factory destroy policy 'never' setting? Let's test that and consider a future 'lazy' destroy policy (or similar) if the performance benefits are worthwhile?

No the problem is not cache getting removed, it's that the hash is overly aggressive.

Does the hash even need to include the parameters? Could we have a whitelist or exclude list?

Discussing with @ibgreen in a separate channel - basically WebGPU render pipeline does not allow dynamically setting parameters so the unified API does not accept it.

Based on ...

https://github.com/visgl/luma.gl/blob/c5c15fbb9ab2fcaad998d030eac9e6312dd70aad/modules/engine/src/lib/pipeline-factory.ts#L111-L115

... it might be optional for WebGL, but will require a new pipeline for WebGPU. My hope with the suggestion of a 'lazy' destroy policy would be that if we have N layers and 1 pipeline, we can at least keep N pipelines in the cache rather than creating and destroying pipelines N times per frame.

WebGPU's equivalent of polygonOffset is depth stencil state bias, and is configured on the render pipeline.

I wonder if it would be possible to combat the z-fighting in the vertex shader via a uniform, something like gl_Position.z += 0.0001 * layerIndex

Yes... we also have instance ID now with WebGL2.

The z fighting work around has long been due for an update. We definitely need to make some changes while working on WebGPU support.

coveralls · 2024-03-13T12:08:24Z

coverage: 89.027% (-0.009%) from 89.036%
when pulling 625d1d3 on felix/deck-layer-parameters-v9
into 1d504e9 on master.

modules/core/src/lib/layer.ts

Pessimistress · 2024-03-14T15:38:01Z

modules/core/src/lib/layer.ts

@@ -184,6 +184,14 @@ export type UpdateParameters<LayerT extends Layer> = {
  changeFlags: ChangeFlags;
 };

+export type DrawOptions = {
+  context: LayerContext;
+  moduleParameters: any;


Not new in this PR, but the name can be really confusing. I think it's better called moduleSettings or shaderInputProps?

felixpalmer · 2024-03-15T09:48:01Z

@Pessimistress I've moved the merge to layers-pass and it works. The pick-layers-pass is the problem, I tried to migrate that over as well, but the parameters used there are the old GLParameters type, and it isn't possible to convert to the new types without changes in luma. In the interests of landing this and fixing the blending I would propose we tackle the migration of the pick-layers-pass in a future PR, once luma has the missing BlendFactor

donmccurdy · 2024-03-15T20:35:19Z

PR for the BlendFactor issue:

feat(core): Add missing blend factors, restrict types in device-parameters luma.gl#2040

felixpalmer · 2024-03-18T13:49:04Z

@donmccurdy @Pessimistress I've updated pick-layers-pass with the new BlendFactor types

modules/core/src/passes/pick-layers-pass.ts

Co-authored-by: Don McCurdy <donmccurdy@cartodb.com>

Pessimistress

The following passes also have withParametersWebGL and need to be merged in the proper order:

ShadowPass
CollisionFilterPass
MaskPass
TerrainPass
TerrainPickingPass

modules/core/src/passes/pick-layers-pass.ts

Pessimistress · 2024-03-19T22:14:07Z

modules/core/src/passes/pick-layers-pass.ts

+    const pickParameters = {
+      ...layer.props.parameters,
+      scissorTest: true,
+      scissor: [x, y, width, height],


For both perf and WebGPU readiness it's probably appropriate to align LayersPassRenderOptions with luma's RenderPassProps/RenderPassParameters and set scissor/depth parameters per-pass instead of per-layer.

I've moved the scissor test to be performed on the pass, but I'm not sure about depth. It is common for layers to override the depth handling via parameters

The picking pass only resets depth to avoid dirty context state handover (in v8 deck.props.parameters is not set before picking buffer redraw). They can be overwritten by per-layer settings. In this case we should reset the depth parameters in render pass creation. In the other cases depth parameters should override user props.

…k.gl into felix/deck-layer-parameters-v9

modules/core/src/lib/layer.ts

modules/core/src/passes/pick-layers-pass.ts

Pessimistress · 2024-03-20T15:20:32Z

modules/core/src/passes/pick-layers-pass.ts

+    const pickParameters = {
+      ...layer.props.parameters,
+      scissorTest: true,
+      scissor: [x, y, width, height],


The picking pass only resets depth to avoid dirty context state handover (in v8 deck.props.parameters is not set before picking buffer redraw). They can be overwritten by per-layer settings. In this case we should reset the depth parameters in render pass creation. In the other cases depth parameters should override user props.

This reverts commit 90683b0.

felixpalmer requested review from ibgreen, Pessimistress and donmccurdy March 12, 2024 11:27

felixpalmer mentioned this pull request Mar 12, 2024

[Bug] Broken website examples in 9.0.0-beta.10 #8582

Closed

felixpalmer marked this pull request as draft March 12, 2024 12:11

Pessimistress reviewed Mar 12, 2024

View reviewed changes

Pessimistress requested changes Mar 13, 2024

View reviewed changes

felixpalmer marked this pull request as ready for review March 13, 2024 12:15

felixpalmer requested a review from Pessimistress March 13, 2024 12:15

Pessimistress reviewed Mar 14, 2024

View reviewed changes

Merge parameters in layers-pass

11dcd54

felixpalmer force-pushed the felix/deck-layer-parameters-v9 branch from c24ab11 to 11dcd54 Compare March 15, 2024 09:41

felixpalmer requested a review from Pessimistress March 15, 2024 09:48

felixpalmer added 3 commits March 18, 2024 14:40

Bump luma to 9.0.4

83d27df

Move pick-layers-pass to string constants

98039b9

Merge branch 'master' into felix/deck-layer-parameters-v9

6391a57

donmccurdy approved these changes Mar 18, 2024

View reviewed changes

modules/core/src/passes/pick-layers-pass.ts Outdated Show resolved Hide resolved

felixpalmer and others added 2 commits March 18, 2024 22:44

Update modules/core/src/passes/pick-layers-pass.ts

4e94f2f

Co-authored-by: Don McCurdy <donmccurdy@cartodb.com>

Merge branch 'master' into felix/deck-layer-parameters-v9

f987b2c

Pessimistress requested changes Mar 19, 2024

View reviewed changes

modules/core/src/passes/pick-layers-pass.ts Show resolved Hide resolved

Pessimistress reviewed Mar 19, 2024

View reviewed changes

felixpalmer added 5 commits March 20, 2024 13:09

Remove withParametersWebGL from mask pass

3bde40d

Remove from shadow pass

d45b9eb

Migrate collisionFilter pass

085cc6a

Tidy

f7e84dc

Migrate TerrainPass

7ae615f

felixpalmer added 3 commits March 20, 2024 14:43

Rename to _BLENDING

1bd0b1c

Merge branch 'felix/deck-layer-parameters-v9' of github.com:visgl/dec…

085671a

…k.gl into felix/deck-layer-parameters-v9

TerrainPickingPass

aa221e6

felixpalmer mentioned this pull request Mar 20, 2024

webgl: Support colorMask in RenderPass visgl/luma.gl#2046

Merged

felixpalmer changed the title ~~Set parameters without withParametersWebGL method~~ Remove withParametersWebGL method in Pass classes Mar 20, 2024

Allow user to override depth in picking pass

bf9176b

felixpalmer requested a review from Pessimistress March 20, 2024 15:05

Pessimistress reviewed Mar 20, 2024

View reviewed changes

felixpalmer and others added 9 commits March 20, 2024 16:56

Comments

1e7c43c

Set colorMask on pass

7abbcbb

Merge branch 'master' into felix/deck-layer-parameters-v9

c1d4179

Fix TerrainPass

86e7a64

Merge branch 'master' into felix/deck-layer-parameters-v9

1523318

Attempt to switch to bitmask colorMask

5b9f382

Bump luma 9.0.6

33266ee

Off by one error

46d3d66

Remove redundant setParametersWebGL call

49fbdf7

felixpalmer requested a review from Pessimistress March 21, 2024 11:11

felixpalmer added 2 commits March 21, 2024 16:56

Set blendConstant on RenderPass

90683b0

Revert "Set blendConstant on RenderPass"

625d1d3

This reverts commit 90683b0.

Pessimistress approved these changes Mar 21, 2024

View reviewed changes

felixpalmer merged commit 7bd54c9 into master Mar 21, 2024
2 checks passed

felixpalmer deleted the felix/deck-layer-parameters-v9 branch March 21, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove withParametersWebGL method in Pass classes #8636

Remove withParametersWebGL method in Pass classes #8636

felixpalmer commented Mar 12, 2024 •

edited

Pessimistress Mar 12, 2024

felixpalmer Mar 13, 2024

Pessimistress Mar 14, 2024

Pessimistress Mar 13, 2024

donmccurdy Mar 13, 2024

Pessimistress Mar 13, 2024

felixpalmer Mar 13, 2024

Pessimistress Mar 13, 2024

donmccurdy Mar 13, 2024 •

edited

felixpalmer Mar 14, 2024

Pessimistress Mar 14, 2024

coveralls commented Mar 13, 2024 •

edited

Pessimistress Mar 14, 2024

felixpalmer commented Mar 15, 2024 •

edited

donmccurdy commented Mar 15, 2024

felixpalmer commented Mar 18, 2024

Pessimistress left a comment •

edited

Pessimistress Mar 19, 2024

felixpalmer Mar 20, 2024

Pessimistress Mar 20, 2024

Pessimistress Mar 20, 2024

Remove withParametersWebGL method in Pass classes #8636

Remove withParametersWebGL method in Pass classes #8636

Conversation

felixpalmer commented Mar 12, 2024 • edited

Change List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 13, 2024 • edited

Choose a reason for hiding this comment

felixpalmer commented Mar 15, 2024 • edited

donmccurdy commented Mar 15, 2024

felixpalmer commented Mar 18, 2024

Pessimistress left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixpalmer commented Mar 12, 2024 •

edited

donmccurdy Mar 13, 2024 •

edited

coveralls commented Mar 13, 2024 •

edited

felixpalmer commented Mar 15, 2024 •

edited

Pessimistress left a comment •

edited