feat(core): js-parity global-state expression support - #4516
Conversation
There was a problem hiding this comment.
Looked through it, looks quite good!
Let's port the render tests from MapLiibre GL JS. https://github.com/maplibre/maplibre-gl-js/tree/main/test/integration/render/tests/global-state
|
@louwers thank you!
|
Bloaty Results 🐋Compared to main Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-4516-compared-to-main.txtCompared to d387090 (legacy) Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-4516-compared-to-legacy.txt |
|
Benchmark Results ⚡ Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/benchmark-results/pr-4516-compared-to-main.txt |
Bloaty Results (iOS) 🐋Compared to main Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-4516-compared-to-main.txt |
|
Nice! |
|
It looks like CI was failing because I had copied v8.json directly from @maplibre/maplibre-gl-style-spec. I’ve now made a few Native-specific adjustments to it. |
|
I'm truly sorry that this turned into such a huge PR... Based on the comments below, I have added tests and confirmed that everything is working as expected. |
9c81af9 to
fc32f1f
Compare
johncarmack1984
left a comment
There was a problem hiding this comment.
Saw Bart asking for reviews on this in the newsletter thread, thought I'd pitch in.
Read the core and platform halves against GL JS's implementation (style.ts, the spec's global_state.ts and visibility.ts). The semantics line up as far as I can trace. Null when unset, defaults from state, null resets to the default, re-evaluation scoped to the keys that changed. Filters in the query paths, format overrides and the color ramps too.
For anyone looking at the two red checks: those are the Android render-test legs from a workflow_run on main at be3f03b, which failed once and passed on the rerun. Every run on this PR's own head (fc32f1f) is green.
Looked into @HarelM's note about porting the rest of the GL JS global-state tests: 59 of the 69 are in here. The other 10 are sky, projection, and the two *-layer-opacity properties, none of which exist in native yet. Should stubs or // TODO comments go in to keep parity, or something else?
Implements the maplibre-gl-js global-state feature (maplibre#3302)
for more information, see https://pre-commit.ci
This reverts commit 04556ef.
fc32f1f to
b8969dd
Compare
|
@johncarmack1984 Thank you for your review!
I rebased upstream/main, and all 50 checks passed.
I think it is fine to exclude these for now. I could not find any precedents in the Native codebase where stubs or TODO comments were added for Mapbox GL JS rendering test features that are not yet supported on the Native side. so I believe it would be better to port them together when implementing those features themselves. |
johncarmack1984
left a comment
There was a problem hiding this comment.
Read the four new commits against the threads. Submodule is back on main's commit. Tile::setLayers carries the state now, and setGlobalState went with it. The query paths hand the state over next to the options instead of on the public structs. The pre-load call throws before the style is marked mutated, and isLoaded() is in the changelog. Range-diff of the rebased series against what I read last week is all =, so nothing else moved. All 49 checks green on the head. Good from me (with the caveat that I am just a citizen-contributor, not a maintainer!). Hope this was helpful! :)
| - (void)setGlobalStateValue:(id)value forProperty:(NSString *)propertyName { | ||
| MLNLogDebug(@"Setting global state property: %@", propertyName); | ||
| self.rawStyle->setGlobalStateProperty(propertyName.UTF8String, MLNValueFromJSONObject(value)); | ||
| } |
There was a problem hiding this comment.
One iOS side effect of the new throw: MLNMapSnapshotter.style hands out an MLNStyle before the style has loaded, and this call goes straight into core. A pre-load call used to drop the value; now it terminates the process, since nothing on the ObjC side catches it. The sibling methods in this file wrap the core call in try/catch and raise an NSException. Same wrapper here would match the Kotlin IllegalStateException. Not blocking.
There was a problem hiding this comment.
Thank you for your review! your review is very helpful to me!
I was satisfied after fixing the Android side... I have now also fixed the iOS side to throw an NSInternalInconsistencyException.
I also wanted to review the globe view PR in appreciation of your review, but it turned out to be quite difficult... ;_;
There was a problem hiding this comment.
haha yeah that one is HUGE, but thanks for taking a crack at it :)

resolve #3302
Summary
global-stateexpressionsstyle::Style::isLoadedso platform bindings can distinguish this point from full resource loadingVerification
cmake --build build-macos-metal --target mbgl-test-runnerand the style/expression suites, including thenew GlobalStateExpression.*,Style.GlobalState*,Style.Visibility*, andLayer.SymbolLayerOverridestests:MapLibreAndroid:testOpenglDebugUnitTestplus instrumentation tests on an emulator (RuntimeStyleTests, MapSnapshotterTest), arm64 native buildbazel test //platform/ios/test:ios_test --//:renderer=metal(MLNStyleTests, MLNExpressionTests)Note
The code in this PR was written with the help of Claude (Fable 5), under my instruction and review.