Merged
Conversation
Member
|
👀 |
Member
|
LGTM so far! |
Member
|
@zerebubuth Please update the example config file based on the table in #988 (comment). We should also add some tests based on a handful of resulting collision_rank values? |
…n rank tests where we want access across multiple layers.
… so that it doesn't break --download-only mode.
Member
Author
rmarianski
approved these changes
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
reviewed
Jan 17, 2019
nvkelso
requested changes
Jan 17, 2019
Member
nvkelso
left a comment
There was a problem hiding this comment.
This looks great!
See comments for a few requested changes. They don't substantively change the logic and are mostly focused on the data config file, but should be changed before merging.
…hing on the tile layer the feature is in. This avoids a clash with the layer property, which is about feature z-ordering.
…r in previous gap calculation. Align ranges to round numbers. Fix tests to match.
…lter the kind in post-processing.
… had the fixed kinds.
…ref or something else that would be labelled in the properties. Extend country test to check for name:left and name:right.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adds a method for assigning collision ranks to features.
Method
The idea of a collision rank is that it's a unique integer assigned to everything that gets labelled. See #988 for more background. This adds a new resource,
spreadsheets/collision_rank.yaml, which is a YAML-encoded list of filters. The syntax of the filters is the same as thekind&min_zoomYAML in theyaml/directory (hopefully this minimises the amount of duplication, and means we can share bug fixes and/or new features).Each feature is checked against each filter in turn, and is assigned the
collision_rankcorresponding to the matching index. Indices start at 1, are incremented by one for each filter, and can have "reserved blocks" which skip the index counter onwards. For example, ifcollision_rank.yamlcontains:Then a feature with:
kind: foowould getcollision_rank: 1,kind: barwould getcollision_rank: 2,kind: bazwould getcollision_rank: 21.Note that indices 3-9 are unused in this example, but if we added an entry after
bar, then it would be assigned 3. The code asserts that indices are unique, so we can't accidentally overflow the reserved blocks. Both thefromandtovalues are inclusive, hencebazis assigned the next index, 21.Why not use a CSV
We've been managing
sort_rankwith CSVs, but the issue is that we often don't care about the exact value of thesort_rank, but want to easily insert values and re-order the following lines. Having thesort_rankexplicitly in the file means that any such re-arrangement has a very large diff, and is hard to check visually. This is an experiment to see if we can make something that still gives the features we want, but without the drawbacks of the CSV.Additionally, the CSVs tended to get mangled by Excel when loading and/or saving, as we were using a bunch of special characters to indicate things such as "any"
*or "must be null"-.Finally, CSV files can't contain comments, but YAML ones can. This will make future maintenance easier, as we can comment
# this reserved block for X, Y or Z.Connects to #988.