-
-
Notifications
You must be signed in to change notification settings - Fork 570
Fix categorized styling for marker icons #1782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ import { | |
| normalizeHexColor, | ||
| proportionalSizeRange, | ||
| styleValue, | ||
| vectorColorExpression, | ||
| type LayerStyle, | ||
| type MarkerShape, | ||
| } from "@geolibre/core"; | ||
|
|
@@ -18,7 +19,9 @@ const MARKER_PIXEL_RATIO = 2; | |
| // enormous canvas; the rendered size is set via the marker image's own pixels. | ||
| const MIN_MARKER_SIZE = 6; | ||
| const MAX_MARKER_SIZE = 96; | ||
| const MAX_SVG_SOURCE_CACHE = 64; | ||
| export const KML_ICON_URL_PROPERTY = "__geolibre_kml_icon_url"; | ||
| const svgSourceCache = new Map<string, Promise<string | null>>(); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| const BUILTIN_SHAPES: ReadonlySet<MarkerShape> = new Set([ | ||
| "circle", | ||
|
|
@@ -88,8 +91,47 @@ export function loadMarkerSvgImage(markup: string): Promise<HTMLImageElement | n | |
| }); | ||
| } | ||
|
|
||
| function loadSvgMarker(markup: string, size: number): Promise<GeneratedImageResult | null> { | ||
| const src = resolveSvgSource(markup); | ||
| function replaceSvgColorParameters(markup: string, color: string): string { | ||
| return markup | ||
| .replace(/param\(fill\)/gi, color) | ||
| .replace(/param\(fill-opacity\)/gi, "1") | ||
| .replace(/param\(outline\)/gi, color) | ||
| .replace(/param\(outline-opacity\)/gi, "1") | ||
| .replace(/param\(outline-width\)/gi, "0"); | ||
| } | ||
|
|
||
| async function colorizedSvgSource(markup: string, color: string): Promise<string | null> { | ||
| let sourceMarkup = markup; | ||
| if (/^(?:https?:|data:image\/svg\+xml)/i.test(markup)) { | ||
| let pending = svgSourceCache.get(markup); | ||
| if (!pending) { | ||
| pending = fetch(markup) | ||
| .then((response) => (response.ok ? response.text() : null)) | ||
| .catch(() => null); | ||
| if (svgSourceCache.size >= MAX_SVG_SOURCE_CACHE) { | ||
| const oldest = svgSourceCache.keys().next().value; | ||
| if (oldest !== undefined) svgSourceCache.delete(oldest); | ||
| } | ||
| svgSourceCache.set(markup, pending); | ||
| } | ||
| const fetched = await pending; | ||
| if (fetched !== null) { | ||
| sourceMarkup = fetched; | ||
| } else { | ||
| // Preserve the original source when a remote host blocks CORS. The | ||
| // marker still renders, although its QGIS color parameters cannot be | ||
| // resolved without access to the SVG text. | ||
| } | ||
| } | ||
| return resolveSvgSource(replaceSvgColorParameters(sourceMarkup, color)); | ||
| } | ||
|
giswqs marked this conversation as resolved.
giswqs marked this conversation as resolved.
Comment on lines
+105
to
+135
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security note (low confidence): this is the first place in The existing |
||
|
|
||
| async function loadSvgMarker( | ||
| markup: string, | ||
| color: string, | ||
| size: number, | ||
| ): Promise<GeneratedImageResult | null> { | ||
| const src = await colorizedSvgSource(markup, color); | ||
| if (!src) return Promise.resolve(null); | ||
| const ratio = MARKER_PIXEL_RATIO; | ||
| const px = size * ratio; | ||
|
|
@@ -193,28 +235,62 @@ export function markerIconSizeValue(style: LayerStyle): number | unknown[] { | |
| * @param style - The layer style. | ||
| * @returns The image id, or `null` when no marker applies. | ||
| */ | ||
| export function prepareMarker(style: LayerStyle): string | null { | ||
| export function prepareMarker(style: LayerStyle, colorOverride?: string): string | null { | ||
| if (!styleValue(style, "markerEnabled")) return null; | ||
| const shape = styleValue(style, "markerShape"); | ||
| const size = markerBakedSize(style); | ||
|
|
||
| if (shape === "custom") { | ||
| const markup = styleValue(style, "markerSvg").trim(); | ||
| if (!markup) return null; | ||
| const id = `geolibre-marker-svg-${hashText(markup)}-${size}`; | ||
| const color = colorOverride ?? markerColor(style); | ||
| const id = `geolibre-marker-svg-${hashText(`${markup}\0${color}`)}-${size}`; | ||
| // Capture the markup in the factory closure so the lazy generator never | ||
| // depends on a separate, evictable cache (which could blank the marker). | ||
| registerGeneratedImage(id, () => loadSvgMarker(markup, size)); | ||
| registerGeneratedImage(id, () => loadSvgMarker(markup, color, size)); | ||
| return id; | ||
| } | ||
|
|
||
| if (!BUILTIN_SHAPES.has(shape)) return null; | ||
| const color = markerColor(style); | ||
| const color = colorOverride ?? markerColor(style); | ||
| const id = `geolibre-marker-${shape}-${color.replace("#", "")}-${size}`; | ||
| registerGeneratedImage(id, () => drawBuiltinMarker(shape, color, size)); | ||
| return id; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve a marker's `icon-image` layout value. Categorized, graduated, and | ||
| * rule-based color expressions select a separately baked sprite per class, | ||
| * because ordinary bitmap sprites cannot be tinted per feature by MapLibre. | ||
| */ | ||
| export function markerImageValue(style: LayerStyle): string | unknown[] | null { | ||
| const fallback = markerColor(style); | ||
| const baseId = prepareMarker(style, fallback); | ||
| if (!baseId) return null; | ||
|
|
||
| const colorValue = vectorColorExpression(style, fallback); | ||
| if (!Array.isArray(colorValue)) return baseId; | ||
|
giswqs marked this conversation as resolved.
Outdated
giswqs marked this conversation as resolved.
Outdated
|
||
|
|
||
| const imageFor = (value: unknown): unknown => { | ||
| if (typeof value === "string") { | ||
| return normalizeHexColor(value) ? (prepareMarker(style, value) ?? baseId) : baseId; | ||
|
giswqs marked this conversation as resolved.
Outdated
|
||
| } | ||
| if (!Array.isArray(value)) return baseId; | ||
|
|
||
| const expression = [...value]; | ||
| const firstOutput = expression[0] === "match" ? 3 : 2; | ||
| if (!new Set(["match", "step", "case"]).has(String(expression[0]))) return baseId; | ||
| for (let index = firstOutput; index < expression.length; index += 2) { | ||
| expression[index] = imageFor(expression[index]); | ||
| } | ||
| if (expression[0] !== "step") { | ||
| expression[expression.length - 1] = imageFor(expression[expression.length - 1]); | ||
| } | ||
| return expression; | ||
| }; | ||
|
Comment on lines
+279
to
+300
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quality / completeness (medium confidence): This is intentional per the "uses the base marker for invalid expression color outputs" test, but the mismatch could be confusing: the same expression correctly colors fill/line/circle layers per-feature (via |
||
| return imageFor(colorValue) as unknown[]; | ||
| } | ||
|
giswqs marked this conversation as resolved.
|
||
|
|
||
| function loadRasterMarker(url: string): Promise<GeneratedImageResult | null> { | ||
| if (!/^data:image\/(?!svg)[\w.+-]+;base64,/i.test(url)) return Promise.resolve(null); | ||
| return new Promise((resolve) => { | ||
|
|
@@ -232,7 +308,7 @@ function loadRasterMarker(url: string): Promise<GeneratedImageResult | null> { | |
| */ | ||
| export function prepareKmlFeatureIcons( | ||
| collection: GeoJSON.FeatureCollection, | ||
| fallbackImage = "", | ||
| fallbackImage: unknown = "", | ||
| ): unknown[] | null { | ||
| const matches: unknown[] = []; | ||
| const seen = new Set<string>(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| import assert from "node:assert/strict"; | ||
| import { describe, it } from "node:test"; | ||
| import { DEFAULT_LAYER_STYLE, type LayerStyle } from "@geolibre/core"; | ||
| import { | ||
| KML_ICON_URL_PROPERTY, | ||
| markerImageValue, | ||
| prepareKmlFeatureIcons, | ||
| } from "../packages/map/src/markers"; | ||
|
|
||
| function categorizedMarker(patch: Partial<LayerStyle> = {}): LayerStyle { | ||
| return { | ||
| ...DEFAULT_LAYER_STYLE, | ||
| markerEnabled: true, | ||
| markerShape: "circle", | ||
| markerColor: "#3b82f6", | ||
| markerSize: 18, | ||
| vectorStyleMode: "categorized", | ||
| vectorStyleProperty: "status", | ||
| vectorStyleStops: [ | ||
| { value: "good", color: "#339084" }, | ||
| { value: "bad", color: "#fde725" }, | ||
| ], | ||
| ...patch, | ||
| }; | ||
| } | ||
|
|
||
| describe("markerImageValue", () => { | ||
|
giswqs marked this conversation as resolved.
|
||
| it("selects a separately colored built-in marker for each category", () => { | ||
| assert.deepEqual(markerImageValue(categorizedMarker()), [ | ||
| "match", | ||
| ["to-string", ["get", "status"]], | ||
| "good", | ||
| "geolibre-marker-circle-339084-18", | ||
| "bad", | ||
| "geolibre-marker-circle-fde725-18", | ||
| "geolibre-marker-circle-3b82f6-18", | ||
| ]); | ||
| }); | ||
|
|
||
| it("creates distinct parameterized SVG sprites for category colors", () => { | ||
| const value = markerImageValue( | ||
| categorizedMarker({ | ||
| markerShape: "custom", | ||
| markerSvg: | ||
| '<svg xmlns="http://www.w3.org/2000/svg"><path fill="param(fill)" d="M0 0h10v10z"/></svg>', | ||
| }), | ||
| ); | ||
|
|
||
| assert.ok(Array.isArray(value)); | ||
| const imageIds = [value[3], value[5], value[6]]; | ||
| assert.ok( | ||
| imageIds.every((id) => typeof id === "string" && id.startsWith("geolibre-marker-svg-")), | ||
| ); | ||
| assert.equal(new Set(imageIds).size, 3); | ||
| }); | ||
|
|
||
| it("creates distinct category sprites when the SVG is supplied by URL", () => { | ||
| const value = markerImageValue( | ||
| categorizedMarker({ | ||
| markerShape: "custom", | ||
| markerSvg: "https://example.com/tree.svg", | ||
| }), | ||
| ); | ||
|
|
||
| assert.ok(Array.isArray(value)); | ||
| assert.equal(new Set([value[3], value[5], value[6]]).size, 3); | ||
| }); | ||
|
|
||
| it("uses the base marker for invalid expression color outputs", () => { | ||
| const value = markerImageValue( | ||
| categorizedMarker({ | ||
| vectorStyleMode: "expression", | ||
| vectorStyleExpression: '["match",["get","status"],"good","red","#fde725"]', | ||
| }), | ||
| ); | ||
|
|
||
| assert.ok(Array.isArray(value)); | ||
| assert.equal(value[3], "geolibre-marker-circle-3b82f6-18"); | ||
| assert.equal(value[4], "geolibre-marker-circle-fde725-18"); | ||
| }); | ||
|
|
||
| it("recursively converts colors in zoom-scoped rule expressions", () => { | ||
| const value = markerImageValue( | ||
| categorizedMarker({ | ||
| vectorStyleMode: "expression", | ||
| vectorStyleExpression: | ||
| '["step",["zoom"],["case",["get","selected"],"#339084","#fde725"],10,["case",["get","selected"],"#fde725","#339084"]]', | ||
| }), | ||
| ); | ||
|
|
||
| assert.deepEqual(value, [ | ||
| "step", | ||
| ["zoom"], | ||
| [ | ||
| "case", | ||
| ["get", "selected"], | ||
| "geolibre-marker-circle-339084-18", | ||
| "geolibre-marker-circle-fde725-18", | ||
| ], | ||
| 10, | ||
| [ | ||
| "case", | ||
| ["get", "selected"], | ||
| "geolibre-marker-circle-fde725-18", | ||
| "geolibre-marker-circle-339084-18", | ||
| ], | ||
| ]); | ||
| }); | ||
|
|
||
| it("keeps categorized marker fallback inside a mixed KML icon expression", () => { | ||
| const markerImage = markerImageValue(categorizedMarker()); | ||
| const value = prepareKmlFeatureIcons( | ||
| { | ||
| type: "FeatureCollection", | ||
| features: [ | ||
| { | ||
| type: "Feature", | ||
| geometry: { type: "Point", coordinates: [0, 0] }, | ||
| properties: { [KML_ICON_URL_PROPERTY]: "data:image/png;base64,AA==" }, | ||
| }, | ||
| { | ||
| type: "Feature", | ||
| geometry: { type: "Point", coordinates: [1, 1] }, | ||
| properties: { status: "good" }, | ||
| }, | ||
| ], | ||
| }, | ||
| markerImage, | ||
| ); | ||
|
|
||
| assert.ok(Array.isArray(value)); | ||
| assert.deepEqual(value[value.length - 1], markerImage); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.