Fix fill-outline-color fallback for feature-state expressions#7420
Fix fill-outline-color fallback for feature-state expressions#7420itisyb wants to merge 3 commits intomaplibre:mainfrom
Conversation
Keep fill-outline-color aligned with fill-color when feature-state is missing so fill layers do not break during initial render or later state updates.
|
Also created a small demo but don't think need to push/show for this case |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7420 +/- ##
==========================================
- Coverage 92.86% 92.58% -0.29%
==========================================
Files 288 289 +1
Lines 23998 24060 +62
Branches 5095 5118 +23
==========================================
- Hits 22286 22276 -10
- Misses 1712 1784 +72 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for taking the time to open this PR. |
Before the fix, there was a problem with how fill-outline-color was handled when using feature-state. If the feature-state value was missing, the expression could return null. The rendering system didn’t treat that as “no value” - it treated it like an actual color. Then when packColor() tried to use it, it attempted to access color.r on null, which caused a runtime error: “Cannot read properties of null (reading 'r')”. it already had a fallback rule where fill-outline-color defaults to fill-color if not specified. But that logic only applied in the constant (non–data-driven) case. In the feature-state (data-driven) path, that fallback wasn’t applied, so instead of defaulting correctly, it crashed. |
| paintVertexBuffer: VertexBuffer; | ||
|
|
||
| constructor(expression: SourceExpression, names: string[], type: string, PaintVertexArray: { | ||
| constructor(expression: SourceExpressionWithRawEvaluation, names: string[], type: string, zoom: number, fillColorFallback: FillColorFallback, PaintVertexArray: { |
There was a problem hiding this comment.
This looks too specific to fill color, are you sure this is the right place to add this code? I would expect to see this code in the fill bucket/fill program or something related to fill, and not in this "shared" program configuration code.
There was a problem hiding this comment.
Seems like this is still relevant after the recent force-push...
6fe9302 to
f32d290
Compare
src/data/program_configuration.ts
Outdated
| type CompositeExpressionWithRawEvaluation = ZoomDependentExpression<'composite'>; | ||
|
|
||
| function getFillColorFallback(layer: TypedStyleLayer, property: string): FillColorFallback { | ||
| return property === 'fill-outline-color' ? (layer.paint as any).get('fill-color').value : null; |
There was a problem hiding this comment.
Is there a way to avoid this as any?
Summary
fill-outline-colorfallback tofill-colorwhenfeature-stateis missing so fill rendering does not break during initial paint populationfill-colorchanges stay in syncTesting
npm run lint -- src/data/program_configuration.ts src/data/program_configuration.test.tsnpm run test-unit -- src/data/program_configuration.test.tsNotes
npm run build-devis currently blocked by unrelated existing TypeScript errors insrc/data/bucket/line_bucket.tsandsrc/source/geojson_*, so this PR keeps the regression coverage focused on unit tests.Fixes #4234