Add walk safety to way property sets and walkSafetyFactor routing parameter

[optionsome]

Summary

Adds options to insert walk safety values in way property sets and adds walkSafetyFactor parameter for controlling how much those values are used in routing.

Issue

closes #4324 #3541

Unit tests

Added

Changelog

From title

[leonardehrenfried]

I really like the direction this is going.

There are quite a few things in the WayProperty code that needs cleanup but it would be unfair to push that on you so I think just copying what is there is ok.

Since the logic in StreetEdge is very complex, one thing I would like to ask is to add tests. Fortunately, there are already some in StreetEdgeCostTest.

[optionsome]

Since the logic in StreetEdge is very complex, one thing I would like to ask is to add tests. Fortunately, there are already some in StreetEdgeCostTest.

I'll add some tests for parsing of the way property sets and for the StreetEdges at least.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 91.25596% with 55 lines in your changes missing coverage. Please review.

Project coverage is 57.85%. Comparing base (096e9ed) to head (ae0b8a9).
Report is 9153 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...ntripplanner/inspector/WalkSafetyEdgeRenderer.java	11.76%	15 Missing ⚠️
...aph_builder/module/osm/UKWayPropertySetSource.java	0.00%	14 Missing ⚠️
...ripplanner/routing/api/request/RoutingRequest.java	14.28%	6 Missing ⚠️
...legacygraphqlapi/generated/LegacyGraphQLTypes.java	0.00%	4 Missing ⚠️
.../graph_builder/module/osm/OpenStreetMapModule.java	81.81%	2 Missing and 2 partials ⚠️
...g/opentripplanner/routing/edgetype/StreetEdge.java	76.47%	2 Missing and 2 partials ⚠️
...rg/opentripplanner/api/common/RoutingResource.java	0.00%	2 Missing ⚠️
...lanner/graph_builder/module/osm/WayProperties.java	83.33%	0 Missing and 2 partials ⚠️
...ner/routing/edgetype/StreetElevationExtension.java	66.66%	1 Missing and 1 partial ⚠️
...qlapi/datafetchers/LegacyGraphQLQueryTypeImpl.java	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #4328      +/-   ##
=============================================
+ Coverage      57.63%   57.85%   +0.21%     
- Complexity     10988    11007      +19     
=============================================
  Files           1449     1451       +2     
  Lines          58390    58758     +368     
  Branches        6783     6794      +11     
=============================================
+ Hits           33653    33994     +341     
- Misses         22703    22722      +19     
- Partials        2034     2042       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[optionsome]

This is now properly ready for review. I created some walk safety rules for the FinlandWayPropertySetSource but those will updated in a later pull request. We might want to consider adding some walk safety values for the DefaultWayPropertySetSource at some point as I think they make as much sense as having bicycle safety values there.

[optionsome]

The walk safety values are not currently used in wheelchair traverse or in bike walking as they use getEffectiveBikeDistance instead of getEffectiveWalkDistance. The getEffectiveWalkSafetyDistance is based on the getEffectiveWalkDistance.

[leonardehrenfried]

I really like the latest changes in particular that you refactored all of the existing sets. I have a suggestion about making them more compact though: leonardehrenfried@647e450

What do you think about omitting the new WayPropertiesBuilder and the .build()?

Whether the new method should be called of() or something more obvious when used as a static import is up for debate.

[optionsome]

What do you think about omitting the new WayPropertiesBuilder and the .build()?

I thought about that too. I was not quite sure how appropriate it would be to pass the builder as an argument for a method in the WayPropertySet and do the build there.

Whether the new method should be called of() or something more obvious when used as a static import is up for debate.

I don't remember seeing an of method that returns a builder instance elsewhere, so I omitted it because I was not sure if it going too much against the standard use of of methods. The use of the static import of method would make the code fit in smaller space but again, it's not something I've seen being used.

Do @t2gran or @hannesj have opinions on these?

[leonardehrenfried]

At today's meeting we decided that we want to use the shortest version suggested by myself. @optionsome is free to rename the method of to something easier to understand when used as a static import.

[optionsome]

I ended up going with the @t2gran suggestion withModes as it's short enough.

[hannesj]

Looks good, couple of minor things