From 84053786ad09351d9775f6cc61d33abd170d1345 Mon Sep 17 00:00:00 2001 From: Igor Dykhta Date: Mon, 20 Nov 2023 17:26:24 +0200 Subject: [PATCH] [chore] Validate parameters for effects (#2450) Signed-off-by: Ihor Dykhta --- src/components/src/map-container.tsx | 7 +- src/constants/src/default-settings.ts | 133 ++++++++++++++++------ src/effects/src/effect.ts | 20 +++- src/effects/src/post-processing-effect.ts | 6 +- src/schemas/src/index.ts | 3 +- src/types/effects.d.ts | 7 +- src/utils/src/effect-utils.ts | 56 ++++++++- src/utils/src/index.ts | 2 +- test/node/reducers/vis-state-test.js | 41 +++++++ 9 files changed, 221 insertions(+), 54 deletions(-) diff --git a/src/components/src/map-container.tsx b/src/components/src/map-container.tsx index 52017b6496..c1d7061251 100644 --- a/src/components/src/map-container.tsx +++ b/src/components/src/map-container.tsx @@ -208,7 +208,12 @@ const DatasetAttributions = ({ {datasetAttributions?.length ? ( {datasetAttributions.map((ds, idx) => ( - + {ds.title} {idx !== datasetAttributions.length - 1 ? ', ' : null} diff --git a/src/constants/src/default-settings.ts b/src/constants/src/default-settings.ts index 7b332cfa66..3258888122 100644 --- a/src/constants/src/default-settings.ts +++ b/src/constants/src/default-settings.ts @@ -1192,11 +1192,7 @@ export const DEFAULT_LIGHT_COLOR: [number, number, number] = [255, 255, 255]; export const DEFAULT_LIGHT_INTENSITY = 1; export const DEFAULT_SHADOW_INTENSITY = 0.5; export const DEFAULT_SHADOW_COLOR: [number, number, number] = [0, 0, 0]; -export const LIGHT_AND_SHADOW_EFFECT: EffectDescription = { - type: 'lightAndShadow', - name: 'Light & Shadow', - parameters: [] -}; + export const LIGHT_AND_SHADOW_EFFECT_TIME_MODES = { pick: 'pick' as 'pick', current: 'current' as 'current', @@ -1223,51 +1219,79 @@ export const DEFAULT_LIGHT_AND_SHADOW_PROPS: { ambientLightIntensity: DEFAULT_LIGHT_INTENSITY }; +export const LIGHT_AND_SHADOW_EFFECT: EffectDescription = { + type: 'lightAndShadow', + name: 'Light & Shadow', + parameters: [ + {name: 'timestamp', min: 0, max: Number.MAX_SAFE_INTEGER}, + {name: 'shadowIntensity', min: 0, max: 1, defaultValue: DEFAULT_SHADOW_INTENSITY}, + {name: 'sunLightIntensity', min: 0, max: 1, defaultValue: DEFAULT_LIGHT_INTENSITY}, + {name: 'ambientLightIntensity', min: 0, max: 1, defaultValue: DEFAULT_LIGHT_INTENSITY}, + {name: 'shadowColor', type: 'color', min: 0, max: 255, defaultValue: DEFAULT_SHADOW_COLOR}, + {name: 'sunLightColor', type: 'color', min: 0, max: 255, defaultValue: DEFAULT_LIGHT_COLOR}, + {name: 'ambientLightColor', type: 'color', min: 0, max: 255, defaultValue: DEFAULT_LIGHT_COLOR} + ] +}; + export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = { ink: { type: 'ink', name: 'Ink', - parameters: [{name: 'strength'}] + parameters: [{name: 'strength', min: 0, max: 1}] }, brightnessContrast: { type: 'brightnessContrast', name: 'Brightness & Contrast', - parameters: [{name: 'brightness'}, {name: 'contrast'}] + parameters: [ + {name: 'brightness', min: -1, max: 1}, + {name: 'contrast', min: -1, max: 1} + ] }, hueSaturation: { type: 'hueSaturation', name: 'Hue & Saturation', - parameters: [{name: 'hue'}, {name: 'saturation', default: 0.25}] + parameters: [ + {name: 'hue', min: -1, max: 1}, + {name: 'saturation', defaultValue: 0.25, min: -1, max: 1} + ] }, vibrance: { type: 'vibrance', name: 'Vibrance', - parameters: [{name: 'amount', default: 0.5}] + parameters: [{name: 'amount', defaultValue: 0.5, min: -1, max: 1}] }, sepia: { type: 'sepia', name: 'Sepia', - parameters: [{name: 'amount'}] + parameters: [{name: 'amount', min: 0, max: 1}] }, dotScreen: { type: 'dotScreen', name: 'Dot Screen', parameters: [ { - name: 'angle' + name: 'angle', + min: 0, + max: Math.PI / 2 }, { - name: 'size' + name: 'size', + min: 1, + max: 20 }, { name: 'center', label: 'Center X', - index: 0 + index: 0, + min: 0, + max: 1 }, { name: 'center', label: 'Center Y', - index: 1 + index: 1, + min: 0, + max: 1 } ] }, @@ -1276,32 +1300,40 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = { name: 'Color Halftone', parameters: [ { - name: 'angle' + name: 'angle', + min: 0, + max: Math.PI / 2 }, { - name: 'size' + name: 'size', + min: 1, + max: 20 }, { name: 'center', label: 'Center X', - index: 0 + index: 0, + min: 0, + max: 1 }, { name: 'center', label: 'Center Y', - index: 1 + index: 1, + min: 0, + max: 1 } ] }, noise: { type: 'noise', name: 'Noise', - parameters: [{name: 'amount'}] + parameters: [{name: 'amount', min: 0, max: 1}] }, triangleBlur: { type: 'triangleBlur', name: 'Blur (Triangle)', - parameters: [{name: 'radius'}, {name: 'delta'}] + parameters: [{name: 'radius', min: 0, max: 100}] }, zoomBlur: { type: 'zoomBlur', @@ -1309,17 +1341,23 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = { parameters: [ { name: 'strength', - default: 0.05 + defaultValue: 0.05, + min: 0, + max: 1 }, { name: 'center', label: 'Center X', - index: 0 + index: 0, + min: 0, + max: 1 }, { name: 'center', label: 'Center Y', - index: 1 + index: 1, + min: 0, + max: 1 } ] }, @@ -1329,41 +1367,56 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = { parameters: [ { name: 'blurRadius', - label: 'Blur' + label: 'Blur', + min: 0, + max: 50 }, { name: 'gradientRadius', - label: 'Gradient' + label: 'Gradient', + min: 0, + max: 400 }, { name: 'start', - index: 0 + index: 0, + min: 0, + max: 1 }, { name: 'start', label: false, - index: 1 + index: 1, + min: 0, + max: 1 }, { name: 'end', - index: 0 + index: 0, + min: 0, + max: 1 }, { name: 'end', label: false, - index: 1 + index: 1, + min: 0, + max: 1 } ] }, edgeWork: { type: 'edgeWork', name: 'Edge work', - parameters: [{name: 'radius'}, {name: 'delta'}] + parameters: [{name: 'radius', min: 1, max: 50}] }, vignette: { type: 'vignette', name: 'Vignette', - parameters: [{name: 'amount'}, {name: 'radius'}] + parameters: [ + {name: 'amount', min: 0, max: 1}, + {name: 'radius', min: 0, max: 1} + ] }, magnify: { type: 'magnify', @@ -1373,18 +1426,23 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = { name: 'screenXY', label: 'Position X', index: 0, - default: 0.5 + defaultValue: 0.5, + min: 0, + max: 1 }, { name: 'screenXY', label: 'Position Y', index: 1, - default: 0.5 + defaultValue: 0.5, + min: 0, + max: 1 }, { name: 'radiusPixels', label: 'Size', - min: 10 + min: 10, + max: 500 }, { name: 'zoom', @@ -1394,15 +1452,16 @@ export const POSTPROCESSING_EFFECTS: {[key: string]: EffectDescription} = { { name: 'borderWidthPixels', label: 'Border Width', - default: 3, - max: 100 + defaultValue: 3, + min: 0, + max: 50 } ] }, hexagonalPixelate: { type: 'hexagonalPixelate', name: 'Hexagonal Pixelate', - parameters: [{name: 'scale', default: 20}] + parameters: [{name: 'scale', defaultValue: 20, min: 0, max: 50}] } }; diff --git a/src/effects/src/effect.ts b/src/effects/src/effect.ts index c6c2ada247..b944b3b3d8 100644 --- a/src/effects/src/effect.ts +++ b/src/effects/src/effect.ts @@ -1,11 +1,15 @@ -import {generateHashId} from '@kepler.gl/utils'; +import {generateHashId, validateEffectParameters} from '@kepler.gl/utils'; import { Effect as EffectInterface, EffectProps, EffectPropsPartial, EffectParameterDescription } from '@kepler.gl/types'; -import {DEFAULT_POST_PROCESSING_EFFECT_TYPE, POSTPROCESSING_EFFECTS} from '@kepler.gl/constants'; +import { + DEFAULT_POST_PROCESSING_EFFECT_TYPE, + POSTPROCESSING_EFFECTS, + LIGHT_AND_SHADOW_EFFECT +} from '@kepler.gl/constants'; export class Effect implements EffectInterface { id: string; @@ -27,9 +31,12 @@ export class Effect implements EffectInterface { this.isEnabled = _props.isEnabled; this.isConfigActive = _props.isConfigActive; this.isJsonEditorActive = _props.isJsonEditorActive; - this.parameters = _props.parameters; - this._uiConfig = POSTPROCESSING_EFFECTS[this.type]?.parameters || []; + this._uiConfig = + LIGHT_AND_SHADOW_EFFECT.type === this.type + ? LIGHT_AND_SHADOW_EFFECT.parameters + : POSTPROCESSING_EFFECTS[this.type]?.parameters || []; + this.parameters = validateEffectParameters(_props.parameters, this._uiConfig); this.deckEffect = null; this._initializeEffect(); @@ -56,7 +63,10 @@ export class Effect implements EffectInterface { this.isEnabled = props.isEnabled ?? this.isEnabled; this.isConfigActive = props.isConfigActive ?? this.isConfigActive; this.isJsonEditorActive = props.isJsonEditorActive ?? this.isJsonEditorActive; - this.parameters = {...this.parameters, ...props.parameters}; + this.parameters = { + ...this.parameters, + ...validateEffectParameters(props.parameters, this._uiConfig) + }; } isValidToSave() { diff --git a/src/effects/src/post-processing-effect.ts b/src/effects/src/post-processing-effect.ts index 11a9f3fcf9..7c0d2082a8 100644 --- a/src/effects/src/post-processing-effect.ts +++ b/src/effects/src/post-processing-effect.ts @@ -90,10 +90,8 @@ const POSTPROCESSING_EFFECTS_DESCS = [ */ const getDefaultValueForParameter = (name, effectDescription) => { const rec = effectDescription.filter(param => param.name === name); - if (rec.length === 1) return rec[0].default; - else if (rec.length === 2 && rec[0].default !== undefined && rec[1].default !== undefined) { - return [rec[0].default, rec[1].default]; - } + if (rec.length === 1) return rec[0].defaultValue; + else if (rec.length === 2) return [rec[0].defaultValue, rec[1].defaultValue]; }; class PostProcessingEffect extends Effect { diff --git a/src/schemas/src/index.ts b/src/schemas/src/index.ts index d7f7d8c603..357b9e5730 100644 --- a/src/schemas/src/index.ts +++ b/src/schemas/src/index.ts @@ -44,7 +44,8 @@ export { filterPropsV1, default as visStateSchema, layerPropsV1, - layerPropsV0 + layerPropsV0, + effectPropsV1 } from './vis-state-schema'; export type { SavedField, diff --git a/src/types/effects.d.ts b/src/types/effects.d.ts index 1ce0778d67..231ff8889c 100644 --- a/src/types/effects.d.ts +++ b/src/types/effects.d.ts @@ -1,10 +1,11 @@ export type EffectParameterDescription = { name: string; + type?: 'number' | 'color'; label?: string | false; index?: number; - min?: number; - max?: number; - default?: number; + min: number; + max: number; + defaultValue?: number | number[]; }; export type EffectDescription = { diff --git a/src/utils/src/effect-utils.ts b/src/utils/src/effect-utils.ts index cb2ebc869d..d06fcd95ab 100644 --- a/src/utils/src/effect-utils.ts +++ b/src/utils/src/effect-utils.ts @@ -1,5 +1,6 @@ import {arrayMove} from '@dnd-kit/sortable'; import SunCalc from 'suncalc'; +import cloneDeep from 'lodash.clonedeep'; import {PostProcessEffect} from '@deck.gl/core/typed'; @@ -9,9 +10,10 @@ import { FILTER_TYPES, FILTER_VIEW_TYPES } from '@kepler.gl/constants'; -import {findById} from './utils'; import {VisState} from '@kepler.gl/schemas'; -import {MapState, Effect} from '@kepler.gl/types'; +import {MapState, Effect, EffectProps, EffectDescription} from '@kepler.gl/types'; +import {findById} from './utils'; +import {clamp} from './data-utils'; export function computeDeckEffects({ visState, @@ -108,3 +110,53 @@ function updateEffect({visState, mapState, effect}) { } } } + +/** + * Validates parameters for an effect, clamps numbers to allowed ranges + * or applies default values in case of wrong non-numeric values. + * All unknown properties aren't modified. + * @param parameters Parameters candidate for an effect. + * @param effectDescription Description of an effect. + * @returns + */ +export function validateEffectParameters( + parameters: EffectProps['parameters'] = {}, + effectDescription: EffectDescription['parameters'] +): EffectProps['parameters'] { + const result = cloneDeep(parameters); + effectDescription.forEach(description => { + const {index, defaultValue, name, type, min, max} = description; + + if (!result.hasOwnProperty(name)) return; + const property = result[name]; + + if (type === 'color') { + 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; + 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; + + if (value !== undefined) { + if (indexed) { + result[name][index as number] = value; + } else { + result[name] = value; + } + } + }); + return result; +} diff --git a/src/utils/src/index.ts b/src/utils/src/index.ts index 98635d8093..6e41248bfd 100644 --- a/src/utils/src/index.ts +++ b/src/utils/src/index.ts @@ -98,7 +98,7 @@ export * from './utils'; export * from './split-map-utils'; export {snapToMarks} from './plot'; -export {computeDeckEffects, fixEffectOrder, reorderEffectOrder} from './effect-utils'; +export {computeDeckEffects, fixEffectOrder, reorderEffectOrder, validateEffectParameters} from './effect-utils'; // Mapbox export {transformRequest, isStyleUsingMapboxTiles} from './map-style-utils/mapbox-utils'; diff --git a/test/node/reducers/vis-state-test.js b/test/node/reducers/vis-state-test.js index 2f1ac2f425..498aec6e2a 100644 --- a/test/node/reducers/vis-state-test.js +++ b/test/node/reducers/vis-state-test.js @@ -5585,6 +5585,47 @@ test('#VisStateUpdater -> updateEffect', t => { t.end(); }); +test('#VisStateUpdater -> addEffect: invalid effect parameters', t => { + const initialState = InitialState.visState; + + let nextState = reducer( + initialState, + VisStateActions.addEffect({ + id: 'e_1', + type: 'lightAndShadow', + parameters: { + timestamp: 'x', + shadowIntensity: 2, + sunLightIntensity: [0, 1], + shadowColor: [0, 1], + sunLightColor: 2, + ambientLightColor: [300, 128, 128], + timezone: 'Not a timezone' + } + }) + ); + + const expectedEffect = { + id: 'e_1', + type: 'lightAndShadow', + parameters: { + timestamp: 0, + timeMode: 'pick', + shadowIntensity: 1, + sunLightIntensity: 1, + shadowColor: [0, 0, 0], + sunLightColor: [255, 255, 255], + ambientLightColor: [255, 128, 128], + ambientLightIntensity: 1, + timezone: 'Not a timezone' + } + }; + + cmpEffects(t, expectedEffect, nextState.effects[0], {id: true}); + + t.end(); +}); + test('#VisStateUpdater -> reorderEffect', t => { const initialState = InitialState.visState;