This repository was archived by the owner on Aug 8, 2023. It is now read-only.
Repository navigation
Port global symbol query from GL JS - #11742
Merged
Merged
Conversation
ChrisLoer
added
GL JS parity
ChrisLoer
force-pushed
the
boba-symbol-query
branch
from
April 19, 2018 23:17
c08fed4 to
6b922ca
Compare
ansis
reviewed
Apr 23, 2018
ansis
left a comment
ansis
left a comment
Contributor
There was a problem hiding this comment.
This looks good to me from the symbol side of things but it might be good to get some extra eyes on the c++ side?
ChrisLoer
force-pushed
the
boba-symbol-query
branch
from
April 23, 2018 23:09
6b922ca to
11b554f
Compare
ChrisLoer
removed
the
⚠️ DO NOT MERGE
ChrisLoer
changed the title
ChrisLoer
force-pushed
the
boba-symbol-query
branch
from
April 24, 2018 01:17
f57fe9b to
f7ad039
Compare
- Symbol querying is now global instead of per-tile - Symbols that bleed over tile boundaries no longer missed in queries - Symbol results now sorted based on rendering order (ie overlapping symbols change their sort order when a bearing change causes their render order to change) - Placement::retainedQueryData now responsible for maintaining symbol querying data for buckets that may no longer be in the TilePyramid.
- Pulls over an update to line.vertex.glsl (looks like a no-op?) - Add test ignores for collator, is-supported-script, line-gradient - Exclude collator, is-supported-script, line-gradient from code generation.
ChrisLoer
force-pushed
the
boba-symbol-query
branch
from
April 24, 2018 18:18
46edc83 to
44b731d
Compare
asheemmamoowala
approved these changes
Apr 24, 2018
| } | ||
|
|
||
| auto bucket = renderTile.tile.getBucket(*symbolLayer.baseImpl); | ||
| GeometryTile& geometryTile = static_cast<GeometryTile&>(renderTile.tile); |
Contributor
There was a problem hiding this comment.
Instead of doubling up on the dyanmic_cast and static_cast, how about using an assert(dynamic_cast<GeometryTile*>(&renderTile.tile). Aren't symbol layers expected to always have a geometry tile?
Contributor
Author
There was a problem hiding this comment.
Yeah, that sounds reasonable -- we're already doing the same thing in RenderSymbolLayer.
|
|
||
| bucket.updateOpacity(); | ||
| bucket.sortFeatures(state.getAngle()); | ||
| if (retainedQueryData.find(bucket.bucketInstanceId) != retainedQueryData.end()) { |
Contributor
There was a problem hiding this comment.
Duplicate calls to find. Pull out of the if condition.
- assert symbol layer tiles must be geometry tiles, instead of dynamically checking - re-use retainedBucketQuery iterator instead of calling find twice.
This was referenced May 18, 2018
22 of 23 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix for #11679.
This is a pretty direct port of the GL JS functionality. Things I'm a little uncomfortable with:
IndexedSubfeatures we're doing, especially since they contain (hopefully short/fixed length) strings. I don't have a reason to think this is a performance hotspot, but it looks awkward.shared_ptr-style ownership feels a little hard-to-audit (although it's the same as the javascript behavior, C++ just calls more attention to it).The fix is not as critical for native because native never had an "incremental placement", but this still fixes (1) querying near tile boundaries, and (2) result sort order for symbol buckets that have been re-sorted after generation. It's also nice that it gets rid of the
commitFeatureIndexespathway./cc @ansis @jfirebaugh