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

Use turn:lanes to display the appropriate symbols for lanes #632

Closed
wants to merge 24 commits into from

Conversation

saiarcot895
Copy link
Contributor

If available and valid, the turn:lanes tag will be used to determine the symbols to show for each lane. Support for all values except U-turn and lane merges have been added; however, the symbols are not aligned yet and may look bad.

Unfortunately, there isn't enough usage of this to get a good testing ground, but this way looks good.

@vshcherb
Copy link
Member

This is very neat change. I like it. I have only 2 comments will put in place.

@@ -543,6 +543,78 @@ private TurnType attachKeepLeftInfoAndLanes(boolean leftSide, RouteSegmentResult
}
// }

String turnLanes = prevSegm.getObject().getValue("turn:lanes");
Copy link
Member

Choose a reason for hiding this comment

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

Please move whole code into separate function and call it "getTurnLanesData" or something

@vshcherb
Copy link
Member

I think this is very important change, please let me know if you need any help.
Thanks

@saiarcot895
Copy link
Contributor Author

I made the changes you mentioned except for having the methods only because there would be 8 methods for this. If you want, I can go ahead and create those methods.

Also, I removed some code that wouldn't be necessary. I didn't realize at the time that I could just use the TurnType.value() method to determine what turn was needed.

Known Bugs

Some ways that have a valid turn:lanes tag and value and that normally have the lanes displayed won't be using the turn:lanes tag. This is because I added a restriction that requires the number of lanes as calculated by the current method in OsmAnd be equal to the total number of allowed "turns". This is so that the right lanes get the right color (yellow/green vs gray). I could remove the restriction and instead use the turn type to determine which lanes to color, but that introduces some breakages/complication (like this turn; if you were taking a U-turn here, none of the lanes would be colored).

In addition, at the next intersection with the right-turn split, the left-most lane would be turning left and is marked as such; however, that lane and the two other lanes going straight would be green since, at that point, all three lanes are allowed.

Currently, the TSLL and TSLR icons are used for "slight left" and "slight right". However, the icons are not that distinguishable from the C (straight) icon. The KL and KR icons could be used instead, but aligning the KR icon with the C icon is a little problematic, since part of the icon is cut off from rendering.

Screenshots

device-2014-05-13-125110

No turn:lanes tag present.

device-2014-05-13-124916

turn:lanes tag present, and this is an exit-only lane.

device-2014-05-13-125132

turn:lanes tag present, and the right-most lane goes straight and exits right.

device-2014-05-13-125522

turn:lanes tag present, and the 3-lane road splits into 2 2-lane road.

device-2014-05-13-134449

turn:lanes tag present, but is not used because of the restriction.

device-2014-05-13-134349

turn:lanes tag present, but the left-turn lane and the straight-only lanes are green because at that point, they're going straight.

public static final int BIT_LANE_USE_SLIGHT_LEFT = 1 << 12;
public static final int BIT_LANE_USE_RIGHT = 1 << 12 | 1 << 11;
public static final int BIT_LANE_USE_LEFT = 1 << 13;
public static final int BIT_LANE_USE_SHARP_RIGHT = 1 << 13 | 1 << 11;
Copy link
Member

Choose a reason for hiding this comment

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

BIT_LANE_USE_SHARP_RIGHT
BIT_LANE_USE_SHARP_LEFT
BIT_LANE_USE_UTURN
BIT_LANE_USE_RIGHT
but they are are using OR , so BIT_LANE_USE_RIGHT = BIT_LANE_USE_SLIGHT_LEFT | BIT_LANE_USE_SLIGHT_RIGHT. What is the logic about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's like bit fields.

BIT_LANE_USE_STRAIGHT = 000
BIT_LANE_USE_SLIGHT_RIGHT = 001
BIT_LANE_USE_SLIGHT_LEFT = 010
BIT_LANE_USE_RIGHT = 011
BIT_LANE_USE_LEFT = 100
BIT_LANE_USE_SHARP_RIGHT = 101
BIT_LANE_USE_SHARP_LEFT = 110
BIT_LANE_USE_UTURN = 111

