fix(isochrones): correct and unify edge splitting

[aoles]

Pull Request Checklist

• 1. I have rebased the latest version of the main branch into my feature branch and all conflicts
  have been resolved.
• 2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the
  [Unreleased] heading.
• 3. I have documented my code using JDocs tags.
• 4. I have removed unnecessary commented out code, imports and System.out.println statements.
• 5. I have written JUnit tests for any new methods/classes and ensured that they pass.
• 6. I have created API tests for any new functionality exposed to the API.
• 7. If changes/additions are made to the ors-config.json file, I have added these to the ors config documentation
  along with a short description of what it is for, and documented this in the Pull Request (below).
• 8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
• 9. I have referenced the Issue Number in the Pull Request (if the changes were from an issue).
• 10. For new features or changes involving building of graphs, I have tested on a larger dataset
  (at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
• 11. For new features or changes involving the graphbuilding process (i.e. changing encoders, updating the
  importer etc.), I have generated longer distance routes for the affected profiles with different options
  (avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
  points generated from the current live ORS.
  If there are differences then the reasoning for these MUST be documented in the pull request.
• 12. I have written in the Pull Request information about the changes made including their intended usage
  and why the change was needed.
• 13. For changes touching the API documentation, I have tested that the API playground renders correctly.

Information about the changes

• Key functionality added: Split edges based on difference in coordinates rather than their actual distance in meters, and use the same splitting length across isochrone builders.
• Reason for change: Distance-based splitting proved to be unreliable, while different defaults across the builders introduced additional differences in the generated geometries.

In order to guarantee correct construction of the concave hull edge splitting operations need to be performed based on the differences computed on geocoordinates rather than on the actual distances in meters. In practice, this switch makes little difference to the produced geometries and has virtually no measurable impact on performance. Typical differences in geometries observed are as follows, where blue is the geometry produced with the previous approach, and red with the new one.

The reduction of the splitting length has more noticeable effects, see report. For regular isochrones going down by 1/3 from 0.012 to 0.009, which corresponds to 1333 and 999 meters in latitude, respectively, comes with a performance penalty of around 15%. However, it helps to increase isochrone accuracy and avoid false inclusion of unreachable areas, which is best illustrated by the following example.

^{Location 10}

Typically, the observed differences are more subtle, such as

^{Location 4}

and

^{Location 7}

[sfendrich]

Could you please elaborate on how this change affects isolines across the globe? Using geo-coordinates means measuring in degrees. The distance between coordinates varies a lot depending on the location and the used coordinate system. Imagine you would invent a coordinate system having its north pole in Berlin. would the isolines still have the same shape?

[aoles]

Thanks @sfendrich, that's a fair point! What this PR tries to achieve is to perform edge splitting in the same coordinate system in which the concave hull is also being computed. The concave hull algorithm is parameterized by the target maximum edge length. Edge splitting makes sure that edge segments which are input to the concave hull algorithm do not exceed this limit. This PR does not make any change to the concave hull part which actually computes the isochrone polygons. This part has alwas been done in the geocoordinate domain. Having said this, I agree that the generated shape is influenced by the location.

[sfendrich]

So, if I build the same town two times, once in Alaska and once in Cameroon, the isochrones would differ in their shape? This sounds undesirable.

[aoles]

While the above is true, this is also how it has always been like. Let me stress this again: this PR does not change how the isochrones are build in general. It just makes adjustments to the logic how the edges are split, and the splitting is there only to facilitate correct results from computing the concave hull.

Having said this, I don't think this discussion should be continued here. If you are not satisfied with the current solution of generating isochrone geometries you are welcome to open an issue. Cheers!

[rtroilo]

While the above is true, this is also how it has always been like. Let me stress this again: this PR does not change how the isochrones are build in general. It just makes adjustments to the logic how the edges are split, and the splitting is there only to facilitate correct results from computing the concave hull.

Having said this, I don't think this discussion should be continued here. If you are not satisfied with the current solution of generating isochrone geometries you are welcome to open an issue. Cheers!

But I think this could have a very easy fix, just make the smoothing-factor depending an the latitude of the center. What do you think?

[aoles]

@rtroilo thanks for chiming in. I've thought about this too, however, the further apart you move from the equator the higher the discrepancy between the length in meters of one latitudinal and one longitudinal degrees. The algorithm which computes the concave hull is parametrized only with one length and does not differentiate between XY directions. So effectively you still have the same issue, or do I miss something? 🤔

[sfendrich]

Just two minor comments.

[sfendrich]

So, to summarize the discussion:

The issue solved by this PR is the following: If a long graph edge is used in an isochrone it should be contained in the concave hull. However, due to the length of the edge, the concave hull algorithm might inappropriately introduce a cave such that the edge is outside the hull. Therefore, we introduce additional nodes on this long edge to split it into multiple shorter ones, yielding more points to be fed into the concave hull algorithm. These additional points prevent the creation of the cave.

Independent of this PR it seems there is something fundamentally wrong with isochrones being computed from geo-coordinates. At the scale of a single isochrone, this is probably not relevant. But when comparing isochrones at different latitudes, the length differences of a longitude degree might affect the shape and size of the isochrone's geometry.
Here is a new issue to continue the discussion on this second topic: #1711.

[MichaelsJP]

@aoles Can we merge this?