Permit post-process merging in custommap schemas #626
Merged
Conversation
This file contains 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
https://github.com/onthegomap/planetiler/actions/runs/5772505468 ℹ️ Base Logs 68c2d7fℹ️ This Branch Logs e1f2c2c |
msbarry
requested changes
Jul 16, 2023
There was a problem hiding this comment.
This is great! Thanks for adding!! Can you also update planetiler.schema.json so this shows up in IDEs and planetiler-custommap/README.md to mention the new fields?
Also to make full use of this, we'll need support for min_size otherwise we'll drop features that are too short so there won't be anything to merge. @wipfli started this in #421 - I'll ping him about if we can merge that soon too.
...er-custommap/src/main/java/com/onthegomap/planetiler/custommap/configschema/PostProcess.java
Outdated
Show resolved
Hide resolved
...stommap/src/main/java/com/onthegomap/planetiler/custommap/configschema/MergeLineStrings.java
Outdated
Show resolved
Hide resolved
...r-custommap/src/main/java/com/onthegomap/planetiler/custommap/configschema/FeatureLayer.java
Outdated
Show resolved
Hide resolved
planetiler-custommap/src/main/java/com/onthegomap/planetiler/custommap/ConfiguredProfile.java
Show resolved
Hide resolved
| geometry: point | ||
| """; | ||
| this.planetilerConfig = PlanetilerConfig.from(Arguments.of(Map.of())); | ||
| assertEquals(null, loadConfig(config).findFeatureLayer("testLayer").postProcess()); |
There was a problem hiding this comment.
minor: use assertNull instead of assertEquals(null, ...)
There was a problem hiding this comment.
Change pushed previously.
In the future I want to make the post processing more flexible (#298), so you can limit the fields features get grouped by, and tell planetiler how to combine the rest (for example bridges) and also make line merging more flexible so it can maintain direction (ie. for oneway roads). Also, I want to move every fixed number to support being an expression so it can vary by zoom/feature attributes. That being said, this is a good first step and we can add those later without breaking compatibility since they will just add extra keys, or allow changing a numeric value to a complex object or expression. |
|
I just merged #421 so you can set |
|
Thanks for reviewing this @msbarry! I have quite a lot of paid work to get through at the moment, but I hope to return to this and implement the points you raised. |
msbarry
reviewed
Jul 20, 2023
Comment on lines
102
to
118
| if (featureLayer.postProcess().mergeLineStrings() != null) { | ||
| var merge = featureLayer.postProcess().mergeLineStrings(); | ||
|
|
||
| return FeatureMerge.mergeLineStrings(items, | ||
| merge.minLength(), // after merging, remove lines that are still less than {minLength}px long | ||
| merge.tolerance(), // simplify output linestrings using a {tolerance}px tolerance | ||
| merge.buffer() // remove any detail more than {buffer}px outside the tile boundary | ||
| ); | ||
| } | ||
|
|
||
| if (featureLayer.postProcess().mergePolygons() != null) { | ||
| var merge = featureLayer.postProcess().mergePolygons(); | ||
|
|
||
| return FeatureMerge.mergeOverlappingPolygons(items, | ||
| merge.minArea() // after merging, remove polygons that are still less than {minArea} in square tile pixels | ||
| ); | ||
| } |
There was a problem hiding this comment.
There is a chance that there are lines and polygons in the same layer and we want to apply merge to both of them. mergeLineStrings will ignore polygons and mergeOverlappingPolygons will ignore linestrings, so let's change this so that if both are set it applies them sequentially to items and returns the result.
| import java.util.Collection; | ||
|
|
||
| public record FeatureLayer( | ||
| String id, | ||
| Collection<FeatureItem> features | ||
| Collection<FeatureItem> features, | ||
| @JsonProperty("tile_post_process") PostProcess postProcess |
There was a problem hiding this comment.
The change looks good! Only thing left is to mention these features in planetiler.schema.json and planetiler-custommap/README.md
|
Hey @zhibek just checking do you think you'd have a chance to add this to the json schema and planetiler-custommap/README.md? Or do you want me to add those in? Would be great to get this merged! |
|
Thanks for the nudge @msbarry. I'm aiming to implement the final points this weekend. But feel free to adjust if I miss anything. |
|
Kudos, SonarCloud Quality Gate passed! |
|
OK @msbarry - I think that's everything. But feel free to modify if I missed something. |
msbarry
approved these changes
Aug 7, 2023
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.
To overcome the limitations in #350, I've been using these modifications locally.
I'm not sure which direction @msbarry would like to take with this functionality, but I'm sharing this PR in case its useful.