Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions packages/map/src/layer-sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ import { prepareFillPattern } from "./fill-patterns";
import { prepareLineDecoration } from "./line-decorations";
import {
KML_ICON_URL_PROPERTY,
markerImageValue,
markerIconSizeValue,
prepareKmlFeatureIcons,
prepareMarker,
} from "./markers";
import { isPlaceholderLayer } from "./placeholders";
import {
Expand Down Expand Up @@ -1636,9 +1636,9 @@ function syncVectorControlPointSymbology(

const circleSpec = getStyleLayerSpec(map, circleNativeId);
ensureGeneratedImageHandler(map);
const markerImageId = prepareMarker(layer.style);
const markerImage = markerImageValue(layer.style);

if (markerImageId && circleSpec) {
if (markerImage && circleSpec) {
// Reuse the control's own base filter (the tracked base when Time-Slider /
// rule extras are active, so they never nest) combined with the current
// extras, mirroring applyExternalNativeFeatureFilters.
Expand All @@ -1662,7 +1662,7 @@ function syncVectorControlPointSymbology(
// on the update path (ensureLayer only diffs keys that exist).
filter: filter ?? undefined,
layout: {
"icon-image": markerImageId,
"icon-image": markerImage as PropertyValueSpecification<string>,
"icon-size": markerIconSizeValue(layer.style) as PropertyValueSpecification<number>,
"icon-allow-overlap": true,
"icon-ignore-placement": true,
Expand Down Expand Up @@ -1920,8 +1920,10 @@ function applyVectorDataRenderLayers(
// a generated image id.
ensureGeneratedImageHandler(map);
const fillPatternId = prepareFillPattern(layer.style);
const markerImageId = prepareMarker(layer.style);
const kmlIconImage = prepareKmlFeatureIcons(layer.geojson!, markerImageId ?? "");
// markerImageValue resolves the same base marker internally, so it is null
// exactly when no marker applies — no separate prepareMarker call is needed.
const markerImage = markerImageValue(layer.style);
const kmlIconImage = prepareKmlFeatureIcons(layer.geojson!, markerImage ?? "");
// Derived companion symbology (inverted mask, geometry generator, dedup
// labels) is built from the raw features, so no MapLibre filter applies to
// it. While a Time Slider window or a rule-based visibility filter is
Expand Down Expand Up @@ -2213,7 +2215,7 @@ function applyVectorDataRenderLayers(
layer,
hasTextMarkers ? nonTextMarkerPointFilter : pointGeometryFilter,
);
if (markerImageId || kmlIconImage) {
if (markerImage || kmlIconImage) {
removeIfExists(map, circleLayerId(layer.id));
ensureLayer(
map,
Expand All @@ -2225,7 +2227,7 @@ function applyVectorDataRenderLayers(
...styleLayerZoomRange(layer.style),
filter: pointFilter,
layout: {
"icon-image": (kmlIconImage ?? markerImageId) as string,
"icon-image": (kmlIconImage ?? markerImage) as PropertyValueSpecification<string>,
// The sprite is baked at its display size, so icon-size stays 1
// unless proportional sizing scales it per feature.
"icon-size": (kmlIconImage
Expand All @@ -2239,7 +2241,7 @@ function applyVectorDataRenderLayers(
},
beforeId,
);
if (kmlIconImage && !markerImageId) {
if (kmlIconImage && !markerImage) {
// Features without a KML icon still use the ordinary circle renderer.
ensureLayer(
map,
Expand Down
103 changes: 96 additions & 7 deletions packages/map/src/markers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
normalizeHexColor,
proportionalSizeRange,
styleValue,
vectorColorExpression,
type LayerStyle,
type MarkerShape,
} from "@geolibre/core";
Expand All @@ -18,7 +19,11 @@ 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>>();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// The expression heads whose outputs markerImageValue rewrites into sprite ids.
const COLOR_BRANCH_HEADS: ReadonlySet<string> = new Set(["match", "step", "case"]);

const BUILTIN_SHAPES: ReadonlySet<MarkerShape> = new Set([
"circle",
Expand Down Expand Up @@ -88,8 +93,53 @@ 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 {
// Do not keep a failed fetch cached: a transient network error would
// otherwise block every later color variant of the same source (and any
// styleimagemissing retry) until the entry is evicted. Dropping it only
// after the await still lets concurrent callers share the in-flight
// promise.
if (svgSourceCache.get(markup) === pending) svgSourceCache.delete(markup);
// 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));
}
Comment thread
giswqs marked this conversation as resolved.
Comment thread
giswqs marked this conversation as resolved.
Comment on lines +105 to +135

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security note (low confidence): this is the first place in markers.ts that reads a remote/data-URL SVG marker's text into JS (previously loadSvgMarker/loadMarkerSvgImage only ever set it as an <img src>, so the response body itself was never exposed to script). markerSvg is free-text project data — a shared/imported .geolibre.json (or a "expression"-mode style) can already point it at an arbitrary URL, and this fetch() runs automatically whenever the marker sprite is generated, with no user interaction beyond opening the project.

The existing console.warn in resolveSvgSource (packages/core/src/marker-shape.ts) already accepts that a remote markerSvg triggers a cross-origin request, so this may be an accepted extension of that trust boundary rather than a new one — flagging mainly because reading the response text (vs. only rendering it as an image) is a meaningfully larger capability than what existed before, worth a second look given untrusted projects are a supported flow (collaboration / imported style files).


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;
Expand Down Expand Up @@ -193,28 +243,67 @@ 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 imageFor = (value: unknown): unknown => {
if (typeof value === "string") {
// Bake the canonical form: prepareMarker uses the color verbatim for both
// the sprite id and the fill, so a bare or shorthand hex ("fff") from a
// hand-authored expression would otherwise draw black, and "#FDE725"
// would bake a second sprite for a color already registered lowercase.
const normalized = normalizeHexColor(value);
return normalized ? (prepareMarker(style, normalized) ?? baseId) : baseId;
}
if (!Array.isArray(value)) return baseId;

const expression = [...value];
const firstOutput = expression[0] === "match" ? 3 : 2;
if (!COLOR_BRANCH_HEADS.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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quality / completeness (medium confidence): imageFor only recolors a branch when its output is a literal hex string (or a nested match/step/case). In "expression" mode (free-form JSON typed by the user), a color output that is a CSS named color ("red", "steelblue"), an rgb()/rgba()/hsl() string, or a nested color-producing sub-expression (["to-color", …], ["rgb", …]) fails normalizeHexColor/COLOR_BRANCH_HEADS and silently collapses to the single fallback marker color for that whole branch — with no warning surfaced to the user.

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 vectorFillColorValue/vectorLineColorValue, which pass the raw expression straight to MapLibre) while the marker icon quietly reverts to one flat color. Worth either a one-line note in the markerImageValue JSDoc calling this out as a known limitation, or (as a follow-up) resolving arbitrary CSS colors to hex via a throwaway canvas ctx.fillStyle round-trip before falling back.

// A flat resolved color still goes through imageFor: rule-based mode with no
// drawable rules returns the else rule's color, which need not equal the
// layer's markerColor that baseId was baked from.
return imageFor(vectorColorExpression(style, fallback)) as string | unknown[];
}
Comment thread
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) => {
Expand All @@ -232,7 +321,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>();
Expand Down
Loading
Loading