fix: accept single-color expressions for colorArray properties#4297
Draft
dts wants to merge 2 commits into
Draft
fix: accept single-color expressions for colorArray properties#4297dts wants to merge 2 commits into
dts wants to merge 2 commits into
Conversation
PR maplibre#3965 promoted hillshade-shadow-color and hillshade-highlight-color from PaintProperty<Color> to PaintProperty<std::vector<Color>> to support multidirectional hillshade. The style spec types these properties as colorArray with single-color defaults (#000000 / #FFFFFF), and maplibre-gl-js continues to accept both color and array<color> expression outputs for them. Before this change, parsing any interpolate expression that outputs a single color against PropertyValue<std::vector<Color>> fails at style-parse time with: Expected array<color> but found string instead. This breaks virtually every published topographic basemap that animates hillshade color over zoom (e.g. LINZ Basemaps topographic style), because those styles use the canonical interpolate(zoom) -> color shape. The whole style fails to parse, producing a blank/gray map on iOS, Android, and maplibre-react-native >= 6.24.0. The runtime ValueConverter<std::vector<Color>>::fromExpressionValue in hillshade_conversions.cpp already wraps a single Color into a one-element vector at evaluation time. The only barrier was the compile-time expected expression output, which valueTypeToExpressionType<std::vector<Color>>() unconditionally pins to array<color>. This patch specializes the expression-parse branch for std::vector<Color>: try parsing as color first (the spec-default single-color shape, which is what gl-js itself accepts), and fall back to array<color> for explicit multidirectional users. Test plan: ./mbgl-test-runner --gtest_filter=StyleConversion.PropertyValue* - PropertyValueVectorColor_SingleColorInterpolate (the regression case) - PropertyValueVectorColor_MultidirectionalConstant - PropertyValueVectorColor_PlainColorConstant - PropertyValue (unchanged) All four pass. The broader 23 StyleConversion tests and 94 Expression and Hillshade tests also continue to pass. Repro: https://gitlab.com/dts1/maplibre-native-hillshade-repro
github-actions
Bot
added
the
core
Member
|
Please make sure to follow our AI Policy https://github.com/maplibre/maplibre/blob/main/AI_POLICY.md |
Contributor
Author
|
Shoot, I didn't look at that before, thanks for the heads-up - how would you like me to proceed? Rewrite the PR description and include an AI disclosure? I'm pretty confident in the solution, but it is AI-generated. I validated it as best I could (testing on physical devices, simulators, etc). |
Member
|
@dts No worries! It's pretty new and only mentioned in the contributor guidelines.
Then, once you're confident. Mark the PR as ready for review. Then, a maintainer will have a look. And here be prepared to answer questions and/or follow up requests. Does that sound reasonable? |
Contributor
Bloaty Results 🐋Compared to main Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-4297-compared-to-main.txtCompared to d387090 (legacy) Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-4297-compared-to-legacy.txt |
Contributor
|
Benchmark Results ⚡ Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-4297-compared-to-main.txt |
Contributor
Bloaty Results (iOS) 🐋Compared to main Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-4297-compared-to-main.txt |
PR maplibre#3965 also promoted hillshade-illumination-direction and hillshade-illumination-altitude from PaintProperty<float> to PaintProperty<std::vector<float>>. The style spec types these as numberArray with single-number defaults (335 and 45). The same parse-time mismatch that broke colorArray properties also rejects spec-valid single-number interpolate expressions for these: Expected array<number> but found number instead. Unlike the colorArray case, maplibre#3965 did not add the runtime coercion either. The primary template ValueConverter<std::vector<T>>::fromExpressionValue only accepts array-shaped Values; a scalar Value falls through and returns nullopt. So the property holder silently substitutes the spec default at every frame -- strictly worse than the parse-time error. This commit closes both halves of the gap for std::vector<float>, mirroring the colorArray fix from the previous commit: 1. src/mbgl/style/conversion/property_value.cpp Generalizes the if constexpr branch to also match std::vector<float>. Element type derived via T::value_type, scalar/array context types via valueTypeToExpressionType<Element>(). 2. src/mbgl/style/expression/value.cpp Adds the missing ValueConverter<std::vector<float>>::fromExpressionValue explicit specialization. Mirrors the existing std::vector<Color> one in hillshade_conversions.cpp: array branch identical to the primary template, plus a single-number branch that wraps into a one-element vector. Test plan: ./mbgl-test-runner --gtest_filter=StyleConversion.PropertyValue* - PropertyValueVectorFloat_SingleNumberInterpolate (regression) - PropertyValueVectorFloat_MultidirectionalConstant (no regression) - PropertyValueVectorFloat_SingleNumberInterpolate_Evaluates(eval smoke) - PropertyValueVectorColor_SingleColorInterpolate_Evaluates (eval smoke, added here to cover the prior commit's claim) All 7 PropertyValue* tests pass. Broader 75 in StyleConversion.*, Style*.*, StyleParser/*, Expression* also continue to pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a fix for issue #4296. There's an overly restrictive validation step that is preventing animated hillshade layers from rendering (e.g. animating opacity during a zoom between levels).
AI Disclosure: I used AI to create the minimal reproduction & the fix here, I am confident it fixes the underlying issue (I've tested it on my personal device).
Repro of bug: https://gitlab.com/dts1/maplibre-native-hillshade-repro