Adding mechanism for overriding OSM WayPropertySetSource in build-config.json - #2389
Conversation
…etSource in build-config.json. Added NorwayWayPropertySetSource for Norway specific OSM mappings and speeds
|
This is very useful feature. Thanks! |
DT-1759 Add penalty for walking on roads
| // Support "default" as well | ||
| case "default": | ||
| retval = new DefaultWayPropertySetSource(); | ||
| break; |
There was a problem hiding this comment.
I find switch statements kind of fragile with their default fall-through behavior and lack of braces around their clauses. Could we replace this with:
if ("default".equals(type)) {return new DefaultWayPropertySetSource();}
else if...
|
|
||
| public WayPropertySet getWayPropertySet(); | ||
|
|
||
| public class WayPropertySetSourceFactory { |
There was a problem hiding this comment.
Can we just eliminate this inner class and put the static method on the containing class?
| * @see WayPropertySetSource | ||
| * @see DefaultWayPropertySetSource | ||
| */ | ||
| public class NorwayWayPropertySetSource extends DefaultWayPropertySetSource { |
There was a problem hiding this comment.
I'd like to avoid inheritance where possible. Could we instead define a WayPropertySetSource that does not extend the default one, and just composes with it? I would think we can construct a default one, get its WayPropertySet, and then add some additional properties. But then I see you want to get your properties in before the default ones.
| } | ||
|
|
||
| /* Populate properties on existing WayPropertySet. Makes it easer to override any properties by sub classes. */ | ||
| protected void populateProperties(WayPropertySet props) { |
There was a problem hiding this comment.
I see that you're making these protected so they are accessible from a subclass, but I'm now realizing maybe they should be public static utility functions on the superclass. They don't seem to need any state fields from the class instance itself.
0e110e1 to
4b41554
Compare
|
Updated code according to comments @abyrd |
|
Thanks @seime, looks good. About WayPropertySetSourceFactory though, I don't think we need a separate class at all to contain a single function. It would be preferable to put the static factory method on the WayPropertySetSource class, so it would be invoked as WayPropertySetSource.fromConfig(). I think the documentation needs more detail as well, explaining the context: we must interpret OSM tags when building the OTP graph, and different OSM way types and tags have different meanings in different countries, and different OTP users may want those tags to be applied differently. |
|
@abyrd will look at this again next week |
|
@abyrd looking ok now? |
|
Can this be merged and closed? |
3 participants
See #1996
Configurable via build parameter "osmWayPropertySet"
Also added a custom property set for Norway where other access rules applies