When merging features, assign IDs ending in 0 to the result #1397
Conversation
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
In FeatureMerge.java, merged features are now given a new ID ending in 0 instead of reusing one of the input feature IDs. This indicates the tile feature doesn't correspond to a single source element. For OSM features where IDs encode as osm_id * 10 + element_type, IDs ending in 0 are reserved for non-OSM or composite features. This helps prevent data consumers from incorrectly linking merged features back to a particular OSM element. Fixes onthegomap#1393
Full logs: https://github.com/onthegomap/planetiler/actions/runs/19561838010 |
msbarry
reviewed
Nov 20, 2025
Contributor
There was a problem hiding this comment.
Thanks for making this change! A few minor comments and it looks like some tests in openmaptiles need to be changed due to this behavior. Easiest might be to disable those tests in this repo temporararily then we can update omt after this merges. Can you revert #1339 and add the failing tests to exclude.txt?
| */ | ||
| private static long makeMergedId(long id) { | ||
| return (id / 10) * 10; | ||
| } |
Contributor
There was a problem hiding this comment.
Thanks for adding this! I'm realizing this will update IDs on all elements, not just OSM elements. I'm not sure that is too important as the IDs are less meaningful in other sources.
I just double checked and confirmed that all features (even non-osm ones) get multiplied by config.featureSourceIdMultiplier() so it should be safe to do this for all features without causing unwanted collisions. Could we use that instead of a hardcoded 10 constant though?
planetiler-core/src/main/java/com/onthegomap/planetiler/FeatureMerge.java
Outdated
Show resolved
Hide resolved
|
Author
|
Thanks for the review! I implemented the suggested I wasn't sure how to use There are still two checks that are failing. One is the standalone example, which I think is failing since the tests have been changed to expect the new behavior but it's fetching the current 0.9.4 build to test with, which has the old behavior. The other is the link checker, which looks like it's failing to fetch some links from Medium, so probably unrelated to these changes? FWIW I can load those links in my browser just fine, so maybe Medium is blocking requests from GitHub Actions as part of an anti-scraping effort. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
In FeatureMerge.java, merged features are now given a new ID ending in 0 instead of reusing one of the input feature IDs. This indicates the tile feature doesn't correspond to a single source element.
For OSM features where IDs encode as osm_id * 10 + element_type, IDs ending in 0 are reserved for non-OSM or composite features. This helps prevent data consumers from incorrectly linking merged features back to a particular OSM element.
Fixes #1393.
I added new tests for this change and updated some existing tests which were expecting the old behavior in their assertions. Note that there are also a couple of tests in the planetiler-openmaptiles submodule that will need to be updated if this change is merged. Happy to open a PR for that too, but wasn't sure if that should be done in tandem with this one or afterwards.
I also tested this by installing my modified version of Planetiler and using it to build Sourdough tiles, then viewed the resulting PMTiles archive and verified that merged features have IDs ending in 0, and unmerged features have their original IDs (encoding the OSM type and ID of the source feature).