Add loop line merger #1083
Add loop line merger #1083
Conversation
Full logs: https://github.com/onthegomap/planetiler/actions/runs/12001709762 |
|
I added some tests for the public functions. I don't really know how private functions are tested in Java. Should I for example write specific tests for the private function |
|
It really only needs tests for the public api. If there's an internal method you wanted to make sure works the way you expected you can make it package protected (no modifier) so tests can use it since they're in the same package. |
I think something like this would make sense for an API to control those parameters: var merger = new LoopLineMerger()
.setMinLength(0.1)
.setLoopMinLength(0.1)
.setPrecision(new PrecisionModel(16)); // to control rounding
merger.add(lineStrings);
var merged = merger.getMergedLineStrings(); |
I can help look into the performance, I think there will be some low hanging fruit to make it fast. JTS line merge also only considers full linestrings, it doesn't break them up into segments so this one will always be a little slower. The first thing that jumps out at me is creating full LineStrings and reversing/comparing them while building up the lists. I think we might want to have the intermediate code work with lists of coordinates that can be concatenated efficiently, then construct the LineStrings at the very end. |
|
Great, thanks for the feedback! I added some more tests and marked the methods that I want to test separately as protected. |
|
I added a |
|
Thanks for adding the benchmarks. I changed the api just now. Let me have a look at the benchmark... |
|
Pushed a few updates to the benchmark, basically it creates between 10 and 1000 line segments in the range from (0,0) to (100, 100) with between 10 and 1000 intermediate points then uses both line merge. I disabled the 1000 lines with 1000 points because it currently does not finish with loop line merger 😬 Let me know if you can think of more realistic scenarios? |
|
I had a look at what is given to line merging if one wants to render |
|
I guess these statistics reflect that in OpenStreetMap a typical Regarding the length of the linestrings, actually almost half of them are too short to occupy distinct start and end points on the 1/16 grid: The bins here are 0.5 * 1/16 wide. That means that the linestrings in the first bracket will snap to the same point on the grid for start and end node. |
|
Cool, I think we probably want to benchmark a few "average" cases (in terms of num lines/num points) and also a worst-case to make sure it doesn't hang. Feel free to update the benchmark test data generation to be more realistic! |
|
That is the Swiss highway=primary network. It has 50k+ input linestrings. |
|
Thanks for adding the benchmarks @msbarry! The numbers on my machine are currently: |
|
Some stats when I run over the planet using openmaptiles profile:
so for worst case we might want a case that's on the order of 500k line segments each with 2 points, and a case that's 200 lines with 5000 points each. For average case, maybe 50x2 10x10 and 2x50? |
|
These number sound perfect. Thanks for investigating |
|
Oh actually I'll try to grab those actual largest input geometries so we can just test against the real thing. |
|
Thanks for your review! I think I have the code roughly in the state I want for the highway=primary z6 data I used as a playground. As a next step I will look into the tests and also work on the remaining review comments. |
planetiler-core/src/main/java/com/onthegomap/planetiler/FeatureMerge.java
Outdated
Show resolved
Hide resolved
planetiler-core/src/main/java/com/onthegomap/planetiler/FeatureMerge.java
Outdated
Show resolved
Hide resolved
planetiler-core/src/main/java/com/onthegomap/planetiler/util/LoopLineMerger.java
Show resolved
Hide resolved
planetiler-core/src/main/java/com/onthegomap/planetiler/FeatureMerge.java
Outdated
Show resolved
Hide resolved
planetiler-core/src/main/java/com/onthegomap/planetiler/util/LoopLineMerger.java
Outdated
Show resolved
Hide resolved
…oopLineMerger.java Co-authored-by: Michael Barry <[email protected]>
|
I fixed some tests and noticed that tolerance=0 can still change lines, e.g., nodes on straight lines are removed so |
Yes I think simplify tolerance = -1 would make sense. |
|
|
||
| if (tolerance >= 0.0) { | ||
| simplify(); | ||
| removeDuplicatedEdges(); |
There was a problem hiding this comment.
Why do we end up with duplicate edges here? Is it possible to remove this duplicate edge removal step?
| } | ||
|
|
||
| @Test | ||
| void testMergeCarriagewaysWithOneSplitShorterThanLoopMinLength() { |
There was a problem hiding this comment.
for the merge carriageways tests I added, I think we just need mergeStrokes=true, and tolerance=-1 (by changing the default value) for them to pass. We may also need to change the output line order.
| "i90.wkb.gz,1,65", | ||
| "i90.wkb.gz,20,4", | ||
| "i90.wkb.gz,30,1", | ||
| }) |
There was a problem hiding this comment.
for these, feel free to just change the expected number of lines so that we pin down the behavior to know if it changes in the future. Also, we could probably insert a boolean simplify argument so that we could test a couple of these inputs with and without line simplification.
This pull request adds a
LoopLineMergerclass which is a replacement for JTSLineMerger. The new class takes loops in the LineString graph into account. Loops that are shorter than aminLoopLengthwill be cut open and only the shortest edge will be kept.Here is a bit of motivation: https://oliverwipfli.ch/improving-linestring-merging-in-planetiler-2024-10-30/
Questions from my side are:
minLoopLength? At the moment I just set it tolengthLimitfromFeatureMerge.mergeLineStrings. Should we maybe add an optionalloopLengthLimitCalculator?LoopLineMergeris slower than JTSLineMerger. I guess it is because they are doing less (no loops taken into account) but also they are doing things more efficiently. Should I investigate in subclassing JTSLineMergeGraphinstead of building my own data structure plus helper functions for the graph?