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

fix country rule ordering #1911

Merged
merged 3 commits into from
Feb 21, 2020
Merged

fix country rule ordering #1911

merged 3 commits into from
Feb 21, 2020

Conversation

karussell
Copy link
Member

@karussell karussell commented Feb 14, 2020

fixes #1875

As we currently already sort the strings https://github.com/graphhopper/graphhopper/blob/master/core/src/main/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupBuilder.java#L86

the only required change for #1875 would be to reorder the Country enum (AUT should come before DEU).

But as this is too error-prone I decided for a better & special handling when using the Country enum in combination with the SpatialRuleFactory. This is currently done in SpatialRuleLookupHelper.buildAndInjectSpatialRuleIntoGH. I have renamed this to a more Country-specific name as it is nothing generic. Also spatial_rules.borders_directory is renamed to country.borders_directory to make usage more clear. To avoid mixing this in the future I have removed the SpatialRuleParser constructor the with enum-Country default.

@karussell karussell added this to the 1.0 milestone Feb 14, 2020
@karussell karussell added the bug label Feb 14, 2020
if (!spatialRuleBordersDirLocation.isEmpty()) {
final BBox maxBounds = BBox.parseBBoxString(configuration.get("spatial_rules.max_bbox", "-180, 180, -90, 90"));
final BBox maxBounds = BBox.parseBBoxString(configuration.get("countries.max_bbox", "-180, 180, -90, 90"));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better singular country?

@otbutz
Copy link
Contributor

otbutz commented Feb 17, 2020

fixes #1875

As we currently already sort the strings https://github.com/graphhopper/graphhopper/blob/master/core/src/main/java/com/graphhopper/routing/util/spatialrules/SpatialRuleLookupBuilder.java#L86

the only required change for #1875 is to reorder the Country enum (AUT should come before DEU).

Looks like a good workaround until we finish #1808

I have renamed this to a more Country-specific name as it is nothing generic. Also spatial_rules.borders_directory is renamed to country.borders_directory to make usage more clear.

I'd like to stick with the more generic term as this directory might contain arbitrary borders in the very near future.

@karussell
Copy link
Member Author

Looks like a good workaround until we finish #1808

Sorry, I meant we could just reorder Country, but I did not do this workaround.

I'd like to stick with the more generic term as this directory might contain arbitrary borders in the very near future

Ok. Still for a more generic usage we would have to implement a more generic approach. Currently I do not see how we can provider arbitrary rules in Java, i.e. I would stick to the "country directory" and people with custom rules can have their custom directories?

@otbutz
Copy link
Contributor

otbutz commented Feb 17, 2020

Ok. Still for a more generic usage we would have to implement a more generic approach. Currently I do not see how we can provider arbitrary rules in Java, i.e. I would stick to the "country directory" and people with custom rules can have their custom directories?

My plan would be to ship our current rules as files. The user may provide further border and rule definitions.

e.g: spatial_rules.borders_directory + spatial_rules.rules_directory

@karussell
Copy link
Member Author

But how would the user provide Java code to implement these custom SpatialRules? Ah, you mean they specify with some properties what should be done with the borders similar to what we currently do with the ChangeGraphResource file format ("overlay.json")?

@otbutz
Copy link
Contributor

otbutz commented Feb 17, 2020

Ah, you mean they specify with some properties what should be done with the borders similar to what we currently do with the ChangeGraphResource file format ("overlay.json")?

Yes. Not sure about the format but i'm leaning towards a similar solution like you're using for the custom weighting support.

@karussell
Copy link
Member Author

Ah, IMO with CustomWeighting this would work the opposite: we define the custom or country borders as separate GeoJSON (or externally) and then use them to avoid the area or cap max_speed:

priority:
 # avoid a certain area
  area_custom1: 0.5

areas:
  custom1:
    type: "Feature"
    geometry: { type: "Polygon", coordinates: [[[13.722, 51.053], [...

@otbutz
Copy link
Contributor

otbutz commented Feb 18, 2020

Ah, IMO with CustomWeighting this would work the opposite: we define the custom or country borders as separate GeoJSON (or externally)

That would be the same for the external rules concept.

and then use them to avoid the area or cap max_speed:

External rules would either provide fallback values if the OSM material lacks sufficient tagging or could be used to overwrite existing values.

IMHO the custom weighting and the spatial rules are quite similar in that they allow to influence routing decisions for a given area. The main difference is that the user can define a lot of SpatialRules with detailed geometries without sacrificing runtime performance e.g for countries, states or environmental protection zones

@karussell
Copy link
Member Author

@otbutz do you think if I revert the renaming of spatial_rules.borders_directory to country.borders_directory we merge this here?

@otbutz
Copy link
Contributor

otbutz commented Feb 21, 2020

I'm just against the name change. The other changes look good 👍

@karussell karussell merged commit b57a55e into master Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PathDetails Country, DEU / AUT switched
2 participants