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

Enable Within Expression with paint property + Filter Expression #16157

Merged
merged 7 commits into from
Feb 12, 2020

Conversation

zmiao
Copy link
Contributor

@zmiao zmiao commented Jan 30, 2020

This pr introduces a new expression type within. Currently the usage is: [within, geoJSONObj]. It returns true if the feature is inside the geometry boundary that defined by geoJSONObj. geoJSONObj should be inlined.
This pr also enables the usages of setting paint property and filter expression with within expression. Following example shows how to set different color based on the feature's geometry location.

{
        "id": "circle",
        "type": "circle",
        "source": "points",
        "paint": {
          "circle-radius": 5,
          "circle-color": ["case", ["within", {
            "type": "Polygon",
            "coordinates": [
                [
                    [
                      0,
                      0
                    ],
                    [
                     0,
                     5
                    ],
                    [
                      5,
                      5
                    ],
                    [
                      5,
                      0
                    ],
                    [
                      0,
                      0
                    ]
                ]
            ]
            }],  ["rgb", 153, 128, 250], ["rgb", 249, 138,175]]
        }

With the variation of geometry types, there would be different within cases, like point within polygon, line within polygon, polygon within polygon, etc. Right now we only support point/points within polygon/polygons.

@zmiao zmiao self-assigned this Feb 3, 2020
@zmiao zmiao changed the title Enable Within Expression with paint property Enable Within Expression with paint property + Filter Expression Feb 3, 2020
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.

Do we have tests for the new expression?

include/mbgl/style/expression/within.hpp Outdated Show resolved Hide resolved
include/mbgl/style/expression/within.hpp Outdated Show resolved Hide resolved
include/mbgl/style/expression/within.hpp Show resolved Hide resolved
include/mbgl/style/expression/expression.hpp Outdated Show resolved Hide resolved
src/mbgl/layout/pattern_layout.hpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Show resolved Hide resolved
@mourner
Copy link
Member

mourner commented Feb 7, 2020

like point within polygon, line within polygon, polygon within polygon

@zmiao do you anticipate extending within in the future with "within X meters from line" type queries, or would that be a different expression? If within will always act against polygons, maybe it's worth dropping the object wrapper (which will always be the same) and accepting coordinates directly.

@zmiao zmiao force-pushed the zmiao-within-expression branch from b9c9918 to 93aa3db Compare February 7, 2020 13:37
}
if (wn != 0) {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Here, you return a result immediately if you're inside the first ring, even if subsequent holes negate that result — this leads to incorrect results with holes.

Another small gotcha is that GL Native resolves overlapping rings in a polygon with an even-odd rule instead of a non-zero one, so you want to return true if wn is even, not necessarily zero.

If so, you could remove the branching into two loops and simplify the code — here's an example from one of my library of the same routine: https://github.com/mapbox/which-polygon/blob/master/index.js#L81-L95

Copy link
Member

Choose a reason for hiding this comment

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

One more point — the naming pointWithinPolygons is a little misleading, since you're testing against a single polygon which has multiple rings (an outer ring and holes), not multiple polygons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

@zmiao zmiao force-pushed the zmiao-within-expression branch 3 times, most recently from 2ccbd68 to 3cdc8dc Compare February 7, 2020 16:11
@zmiao zmiao requested a review from 1ec5 as a code owner February 7, 2020 16:11
@zmiao zmiao force-pushed the zmiao-within-expression branch 8 times, most recently from dfe8285 to a48cc17 Compare February 10, 2020 11:12
@zmiao
Copy link
Contributor Author

zmiao commented Feb 10, 2020

@zmiao do you anticipate extending within in the future with "within X meters from line" type queries, or would that be a different expression? If within will always act against polygons, maybe it's worth dropping the object wrapper (which will always be the same) and accepting coordinates directly.

@mourner In our initial plan, we planned to implement the within covering all the geometry types. Right now, we will start with polygon as it is required urgently. It is not necessarily to be within X meters from line, we could check if a point is on a line, line lies on lineString, etc.
And further more, it is not flexible to use inlined geojson source, in the future, we plan to have mechanism that could read from url and share the geojson source with all within expressions, in stead of saving geojson inside every expression.

@zmiao zmiao force-pushed the zmiao-within-expression branch 2 times, most recently from 21ae274 to 7dd1483 Compare February 10, 2020 12:15
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

The algorithmic fixes look 👍 (will leave the C++/native-specific code for others to review)

@chloekraw chloekraw added the needs changelog Indicates PR needs a changelog entry prior to merging. label Feb 10, 2020
@zmiao zmiao force-pushed the zmiao-within-expression branch 4 times, most recently from 3c3d0f8 to e9c6eba Compare February 11, 2020 10:01
expression-test/expression_test_parser.cpp Outdated Show resolved Hide resolved
expression-test/expression_test_parser.cpp Outdated Show resolved Hide resolved
include/mbgl/style/expression/within.hpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
src/mbgl/style/expression/within.cpp Outdated Show resolved Hide resolved
@zmiao zmiao force-pushed the zmiao-within-expression branch from a868c3c to 2f3cd78 Compare February 11, 2020 13:23
Add geometry type checker
add canonical as pointer

fix review findings
Fix polygon within algorithm
Add Unit tests

Fix incorrect metrics folder for ios-render-test-runner job
@zmiao zmiao force-pushed the zmiao-within-expression branch from 2f3cd78 to 4e66246 Compare February 11, 2020 13:37
Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

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

lgtm, +changelog

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs changelog Indicates PR needs a changelog entry prior to merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants