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

instruction steps should include mode separately #603

Closed
emiltin opened this issue Mar 16, 2013 · 18 comments
Closed

instruction steps should include mode separately #603

emiltin opened this issue Mar 16, 2013 · 18 comments

Comments

@emiltin
Copy link
Contributor

emiltin commented Mar 16, 2013

special instructions have been added to indicate when you enter/leave contraflow, ex when you start/stop pushing your bike.

the problem is that you're not told which way to turn. for example you're told to continue on bike, but should you go left, right or straight?

i suggest to move the mode out as a separate code. this would also have the benefit that you can easily identify sections of the route as ferry/train/etc.

@emiltin
Copy link
Contributor Author

emiltin commented Mar 20, 2013

another major problem with the current implementation is that you cannot mark footways, pedestrian streets, etc as pushing in both direction. you have only the concept of contraflow, but this doesn't cover the use cases.

(see ExtractorCallback.cpp line 100 onwards.)

again, a mode flag for each way would solve it.

@emiltin
Copy link
Contributor Author

emiltin commented Mar 21, 2013

lua script should return a mode for each direction. pushing of bikes sometimes happen in one direction, sometimes in both.

@emiltin
Copy link
Contributor Author

emiltin commented Jun 13, 2014

when would be a good time to work on this? we have a working implementation in iibjkecph fork that could be used as a starting point. only thing is repo is now quite far behind the main repo.

@emiltin
Copy link
Contributor Author

emiltin commented Aug 8, 2014

Hey @DennisOSRM, if a PR was created, would you want to merge it? This issues is one of the things blocking us from moving from our custom branch back to the mainline dev. For bike routing it's important.

Our implementation has worked for more than a year, and includes cucumber tests. The main commit was at ibikecph@b03ccfa, but it did include a few other changes if I recall correctly. basically the 'contraFlow' field is replaced with a 'mode' field. When processing a way segment, the lua script sets the mode value (ex 1=bike, 2=push, 3=ferry), see ex https:/ibikecph/Project-OSRM/blob/edge/profiles/bicycle_fast.lua#L298.

The mode value is then passed on to the as part of query rresults, so you can show 'push the bike' or 'take the ferry' to the user when approriate.

@emiltin
Copy link
Contributor Author

emiltin commented Aug 8, 2014

@DennisOSRM
Copy link
Collaborator

Happy to consider a PR. But there is no blanket yes or no to getting a PR merged. It boils down to three things:

  • clear approach
  • efficiency (in terms of space and time)
  • code quality (think not inelegant)

@emiltin
Copy link
Contributor Author

emiltin commented Aug 8, 2014

right. i just wanted to get your opinion on the basic ideas - replacing the contraflow bool with a mode value.

@emiltin
Copy link
Contributor Author

emiltin commented Aug 8, 2014

it would mean a few changes to the lua interface and to the instructions returned.

@DennisOSRM
Copy link
Collaborator

right. i just wanted to get your opinion on the basic ideas - replacing the contraflow bool with a mode value.

What does that mean for data structure size?

it would mean a few changes to the lua interface and to the instructions returned.

What kind of changes?

@emiltin
Copy link
Contributor Author

emiltin commented Aug 8, 2014

regarding data size, it don't think it changed anything in our branch, since the contraflow bool was not a bit field, and we just changed it to an unsigned char. if the current develop branch only uses 1 bit for the contraflow, you probably need to expand this to 2 or 3 bits for the mode field if you want to support a list like: bike, push, ferry, train. i haven't looked into how structs are packed etc. but i would expect a minor increase in data size. alternatively a separate list of modes could be constructed, with data only for those segment that has a non-zero mode, although this would incur some performance penalty.

in the lua script, you can set the mode for each way segment. of course this can easily defualt to mode 0, so you don't have to set it explicitly, unless the mode is something else than the normal mode.

for the instructions returned in queries, the special instructions codes 'enter contraflow' and 'leave contraflow' would not be used anymore. instead, and additional integer is passed for each instruction, containing the mode (0,1,2,3..)

@DennisOSRM
Copy link
Collaborator

regarding data size, it don't think it changed anything in our branch, since the contraflow bool was not a bit field, and we just changed it to an unsigned char. if the current develop branch only uses 1 bit for the contraflow, you probably need to expand this to 2 or 3 bits for the mode field if you want to support a list like: bike, push, ferry, train. i haven't looked into how structs are packed etc. but i would expect a minor increase in data size.

Can you provide links to current develop branch files (incl lines) where you would introduce changes? Otherwise this is too much of a speculation. Also, we need solid numbers on the size of the structs, because even minor changes replicate to lots of RAM on the planet-wide scale.

alternativelive a separate list of modes could be constructed, with data only for those segment that has a non-zero mode, although this would incur some performance penalty.

sounds complicated.

in the lua script, you can set the mode for each way segment. of course this can easily defualt to mode 0, so you don't have to set it explicitly, unless the mode is something else than the normal mode.

makes sense.

for the instructions returned in queries, the special instructions codes 'enter contraflow' and 'leave contraflow' would not be used anymore. instead, and additional integer is passed for each instruction, containing the mode (0,1,2,3..)

The client has to make sure to interpret these modes correctly, right?

@emiltin
Copy link
Contributor Author

emiltin commented Aug 8, 2014

the client can use the modes to enhance the instructions shown to the user. ex by showing 'take the ferry ...' instead of just 'turn right at..'. but ignoring them will not break anything as such.

in case you think the changes to lua and query results makes sense, and would be something you would merge (if implementated nicely) then i could look into the data sizes more closely, ie try to port the change to the current dev branch so see the effect.

@DennisOSRM
Copy link
Collaborator

the client can use the modes to enhance the instructions shown to the user. ex by showing 'take the ferry ...' instead of just 'turn right at..'. but ignoring them will not break anything as such.

Existing clients would need to be updated as the output format changes. Could this be done without breaking the JSON output format but enhancing it?

in case you think the changes to lua and query results makes sense, and would be something you would merge (if implementated nicely) then i could look into the data sizes more closely, ie try to port the change to the current dev branch so see the effect.

It seems that no change is necessary if one wants to stay on a default mode. This makes sense and allows for legacy scripts to continue working. Or at least conflict only minimal pain.

@emiltin
Copy link
Contributor Author

emiltin commented Aug 8, 2014

you mean could the info be stuffed somewhere inside the existing json structure? i don't really see how to do that.
however the only change to the json would be to append an additional value to each instruction array. most client would probably just ignore the extra value?

@DennisOSRM
Copy link
Collaborator

you mean could the info be stuffed somewhere inside the existing json structure? i don't really see how to do that.

Yeah, feared that. But that's a common inconvenience with new features

however the only change to the json would be to append an additional value to each instruction array. most client would probably just ignore the extra value?

Probably, yes. And if not, should be an easy fix.

@emiltin
Copy link
Contributor Author

emiltin commented Aug 8, 2014

sounds like you're positive towards adding this feature, if it can be implemented efficiently?

@DennisOSRM
Copy link
Collaborator

sure

DennisOSRM added a commit that referenced this issue Aug 21, 2014
@DennisOSRM
Copy link
Collaborator

@emiltin can we close here?

@emiltin emiltin closed this as completed Aug 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants