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.

Sign up for GitHub

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

Jump to bottom

distance_influence should not get a default value if no value is specified #2716

Merged
merged 5 commits into from Jan 10, 2023
Merged

distance_influence should not get a default value if no value is specified #2716

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2091d33
distance_influence should only be sent to server when explicitly spec…
karussell Jan 6, 2023
1a32582
fix changelog
karussell Jan 6, 2023
3a443b5
changelog
karussell Jan 6, 2023
8ae8077
CustomModel.getDistanceInfluence: allow null values
karussell Jan 9, 2023
945f484
remove comment
karussell Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
@@ -1,5 +1,6 @@
### 7.0 [not yet released]

- users of client-hc now have to set the distanceInfluence to 70 if a lower value is defined on the server-side and can remove it if they want to match the server-side value which is highly recommended for performance reasons, see #2716
- there is a new, required 'import.osm.ignored_highways' configuration option that must be used to not increase the graph size and decrease performance for motorized-only routing compared to previous versions, #2702
Copy link
Member

Choose a reason for hiding this comment

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

Can we not put it the other way around: Normally it should be null (so the server-side value will be taken) and users don't need to do anything. And if they set it to some value, well then this value will be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I phrased my comment in a way to know what to do when people use an old version and plan to upgrade. But probably this is misleading. What I meant is that after they upgraded to the new version it is recommended to remove the distanceInfluence for performance reasons and if they had a lower value than 70 on the server-side then (after the upgrade) they have to change it to 70 to get the identical routes as before.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see :) Maybe this:

There is no longer a default value for the distanceInfluence Parameter in custom models sent via client-hc. Previously it was 70. Not setting it explicitly now means the server-side value will be used.

- new osm_way_id encoded value, #2701
- the parameters vehicle, weighting, edge_based and turn_costs are no longer supported, use the profile parameter instead
Expand Down
2 changes: 1 addition & 1 deletion client-hc/src/test/java/com/graphhopper/api/GraphHopperWebTest.java
View file
Open in desktop
Expand Up @@ -95,7 +95,7 @@ public void customModel() throws JsonProcessingException {
.addToSpeed(Statement.If("road_class == MOTORWAY", Statement.Op.LIMIT, "80"))
.addToPriority(Statement.If("surface == DIRT", Statement.Op.MULTIPLY, "0.7"))
.addToPriority(Statement.If("surface == SAND", Statement.Op.MULTIPLY, "0.6"))
.setDistanceInfluence(69)
.setDistanceInfluence(69d)
.setHeadingPenalty(22)
.setAreas(areas);
GHRequest req = new GHRequest(new GHPoint(42.509225, 1.534728), new GHPoint(42.512602, 1.551558))
Expand Down
5 changes: 2 additions & 3 deletions core/src/main/java/com/graphhopper/routing/weighting/custom/FindMinMax.java
View file
Open in desktop
Expand Up @@ -23,10 +23,9 @@ public class FindMinMax {
public static void checkLMConstraints(CustomModel baseModel, CustomModel queryModel, EncodedValueLookup lookup) {
if (queryModel.isInternal())
throw new IllegalArgumentException("CustomModel of query cannot be internal");
if (queryModel.hasDistanceInfluence() && queryModel.getDistanceInfluence() < baseModel.getDistanceInfluence())
if (queryModel.getDistanceInfluence() < baseModel.getDistanceInfluence())
throw new IllegalArgumentException("CustomModel in query can only use " +
"distance_influence bigger or equal to " + baseModel.getDistanceInfluence() +
", but was: " + queryModel.getDistanceInfluence());
"distance_influence bigger or equal to " + baseModel.getDistanceInfluence() + ", but was: " + queryModel.hasDistanceInfluence());

checkMultiplyValue(queryModel.getPriority(), lookup);
checkMultiplyValue(queryModel.getSpeed(), lookup);
Expand Down
12 changes: 6 additions & 6 deletions core/src/test/java/com/graphhopper/GraphHopperTest.java
View file
Open in desktop
Expand Up @@ -271,7 +271,7 @@ private void testImportCloseAndLoad(boolean ch, boolean lm, boolean sort, boolea
new Coordinate(7.4198, 43.7355),
new Coordinate(7.4207, 43.7344),
new Coordinate(7.4174, 43.7345)}));
CustomModel customModel = new CustomModel().setDistanceInfluence(0);
CustomModel customModel = new CustomModel().setDistanceInfluence(0d);
customModel.getPriority().add(Statement.If("in_area51", Statement.Op.MULTIPLY, "0.1"));
customModel.getAreas().put("area51", area51Feature);
profile = new CustomProfile(profileName).setCustomModel(customModel).setVehicle(vehicle);
Expand Down Expand Up @@ -692,8 +692,8 @@ public void testCustomModel() {
);
assertDistance(hopper, customCar, customModelWithUnclassifiedRule, 19289);
// now we use distance influence to avoid the detour
assertDistance(hopper, customCar, new CustomModel(customModelWithUnclassifiedRule).setDistanceInfluence(200), 8725);
assertDistance(hopper, customCar, new CustomModel(customModelWithUnclassifiedRule).setDistanceInfluence(100), 14475);
assertDistance(hopper, customCar, new CustomModel(customModelWithUnclassifiedRule).setDistanceInfluence(200d), 8725);
assertDistance(hopper, customCar, new CustomModel(customModelWithUnclassifiedRule).setDistanceInfluence(100d), 14475);
}

