GNOME Bugzilla – Bug 745242
Support roundabouts in GraphHopper route results
Last modified: 2016-01-11 20:31:16 UTC
GraphHopper just added support for round-abouts in its search results. The relevant result data is: paths[0].instructions[0].sign (USE_ROUNDABOUT = 6) paths[0].instructions[0].exit_number and perhaps paths[0].instructions[0].turn_angle Adding support for this in Maps should be fairly easy. Look in RouteService::_createTurnPointType (in routeService.js) and TurnPointType (in route.js).
We also need icons for this. The question is how many and if it should rely on the turn_angle property.
Thanks for filing this, Mattias! From https://github.com/graphhopper/directions-api/blob/master/docs-routing.md turn_angle: "[optional] Only available for USE_ROUNDABOUT instructions. The radian of the route within the roundabout: 0<r<2*PI for clockwise and -2PI<r<0 for counterclockwise transit. Is null the direction of rotation is undefined." exit_number: "[optional] Only available for USE_ROUNDABOUT instructions. The count of exits at which the route leaves the roundabout." So what you are asking about icons is whether the icon should be rotated based on the turn_angle? I think not, but not sure. Andreas? So for requesting icons, either I think, ask in #gnome-design. And/or add the request here: https://wiki.gnome.org/action/show/Design/ArtRequests?action=show&redirect=GnomeArt%2FArtRequests
I recall drawing some roundabout icons in the past, so I might actually still have them around. What states are needed? Go Straight, Go Right, Go Left? - Andreas
(In reply to Andreas Nilsson from comment #3) > I recall drawing some roundabout icons in the past, so I might actually > still have them around. > What states are needed? Go Straight, Go Right, Go Left? Maybe in small cities that would suffice but out here there are roundabouts with 5 or even 6 exits. I think I'd do it like Waze does: Show a roundabout (circle) with the number of the exit in the middle of it. If you could somehow nicely represent the exit number and direction of the exit, that would be awesome.
(In reply to Zeeshan Ali (Khattak) from comment #4) > (In reply to Andreas Nilsson from comment #3) > > I recall drawing some roundabout icons in the past, so I might actually > > still have them around. > > What states are needed? Go Straight, Go Right, Go Left? Oh and don't forget 'going around' state. :)
My suggestion is that we make seven icons, one for each 45 degrees of roundabout exit. These would map to the current turn-point icons we already have in the following way: Angle New Roundabot turn-point Old plain turn-point ————————————————————————————————————————————————————— 45° ROUNDABOUT_SHARP_LEFT SHARP_LEFT 90° ROUNDABOUT_LEFT LEFT 135° ROUNDABOUT_SLIGHT_LEFT SLIGHT_LEFT 180° ROUNDABOUT_CONTINUE CONTINUE 225° ROUNDABOUT_SLIGHT_RIGHT SLIGHT_RIGHT 270° ROUNDABOUT_RIGHT RIGHT 315° ROUNDABOUT_SHARP_RIGHT SHARP_RIGHT Then since we have the turn angle we map that to the closest icon there is. Regarding the exit number: that is available in the turnpoint description and I don't think we want to complicate these icons any more. Regarding going full round in the roundabout I'm not sure GraphHopper understands that yet, since it doesn't understand U-turns. But yeah, we could ask Peter, and if it does add ROUNDABOUT_U_TURN (or something).
FWIW, I'd like to work on this during our little hack weekend in GBG in two weeks.
Created attachment 301387 [details] Icons Something like this attachment (but pretty).
How do i exactly draw these icons for Roundabout as given in the above attachment by Mattias any suggestion??
I am almost done with the coding part for this feature i need icons for roundabout angles.
(In reply to Prashant Tyagi from comment #10) > I am almost done with the coding part for this feature i need icons for > roundabout angles. Excellent, maybe you could attach a patch with the code you have, and use dummy icons or missing icons for the image for now? So we can start the review process?
Created attachment 318510 [details] roundabouts preview Something like this? If it looks good I'll export them out as individual files.
Created attachment 318521 [details] roundabout icons
Created attachment 318556 [details] [review] A patch for roundabout support in GraphHopper search. i have temporarily use the icon name as maps-aboutdirectio[angle]-symbolic.Let me know if there is better possible method to find closest possible angle with aboutpoint_turn_angle.
(In reply to Andreas Nilsson from comment #13) > Created attachment 318521 [details] > roundabout icons Really nice!
Review of attachment 318556 [details] [review]: Thanks Prashant! For the commit message: "Add roundabout support to route service" maybe? Andreas has posted icons for you, please include them in your patch. Look at data/icons/Makefile.am for how to do that. I have some more comments below. We also need to solve the issue were we get no angle, do we need a generic icon? ::: src/route.js @@ +35,3 @@ END: 7, VIA: 8, + ROUND_ABOUT: 9, This should be ROUNDABOUT. @@ +81,3 @@ Name: 'TurnPoint', + _init: function({ coordinate, type, distance, instruction, turn_angle }) { We use camelCase in Maps: turnAngle. @@ +89,1 @@ this.iconName = this._getIconName(); send turnAngle here: this._getIconName(turnAngle) and we do not need this.turnAngle; @@ +89,3 @@ this.iconName = this._getIconName(); + this.min_diff = 360 + this.angle = 0 No need for these to be part of the class. They can be local variables to the getRoundaboutIconName function @@ +115,3 @@ case TurnPointType.VIA: return 'maps-point-end-symbolic'; case TurnPointType.END: return 'maps-point-end-symbolic'; + case TurnPointType.ROUND_ABOUT: We also need to deal with the case that there is no angle, the docs say it can be unavailable, rigt? So what should we do then do you think? Do we need another icon? Request one from Andreas in that case. @@ +117,3 @@ + case TurnPointType.ROUND_ABOUT: + if(this.turn_angle < 0) + this.turn_angle += 360 The angle is in radians. @@ +119,3 @@ + this.turn_angle += 360 + for(var x = 0;x < 360;x += 45) + { curly braces on same line: for (...) { @@ +120,3 @@ + for(var x = 0;x < 360;x += 45) + { + if(Math.abs(x - this.turn_angle) < this.min_diff) if (...) { @@ +126,3 @@ + } + } + return 'maps-aboutdirection-' + this.angle + '-symbolic'; It gets kind of messy with all this code in the case-statement. Could we break it out to a function? case TurnPointType.ROUNDABOUT: return this._getRoundaboutIconName(turnAngle); The name should be 'maps-direction-roundabout-%d-symbolic',format(angle); ::: src/routeService.js @@ +168,3 @@ instruction: _("Start!"), + time: 0, + turn_angle: 0 turnAngle here and everywhere
when roundabout angle is not defined may be we could have a generic icon for that ,andreas please make a generic icon for that.
(In reply to Prashant Tyagi from comment #17) > when roundabout angle is not defined may be we could have a generic icon for > that > ,andreas please make a generic icon for that. Sure! I'll get you something ASAP.
Created attachment 318605 [details] [review] Roundabout support for map. updated patch.
Review of attachment 318605 [details] [review]: Thanks Prashant, looking good! Please obsolete old patches when submitting a new one, to make the bugzolla less confusing! And take a moment to set up git-bz, read more here: https://wiki.gnome.org/Projects/GnomeShell/Development/WorkingWithPatches It will help you work with patches in bugzilla easier. ::: data/icons/Makefile.am @@ +33,3 @@ + hicolor_apps_32x32_maps-direction-roundabout-225-symbolic.svg \ + hicolor_apps_32x32_maps-direction-roundabout-270-symbolic.svg \ + hicolor_apps_32x32_maps-direction-roundabout-315-symbolic.svg \ Please align these at 72 like all other \ ::: src/route.js @@ +123,3 @@ + if(turnAngle < 0) + turnAngle += 2 * Math.PI; + var min_diff = 2 * Math.PI; Two things. 1) look around in Maps code. We use "let" to declare variables, not "var". Read more here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let 'let' is for local variables, bound by the scope it is declared in. 2) camelCase! :) This should be let minDiff @@ +126,3 @@ + var angle = 0; + + for(var x = 0;x < 360; x += 45) { for (let x = ... @@ +127,3 @@ + + for(var x = 0;x < 360; x += 45) { + if(Math.abs(turnAngle - (x / 180) * Math.PI) < min_diff) { if (...) Space after if and for! @@ +135,1 @@ } Maybe a /* * Type of comment above to explain this function? * */
Created attachment 318646 [details] Screenshot of icons in action Also the icons looks weird in my Maps, do they look the same at you? Is this the icons fault or something else?
Created attachment 318647 [details] screenshot. Icon is not a problem for me.As you can see in attachment.
Created attachment 318649 [details] [review] A Patch for Roundabout support. This to add roundabout support to routes service.
Review of attachment 318649 [details] [review]: Did you miss to update this? Looks like the same
Created attachment 318653 [details] [review] A Patch for roundabout support Sorry i submited the previous patch. Here is a new updated patch.
Review of attachment 318653 [details] [review]: Awesome, thanks! So for the commit message, it does not really make sense, we want the short log to give us an idea of what and where. So maybe: "route: Add roundabout support" ? And no space before ":". And "route" not "routes" in the body. ::: data/icons/Makefile.am @@ +33,3 @@ + hicolor_apps_32x32_maps-direction-roundabout-225-symbolic.svg \ + hicolor_apps_32x32_maps-direction-roundabout-270-symbolic.svg \ + hicolor_apps_32x32_maps-direction-roundabout-315-symbolic.svg \ These are still not aligned with the others! :) ::: src/route.js @@ +118,3 @@ + + _getRoundaboutIconName: function(turnAngle) { + /* To map turnAngle with closest roundabout turning angle symbol available */ The comment is a bit long. Split it up in multiple line, formated like: /* * ... * ... */ And maybe explain a bit how the algo below works? @@ +127,3 @@ + let angle = 0; + + for (let x = 0;x < 360; x += 45) { Be consistent with whitespace, "; x < 360;
Created attachment 318659 [details] [review] A patch for roundabout support A patch for roundabout support to route.
Review of attachment 318659 [details] [review]: Thanks, looks very nice! Now we just need to wait for the generic icon then! Some nit comments below while we wait! Overall looks great! ::: src/route.js @@ +122,3 @@ + * calculates the minimum of absolute difference + * between turnAngle and the angle of which map + * has turning symbols. */ Nice, so please, format like: /* * To map ... * ... * ... * ... * ... */ @@ +129,3 @@ + turnAngle += 2 * Math.PI; + let minDiff = 2 * Math.PI; + let angle = 0; Define these variables at the top of the function please. @@ +131,3 @@ + let angle = 0; + + for (let x = 0;x < 360;x += 45) { I did prefer the space after ; :) @@ +140,2 @@ } + Do not add extra blanks!
Created attachment 318678 [details] [review] A patch for roundabout support to route A patch for roundabout support for route.
(In reply to Prashant Tyagi from comment #29) > Created attachment 318678 [details] [review] [review] > A patch for roundabout support to route > > A patch for roundabout support for route. Please. Obsolete the last patch when uploading a new. Otherwise it makes working with git-bz harder for me.
Created attachment 318707 [details] [review] Updated patch. A patch for roundabout support tp route service.
Created attachment 318712 [details] generic roundabout icon (In reply to Prashant Tyagi from comment #17) > when roundabout angle is not defined may be we could have a generic icon for > that > ,andreas please make a generic icon for that. Here is a generic roundabout icon, for undefined angles.
(In reply to Andreas Nilsson from comment #32) > Created attachment 318712 [details] > generic roundabout icon > > (In reply to Prashant Tyagi from comment #17) > > when roundabout angle is not defined may be we could have a generic icon for > > that > > ,andreas please make a generic icon for that. > > > Here is a generic roundabout icon, for undefined angles. can you please give me a .svg format as you have created previously.
(In reply to Prashant Tyagi from comment #33) > (In reply to Andreas Nilsson from comment #32) > > Created attachment 318712 [details] > > generic roundabout icon > > > > (In reply to Prashant Tyagi from comment #17) > > > when roundabout angle is not defined may be we could have a generic icon for > > > that > > > ,andreas please make a generic icon for that. > > > > > > Here is a generic roundabout icon, for undefined angles. > > can you please give me a .svg format as you have created previously. It is in svg, right?
Created attachment 318719 [details] [review] A patch for roundabout support to route A updated patch with generic icon of roundabout.
Review of attachment 318719 [details] [review]: Thanks! Something has happen with this patch :) Where is the routeService changes? And the Makefile diff is out of control... and the name of the icon is wrong. ::: data/icons/Makefile.am @@ +60,1 @@ $(NULL) What happend with this file? The diff should not look like this :) ::: src/route.js @@ +122,3 @@ + * between turnAngle and the angle of which map + * has turning symbols. + /* To map turnAngle with closest roundabout If you are re-spinning ,could you also fix the format of the comment to be: /* * text... * text ... * ... */ @@ +126,3 @@ + let angle = 0; + if (turnAngle === null) + * calculates the minimum of absolute difference wasn't it generic? not undefined?
Created attachment 318722 [details] [review] roundabout support for route service. updated patch with few fixes.
Review of attachment 318722 [details] [review]: Thanks! ::: src/route.js @@ +126,3 @@ + let angle = 0; + if (turnAngle === null) + * calculates the minimum of absolute difference map? not maps? have you tested this? And could you format the comment the way I commented earlier?
Created attachment 318737 [details] [review] A patch for roundabout suppport A updated patch. last time i didnt tested that so it happens.
Review of attachment 318737 [details] [review]: Thanks! Looks great!
Review of attachment 318737 [details] [review]: Pushed, thanks!