-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OSM area processing obeys tag mapping #6164
base: dev-2.x
Are you sure you want to change the base?
Conversation
Walkable area builder used hardcoded bike+pedestrial default permissions for area edges. Now default permissions take tag mapping into account.
Rule whicgh assigns pedestrian permissions to transit platforms is not enough, because another rule which contains more tags overrules it.
OSM tag mappers now use normal OOP inheritance. It is usually a bad idea to implement classes directly from an inteface.
Codecov ReportAttention: Patch coverage is
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. |
About your second point, this kind of fuzzy matching has caused me problems before, so I added Lines 8 to 20 in f4bfedf
It think it could be helpful here. There was talk about making it the default but we were scared of the unintended consequences. |
I wonder if absence of a tag in BestMatchSpecifier should actually reduce match scores? Less than conflicting value but something at least. |
I find |
* prevailing 'props.setProperties' statement) has a 'foot=yes' tag the permissions are overridden | ||
* and walking is allowed on that way. | ||
* <p> | ||
* TODO clarify why this needs a separate factory interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 byOsmProvider
getCarSpeedForWay(OsmWithTags way, boolean backward)
- used byOsmModule and WalkableAreaBuilder
getMaxUsedCarSpeed(WayPropertySet wayPropertySet)
- used byOsmModule
boolean isMotorVehicleThroughTrafficExplicitlyDisallowed(OsmWithTags way)
used bySafetyValueNormalizer
boolean isBicycleNoThroughTrafficExplicitlyDisallowed(OsmWithTags way)
used bySafetyValueNormalizer
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😃
Instead of adding competing best match rules, start using the well designed exact match specifier. This may cause some unexpected changes but as the change concerns only one specific rule, we should be able to deal with them,
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 plainhighway=footway
because the former rule often has more matching tags. This is a bit surprising because absence ofbicycle=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.