opentripplanner / OpenTripPlanner

An open source multi-modal trip planner
http://www.opentripplanner.org
Other
2.2k stars 1.03k forks source link

WayPropertySetSource configuration #1787

Closed thcrock closed 2 years ago

thcrock commented 9 years ago

Customizing the WayPropertySetSource for a graph is pretty important, so configuration in build-config.json should be added. From the dev forums: https://groups.google.com/forum/#!topic/opentripplanner-dev/McUPcFcials

I've started work on a sample config file; specifically, porting over the current DefaultWayPropertySetSource to a slightly more concise JSON format and merging it in with a sample build-config.json. When I'm finished I'll link to it here.

thcrock commented 9 years ago

I made a sample build-config.json file (nevermind that I named it something else, or put it in a dumb directory) using all of the default settings from DefaultWayPropertySetSource.java. How does this look for configuration?

https://github.com/thcrock/OpenTripPlanner/blob/wayPropertyConfig/src/main/java/org/opentripplanner/test.json

abyrd commented 9 years ago

This looks like the right idea to me. I might change the field names though. From the point of view of the user configuring this it is all about OSM tags, so I'd just call "spec" "tags". I was also thinking, is there any legitimate case where the same set of tags appears twice? If not then the tag specs can be the keys, and the whole osmProperties value is an object instead of an array. It might be more readable.

Also note that while "danger" makes it clear what the floating point values mean in most cases, internally it's just a cost / difficulty multiplier. For example, sand surfaces have a factor of 100 not because they are dangerous but because you can't really bike on them. This also highlights the fact that these danger values are very bike-specific. That needs to be clearly explained in the documentation. It's not dangerous to walk on sand at all.

abyrd commented 9 years ago

I also find it strange that we're using special combined permissions values such as "PEDESTRIAN_AND_BICYCLE" instead of just a list of modes that are allowed. This is already the case in the existing way properties code.

thcrock commented 9 years ago

Making the tagsets into keys is possible, but I wonder if it would be less readable for people who aren't editing the entire thing. For instance, if you are primarily interested in car speeds, you may want all of the properties with car speeds next to each other. Or somebody may want to keep all of the tags with assymetrical bike costs next to each other. Or all of the notes and names. Leaving it this way would afford people some more flexibility with how they organize their config files, at the cost of potential rule overwrites. I don't have a strong opinion one way or the other.

I can definitely rename 'specs' and 'danger'. I had 'danger' as 'safety' until the last minute but remembered that there was some discussion about 'safety' being misleading. 'bike_cost_multiplier' might work, or more concisely 'bike_cost'.

abyrd commented 9 years ago

How would making the tag sets into keys affect the ordering? I think we'd still be completely free to order them however we like. I agree that 'danger' is better than 'safety' because an increase in the number indicates a worse road. Let's keep it short though, I can't imagine repeating "bike_cost_multiplier" on every line of that file. I think danger or bikeDanger with a short explanation in the documentation should work.

thcrock commented 9 years ago

The ordering would come into play when you have the same tagset used for two different things. For instance, in the current config file, you have these two lines in different places:

    {"spec": "RLIS:bicycle=caution_area", "perms": "ALL", "danger": 1.45, "mixin": true},
    {"spec": "RLIS:bicycle=caution_area", "note": "caution", "matcher": "BICYCLE_MATCHER"},

Using a key-based system, these would have to be merged. Operationally, this isn't a problem, but if you wanted the bike_cost rules in one place, and the notes in another place, you couldn't do that.

thcrock commented 9 years ago

There's also the question about how to merge a configured file with any defaults. There are a couple ways to do it:

  1. Configure no defaults if properties are given in the config file, and only use what's in the config file
  2. Merge with the defaults

If we want to merge, there is a question of what property types make sense to add to the WayPropertySet and what property types make sense to replace existing properties. For instance, a bike danger multiplier would make sense to replace any existing bike danger multiplier for that tagset, but a note might be something you simply add. Does this make sense?

laurentg commented 9 years ago

It maybe more flexible to always replace. In that case if one want to simply add, he will have to re-add the initial value. It's a bit more verbose but probably in most case people will like to modify the existing default note, by simply adding the note it would not be possible.

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days