private void assertDistance(GraphHopper hopper, String profile, CustomModel customModel, double expectedDistance) {
Expand Down Expand Up @@ -1631,8 +1631,8 @@ public void testLMConstraintsForCustomProfiles() {
setGraphHopperLocation(GH_LOCATION).
setOSMFile(MONACO).
setProfiles(
new CustomProfile("p1").setCustomModel(new CustomModel().setDistanceInfluence(100)).setVehicle("car"),
new CustomProfile("p2").setCustomModel(new CustomModel().setDistanceInfluence(100)).setVehicle("car")).
new CustomProfile("p1").setCustomModel(new CustomModel().setDistanceInfluence(100d)).setVehicle("car"),
new CustomProfile("p2").setCustomModel(new CustomModel().setDistanceInfluence(100d)).setVehicle("car")).
setStoreOnFlush(true);

hopper.getLMPreparationHandler().setLMProfiles(new LMProfile("p1"));
Expand All @@ -1644,7 +1644,7 @@ public void testLMConstraintsForCustomProfiles() {
assertEquals(3587, response.getBest().getDistance(), 1);

// use smaller distance influence to force violating the LM constraint
final CustomModel customModel = new CustomModel().setDistanceInfluence(0);
final CustomModel customModel = new CustomModel().setDistanceInfluence(0d);
response = hopper.route(new GHRequest(43.727687, 7.418737, 43.74958, 7.436566).
setCustomModel(customModel).
setProfile("p1").putHint("lm.disable", false));
Expand Down
8 changes: 4 additions & 4 deletions core/src/test/java/com/graphhopper/reader/osm/GraphHopperOSMTest.java
View file
Open in desktop
Expand Up @@ -816,7 +816,7 @@ public void testProfilesMustNotBeChanged() {
{
GraphHopper hopper = createHopperWithProfiles(Arrays.asList(
new Profile("car").setVehicle("car").setWeighting("fastest"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3)).setVehicle("car")
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3d)).setVehicle("car")
));
hopper.importOrLoad();
hopper.close();
Expand All @@ -825,7 +825,7 @@ public void testProfilesMustNotBeChanged() {
// load without problem
GraphHopper hopper = createHopperWithProfiles(Arrays.asList(
new Profile("car").setVehicle("car").setWeighting("fastest"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3)).setVehicle("car")
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3d)).setVehicle("car")
));
hopper.importOrLoad();
hopper.close();
Expand All @@ -834,7 +834,7 @@ public void testProfilesMustNotBeChanged() {
// problem: the profile changed (slightly). we do not allow this because we would potentially need to re-calculate the subnetworks
GraphHopper hopper = createHopperWithProfiles(Arrays.asList(
new Profile("car").setVehicle("car").setWeighting("fastest"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(80)).setVehicle("car")
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(80d)).setVehicle("car")
));
IllegalStateException e = assertThrows(IllegalStateException.class, hopper::importOrLoad);
assertTrue(e.getMessage().contains("Profiles do not match"), e.getMessage());
Expand All @@ -844,7 +844,7 @@ public void testProfilesMustNotBeChanged() {
// problem: we add another profile, which is not allowed, because there would be no subnetwork ev for it
GraphHopper hopper = createHopperWithProfiles(Arrays.asList(
new Profile("car").setVehicle("car").setWeighting("fastest"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3)).setVehicle("car"),
new CustomProfile("custom").setCustomModel(new CustomModel().setDistanceInfluence(3d)).setVehicle("car"),
new Profile("car2").setVehicle("car").setWeighting("fastest")
));
IllegalStateException e = assertThrows(IllegalStateException.class, hopper::importOrLoad);
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/com/graphhopper/routing/RoutingAlgorithmWithOSMTest.java
View file
Open in desktop
Expand Up @@ -130,7 +130,7 @@ public void testMonacoMotorcycleCurvature() {
queries.add(new Query(43.733802, 7.413433, 43.739662, 7.424355, 2423, 141));
queries.add(new Query(43.730949, 7.412338, 43.739643, 7.424542, 2253, 120));
queries.add(new Query(43.727592, 7.419333, 43.727712, 7.419333, 0, 1));
CustomModel model = new CustomModel().addToPriority(Statement.If("true", MULTIPLY, "curvature"));
CustomModel model = new CustomModel().setDistanceInfluence(70d).addToPriority(Statement.If("true", MULTIPLY, "curvature"));

GraphHopper hopper = createHopper(MONACO, new CustomProfile("motorcycle").setCustomModel(model).
setVehicle("motorcycle").setWeighting("custom"));
Expand Down