diff --git a/CHANGELOG.md b/CHANGELOG.md index 5775ece8b7..8f8c7f97c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ - _...Add new stuff here..._ ### 🐞 Bug fixes +- Fix `fill-outline-color` expressions using `feature-state` so missing state falls back to `fill-color` instead of breaking fill rendering ([#4234](https://github.com/maplibre/maplibre-gl-js/issues/4234)) - _...Add new stuff here..._ ## 5.23.0 diff --git a/src/data/program_configuration.test.ts b/src/data/program_configuration.test.ts new file mode 100644 index 0000000000..916df73dda --- /dev/null +++ b/src/data/program_configuration.test.ts @@ -0,0 +1,160 @@ +import {afterEach, describe, expect, test, vi} from 'vitest'; + +import {Color, type Feature} from '@maplibre/maplibre-gl-style-spec'; +import type {VectorTileLayerLike} from '@maplibre/vt-pbf'; + +import {createStyleLayer} from '../style/create_style_layer'; +import {FeaturePositionMap} from './feature_position_map'; +import {type EvaluationParameters} from '../style/evaluation_parameters'; +import {type TransitionParameters} from '../style/properties'; +import {packUint8ToFloat} from '../shaders/encode_attribute'; +import {ProgramConfiguration} from './program_configuration'; + +import type {FillStyleLayer} from '../style/style_layer/fill_style_layer'; + +const feature = { + type: 'Polygon', + id: 'building', + properties: {}, +} as Feature; + +const options = {imagePositions: {}}; +const vtLayer = {feature: () => feature} as unknown as VectorTileLayerLike; + +function createFillLayer(fillOutlineColor: any, fillColor: string = '#00f') { + const layer = createStyleLayer({ + id: 'building', + type: 'fill', + source: 'streets', + paint: { + 'fill-color': fillColor, + 'fill-outline-color': fillOutlineColor, + }, + }, {}) as FillStyleLayer; + layer.recalculate({zoom: 0, zoomHistory: {}} as EvaluationParameters, []); + return layer; +} + +function getPackedColor(color: Color) { + return [ + packUint8ToFloat(255 * color.r, 255 * color.g), + packUint8ToFloat(255 * color.b, 255 * color.a), + ]; +} + +function readOutlineColor(programConfiguration: ProgramConfiguration) { + const binder = programConfiguration.binders['fill-outline-color'] as unknown as { + paintVertexArray: {arrayBuffer: ArrayBuffer}; + }; + return Array.from(new Float32Array(binder.paintVertexArray.arrayBuffer).slice(0, 2)); +} + +function createIndexedFeatureMap(featureId: string | number, index: number = 0, start: number = 0, end: number = 1) { + const featureMap = new FeaturePositionMap(); + featureMap.add(featureId, index, start, end); + return FeaturePositionMap.deserialize(FeaturePositionMap.serialize(featureMap, [])); +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe('ProgramConfiguration', () => { + test('does not throw or warn when fill-outline-color feature-state is missing', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const layer = createFillLayer(['feature-state', 'outline-color']); + const programConfiguration = new ProgramConfiguration(layer, 0, () => true); + + expect(() => { + programConfiguration.populatePaintArrays(1, feature, options); + }).not.toThrow(); + expect(warn).not.toHaveBeenCalled(); + }); + + test('updates fill-outline-color when source feature-state changes', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const layer = createFillLayer(['feature-state', 'outline-color']); + const programConfiguration = new ProgramConfiguration(layer, 0, () => true); + const featureMap = createIndexedFeatureMap('building'); + + programConfiguration.populatePaintArrays(1, feature, options); + expect(readOutlineColor(programConfiguration)).toEqual(getPackedColor(new Color(0, 0, 1, 1))); + + expect(programConfiguration.updatePaintArrays({'building': {'outline-color': '#f00'}}, featureMap, vtLayer, layer, options)).toBe(true); + expect(readOutlineColor(programConfiguration)).toEqual(getPackedColor(new Color(1, 0, 0, 1))); + expect(warn).not.toHaveBeenCalled(); + }); + + test('does not throw or warn when composite fill-outline-color feature-state is missing', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const layer = createFillLayer([ + 'interpolate', + ['linear'], + ['zoom'], + 0, + ['feature-state', 'outline-color'], + 1, + ['feature-state', 'outline-color'], + ]); + const programConfiguration = new ProgramConfiguration(layer, 0, () => true); + + expect(() => { + programConfiguration.populatePaintArrays(1, feature, options); + }).not.toThrow(); + expect(warn).not.toHaveBeenCalled(); + }); + + test('updates fill-outline-color when composite feature-state changes', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const layer = createFillLayer([ + 'interpolate', + ['linear'], + ['zoom'], + 0, + ['feature-state', 'outline-color'], + 1, + ['feature-state', 'outline-color'], + ]); + const programConfiguration = new ProgramConfiguration(layer, 0, () => true); + const featureMap = createIndexedFeatureMap('building'); + + programConfiguration.populatePaintArrays(1, feature, options); + expect(readOutlineColor(programConfiguration)).toEqual(getPackedColor(new Color(0, 0, 1, 1))); + + expect(programConfiguration.updatePaintArrays({'building': {'outline-color': '#f00'}}, featureMap, vtLayer, layer, options)).toBe(true); + expect(readOutlineColor(programConfiguration)).toEqual(getPackedColor(new Color(1, 0, 0, 1))); + expect(warn).not.toHaveBeenCalled(); + }); + + test('uses the latest fill-color as the fallback after paint updates', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const layer = createFillLayer(['feature-state', 'outline-color']); + const programConfiguration = new ProgramConfiguration(layer, 0, () => true); + const featureMap = createIndexedFeatureMap('building'); + + programConfiguration.populatePaintArrays(1, feature, options); + expect(readOutlineColor(programConfiguration)).toEqual(getPackedColor(new Color(0, 0, 1, 1))); + + layer.setPaintProperty('fill-color', '#0f0'); + layer.updateTransitions({} as TransitionParameters); + layer.recalculate({zoom: 0, zoomHistory: {}} as EvaluationParameters, []); + expect(programConfiguration.updatePaintArrays({'building': {}}, featureMap, vtLayer, layer, options)).toBe(true); + + expect(readOutlineColor(programConfiguration)).toEqual(getPackedColor(new Color(0, 1, 0, 1))); + expect(warn).not.toHaveBeenCalled(); + }); + + test('warns for invalid feature-state colors but does not break rendering', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const layer = createFillLayer(['feature-state', 'outline-color']); + const programConfiguration = new ProgramConfiguration(layer, 0, () => true); + const featureMap = createIndexedFeatureMap('building'); + + programConfiguration.populatePaintArrays(1, feature, options); + + expect(programConfiguration.updatePaintArrays({'building': {'outline-color': 'not-a-color'}}, featureMap, vtLayer, layer, options)).toBe(true); + + expect(readOutlineColor(programConfiguration)).toEqual(getPackedColor(new Color(0, 0, 1, 1))); + expect(warn).toHaveBeenCalledWith('Could not parse color from value \'not-a-color\''); + }); +}); diff --git a/src/data/program_configuration.ts b/src/data/program_configuration.ts index c550a2d9c4..73306000f6 100644 --- a/src/data/program_configuration.ts +++ b/src/data/program_configuration.ts @@ -19,13 +19,15 @@ import type {CrossfadeParameters} from '../style/evaluation_parameters'; import type {StructArray, StructArrayMember} from '../util/struct_array'; import type {VertexBuffer} from '../webgl/vertex_buffer'; import type {ImagePosition} from '../render/image_atlas'; +import type {FillStyleLayer} from '../style/style_layer/fill_style_layer'; import type { Feature, FeatureState, GlobalProperties, - SourceExpression, CompositeExpression, - FormattedSection + FormattedSection, + ZoomConstantExpression, + ZoomDependentExpression } from '@maplibre/maplibre-gl-style-spec'; import type {FeatureStates} from '../source/source_state'; import type {DashEntry} from '../render/line_atlas'; @@ -56,6 +58,28 @@ type PaintOptions = { globalState?: Record; }; +type FillColorFallback = PossiblyEvaluatedPropertyValue['value'] | null; + +type SourceExpressionWithRawEvaluation = ZoomConstantExpression<'source'>; + +type CompositeExpressionWithRawEvaluation = ZoomDependentExpression<'composite'>; + +function getFillColorFallback(layer: TypedStyleLayer, property: string): FillColorFallback { + if (property !== 'fill-outline-color' || !isFillStyleLayer(layer)) { + return null; + } + + return layer.paint.get('fill-color').value; +} + +function isFillStyleLayer(layer: TypedStyleLayer): layer is FillStyleLayer { + return layer.type === 'fill'; +} + +function isNullColorEvaluationError(error: unknown): boolean { + return error instanceof Error && error.message === 'Could not parse color from value \'null\''; +} + /** * `Binder` is the interface definition for the strategies for constructing, * uploading, and binding paint property data as GLSL attributes. Most style- @@ -194,19 +218,22 @@ class CrossFadedConstantBinder implements UniformBinder { } class SourceExpressionBinder implements AttributeBinder { - expression: SourceExpression; + expression: SourceExpressionWithRawEvaluation; type: string; + zoom: number; maxValue: number; + fillColorFallback: FillColorFallback; paintVertexArray: StructArray; paintVertexAttributes: StructArrayMember[]; paintVertexBuffer: VertexBuffer; - constructor(expression: SourceExpression, names: string[], type: string, PaintVertexArray: { + constructor(expression: SourceExpressionWithRawEvaluation, names: string[], type: string, zoom: number, fillColorFallback: FillColorFallback, PaintVertexArray: { new (...args: any): StructArray; }) { this.expression = expression; this.type = type; + this.zoom = zoom; this.maxValue = 0; this.paintVertexAttributes = names.map((name) => ({ name: `a_${name}`, @@ -214,18 +241,19 @@ class SourceExpressionBinder implements AttributeBinder { components: type === 'color' ? 2 : 1, offset: 0 })); + this.fillColorFallback = fillColorFallback; this.paintVertexArray = new PaintVertexArray(); } populatePaintArray(newLength: number, feature: Feature, options: PaintOptions) { const start = this.paintVertexArray.length; - const value = this.expression.evaluate(new EvaluationParameters(0, options), feature, {}, options.canonical, [], options.formattedSection); + const value = this._evaluateValue(feature, {}, options, options.canonical, options.formattedSection); this.paintVertexArray.resize(newLength); this._setPaintValue(start, newLength, value); } updatePaintArray(start: number, end: number, feature: Feature, featureState: FeatureState, options: PaintOptions) { - const value = this.expression.evaluate(new EvaluationParameters(0, options), feature, featureState); + const value = this._evaluateValue(feature, featureState, options); this._setPaintValue(start, end, value); } @@ -243,6 +271,32 @@ class SourceExpressionBinder implements AttributeBinder { } } + _evaluateFillColorFallback(feature: Feature, featureState: FeatureState, options: PaintOptions, canonical?: CanonicalTileID, formattedSection?: FormattedSection): Color | null | undefined { + if (!this.fillColorFallback) return null; + if (this.fillColorFallback.kind === 'constant') { + return this.fillColorFallback.value; + } + return this.fillColorFallback.evaluate(new EvaluationParameters(this.zoom, options), feature, featureState, canonical, [], formattedSection); + } + + _evaluateValue(feature: Feature, featureState: FeatureState, options: PaintOptions, canonical?: CanonicalTileID, formattedSection?: FormattedSection) { + if (this.type !== 'color' || !this.fillColorFallback) { + return this.expression.evaluate(new EvaluationParameters(0, options), feature, featureState, canonical, [], formattedSection); + } + + try { + const value = this.expression.evaluateWithoutErrorHandling(new EvaluationParameters(0, options), feature, featureState, canonical, [], formattedSection); + return value == null ? this._evaluateFillColorFallback(feature, featureState, options, canonical, formattedSection) : value; + } catch (error) { + if (isNullColorEvaluationError(error)) { + return this._evaluateFillColorFallback(feature, featureState, options, canonical, formattedSection); + } + + const value = this.expression.evaluate(new EvaluationParameters(0, options), feature, featureState, canonical, [], formattedSection); + return value == null ? this._evaluateFillColorFallback(feature, featureState, options, canonical, formattedSection) : value; + } + } + upload(context: Context) { if (this.paintVertexArray?.arrayBuffer.byteLength) { if (this.paintVertexBuffer?.buffer) { @@ -261,18 +315,19 @@ class SourceExpressionBinder implements AttributeBinder { } class CompositeExpressionBinder implements AttributeBinder, UniformBinder { - expression: CompositeExpression; + expression: CompositeExpressionWithRawEvaluation; uniformNames: string[]; type: string; useIntegerZoom: boolean; zoom: number; maxValue: number; + fillColorFallback: FillColorFallback; paintVertexArray: StructArray; paintVertexAttributes: StructArrayMember[]; paintVertexBuffer: VertexBuffer; - constructor(expression: CompositeExpression, names: string[], type: string, useIntegerZoom: boolean, zoom: number, PaintVertexArray: { + constructor(expression: CompositeExpressionWithRawEvaluation, names: string[], type: string, useIntegerZoom: boolean, zoom: number, fillColorFallback: FillColorFallback, PaintVertexArray: { new (...args: any): StructArray; }) { this.expression = expression; @@ -281,6 +336,7 @@ class CompositeExpressionBinder implements AttributeBinder, UniformBinder { this.useIntegerZoom = useIntegerZoom; this.zoom = zoom; this.maxValue = 0; + this.fillColorFallback = fillColorFallback; this.paintVertexAttributes = names.map((name) => ({ name: `a_${name}`, type: 'Float32', @@ -290,17 +346,43 @@ class CompositeExpressionBinder implements AttributeBinder, UniformBinder { this.paintVertexArray = new PaintVertexArray(); } + _evaluateFillColorFallback(zoom: number, feature: Feature, featureState: FeatureState, options: PaintOptions, canonical?: CanonicalTileID, formattedSection?: FormattedSection): Color | null | undefined { + if (!this.fillColorFallback) return null; + if (this.fillColorFallback.kind === 'constant') { + return this.fillColorFallback.value; + } + return this.fillColorFallback.evaluate(new EvaluationParameters(zoom, options), feature, featureState, canonical, [], formattedSection); + } + + _evaluateValue(zoom: number, feature: Feature, featureState: FeatureState, options: PaintOptions, canonical?: CanonicalTileID, formattedSection?: FormattedSection) { + if (this.type !== 'color' || !this.fillColorFallback) { + return this.expression.evaluate(new EvaluationParameters(zoom, options), feature, featureState, canonical, [], formattedSection); + } + + try { + const value = this.expression.evaluateWithoutErrorHandling(new EvaluationParameters(zoom, options), feature, featureState, canonical, [], formattedSection); + return value == null ? this._evaluateFillColorFallback(zoom, feature, featureState, options, canonical, formattedSection) : value; + } catch (error) { + if (isNullColorEvaluationError(error)) { + return this._evaluateFillColorFallback(zoom, feature, featureState, options, canonical, formattedSection); + } + + const value = this.expression.evaluate(new EvaluationParameters(zoom, options), feature, featureState, canonical, [], formattedSection); + return value == null ? this._evaluateFillColorFallback(zoom, feature, featureState, options, canonical, formattedSection) : value; + } + } + populatePaintArray(newLength: number, feature: Feature, options: PaintOptions) { - const min = this.expression.evaluate(new EvaluationParameters(this.zoom, options), feature, {}, options.canonical, [], options.formattedSection); - const max = this.expression.evaluate(new EvaluationParameters(this.zoom + 1, options), feature, {}, options.canonical, [], options.formattedSection); + const min = this._evaluateValue(this.zoom, feature, {}, options, options.canonical, options.formattedSection); + const max = this._evaluateValue(this.zoom + 1, feature, {}, options, options.canonical, options.formattedSection); const start = this.paintVertexArray.length; this.paintVertexArray.resize(newLength); this._setPaintValue(start, newLength, min, max); } updatePaintArray(start: number, end: number, feature: Feature, featureState: FeatureState, options: PaintOptions) { - const min = this.expression.evaluate(new EvaluationParameters(this.zoom, options), feature, featureState); - const max = this.expression.evaluate(new EvaluationParameters(this.zoom + 1, options), feature, featureState); + const min = this._evaluateValue(this.zoom, feature, featureState, options); + const max = this._evaluateValue(this.zoom + 1, feature, featureState, options); this._setPaintValue(start, end, min, max); } @@ -498,7 +580,10 @@ export class ProgramConfiguration { for (const property in layer.paint._values) { if (!filterProperties(property)) continue; const value = (layer.paint as any).get(property); - if (!(value instanceof PossiblyEvaluatedPropertyValue) || !supportsPropertyExpression(value.property.specification)) { + if ( + !(value instanceof PossiblyEvaluatedPropertyValue) || + !supportsPropertyExpression(value.property.specification) + ) { continue; } const names = paintAttributeNames(property, layer.type); @@ -507,25 +592,24 @@ export class ProgramConfiguration { const useIntegerZoom = (value.property as any).useIntegerZoom; const propType = value.property.specification['property-type']; const isCrossFaded = propType === 'cross-faded' || propType === 'cross-faded-data-driven'; + const fillColorFallback = getFillColorFallback(layer, property); if (expression.kind === 'constant') { this.binders[property] = isCrossFaded ? new CrossFadedConstantBinder(expression.value, names) : new ConstantBinder(expression.value, names, type); keys.push(`/u_${property}`); - } else if (expression.kind === 'source' || isCrossFaded) { const StructArrayLayout = layoutType(property, type, 'source'); this.binders[property] = isCrossFaded ? property === 'line-dasharray' ? new CrossFadedDasharrayBinder(expression as CompositeExpression, type, useIntegerZoom, zoom, StructArrayLayout, layer.id) : new CrossFadedPatternBinder(expression as CompositeExpression, type, useIntegerZoom, zoom, StructArrayLayout, layer.id) : - new SourceExpressionBinder(expression as SourceExpression, names, type, StructArrayLayout); + new SourceExpressionBinder(expression as SourceExpressionWithRawEvaluation, names, type, zoom, fillColorFallback, StructArrayLayout); keys.push(`/a_${property}`); - } else { const StructArrayLayout = layoutType(property, type, 'composite'); - this.binders[property] = new CompositeExpressionBinder(expression, names, type, useIntegerZoom, zoom, StructArrayLayout); + this.binders[property] = new CompositeExpressionBinder(expression as CompositeExpressionWithRawEvaluation, names, type, useIntegerZoom, zoom, fillColorFallback, StructArrayLayout); keys.push(`/z_${property}`); } } @@ -581,6 +665,9 @@ export class ProgramConfiguration { binder instanceof CrossFadedBinder) && binder.expression.isStateDependent === true) { //AHM: Remove after https://github.com/mapbox/mapbox-gl-js/issues/6255 const value = (layer.paint as any).get(property); + if (binder instanceof SourceExpressionBinder || binder instanceof CompositeExpressionBinder) { + binder.fillColorFallback = getFillColorFallback(layer, property); + } binder.expression = value.value; binder.updatePaintArray(pos.start, pos.end, feature, featureStates[id], options); dirty = true; @@ -811,7 +898,7 @@ function layoutType(property: string, type: string, binderType: string) { }; const layoutException = getLayoutException(property); - return layoutException?.[binderType] || defaultLayouts[type][binderType]; + return layoutException?.[binderType] || defaultLayouts[type][binderType]; } register('ConstantBinder', ConstantBinder); diff --git a/test/build/bundle_size.json b/test/build/bundle_size.json index 84a0725b1c..287ecffc0d 100644 --- a/test/build/bundle_size.json +++ b/test/build/bundle_size.json @@ -1 +1 @@ -1053826 +1055397