Generic module props handling in Layer class #9192

Pessimistress · 2024-09-28T00:55:33Z

Change List

Remove hard-coded mapping of module props in Layer.draw
LayersPass merges nested module settings
Effects/passes getModuleParameters methods return namespaced settings
- LayersPass
- PickLayersPass
- TerrainEffect/TerrainPass
- CollisionFilterEffect/CollisionFilterPass
- MaskEffect
- LightingEffect/ShadowPass

Pessimistress · 2024-09-28T00:56:32Z

modules/extensions/src/collision-filter/shader-module.ts

@@ -72,7 +72,7 @@ const inject = {
    collision_fade = collision_isVisible(collision_texCoords, geometry.pickingColor / 255.0);
    if (collision_fade < 0.0001) {
      // Position outside clip space bounds to discard
-      position = vec4(0.0, 0.0, 2.0, 1.0);
+      // position = vec4(0.0, 0.0, 2.0, 1.0);


@felixpalmer Please give this a check - I can't get it to work with this line enabled

This must not run when drawing to the collision map, where we never want to hide the features: 458b1f8

modules/extensions/src/mask/shader-module.ts

Pessimistress · 2024-09-28T01:01:26Z

modules/core/src/shaderlib/shadow/shadow.ts

-      shadow_uShadowMap0: opts.dummyShadowMap,
-      shadow_uShadowMap1: opts.dummyShadowMap
+      shadow_uShadowMap0: opts.dummyShadowMap!,
+      shadow_uShadowMap1: opts.dummyShadowMap!
    };
  }
  const projectUniforms = project.getUniforms(opts) as ProjectUniforms;


@ibgreen In v8 getUniforms can access uniforms created by this shader module's dependencies (project in this case). As of the latest master, several shader modules are calling project.getUniforms themselves, which technically is incorrect, because they do not receive the full set of project module props (modelMatrix, coordinateOrigin, etc.). Is this by design, or is it a regression?

I wasn't really aware of this usage, so I suppose it was at most semi-intentional, the goal was simply to have a clean, simple-to-understand API.

If it is a crucial feature we can consider adding it back, but I fear that making types of dependency modules flow into a dependent module could require more typescript machinery than it is worth.

Perhaps we can look at the cases where it is used and see what are options are.

Related: visgl/luma.gl#2138

My view is that it is too much magic to have the dependent module uniforms be available, and not as flexible. I started adding the project.getUniforms pattern as it feels more clear what is going on and all the uniform generation logic is then happening in one place and at the same time. I don't think it is a problem to pass the project module props where needed

FWIW, I am not opposed to adding back the previous behavior, but it would be nice to find another way to handle this. My concernsL

To me, giving the shader modules access to other uniforms felt like opaque and non-obvious behavior.

It was compounded by this not being the props that are forwarded, but the calculated uniforms with seems more internal.

This mainly seems to be a use case for the projection module, so a fairly heavy solution for a limited problem.

Generally, projection is a pretty complex topic and a very central concern.

One can easily end up with lots of derived uniforms that may need to be calculated for various shaders, inverse matrices, inverse uniform matrices etc etc.

Calculating new uniforms from a set of primary JS values rather than a set of derived uniforms makes a lot of sense to me.

A solution may be having a central ProjectionState type class that generates values that can be used as properties for dependent shader modules.

That ProjectionState class could technically be exported by the shader module (shadertools), but would perhaps best be located in the luma.gl engine module.

I have already have such helper classes in engine module for the new "advanced" picking module and of course the ShaderPasses.

coveralls · 2024-09-28T05:10:17Z

coverage: 91.639%. remained the same
when pulling 479f1c7 on x/module-props
into 2e9d6dc on master.

ibgreen

Solid consolidation!

Could be nice to follow up with a renaming of the moduleSettings and moduleParameters (perhaps we can offer backwards compatibility)?
Maybe align on moduleProps or shaderModuleProps?

modules/aggregation-layers/src/common/aggregator/gpu-aggregator/webgl-bin-sorter.ts

modules/aggregation-layers/src/contour-layer/contour-layer.ts

ibgreen · 2024-09-28T13:08:53Z

modules/core/src/effects/lighting/lighting-effect.ts

@@ -13,7 +13,7 @@ import shadow from '../../shaderlib/shadow/shadow';

 import type Layer from '../../lib/layer';
 import type {Effect, EffectContext, PreRenderOptions} from '../../lib/effect';
-import {LightingProps} from '@luma.gl/shadertools';
+import type Viewport from '../../viewports/viewport';


Nit: I have remove all default exports inside other frameworks... just a suggestion to try to avoid them in new deck code.

modules/core/src/effects/lighting/lighting-effect.ts

modules/core/src/lib/layer.ts

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

ibgreen · 2024-09-28T13:14:55Z

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


    if (effects) {
      for (const effect of effects) {
-        Object.assign(moduleParameters, effect.getModuleParameters?.(layer));
+        mergeModuleParameters(moduleParameters, effect.getModuleParameters?.(layer));


Should we offer this function in luma.gl? loaders.gl has a similar two-level mergeOptions export.

ibgreen · 2024-09-28T13:19:52Z

modules/core/src/shaderlib/shadow/shadow.ts

-      shadow_uShadowMap0: opts.dummyShadowMap,
-      shadow_uShadowMap1: opts.dummyShadowMap
+      shadow_uShadowMap0: opts.dummyShadowMap!,
+      shadow_uShadowMap1: opts.dummyShadowMap!
    };
  }
  const projectUniforms = project.getUniforms(opts) as ProjectUniforms;


I wasn't really aware of this usage, so I suppose it was at most semi-intentional, the goal was simply to have a clean, simple-to-understand API.

If it is a crucial feature we can consider adding it back, but I fear that making types of dependency modules flow into a dependent module could require more typescript machinery than it is worth.

Perhaps we can look at the cases where it is used and see what are options are.

modules/extensions/src/mask/shader-module.ts

felixpalmer · 2024-09-30T08:13:59Z

modules/extensions/src/collision-filter/shader-module.ts

@@ -72,7 +72,7 @@ const inject = {
    collision_fade = collision_isVisible(collision_texCoords, geometry.pickingColor / 255.0);
    if (collision_fade < 0.0001) {
      // Position outside clip space bounds to discard
-      position = vec4(0.0, 0.0, 2.0, 1.0);
+      // position = vec4(0.0, 0.0, 2.0, 1.0);


This must not run when drawing to the collision map, where we never want to hide the features: 458b1f8

felixpalmer · 2024-09-30T08:16:44Z

modules/aggregation-layers/src/contour-layer/contour-layer.ts

-    if (opts.moduleParameters.viewport) {
-      opts.moduleParameters.viewport = this.state.aggregatorViewport;
+    if (opts.shaderModuleProps.project) {
+      opts.shaderModuleProps.project.viewport = this.state.aggregatorViewport;


Better to clone the project object to avoid mutating it in the calling code?

The project object is soft referenced by other dependent modules (shadow, terrain, etc.)

modules/core/src/shaderlib/misc/layer-uniforms.ts

modules/extensions/src/collision-filter/collision-filter-extension.ts

Pessimistress · 2024-09-30T17:13:32Z

I've pushed an alternative solution - instead of passing contextual uniforms to getUniforms, I'm sending contextual module props in Effect.getShaderModuleProps / Pass.getShaderModuleProps:

deck.gl/modules/core/src/passes/shadow-pass.ts

Lines 88 to 95 in 479f1c7

    
           getShaderModuleProps(layer: Layer, effects: any, otherShaderModuleProps: Record<string, any>) { 
        
             return { 
        
               shadow: { 
        
                 project: otherShaderModuleProps.project, 
        
                 drawToShadowMap: true 
        
               } 
        
             }; 
        
           }

ibgreen · 2024-09-30T18:13:06Z

I've pushed an alternative solution - instead of passing contextual uniforms to getUniforms, I'm sending contextual module props in Effect.getShaderModuleProps / Pass.getShaderModuleProps:

Interesting... If I understand correctly, this sends the project module props instead of the calculated project module uniforms? So potentially a bit more work for the extension modules' `getUniforms()...? Or perhaps not?

If it works well then that seems like a reasonable way to handle this for now.

Pessimistress requested review from felixpalmer and ibgreen September 28, 2024 00:55

Pessimistress commented Sep 28, 2024

View reviewed changes

Pessimistress force-pushed the x/module-props branch from 29864aa to 4ba83d0 Compare September 28, 2024 05:02

ibgreen reviewed Sep 28, 2024

View reviewed changes

felixpalmer reviewed Sep 30, 2024

View reviewed changes

Pessimistress marked this pull request as ready for review September 30, 2024 17:08

Pessimistress and others added 9 commits September 30, 2024 10:08

generic module props handling in Layer class

100e120

Fix GPUAggregator

2ec1ffa

fixes

9b1af89

tests

94acd83

address comments

5e1c095

fix tests

453b99b

Fix collision vertex discard

0432b7a

Lint

073bfd1

Remove reference of moduleSettings

479f1c7

Pessimistress force-pushed the x/module-props branch from dc9b79f to 479f1c7 Compare September 30, 2024 17:08

felixpalmer approved these changes Oct 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic module props handling in Layer class #9192

Generic module props handling in Layer class #9192

Pessimistress commented Sep 28, 2024 •

edited

Loading

Pessimistress Sep 28, 2024

felixpalmer Sep 30, 2024

Pessimistress Sep 28, 2024

ibgreen Sep 28, 2024

felixpalmer Sep 30, 2024

ibgreen Sep 30, 2024

coveralls commented Sep 28, 2024 •

edited

Loading

ibgreen left a comment

ibgreen Sep 28, 2024

ibgreen Sep 28, 2024

ibgreen Sep 28, 2024

felixpalmer Sep 30, 2024

felixpalmer Sep 30, 2024

Pessimistress Sep 30, 2024

Pessimistress commented Sep 30, 2024

ibgreen commented Sep 30, 2024

Generic module props handling in Layer class #9192

Are you sure you want to change the base?

Generic module props handling in Layer class #9192

Conversation

Pessimistress commented Sep 28, 2024 • edited Loading

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

coveralls commented Sep 28, 2024 • edited Loading

ibgreen left a 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

Pessimistress commented Sep 30, 2024

ibgreen commented Sep 30, 2024

Pessimistress commented Sep 28, 2024 •

edited

Loading

coveralls commented Sep 28, 2024 •

edited

Loading