OSM area processing obeys tag mapping

[vesameskanen]

Summary

OSM parsing and graph building code always applied hard coded bicycle and pedestrian permissions to areas. Now the code uses the configured wayPropertySet and national tag mapping to define the permissions.

Secondly, detailed propertly rule for highway=footway;bicycle=yes;area=yes tag combination overruled simpler rules like plain highway=footway because the former rule often has more matching tags. This is a bit surprising because absence of bicycle=yes tag does not reduce scores. This problem is fixed by adding more detailed tag combination rules to the default and Finland mappers.

Thirdly, OSM tag mapping is a typical example of a situation where OOP works very well. Class hierarchy is now refactored to use very simple inheritance with a base class and national sub classes.

Issue

Closes #5354

Unit tests

Area permission tests for Finland and Germany tag mappers added.

Documentation

Updated.

[codecov]

Codecov Report

Attention: Patch coverage is 99.48052% with 2 lines in your changes missing coverage. Please review.

Project coverage is 69.91%. Comparing base (ead941e) to head (36c1006).
Report is 112 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
.../graph_builder/module/osm/WalkableAreaBuilder.java	90.90%	1 Missing ⚠️
...ripplanner/osm/tagmapping/ConstantSpeedMapper.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6164      +/-   ##
=============================================
- Coverage      69.93%   69.91%   -0.03%     
- Complexity     17715    17723       +8     
=============================================
  Files           1996     1997       +1     
  Lines          75365    75441      +76     
  Branches        7705     7718      +13     
=============================================
+ Hits           52705    52742      +37     
- Misses         19989    20021      +32     
- Partials        2671     2678       +7     

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

[leonardehrenfried]

About your second point, this kind of fuzzy matching has caused me problems before, so I added

OpenTripPlanner/application/src/main/java/org/opentripplanner/osm/wayproperty/specifier/ExactMatchSpecifier.java

Lines 8 to 20 in f4bfedf

	/**
	* This specifier allows you to specify a very precise match. It will only result in a positive when
	* _all_ key/value pairs match exactly.
	* <p>
	* It's useful when you want to have a long, very specific specifier that should only match a very
	* limited number of ways.
	* <p>
	* If you'd use a {@link BestMatchSpecifier} then the likelihood of the long spec matching unwanted
	* ways would be high.
	*
	* @see org.opentripplanner.osm.tagmapping.HoustonMapper
	*/
	public class ExactMatchSpecifier implements OsmSpecifier {

It think it could be helpful here.

There was talk about making it the default but we were scared of the unintended consequences.

[vesameskanen]

I wonder if absence of a tag in BestMatchSpecifier should actually reduce match scores? Less than conflicting value but something at least.

[leonardehrenfried]

I find BestMatchSpecifier quite confusing (for a while it was the only one we had) and would love to see it go away entirely. That said, there are probably improvements we can make to it.

[t2gran]

First I don´t know this code well. But, inheritance is usually a bad chose if all you want to do is to share code - or even worse, save typing on the keyboard. If there is no polymorphic behaviour and all you want is to reuse code - use delegation instead.

Interfaces are a contract between the caller and the implementation. Interfaces should be used if you want to document the API needed to plug into something else. In this case the contract is:

• populateProperties(WayPropertySet wayPropertySet) - used by OsmProvider
• getCarSpeedForWay(OsmWithTags way, boolean backward) - used by OsmModule and WalkableAreaBuilder
• getMaxUsedCarSpeed(WayPropertySet wayPropertySet) - used by OsmModule
• boolean isMotorVehicleThroughTrafficExplicitlyDisallowed(OsmWithTags way) used by SafetyValueNormalizer
• boolean isBicycleNoThroughTrafficExplicitlyDisallowed(OsmWithTags way) used by SafetyValueNormalizer

I am not sure if OsmTagMapper play the same role in OsmProvider, OsmModule,WalkableAreaBuilder, SafetyValueNormalizer. But if so an interface with the methods above would be nice. There is no overlap in the usage by OsmProvider, OsmModule and WalkableAreaBuilder so maybe zero, two or even three interfaces is the right thing.

What about the other methods?

• boolean doesTagValueDisallowThroughTraffic(String tagValue)
• boolean isGeneralNoThroughTraffic(OsmWithTags way)
• boolean isVehicleThroughTrafficExplicitlyDisallowed(OsmWithTags way)

These methods are abstract factory methods (I think). So, they do not belong in the contract - and they should be package local.

I looked at the overridden methods as well, but quickly fall off. One alternative to factory methods is the strategy pattern. I do not know what is the best solution in this case.

I would call this class the DefaultOsmTagMapper, extract a OsmTagMapper interface with 5 methods in it - but as I said, I do not know this code.

[vesameskanen]

I would think that a regular java class with its public methods defines equally good contract for the caller. Am I wrong?

The changes I made to the tag mapper classes clearly reduce code lines and also make the responsibilities obvious. Previously, some methods were inherited to interface, some were delegated to the default tag mapper. This kind of complication should be avoided.

[t2gran]

A regular base Java class is not as clear. Some times you need to add public methods for other reasons and the contract get cluttered with other stuff, like the implementation of the contract and fields/other methods. But in this case I think it is ok - it depends on what the "bounded context" is. When you cross boarder you want clear contract, while it it is less important inside a module. To me adding on the getCarSpeedForWay(OsmWithTags way, boolean backward), and getMaxUsedCarSpeed(WayPropertySet wayPropertySet) seams like a hack to solve the constant-speed problem. The OsmTagMapper does not have a single-responsibility. Maybe we can spend 10 minutes on this in the dev meeting today.

I agree that the changes you did is a good improvement 😃