-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make featuresAt return symbols and use style properties #2052
Conversation
Query tests? 😉 cc @mourner |
} | ||
} | ||
|
||
callback(null, tile.featureTree.query(params, this.styleLayersByID)); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucaswoj I just added these style layer creations and recalculations here as part of the style-property-aware featuresAt but I don't think they should stay here. Some of this is a duplication of what Style
does but we don't need a full Style
here. Any ideas on how this should be split out? Maybe a StyleLayers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you read the Style
from tile.symbolBuckets[i].style
? We could even add a symbolBucketsById
property, if you think that'd have better perf.
EDIT: I see. These are not necessarily symbol layers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently create an instance of Style
per Bucket
. That now strikes me as silly. We should create a single persistent Style
per Worker
and update it whenever we receive set layers
EDIT: Ugh. Sorry. Bucket
s just have StyleLayer
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create a single persistent Style
per Worker
and update it whenever we receive set layers
. We can add flags to disable any extra overhead on Style
(i.e. fetching TileJSON).
Will this addition allow me to distinguish "geographical" hits from text/icon hits? If you look at my example in #1882 where I have three icons (i.e. three layers where two have icon-offset) based on a single geographical point; would I be able to sort out a click on the middle icon (where the point actually is) or will it return all three layers with no way to tell that it was one icon hit and three "geo" hits? Ideally I would want to be able to distinguish clicks on all three icons.. |
After this and #316 there will be no more "geo" hits if I'm understanding you correctly. All hits would be based on the rendered representation of features, not the source geometry. In your case, if you click in the center you would get a hit only for the center icon. |
@ansis Exactly what I was hoping for! Thanks! |
f9ea9a7
to
d1861dc
Compare
d1861dc
to
8f109eb
Compare
@jfirebaugh added query tests: https://github.com/mapbox/mapbox-gl-test-suite/compare/query-tests?expand=1 This is ready for review |
8f109eb
to
40426a4
Compare
I'm generally feeling good about this code but I'm not sure we can afford to suddenly switch from "geographical" hits to "symbol" hits. Have you thought any more about making that a separate API @ansis? |
d354096
to
6c1b03c
Compare
Can we call the APIs If I could do a clean-slate rename, I might put a verb in the name (and remove the preposition at the end of the name) like |
The current implementation doesn't have very many similarities. They accept different parameters and return different things. I think the names should be very different.
I think it's ok to rename it since we're already breaking it's behavior heavily. I don't like |
6c1b03c
to
f5b0a00
Compare
`getSourceTileData` calls the callback with an array of GeoJSON FeatureCollections (one for each tile). fixes #2106
We need to take a moment to think about the linguistic semantics of these API methods
In particular, we need to
If we can tolerate a clean-slate API redesign (preserving the existing methods as "deprecated" for a few releases) then I propose the following method names, each of which can handle both bbox and point-and-radius queries.
|
#2125 (comment) makes me think we might want to drop |
For rendering queries, I think we'd keep You could implement |
I'm fine with a breaking API redesign without deprecation. We don't want to support the old version for any meaningful length of time so we might as well drop it now.
I think we should leave
I'm a bit afraid of overloading the name
Could the radius option still be useful for less precise selection like touch?
Sounds good to me. |
For that use case you want to use |
Perhaps there is some inspiration and experience that could be drawn from OpenLayers 3 here. They have been doing pixel perfect hit detection of vector data since their early releases:
|
@averas thanks for the suggestion. We considered that approach but some experiments showed that rendering each feature one-by-one would be too slow and that reading back the pixels with |
67078f3
to
e6d8764
Compare
I dropped
|
@ansis reviewed the feature tree geometry code. It generally looks good, the only thing that caught my attention is that you rebuild full geometries on every query to account for offsets/etc., which is expensive. I'd prefer the intersection code to account for offsets during the checks, so that we only touch a fraction of all geometries due to early rejects/accepts. |
@ansis yeah, I would also suggest to settle on worker-based featuresAt for now and revisit later while keeping compatibility. The PR is already pretty big. |
fixes #1785 |
I thought I'd raise an issue I've ran into a couple of times before this PR is merged, since a solution might also involve breaking changes... When calling featureIn on multiple layers, such as In many of my use cases, however, I want to pass on the results, or an aggregation of the results (for example a count), to a visualization (say d3), but before I can do that I have to filter/aggregate the data set again! Why? Because the result returned by featuresIn is flattened to a single array with the layer information present on each object, such as:
To pass this data on I first have to do a Would it make sense to evaluate whether it would actually be better to return a hierarchical representation of the results where features are nested under their respective layer? |
8bdf3c7
to
68f186d
Compare
rebased on master. 8bdf3c7 was the latest before the rebase. |
56ccedb
to
9a5432d
Compare
fix #303 This increases worker memory usage by around 10% (a couple of MB). 1/3 of the increase comes from retaining the CollisionTile. 2/3 of the increase comes from retaining VectorTileFeatures.
map.featuresAt now includes features whose rendered representation matches the query, not features whose geometry matches the query. A query with `radius: 0` will now match lines and circles if the point is within the rendered line. also fix #2053 It now checks intersection based on the render type not the geometry type. A polygon that is rendered as a line will only match if the query matches the line. It will not include it if the query only matches in the internal part of the polygon. implemented: circle-radius circle-translate fill-translate line-width line-offset line-translate not implemented yet (hard with the current symbol index): text-translate icon-translate
`getSourceTileData` calls the callback with an array of GeoJSON FeatureCollections (one for each tile). fixes #2106
`.map` calls functions with two arguments. The second argument, the index, was being passed to `pointCoordinate` which was using it as the targetZ. The targetZ is set to anything other than 0 so remove the option.
and rename getSourceTileData into querySourceFeatures
Both now return array of GeoJSON features. Each feature has a `tile` property that is an object with `x`, `y`, and `z` properties containing the feature's tile's coord.
9a5432d
to
e5a4152
Compare
This adds currently visible text and icons to featuresAt results.
Style properties are used to return features whose rendered representation matches the query.
query test pr: mapbox/mapbox-gl-test-suite#77
fix #303
fix #316
fix #862
fix #902
fix #1822
fix #2100
fix #2103
This increases worker memory usage by around 10% (a couple of MB).
1/3 of the increase comes from retaining the CollisionTile.
2/3 of the increase comes from retaining VectorTileFeatures.
SymbolBucket merges lines to improve placement. This means that sometimes there is more than one VectorTileFeature responsible for that symbol. It currently returns the first VectorTileFeature and ignores the others that were merged into it. Should it do something else?
👀 @jfirebaugh