Skip to content

Commit

Permalink
[fix] Effects: fix possible 'undefined' in effect parameters (#2452)
Browse files Browse the repository at this point in the history 
Signed-off-by: Ihor Dykhta <dikhta.igor@gmail.com>
  • Loading branch information
@igorDykhta
igorDykhta committed Nov 20, 2023
1 parent 8405378 commit 8e7b0ad
Show file tree
Hide file tree
Showing 7 changed files with 122 additions and 84 deletions.
35 changes: 32 additions & 3 deletions src/components/src/effects/effect-configurator.tsx
View file
Expand Up @@ -143,6 +143,14 @@ const COMMON_SLIDER_PROPS = {
label: 'value'
};

type EffectParameterDescriptionFlattened = {
name: string;
label?: string | false | (string | false)[];
min: number;
max: number;
index?: number;
};

EffectConfiguratorFactory.deps = [RangeSliderFactory, EffectTimeConfiguratorFactory];

export default function EffectConfiguratorFactory(
Expand Down Expand Up @@ -272,8 +280,29 @@ export default function EffectConfiguratorFactory(
const uniforms = effect.deckEffect?.module.uniforms || {};
const parameterDescriptions = effect.getParameterDescriptions();

const flatParameterDescriptions = useMemo(() => {
return parameterDescriptions.reduce((acc, description) => {
if (description.type === 'array') {
// split arrays of controls into a separate controls for each component
if (Array.isArray(description.defaultValue)) {
description.defaultValue.forEach((_, index) => {
acc.push({
...description,
index,
label: description.label?.[index]
});
});
}
} else {
acc.push(description);
}

return acc;
}, [] as EffectParameterDescriptionFlattened[]);
}, [parameterDescriptions]);

const controls = useMemo(() => {
return parameterDescriptions.map(desc => {
return flatParameterDescriptions.map(desc => {
const paramName = desc.name;

const uniform = uniforms[desc.name];
Expand Down Expand Up @@ -333,11 +362,11 @@ export default function EffectConfiguratorFactory(
// ignore everything else for now
return null;
});
}, [parameterDescriptions, effect, effect.parameters, updateEffectConfig]);
}, [flatParameterDescriptions, effect, effect.parameters, updateEffectConfig]);

return (
<StyledEffectConfigurator key={effect.id}>
{parameterDescriptions.map((desc, parameterIndex) => {
{flatParameterDescriptions.map((desc, parameterIndex) => {
const control = controls[parameterIndex];
if (!control) {
return null;
Expand Down
72 changes: 18 additions & 54 deletions src/constants/src/default-settings.ts
View file
Expand Up @@ -1281,15 +1281,9 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = {
},
{
name: 'center',
label: 'Center X',
index: 0,
min: 0,
max: 1
},
{
name: 'center',
label: 'Center Y',
index: 1,
type: 'array',
label: ['Center X', 'Center Y'],
defaultValue: [0.5, 0.5],
min: 0,
max: 1
}
Expand All @@ -1311,15 +1305,9 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = {
},
{
name: 'center',
label: 'Center X',
index: 0,
min: 0,
max: 1
},
{
name: 'center',
label: 'Center Y',
index: 1,
type: 'array',
label: ['Center X', 'Center Y'],
defaultValue: [0.5, 0.5],
min: 0,
max: 1
}
Expand Down Expand Up @@ -1347,15 +1335,9 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = {
},
{
name: 'center',
label: 'Center X',
index: 0,
min: 0,
max: 1
},
{
name: 'center',
label: 'Center Y',
index: 1,
type: 'array',
label: ['Center X', 'Center Y'],
defaultValue: [0.5, 0.5],
min: 0,
max: 1
}
Expand All @@ -1379,27 +1361,17 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = {
},
{
name: 'start',
index: 0,
min: 0,
max: 1
},
{
name: 'start',
label: false,
index: 1,
type: 'array',
label: ['Start', false],
defaultValue: [0.0, 0.0],
min: 0,
max: 1
},
{
name: 'end',
index: 0,
min: 0,
max: 1
},
{
name: 'end',
label: false,
index: 1,
type: 'array',
label: ['End', false],
defaultValue: [1, 1],
min: 0,
max: 1
}
Expand All @@ -1424,17 +1396,9 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = {
parameters: [
{
name: 'screenXY',
label: 'Position X',
index: 0,
defaultValue: 0.5,
min: 0,
max: 1
},
{
name: 'screenXY',
label: 'Position Y',
index: 1,
defaultValue: 0.5,
type: 'array',
label: ['Position X', 'Position Y'],
defaultValue: [0.5, 0.5],
min: 0,
max: 1
},
Expand Down
25 changes: 15 additions & 10 deletions src/effects/src/post-processing-effect.ts
View file
Expand Up @@ -18,7 +18,7 @@ import {
} from '@luma.gl/shadertools';

import {POSTPROCESSING_EFFECTS, DEFAULT_POST_PROCESSING_EFFECT_TYPE} from '@kepler.gl/constants';
import {EffectPropsPartial} from '@kepler.gl/types';
import {EffectPropsPartial, EffectParameterDescription} from '@kepler.gl/types';

import Effect from './effect';

Expand Down Expand Up @@ -86,12 +86,20 @@ const POSTPROCESSING_EFFECTS_DESCS = [
];

/**
* Temp. Get custom default values from effect description
* Returns default parameter value based on effect description.
* @param name Name of the parameter.
* @param effectDescription Effect's description.
* @param uniformsDesc Effect's uniforms.
* @returns
*/
const getDefaultValueForParameter = (name, effectDescription) => {
const rec = effectDescription.filter(param => param.name === name);
if (rec.length === 1) return rec[0].defaultValue;
else if (rec.length === 2) return [rec[0].defaultValue, rec[1].defaultValue];
export const getDefaultValueForParameter = (
name: string,
effectDescription: EffectParameterDescription[],
uniformsDesc: any
) => {
const description = effectDescription.find(param => param.name === name);
const uniform = uniformsDesc[name];
return description?.defaultValue ?? uniform?.value ?? uniform ?? description?.min;
};

class PostProcessingEffect extends Effect {
Expand All @@ -112,10 +120,7 @@ class PostProcessingEffect extends Effect {
const keys = Object.keys(uniforms);
const defaultParameters = {};
keys.forEach(key => {
defaultParameters[key] =
getDefaultValueForParameter(key, this._uiConfig) ??
uniforms[key].value ??
uniforms[key];
defaultParameters[key] = getDefaultValueForParameter(key, this._uiConfig, uniforms);
});
this.parameters = {...defaultParameters, ...this.parameters};
this.deckEffect?.setProps(this.parameters);
Expand Down
7 changes: 3 additions & 4 deletions src/types/effects.d.ts
View file
@@ -1,8 +1,7 @@
export type EffectParameterDescription = {
type EffectParameterDescription = {
name: string;
type?: 'number' | 'color';
label?: string | false;
index?: number;
type?: 'number' | 'array' | 'color';
label?: string | false | (string | false)[];
min: number;
max: number;
defaultValue?: number | number[];
Expand Down
16 changes: 5 additions & 11 deletions src/utils/src/effect-utils.ts
View file
Expand Up @@ -125,37 +125,31 @@ export function validateEffectParameters(
): EffectProps['parameters'] {
const result = cloneDeep(parameters);
effectDescription.forEach(description => {
const {index, defaultValue, name, type, min, max} = description;
const {defaultValue, name, type, min, max} = description;

if (!result.hasOwnProperty(name)) return;
const property = result[name];

if (type === 'color') {
if (type === 'color' || type === 'array') {
if (!Array.isArray(defaultValue)) return;
if (property.length !== defaultValue?.length) {
result[name] = defaultValue;
return;
}
defaultValue.forEach((v, i) => {
let value = property[i];
value = Number.isFinite(value) ? clamp([min, max], value) : defaultValue[i] || min;
value = Number.isFinite(value) ? clamp([min, max], value) : defaultValue[i] ?? min;
if (value !== undefined) {
property[i] = value;
}
});
return;
}

const indexed = Number.isFinite(index);
let value = indexed ? property[index as number] : property;
value = Number.isFinite(value) ? clamp([min, max], value) : defaultValue || min;
const value = Number.isFinite(property) ? clamp([min, max], property) : defaultValue ?? min;

if (value !== undefined) {
if (indexed) {
result[name][index as number] = value;
} else {
result[name] = value;
}
result[name] = value;
}
});
return result;
Expand Down
1 change: 0 additions & 1 deletion test/node/reducers/vis-state-merger-test.js
View file
Expand Up @@ -89,7 +89,6 @@ import {cmpFilters, cmpLayers, cmpDatasets} from 'test/helpers/comparison-utils'
import {
InitialState,
StateWFilters,
StateWEffects,
StateWMultiFilters,
StateWFilesFiltersLayerColor,
StateWSplitMaps,
Expand Down
50 changes: 49 additions & 1 deletion test/node/utils/effect-utils-test.js
View file
Expand Up @@ -21,7 +21,7 @@
import {LightingEffect, PostProcessEffect} from '@deck.gl/core';
import test from 'tape';

import {computeDeckEffects} from '@kepler.gl/utils';
import {computeDeckEffects, validateEffectParameters} from '@kepler.gl/utils';
import {VisStateActions} from '@kepler.gl/actions';
import {visStateReducer} from '@kepler.gl/reducers';
import {createEffect} from '@kepler.gl/effects';
Expand Down Expand Up @@ -95,3 +95,51 @@ test('effectUtils -> createEffect', t => {

t.end();
});

test('effectUtils -> validateEffectParameters', t => {
const testCases = [
{
input: {
parameters: {},
config: POSTPROCESSING_EFFECTS.magnify.parameters
},
expected: {}
},
{
input: {
parameters: {
screenXY: [10, 'x'],
radiusPixels: 1000,
zoom: 0,
borderWidthPixels: 'str'
},
config: POSTPROCESSING_EFFECTS.magnify.parameters
},
expected: {
screenXY: [1, 0.5],
radiusPixels: 500,
zoom: 0.5,
borderWidthPixels: 3
}
},
{
input: {
parameters: {
blurRadius: null,
gradientRadius: [20, 10],
start: [10],
end: 'str'
},
config: POSTPROCESSING_EFFECTS.tiltShift.parameters
},
expected: {blurRadius: 0, gradientRadius: 0, start: [0, 0], end: [1, 1]}
}
];

testCases.forEach((testCase, index) => {
const result = validateEffectParameters(testCase.input.parameters, testCase.input.config);
t.deepEqual(result, testCase.expected, `parameters should be property validated ${index}`);
});

t.end();
});

0 comments on commit 8e7b0ad

Please sign in to comment.