Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Implement image expression #15877

Merged
merged 10 commits into from
Nov 11, 2019
Merged

[core] Implement image expression #15877

merged 10 commits into from
Nov 11, 2019

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented Nov 1, 2019 •
edited
Loading

Backport of mapbox/mapbox-gl-js#8684

The image expression allows determining an image's availability. Comparing to a simple string, it adds semantic information, for instance, in case of formatted text, now it is possible to make distinction between image and text sections ["format", "Text", ["image", "illustration"]]

In this PR image expression is used in places where strings were used to represent images (*-pattern, icon-image), however, in follow-up PR's image expression will be used in format expression in order to render images in labels.

mapbox/mapbox-gl-js#8604
mapbox/mapbox-gl-js#8904

Fixes: #15800

@alexshalamov
Copy link
Contributor Author

alexshalamov commented Nov 1, 2019 •
edited
Loading

@tobrun @julianrex @tmpsantos What is our strategy for platform bindings? I can't implement and land core part without writing bindings for all platforms 😞

Update: I added basic integration bits for Android and Darwin platforms, image expression bindings need to be exposed in follow-up PRs.

@alexshalamov alexshalamov force-pushed the alexshalamov_image_expression branch 6 times, most recently from 60bcbec to cad7d0f Compare November 5, 2019 21:23
@tobrun tobrun mentioned this pull request Nov 6, 2019
Closed
@alexshalamov alexshalamov self-assigned this Nov 7, 2019
@alexshalamov alexshalamov added the needs changelog Indicates PR needs a changelog entry prior to merging. label Nov 7, 2019
@alexshalamov alexshalamov marked this pull request as ready for review November 7, 2019 14:27
@alexshalamov
Copy link
Contributor Author

@julianrex @tobrun @springmeyer Could you please take a look at [darwin], [android] and [node] commits.

tobrun
tobrun reviewed Nov 7, 2019
View reviewed changes
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Android code changes look good

platform/android/scripts/generate-style-code.js Outdated Show resolved Hide resolved
@ryanhamley
Copy link
Contributor

Thanks so much for taking this on @alexshalamov It looks great

pozdnyakov
pozdnyakov reviewed Nov 8, 2019
View reviewed changes
include/mbgl/style/expression/formatted.hpp Outdated Show resolved Hide resolved
include/mbgl/style/expression/expression.hpp Outdated Show resolved Hide resolved
include/mbgl/style/expression/image.hpp Show resolved Hide resolved
include/mbgl/style/expression/image.hpp Show resolved Hide resolved
include/mbgl/style/expression/image.hpp Outdated Show resolved Hide resolved
include/mbgl/style/expression/type.hpp Outdated Show resolved Hide resolved
src/mbgl/renderer/image_manager.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/is_constant.cpp Show resolved Hide resolved
@alexshalamov alexshalamov force-pushed the alexshalamov_image_expression branch 3 times, most recently from c9bf76e to c764daf Compare November 8, 2019 13:38
pozdnyakov
pozdnyakov approved these changes Nov 8, 2019
View reviewed changes
Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

LGTM % nits

test/style/property_expression.test.cpp Outdated Show resolved Hide resolved
test/style/property_expression.test.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/coercion.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/is_constant.cpp Show resolved Hide resolved
src/mbgl/style/layers/symbol_layer_properties.hpp Outdated Show resolved Hide resolved
@alexshalamov alexshalamov force-pushed the alexshalamov_image_expression branch 2 times, most recently from 012802d to dd70cb3 Compare November 11, 2019 09:18
@alexshalamov alexshalamov removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Nov 11, 2019
@alexshalamov
[android] Update image expression evaluated type name
eb2ef8c
@alexshalamov
[android] Placeholders for conversion from:to Image
9b3a9bf
@alexshalamov
[darwin] Update image expression evaluated type name
38c84cf
@alexshalamov
[darwin] Add conversion for resolvedImage
3cd1b3f
@alexshalamov
[darwin] Fix image roundrip conversion
bb3c2ae
@alexshalamov
[node] Update changelog
7d362de
@alexshalamov
[android] Update changelog
b15c6d0
@alexshalamov
[darwin] Update changelog
d0f3bca
@alexshalamov
[core] Implement image expression (#15877)
b9aa337 
* [core] Bump gl-js version
* [core] Implement image expression
* [core] Use new image expression
* [core] Coerce image expression to / from string
* [core] Serialize evaluated image
* [core] Pass available images to layout
* [core] Pass images to evaluation context
* [core] Set available flag value based on image availability
* [core] Allow image coercion to boolean to indicate image availability
* [core] Coalesce image expression
* [core] Add image expression to next build system
* [core] Align serialization format and evaluated type with gl-js
* [core] Add images to expression evaluation method
* [core] Add support for Image expression to expression test runner
* [core] Unskip image expression tests
* [core] Update unit tests
* [core] Use image expression in annotation manager
* [core] Add string to ImageExpression conversion
* [core] Add image expression to expression dsl
* [core] Convert tokens for implicitly created Image literal
* [core] Fix clang format
* [core] Split generated style code lines that are over 120 characters
* [core] Add unit test for image expression equality
* [core] Add image property expression evaluation unit test
* [core] Unskip image expression render test
* [core] Skip 'in' expression tests
* [core] Ignore fill-pattern/update-feature-state render test
* [core] Rename Image::serialize to Image::toValue
@alexshalamov
[node] Add support for Image expression
9924808
@alexshalamov alexshalamov merged commit bcbaa35 into master Nov 11, 2019
@alexshalamov alexshalamov deleted the alexshalamov_image_expression branch November 11, 2019 16:20
alexshalamov added a commit that referenced this pull request Nov 11, 2019
@alexshalamov
[core] Implement image expression (#15877)
c6f3cc8 
* [core] Bump gl-js version
* [core] Implement image expression
* [core] Use new image expression
* [core] Coerce image expression to / from string
* [core] Serialize evaluated image
* [core] Pass available images to layout
* [core] Pass images to evaluation context
* [core] Set available flag value based on image availability
* [core] Allow image coercion to boolean to indicate image availability
* [core] Coalesce image expression
* [core] Add image expression to next build system
* [core] Align serialization format and evaluated type with gl-js
* [core] Add images to expression evaluation method
* [core] Add support for Image expression to expression test runner
* [core] Unskip image expression tests
* [core] Update unit tests
* [core] Use image expression in annotation manager
* [core] Add string to ImageExpression conversion
* [core] Add image expression to expression dsl
* [core] Convert tokens for implicitly created Image literal
* [core] Fix clang format
* [core] Split generated style code lines that are over 120 characters
* [core] Add unit test for image expression equality
* [core] Add image property expression evaluation unit test
* [core] Unskip image expression render test
* [core] Skip 'in' expression tests
* [core] Ignore fill-pattern/update-feature-state render test
* [core] Rename Image::serialize to Image::toValue
@springmeyer
Copy link
Contributor

@julianrex @tobrun @springmeyer Could you please take a look at [darwin], [android] and [node] commits.

It looks like the [node] changes relate only to the expression API exposed in node and don't impact the node.js rendering logic at all. Is that right? I'm not familiar with the expression API code, so I'm not in a good position to provide a review. Perhaps someone else on @mapbox/gl-core knows what platform/node/src/node_expression.cpp is used for? Testing perhaps (before we moved to more C++ tests)?

@alexshalamov
Copy link
Contributor Author

@springmeyer platform/node/src/node_expression.cpp was used for expression tests and also can be used to set / get expressions for layer properties at runtime, not sure if anyone is using node expression bindings for runtime styling.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tobrun tobrun tobrun left review comments

@pozdnyakov pozdnyakov pozdnyakov approved these changes

@1ec5 1ec5 Awaiting requested review from 1ec5

@tmpsantos tmpsantos Awaiting requested review from tmpsantos

@julianrex julianrex Awaiting requested review from julianrex

@springmeyer springmeyer Awaiting requested review from springmeyer

Assignees

@alexshalamov alexshalamov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Port Image Expressions
5 participants
@alexshalamov @ryanhamley @springmeyer @pozdnyakov @tobrun