abrensch / brouter

configurable OSM offline router with elevation awareness, Java + Android
MIT License
490 stars 118 forks source link

Introducing engineMode for future use #549

Closed afischerdev closed 1 year ago

afischerdev commented 1 year ago

To build a base for future an engine mode is added. This is related to #460 or #222 or other things that will not working with 'normal' routing mode.

afischerdev commented 1 year ago

I'm not really sure how this PR is related to the linked issues, because the modes are called "seed" and "other" but the issues request automatic route suggestion and map matching.

The idea is to open the engine to more functionality than standard routing. The linked issues are samples to let you know what I'm thinking off. The predefined modes have to change when we have a real new feature. "seed" already exists, but I think it needs changing - anyway we should not ignore it.

As with your last huge PR I would suggest first adding the feature and afterwards exposing the API because it allows you to freely change the implementation without modifying a public API.

May be. For me, this way is the better way. This offers you and others the chance to place new features on these terms. Otherwise we develop nice new features and when the first one comes out with it, the others notice that the api implementation is different.

afischerdev commented 1 year ago

Here a first new function that uses this new entry:

get elevation for one point

Returns a wpt gpx with elevation when found.

zod commented 1 year ago

I think it's not sensible to extend the getTrackFromParams to provide elevation data using this method. It's rather complicated and the function would not return a track but just a single waypoint in a GPX structure, but the API should just return the elevation.

Shouldn't we just extend the AIDL with a new method to retrieve elevation?

afischerdev commented 1 year ago

@zod Yes, I was thinking of this as well. We already have a hell of parameter. An extra aidl definition is a smart way but would not work for server or command line situation. So I would like to stay with engineMode. For the parameter hell, I'm planning a small rework to get one entry point for everyone

zod commented 1 year ago

We could also add an API endpoint for the server to provide elevation which could be used e.g. by brouter-web to show elevation at POIs. I think brouter-web would rather consume a simple JSON instead of parsing a GPX file with waypoints just for elevation.

For the CLI it should be possible to provide a different command.

I just skimmed through the code and if we would pursue the engineMode way it requires a bit of work (e.g. parameter order in RoutingEngine constructor)

afischerdev commented 1 year ago

@zod

I just skimmed through the code and if we would pursue the engineMode way it requires a bit of work (e.g. parameter order in RoutingEngine constructor)

What do you mean be that?

And yes, for the server I also thought on json export as well. As usual initialized by parameter. You wanted me to publish smaller pieces. So next, let me introduce the server and the command line variant along with a first parameter merge

zod commented 1 year ago

I'm quite surprised that it was merged. I still fail to understand why it's a good idea to add reading points elevation data to RoutingEngine. This class already does way to much.

What's your reason to not provide a new API endpoint & Android service which provides just elevation data without the GPX stuff?

afischerdev commented 1 year ago

@zod

What's your reason to not provide a new API endpoint & Android service which provides just elevation data without the GPX stuff?

As discussed before the server also could use this call. You wanted me not to add an api call without feature. So I added a small function. This is a sample for engineMode but main intentions are others.