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

Move ProfileResolver out of GraphHopper class #1958

Merged
merged 38 commits into from Mar 23, 2020
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
1f890aa
Throw error if request has weighting or vehicle, use profile paramete…
easbar Mar 10, 2020
b461512
Fix some of the failing tests
easbar Mar 10, 2020
aeffb34
Add a few more checks
easbar Mar 11, 2020
a021160
Move profile resolver from GraphHopper to Route/Isochrone/SPTResource…
easbar Mar 15, 2020
4267ea5
Minor
easbar Mar 15, 2020
aedcc4d
Add/move some profile resolving tests
easbar Mar 17, 2020
9a43eef
Minor fixes
easbar Mar 17, 2020
8d1eeda
Fix test
easbar Mar 17, 2020
e3e628b
Remove vehicle/weighting parameters in measurement
easbar Mar 17, 2020
13c02ea
Add todo to respect profile parameter later
easbar Mar 17, 2020
18d117a
Add integration test for edge_based/turn_costs parameters
easbar Mar 17, 2020
2c97604
Fix formatting in Measurement.java
easbar Mar 17, 2020
ff55558
Fix measurement: Add LM profiles with and without turncosts.
easbar Mar 17, 2020
3751974
Add javadocs for ProfileResolver
easbar Mar 17, 2020
c9c7e36
Replace hints.get(EDGE_BASED, "") with hints.has(EDGE_BASED)
easbar Mar 18, 2020
798177a
Merge branch 'master' into profile_parameter2
easbar Mar 19, 2020
96fec57
Fixes after merge
easbar Mar 19, 2020
12f06fb
Update api-doc
easbar Mar 19, 2020
7a0844b
minor
easbar Mar 19, 2020
df15f83
Strictly separate profiles with/without turn costs also for LM
easbar Mar 19, 2020
8d57c53
Update todos
easbar Mar 19, 2020
757cc1d
Update issue references and todos
easbar Mar 19, 2020
4c770ec
Merge profile hints with request hints before creating the weighting.
easbar Mar 19, 2020
cb7b451
prepare merge
easbar Mar 19, 2020
409cad2
Merge branch 'master' into profile_parameter2
easbar Mar 19, 2020
95d5ed2
minor
easbar Mar 19, 2020
e774c1c
Fix test after merging master
easbar Mar 19, 2020
8696944
Merge branch 'master' into profile_parameter2
easbar Mar 20, 2020
ac9280f
Merge branch 'master' into profile_parameter2
easbar Mar 21, 2020
1427822
Adjust after merging master, but test still fails
easbar Mar 23, 2020
00c74ab
Pick another test from master
easbar Mar 23, 2020
48c997b
Pick recent changes from master, remove default_vehicle=encoders[0] rule
easbar Mar 23, 2020
4d60cfb
Remove default vehicle for GET requests from RouteResource
easbar Mar 23, 2020
f3c63e9
Update profile selection test
easbar Mar 23, 2020
e55c94d
Merge branch 'master' into profile_parameter2
easbar Mar 23, 2020
50116c5
Remove todos
easbar Mar 23, 2020
019beb4
Some minor cleanups
easbar Mar 23, 2020
116a864
Add comments
easbar Mar 23, 2020
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
12 changes: 12 additions & 0 deletions api/src/main/java/com/graphhopper/GHRequest.java
Expand Up @@ -34,6 +34,9 @@
*/
public class GHRequest {
private List<GHPoint> points;
// todo #1934: keep this here or put it into hints, and even more important: remove vehicle+weighting from
// hints?
private String profile = "";
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 think we discussed this in #1859 already: We'll def. remove vehicle+weighting from the HintsMap, but maybe in a follow-up PR? Since the profile parameter will be required it makes sense to keep it as a separate field?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would also use it here and remove weighting and vehicle in a later PR

private final HintsMap hints = new HintsMap();
// List of favored start (1st element) and arrival heading (all other).
// Headings are north based azimuth (clockwise) in (0, 360) or NaN for equal preference
Expand Down Expand Up @@ -223,6 +226,15 @@ public GHRequest setLocale(String localeStr) {
return setLocale(Helper.getLocale(localeStr));
}

public String getProfile() {
return profile;
}

public GHRequest setProfile(String profile) {
this.profile = profile;
return this;
}

public String getWeighting() {
return hints.getWeighting();
}
Expand Down
3 changes: 2 additions & 1 deletion config-example.yml
Expand Up @@ -22,7 +22,8 @@ graphhopper:

##### Routing Profiles ####

# Routing can be done for the following list of profiles. The fields of each profile are as follows:
# Routing can be done for the following list of profiles. Note that it is required to specify all the profiles you
# would like to use here. The fields of each profile are as follows:
# - name (required): a unique string identifier for the profile
# - vehicle (required): refers to the `graph.flag_encoders` used for this profile
# - weighting (required): the weighting used for this profile, e.g. fastest,shortest or short_fastest
Expand Down
2 changes: 2 additions & 0 deletions core/files/changelog.txt
@@ -1,4 +1,6 @@
1.0
removed vehicle,weighting and edge_based from GraphHopper class, replaced with new profile parameter, #1958
all routing profiles have to be configured when setting up GraphHopper, #1958
removed IPFilter. Use a firewall instead.
PMap refactored. It is recommended to use putObject(String, Object) instead of put, #1956
removed UnsafeDataAccess as not maintained, see #1620
Expand Down
64 changes: 35 additions & 29 deletions core/src/main/java/com/graphhopper/GraphHopper.java
Expand Up @@ -24,7 +24,10 @@
import com.graphhopper.reader.DataReader;
import com.graphhopper.reader.dem.*;
import com.graphhopper.reader.osm.conditional.DateRangeParser;
import com.graphhopper.routing.*;
import com.graphhopper.routing.AlgorithmOptions;
import com.graphhopper.routing.Path;
import com.graphhopper.routing.RoutingAlgorithmFactory;
import com.graphhopper.routing.RoutingAlgorithmFactorySimple;
import com.graphhopper.routing.ch.CHPreparationHandler;
import com.graphhopper.routing.ch.CHRoutingAlgorithmFactory;
import com.graphhopper.routing.lm.LMPreparationHandler;
Expand Down Expand Up @@ -102,8 +105,6 @@ public class GraphHopper implements GraphHopperAPI {
private boolean smoothElevation = false;
// for routing
private final RoutingConfig routingConfig = new RoutingConfig();
private ProfileResolver profileResolver = new ProfileResolver();

// for index
private LocationIndex locationIndex;
private int preciseIndexResolution = 300;
Expand Down Expand Up @@ -140,13 +141,6 @@ protected GraphHopper loadGraph(GraphHopperStorage g) {
return this;
}

FlagEncoder getDefaultVehicle() {
if (encodingManager == null)
throw new IllegalStateException("No encoding manager specified or loaded");

return profileResolver.getDefaultVehicle(encodingManager);
}

public EncodingManager getEncodingManager() {
return encodingManager;
}
Expand Down Expand Up @@ -491,15 +485,6 @@ public GraphHopper setTagParserFactory(TagParserFactory factory) {
return this;
}

public ProfileResolver getProfileResolver() {
return this.profileResolver;
}

public GraphHopper setProfileResolver(ProfileResolver profileResolver) {
this.profileResolver = profileResolver;
return this;
}

/**
* Reads the configuration from a {@link GraphHopperConfig} object which can be manually filled, or more typically
* is read from `config.yml`.
Expand Down Expand Up @@ -824,6 +809,9 @@ public boolean load(String graphHopperFolder) {
}

private void checkProfilesConsistency() {
if (profilesByName.isEmpty()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

There are a few odd cases where we do not use GraphHopper for routing (like NearestResource) so these do not necessarily require profiles. Not sure, to me this seems rather the exception so I am checking the existence of at least one profile here. Once we do #1901 we can move this check into GraphHopperRouter's constructor for example.

throw new IllegalArgumentException("No routing profiles have been specified, you need to configure at least one");
}
for (ProfileConfig profile : profilesByName.values()) {
if (!encodingManager.hasEncoder(profile.getVehicle())) {
throw new IllegalArgumentException("Unknown vehicle '" + profile.getVehicle() + "' in profile: " + profile + ". Make sure all vehicles used in 'profiles' exist in 'graph.flag_encoders'");
Expand Down Expand Up @@ -864,13 +852,6 @@ private void checkProfilesConsistency() {
}
}

public ProfileConfig resolveProfile(HintsMap hints) {
if (encodingManager == null)
throw new IllegalStateException("No encoding manager specified or loaded");

return profileResolver.resolveProfile(encodingManager, chPreparationHandler.getCHProfiles(), lmPreparationHandler.getLMProfiles(), hints);
}

public RoutingAlgorithmFactory getAlgorithmFactory(String profile, boolean disableCH, boolean disableLM) {
if (chPreparationHandler.isEnabled() && disableCH && !chPreparationHandler.isDisablingAllowed()) {
throw new IllegalArgumentException("Disabling CH is not allowed on the server side");
Expand Down Expand Up @@ -1030,6 +1011,17 @@ public List<Path> calcPaths(GHRequest request, GHResponse ghRsp) {
Lock readLock = readWriteLock.readLock();
readLock.lock();
try {
if (!request.getVehicle().isEmpty())
throw new IllegalArgumentException("GHRequest may no longer contain a vehicle, use the profile parameter instead, see #1958");
if (!request.getWeighting().isEmpty())
throw new IllegalArgumentException("GHRequest may no longer contain a weighting, use the profile parameter instead, see #1958");
if (request.getHints().has(Routing.TURN_COSTS))
throw new IllegalArgumentException("GHRequest may no longer contain the turn_costs=true/false parameter, use the profile parameter instead, see #1958");
if (request.getHints().has(Routing.EDGE_BASED))
throw new IllegalArgumentException("GHRequest may no longer contain the edge_based=true/false parameter, use the profile parameter instead, see #1958");

// todo later: do not allow things like short_fastest.distance_factor or u_turn_costs unless CH is disabled and only under certain conditions for LM

HintsMap hints = request.getHints();
boolean disableCH = hints.getBool(CH.DISABLE, false);
if (chPreparationHandler.isEnabled() && !chPreparationHandler.isDisablingAllowed() && disableCH)
Expand All @@ -1056,10 +1048,17 @@ public List<Path> calcPaths(GHRequest request, GHResponse ghRsp) {
// For example see #734
checkIfPointsAreInBounds(points);

ProfileConfig profile = resolveProfile(hints);
if (Helper.isEmpty(request.getProfile())) {
throw new IllegalArgumentException("You need to specify a profile to perform a routing request, see #1958");
}
ProfileConfig profile = profilesByName.get(request.getProfile());
if (profile == null) {
throw new IllegalArgumentException("The requested profile '" + request.getProfile() + "' does not exist");
}
if (!profile.isTurnCosts() && !request.getCurbsides().isEmpty())
throw new IllegalArgumentException("To make use of the " + CURBSIDE + " parameter you need to use a profile that supports turn costs");

// todo later: should we be able to control this using the edge_based parameter?
TraversalMode tMode = profile.isTurnCosts() ? TraversalMode.EDGE_BASED : TraversalMode.NODE_BASED;

RoutingAlgorithmFactory algorithmFactory = getAlgorithmFactory(profile.getName(), disableCH, disableLM);
Expand Down Expand Up @@ -1350,13 +1349,20 @@ public DefaultWeightingFactory(EncodingManager encodingManager, GraphHopperStora
this.ghStorage = ghStorage;
}

public Weighting createWeighting(ProfileConfig profile, PMap hints) {
public Weighting createWeighting(ProfileConfig profile, PMap requestHints) {
// Merge profile hints with request hints, the request hints take precedence.
// Note that so far we do not check if overwriting the profile hints actually works with the preparation
// for LM/CH. Later we should also limit the number of parameters that can be used to modify the profile.
PMap hints = new PMap();
hints.putAll(profile.getHints());
hints.putAll(requestHints);
Copy link
Member Author

@easbar easbar Mar 20, 2020

Choose a reason for hiding this comment

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

These are (the very simple) rules how we merge the profile hints with the request hints. The only thing I am wondering now is how we do this for block_area? Shall we do this in a similar way (use hints from profile and overwrite with request hints)? Do we need this here or is it ok to only create block areas from request hints (not the profile) here, and add this in #1776? Its still a bit unfortunate that we do not do the block_area stuff inside createWeighting. Now it looks all we are missing to do this in createWeighting are the points?

Copy link
Member

Choose a reason for hiding this comment

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

The only thing I am wondering now is how we do this for block_area?

Hmmh, we would need to add it to the list of shapes

Do we need this here or is it ok to only create block areas from request hints (not the profile) here

I would postpone this and properly support this in CustomWeighting.

Now it looks all we are missing to do this in createWeighting are the points

Yeah, although we only need this because of the is-in-area check which we could keep out of the Weighting creation somehow.


FlagEncoder encoder = encodingManager.getEncoder(profile.getVehicle());
TurnCostProvider turnCostProvider;
if (profile.isTurnCosts() && !hints.getBool("__disable_turn_costs_for_lm_preparation", false)) {
if (!encoder.supportsTurnCosts())
throw new IllegalArgumentException("Encoder " + encoder + " does not support turn costs");
int uTurnCosts = profile.getHints().getInt(Routing.U_TURN_COSTS, INFINITE_U_TURN_COSTS);
int uTurnCosts = hints.getInt(Routing.U_TURN_COSTS, INFINITE_U_TURN_COSTS);
turnCostProvider = new DefaultTurnCostProvider(encoder, ghStorage.getTurnCostStorage(), uTurnCosts);
} else {
turnCostProvider = NO_TURN_COST_PROVIDER;
Expand Down