That being said, I'm considering removing those fields, since they probably won't be used.

@vshcherb
Copy link
Member

We need to show green only direction which is needed. For example if one lane straight/right we need to indicate only "right" but keep straight grey.
Also we need to show icons in the right order. So the green direction should be on top of the grey. But I'm not sure if it is possible to combine turn_slightly and go straight.
I think we might need to test all combinations )

  • straight + slightly/normally/sharply right
  • left + uturn ( here I think it doesn't make sense to show left turn at all or anything else)
  • slightly/normally left + slightly/normally right

My idea is to develop new appendix which could be drawn separately from main turn and will be drawn under main turn, so little overlap is possible . If it is not possible, then I would strongly suggest to not combine turns at all and only show main direction turn.

'turn:lanes tag present, but is not used because of the restriction.'
Could you please explain screenshot with restriction, why all directions are green?

@saiarcot895
Copy link
Contributor Author

The screenshot for the text is the one above, so it's the one with just the straight arrows. I'll edit the post to add some spacing.

Edit: something in Github's system is preventing me from adding spacing.

@saiarcot895
Copy link
Contributor Author

If the user is on a freeway and taking an exit, the turn will be considered a keep-right or slightly right turn, and so the coloring should be fine (once I add in the code for selective rendering). However, if the user isn't taking the exit, the turn will be considered as a keep-left, but in actuality, you're just going straight (and is probably going to be marked as such in turn:lanes). To get around this, I might be able to add a continue turn into the attachKeepLeftInfoAndLanes method if the difference in degrees is less than, say, 5, and the names before and after are the same. I'm not sure what potential regressions there might be in other situations.

In the left+U-turn case, couldn't just the left turn arrow be shown, which might imply that U-turns are allowed?

@vshcherb
Copy link
Member

Mostly left-turn implies U-turn, if there is no specific restriction. But many times there is that restriction. So I would say we should have only logic how do we display! That means if we are interested in U-turn we should show u-turn, if not we should show just left turn.

I guess you've got about coloring issue, which basically means that only interested directions should be green, even though interested and non-interested direction could share the line. Again if we can't develop additional icons to put under main direction, than it is better to show only 1 direction per line!

I'm not sure if I get your logic about attachKeepLeftInfoAndLanes.

Also I think you misunderstand my question about BIT_LANE_USE_SHARP_RIGHT, this constant is never used what is it about?

@saiarcot895
Copy link
Contributor Author

On the freeway, (assuming there's an exit coming up on the right) if the user isn't taking the exit, then that method calculates the turn to be a keep left, but it's not announced. However, turn:lanes is only going to have straight and slight right, no lefts. Therefore, I would need to see that the keep left here is equivalent to a straight. The method I thought was to add in a continue turn in that method which would be used if you are on the same road and the difference in angles is less than 5 degrees.

The constant was for determining what turn we're making (primarily in the case of lanes that have multiple allowed turns). However, I later realized I could just use TurnInfo's value to get the value. I've taken it out of the code locally.

@vshcherb
Copy link
Member

Yes, I understand about keep left. Basically we need to have "keep left" as visible indicator on the right, lanes should represent turn:lanes. Basically this is a different question, once there is a deviation we assume 2 (1 + 1) lanes straight. In reality it could be 3 + 3 lanes straight.
I think we have too many assumptions, if I did it from scratch, I would suggest following :

  1. Lanes encoding : 1-st bit selected or not, next 4 bits - turn indicator (0 - straight, 1 - left, 2, - sharp left, ... ), next 4 bits - additional turn indicator ( 0 - straight, 1 - left, 2 - sharp left ..). 3rd indicator is ignored because it is hard to display.
  2. When there is a split, all roads are sorted from left to right, so we have a boolean array [F, F, T, F] - true means selected direction. We also have array of target lanes [0, 1, 2, 1] - 0 means information is missing.
    2.1 If turn:lanes is missing, we replace all 0 with 1 and generate straight indicators or even replace the left with lefth [SL_LEFT, STRAIGHT, STRAIGHT, STRAIGHT | SELECTED, SL_RIGHT].
  3. On the other hand we could have number of source lanes or turn:lanes (we can't use number of source lanes without turn:lanes). If number of turn:lanes is = sum of target lanes (0 should be replaced with 1). We can safely assign each turn indicator to lane, the only problem to understand which direction is selected. That we can do by comparing left neighbor and right, if right is not selected and has common indicator, than it is definitely not selected (just additional), the same with left. So this could be done in 4 checks.
  4. How to map turn:lanes to target roads?

@vshcherb
Copy link
Member

  1. The issue is when number of target roads != number of source lanes (turn:lanes). Initially we can assume that each turn indicator correspond one target road. So if we split it into groups by turn indicator we can assign one indicator per target road. Unfortunately if that number doesn't match (!)
    in case turn:lanes is < :
    we have ambiguity problem, we don't know if the turn:lanes is missing one lane or actually one turn indicator means 2 target roads. Of course we could target:lines, but that would be to complicated, so I suggest just to add something to the right or to the left (and we can solve that problem later)
    is > :
    We need to combine 2 indicators, so we can combine first (u-turn - left turn), than any left turn or any right turn or in the end something with something

@saiarcot895
Copy link
Contributor Author

I think I get what you're saying, but what is the [0, 1, 2, 1]? Is this the number of lanes in the target/outgoing roads?

@vshcherb
Copy link
Member

[0, 1, 2, 1] - is number of lanes from target roads.

@vshcherb
Copy link
Member

Well you might think that I'm trying to over complicate situation, but I do know people who would be more confused with your current implementation, rather with default now, because of highlighting 2 directions for the same lane.

Also we need to have combined approach taking into account tag lanes and tag turn:lanes.

@saiarcot895
Copy link
Contributor Author

I understand. I'll see what I can do.

@saiarcot895
Copy link
Contributor Author

I've made progress on this and completed most of the points you mentioned. I'm still testing this.

Known bugs (to be fixed):

  1. Rendering order. The route to take (green/yellow arrow) isn't always on top.
  2. One case of incorrect lane symbols.
  3. Some breakage for roads not having turn:lanes.

@vshcherb
Copy link
Member

vshcherb commented Jun 4, 2014

Cool looking forward.

@aceman444
Copy link
Contributor

Could you please check if the code will work right in the situation I describe at
https://groups.google.com/forum/#!searchin/osmand/turn$3Alanes/osmand/EaQPOkiLyn4/clozeEhAwwoJ , comment from 2014-03-31 ?

@saiarcot895
Copy link
Contributor Author

Yes and no.

Yes, the turn indicators will appear for the turn at the right-turn islands (just before the intersection) (assuming I can get the bugs in this case worked out). No, at the moment, the turn indicators will not appear at the main intersection; I'm planning to have this in a later pull request. No, turn indicators will not appear if turn:lanes is in a relation.

The turn:lanes and the lanes keys do seem to be correctly tagged (without looking at satellite imagery. The code will check to make sure that the number of lanes as per turn:lanes and the number in lanes agree; otherwise, it will fall back to the older style.

@saiarcot895
Copy link
Contributor Author

Update: because of how turns are considered and calculated, the highlighting of lanes won't be too accurate at signals where there is a right-turn island (like the case in @aceman444's link). In these cases, all of the lanes that can be taken to go onto the correct road will be highlighted. This may mean that all lanes are highlighted.

@aceman444
Copy link
Contributor

I was more interested if you highlight only the left most lane to be in when wanting to turn left after first crossing the intersecting oneway road to right.

If there is the right turn island then yes, the number of lanes may (sometimes does not) change after the right lane branching off. If the number or direction of lanes changes in the last 10 meters before intersection, there is not much you can do.

I'm OK with not supporting the turnlanes relations. Not even JOSM supports them (only with plugin) and I think they are not approved yet.

May I also point you to a bug where OsmAnd displays wrong number of lanes on a road? Would that be something you could look into?

@saiarcot895
Copy link
Contributor Author

It's slowly getting there. I'm working on "merging" the lane indicators if there are two turns that are within 60 meters of each other. This will make it possible to highlight the left-most lane in the case of a left turn, and possibly highlight only the straight lanes in the case of going straight.

Sure. I already did a bit of an overhaul of the lane parsing/rendering methods, so I could fix that bug as well.

@saiarcot895
Copy link
Contributor Author

@vshcherb, commits up to and including saiarcot895/Osmand@4da28ad should be beta-ish, and could go in for testing the basic turn:lanes rendering, primarily on highways. saiarcot895/Osmand@f0b143e is experimental.

@aceman444
Copy link
Contributor

saiarcot895, the bug I talk about in my previous comment is at https://groups.google.com/d/msg/osmand/WPOK6_vK9RY/4akWHyGru6cJ . You can open a new issue for it as you wish. Thanks.

@saiarcot895
Copy link
Contributor Author

I've added support for showing the turn lanes in the case of left turns and right turns (assuming the segment connected to the turning point is tagged). There's just one small bug: in the case of left turns on a divided road, any right turn lanes won't be displayed.

@aceman444
Copy link
Contributor

Who can look at this and merge it before it bitrots again?

@saiarcot895
Copy link
Contributor Author

Anybody there?

If there is some other part of turn:lanes that needs to be implemented first before merging, please do note that here.

@demeterm
Copy link

"Anybody there?

If there is some other part of turn:lanes that needs to be implemented first before merging, please do note that here."

I edit OSM in JOSM, I use lanes:forward, lanes:backward, turn:lanes:forward, turn:lanes:backward tag keys with different values (slight_right|through;right...) I think displaying turn lanes indicators should be based on logical analysis of turn:lanes tags. "Lane and road atrributes" view in JOSM shows correctly the structure of lanes. I think lane symbols could be optionally showed not only at routing helper points but where such information is available (just like as speed limit, other comment: using maxspeed:forward, maxspeed:backward tags cause problems showing speed limits where speed limits are different by directions. The maxspeed:lane tag key is also a valid option.)

josm

@saiarcot895
Copy link
Contributor Author

In my opinion, it can get a little complicated if displaying turn:lanes even when routing isn't active.

Regarding maxspeed:lanes, since it's not accurately possible to determine which lane you're on (well, the accuracy varies depending where you're at and what kind of device you have), all of the maxspeeds would have to be displayed, perhaps using a similar layout of the lane display.

My goal is to try to support the turn:lanes:forward and turn:lanes:backward tags when I find the time, so that even two-way commercial and residential streets can get the lane information.

@demeterm
Copy link

Ok, maybe realtime displaying of lane symbols at no turning point is not so important during routing (optional).

One thing what I forgot to mention before: destination:lanes (also destination:lanes:forward, destination:lanes:backward).

@aceman444
Copy link
Contributor

Yes, please. turn:lanes:forward and turn:lanes:backward are very important to support.

@saiarcot895
Copy link
Contributor Author

@aceman444 @demeterm Do you know of any :forward or :backward tags that are working in OsmAnd? Based on a simple grep, I can't find support for directional tags.

@demeterm
Copy link

I do not know OsmAnd source code well, but what I have found are related routing (road direction in BinaryRoutePlanner) transport (ForwardStops in TransportReader...?). I know from my routing experiences where maxspeed:forward, maxspeed:backward are set there is no displayed maxspeed information. I think direction related tags are nor supported yet.

@saiarcot895
Copy link
Contributor Author

My first guess was that I would have to use the points in RouteDataObject to determine the direction, but looking at the code where I implemented the turn lanes, there's another class called RouteSegmentResult which is for the segments to be used for the route, and that class includes a start and end point index. It was pretty easy to check to see if that was in the forward direction or not.

Now I need to test this to see if the :forward and :backward directions for turn:lanes and lanes are working.

@saiarcot895
Copy link
Contributor Author

Well, that was easy.

Support has been added for turn:lanes:forward, turn:lanes:backward, lanes:forward, and lanes:backward.

@aceman444
Copy link
Contributor

@saiarcot895 Do you know of any :forward or :backward tags that are working in OsmAnd? Based >on a simple grep, I can't find support for directional tags.

There probably is not support for directional tags in OsmAnd and that it quite bad. That is why I wish there would be some as in my country most roads are mapped as single way for both directions and that is where turns:lanes: and lanes: are used. Only some very big intersections (and motorways) have separate way for each direction.

So thanks for looking into it. I hope somebody reviews the patches soon.

@demeterm
Copy link

demeterm commented Oct 2, 2014

I use your branch (compiled, other comment: your branch is 1185 commits behind osmandapp:master) but there must be a bug. The app shows lanes where lanes & turn:lanes tags are set, but does not show any lanes where lanes:forward & turn:lanes:forward tags are used (no lanes tags are set because number of lanes should be the number of lanes:forward + the number of lanes:backward).

@saiarcot895
Copy link
Contributor Author

@demeterm, do you mean that the lanes aren't showing up where lanes:forward and turn:lanes:foward are set, but lanes and turn:lanes are not set? A link to a sample way in OSM would help.

As for the branch, it was initially forked early May, but then after merge conflicts came up late July/early August, I rebased my branch so that my commits were on top of the commits in master at that time. I'm considering doing another rebase soon so that v1.9 features are in the branch.

@saiarcot895
Copy link
Contributor Author

Also, make sure you're on the turn:lanes branch.

@aceman444
Copy link
Contributor

Do you plan to get this into 1.9?

@saiarcot895
Copy link
Contributor Author

That depends mostly on @vshcherb, whose last comment on this PR was on July 6. I will continue to have my own branch be close to master until this is merged, so that the latest features are also in my version.

That being said, it looks like there are some new merge conflicts. Time to rebase this.

@saiarcot895
Copy link
Contributor Author

Correction: at least some of the code in this PR (including support for getting the direction of the way) has been added into the master branch (in particular, commits 2c6b19b, e27a425, and 19ef80c). Because of this, I might hold off on the rebase for a couple of days until @vshcherb can add in the code here (in a more maintainable way) to the master branch.

@demeterm
Copy link

demeterm commented Oct 2, 2014

@saiarcot895
http://www.openstreetmap.org/#map=19/47.95095/21.72699 (I edited this crossing so if you have any advise tagging osm just let me know. In this city there are many turn:lanes tags.)
Where lanes:forward and lanes:backward are set there is no lanes tag. The same is true for turn:lanes:forward tags.

I have compiled turn-lanes branch version, it works fine so far. I will test it more deeply.
I think "icons" of complex lanes look weird a bit in some cases.

What is your opinion about supporting destination:lanes tag?

@saiarcot895
Copy link
Contributor Author

I'll see what's going on with that way.

For destination:lanes, (assuming it will be used) I think it would be better for the voice prompts and maybe for the display at the top bar, but that's a little outside the scope of this pull request.

Regarding the tagging, this page seems to imply the best practice is to have the lanes:forward and lanes:backward tags in addition to the lanes tag, but regardless, the code here should have worked.

@demeterm
Copy link

demeterm commented Oct 3, 2014

@saiarcot895

The compiled turn:lanes version works fine, your master branch was problematic.
So lanes tags should be added. OK.

@vshcherb
Copy link
Member

vshcherb commented Oct 3, 2014

This pull request can't be merged automatically and even manually it is a difficult process.
First steps were made and the infrastructure to store/display was merged. Now it should be step by step, better by small pull-requests with documentation should be merged again to master.
We still consider it is high-prio work that's why non-merged patch is stored in the repo https://github.com/osmandapp/Osmand/blob/master/diff_turn_lanes.

Please continue to work, but keep in mind that complex should be merged step by step which easy to review.

@vshcherb vshcherb closed this Oct 3, 2014
@saiarcot895
Copy link
Contributor Author

Will do.

To others, note that the turn-lanes branch will be deleted.

@saiarcot895 saiarcot895 deleted the turn-lanes branch October 4, 2014 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants