Skip to content
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

Introduce RoadClass, RoadEnvironment, CarMaxSpeed, Surface, Toll and more EncodedValues #1548

Merged
merged 51 commits into from Jun 14, 2019

Conversation

3 participants
@karussell
Copy link
Member

commented Feb 21, 2019

This PR shows how shared EncodedValues look like and how we populate them (via TagParsers). The prefix "OSM" indicates that such parsers can look differently for a different data source. A TagParser has not necessarily a one-to-one relationship to an EncodedValue. With the new mechanism we can have multiple EncodedValues and just one TagParser like e.g. for height, width and weight.

We likely need RoadClass, RoadEnvironment and a few more to be added by default. (As we could really need them for improving instructions, improving ETAs, finally fixing bugs like #713 for all vehicles, ...)

image

image

The following EncodedValues are available as path detail and could be used to influence the routing for all vehicle profiles (see removed AvoidWeighting):

  • roundabout
  • road_class, road_class_link
  • road_environment
  • road_access
  • max_speed (earlier versions: car_max_speed)
  • surface
  • country
  • max_height, max_width, max_weight
  • toll

Done:

  • refactor DataFlagEncoder to use those new EncodedValues
  • improve calculating instructions with e.g. roadclass -> separate issue
  • refactor Tunnel&Bridge interpolator to use RoadEnvironment and enable it for all vehicles!
  • The new path details classes show that as soon as there is an EncodedValue, we can show it along the path, e.g. replace AverageSpeedDetails (see #1551)
  • added toll and car_max_speed EncodedValue and parser
  • add a few of these EncodedValues per default
  • introduced a simple AvoidWeighting that supports toll, motorway, ferry etc avoidance (or preference)
  • remove AvoidWeighting (move to separate issue)
  • implement a loading mechanism like we have for flag encoders
  • make it possible to enable storing those new fields without the DataFlagEncoder (Should be possible with graph.encoded_values=surface,road_class!)
  • new EncodedValueFactory and TagParserFactory to provide custom builds also loading from configuration
  • new SpatialRuleParser to provide country-based decisions to other vehicle profiles
  • It could be even good to introduce a dependency of TagParsers. E.g. the SpatialRuleParser requires the info from RoadAccessParser. We could enforce this via checking the EncodedValueLookup in createEncodedValue. Can be done with lookup.hasEncodedValue
  • maxspeed returns 0 for default value (no sign) in Java API only, breaks backward compatibility => if fixed remove TODO NOW
  • DataFlagEncoder requires e.g. road_environment - throw an exception if not added before. Breaks backward compatibility.
  • remove all TODO NOW in src and test
  • should we introduce special rounding behaviour? E.g. for max_weight or max_height it can happen that the value increases due to rounding which might be a problem. For now do not do this
  • generic MappedStringEncodedValue -> different issue
  • a real ObjectEncodedValue that uses e.g. MapDB under the hood and just stores the integer ID in the edge -> different issue
  • move back to EnumEncodedValue and remove the IndexedBase stuff, see #1570, see this branch and the resulting changes
  • rename max_speed to speed_limit? no. let's just use max and average speed...
  • default value is 0, but use a different value e.g. for max_speed (infinity)
  • support "unlimited" on German autobahns: maybe we just use the maximum value. E.g. 5 bits and factor==5 => 5*(2⁵-1)=155. But then custom implementations couldn't easily change the precision of maxspeed as this changes the maximum value too. Maybe let's define 30*5=150 (can be precisely returned if precision is 5, 3, 2, 1). As the 140 limit exists several times and 160 is very rare (basically 2 ways world wide) and everything above is a tagging mistake or something like this, 2, 3, 4. Valhalla uses 255 (8 bits). If we use 150 max speed, then the max average speed is 0.9*150=135
  • other should not be the first (default) access
  • rename file back to map.geo.json
  • improve error message of MappedDecimalEncodedValue; for now remove unused MappedDecimalEncodedValue?
  • should int details also get returnMinus like decimals? removed this param for now.
  • try returning destination access for tracks
  • fix toll order handling (see comment)
  • there should be an exception if only one direction is supported but setReverse is used. Also make the twoDirections variable accessible from outside.
  • for JSON return -1 instead of the default "infinity" string value. Update: return null
  • Forestry is not a highway (we would tag it via track and maybe add access=forestry) but add briddleway.

later:

  • For the JSON API we should omit default entries if they are infinity and not return -1
  • important follow up: #1269 and #1551

karussell added some commits Feb 25, 2019

added a bunch of helpful EncodedValues: RoadClass, RoadEnvironment, .…
….. and make TagParser better separated but easy to add via GHUtility.addDefaultEncodedValues

@aramzl aramzl referenced this pull request Feb 26, 2019

Open

Lane info based on encoder refactoring #1269

0 of 3 tasks complete

@karussell karussell added this to the 0.13 milestone Feb 27, 2019

@karussell karussell changed the title (WIP) add some EncodedValue per default Introduce RoadClass, RoadEnvironment, CarMaxSpeed, Surface and more EncodedValues Feb 27, 2019

karussell added some commits Feb 28, 2019

karussell added some commits Mar 6, 2019

fixed bug in EncodingManager.toEncodedValuesString; separated max_wid…
…th, max_weight, max_height; differentiate between shared and FlagEncoder-bound EncodedValues to avoid loading them

@karussell karussell referenced this pull request Mar 7, 2019

Open

Truck profile #223

karussell added some commits Mar 7, 2019

karussell added a commit that referenced this pull request Mar 11, 2019

@@ -473,7 +473,7 @@ public void testStallOnDemandViaVirtuaNode_issue1574() {
// would not happen, because node 2 would not even be explored in the forward search, but because of the virtual
// node the strict upward search is modified and goes like 0-3-1-2 (i.e. it finds node 2).
// so no we fine tune the weight for 2-4 such that node 4 gets also stalled
edge24.setReverse(carEncoder.getAverageSpeedEnc(), 27.5);
edge24.set(carEncoder.getAverageSpeedEnc(), 27.5);

This comment has been minimized.

Copy link
@karussell

karussell May 29, 2019

Author Member

@easbar this was necessary to make the test pass.

When I allow both directions via:

new CarFlagEncoder(new PMap("speed_two_directions=true"))

then I get 4 shortcuts (not 2) and a different path:

java.lang.AssertionError: wrong or no path found
Expected :[0, 3, 8, 1, 2, 4, 5, 6, 7]
Actual   :[0, 3,    1, 2, 4, 5, 6, 7]

This comment has been minimized.

Copy link
@easbar

easbar May 30, 2019

Collaborator

Ok this is strange. setting speed_two_directions only allows setting two different speeds right ? all the edges are bidirectional anyway. And what I really do not understand is that the calculated path seems to skip the (virtual) node 8 (probably because some shortcut is used, but this should not be). I will take a closer look to see what is going on here.

This comment has been minimized.

Copy link
@easbar

easbar Jun 1, 2019

Collaborator

Ok I took a look at this test and fixed some inaccurate comments, see here: ec01e5e.
I also explicitly set the speed for both directions in this test now, see here:
c979cbd

then I get 4 shortcuts (not 2)

Ok this is clear, because here we are only counting the number of shortcut edges (independent of their access flags). so far set() set the speed for both directions, but with the new code only one of the directions is set, right ? if the edge speeds are different for the two directions the previous two 'bidirectional' shortcuts will turn into four unidirectional ones.

If I comment out the assertEquals(there are two shortcuts), the next thing that fails is this:
sptWeight at node 4 should be smaller than shortcut weight 3->4 to make sure node 4 gets stalled
This is because you changed setReverse to set and the getWeight method read the wrong weight (it encounters the 'wrong' edge first and returns it), I fixed this here: a529753

And what I really do not understand is that the calculated path seems to skip the (virtual) node 8

This is because when we use setReverse instead of set the edge weights are not such that the stalling happens. When the best path is found via the 3->4 shortcut the virtual node will not be included in the final path (because when the shortcut is unpacked it does not know about QueryGraphs virtual node).

setReverse was wrong in the test so far, but so far this was no problem, because it did the same as set (it set both directions).

This comment has been minimized.

Copy link
@easbar

easbar Jun 1, 2019

Collaborator

If you update to master no changes should be required for the test to work and setting speed_two_directions=true should be fine.

The best way to check everything is fine is reverting the #1574 fix (set precision back to 0 in the stalling condition in DijkstraBidirectionCH) and make sure no path is found. Also run RandomCHRouting#random many times as described in it's comment, but be aware that it currently does not use speed_two_directions=true. And if you enabled different speed for both directions it actually fails for edge-based unless we disable loops, see #1631.

@@ -107,7 +107,10 @@ public final void setInt(boolean reverse, IntsRef ref, int value) {
}

final void uncheckedSet(boolean reverse, IntsRef ref, int value) {
if (storeBothDirections && reverse) {
if (reverse && !storeBothDirections)
throw new IllegalArgumentException(getName() + ": value for reverse direction would overwrite forward direction. Enable 'storeBothDirections' or always use reverse==true when storing the value");

This comment has been minimized.

Copy link
@easbar

easbar May 29, 2019

Collaborator

Nice! Will this fix #1588 ?

This comment has been minimized.

Copy link
@karussell

karussell May 30, 2019

Author Member

I think so. As it avoids the overwriting of the reverse property (setReverse throws an exception now).

Do you think we should do the same for getReverse without bi-direction support? Currently I don't do this for convenience reasons (no need to do if(encodedValue.isStoreBothDirections) everytime you access a property and you want bi-directional support e.g. in a Weighting).

This comment has been minimized.

Copy link
@easbar

easbar May 30, 2019

Collaborator

Do you think we should do the same for getReverse

I would say no. If you call getReverse even though its not bidirectional that's just fine you get the same value as if you had called get. I do not think this would be something unexpected. You do not know the value beforehand and calling get or getReverse gives you some value you need to trust. Maybe both directions are the same or the property is not bi-directional - this should not be of your concern when just calling the getter.

In comparison, calling setReverse, really gives you the impression that you can set a special reverse value, but if its not bidirectional it will be lost or even overwrite the other direction, which is a nasty surprise, so here I think it is good that it throws now.

karussell added some commits May 30, 2019

karussell added some commits May 30, 2019

rename property from storeBothDirections into storeTwoDirections (sim…
…ilar to speed_two_directions for CarFlagEncoder)
@karussell

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Currently testing this in production and the minor problem is motorroad=yes. Currently we have this in RoadClass.java but the following is missing in the OSMRoadClassParser:

  if (!Helper.isEmpty(highwayValue) && way.hasTag("motorroad", "yes")
                && highwayValue != "motorway" && highwayValue != "motorway_link")
    highwayValue = "motorroad"; // or remove RoadClass.MOTORROAD and use trunk or motorway here?

(this is also done in master CarFlagEncoder.getSpeed and we wouldn't loose much information as motorroad=yes is often a trunk highway)

Will have a look how this is done for other road data. Update: it seems there is no such property in the other data set and these roads are just trunks with additional access restrictions.

karussell added some commits Jun 9, 2019

@boldtrn

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

I personally would prefer to use trunk or motorway instead of motorroad. In OSM the highway is set to trunk or motorway, so returning motorroad seems a bit wrong IMHO. Maybe it would make sense to store motorrad as a Boolean?

@karussell

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2019

Yeah, I feel the same. I have removed motorroad from the RoadClass enum. And we can later support motorroad maybe with some country dependent access tags. The OSM wiki says that there are only some access differences to trunk.

I think this PR is now in a good shape and will merge it in the next days if no objections.

karussell added some commits Jun 12, 2019

@karussell karussell merged commit 4e143c4 into master Jun 14, 2019

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk - pom.xml (karussell) No new issues
Details
security/snyk - web/package.json (karussell) No manifest changes detected

karussell added a commit that referenced this pull request Jun 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.