NotificationsYou must be signed in to change notification settings
Fork1.9k
Star6.2k

fix: enable last intersection of last step before a via point#3176

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

karussell merged 8 commits intographhopper:masterfromOlafFlebbeBosch:intersection

Aug 7, 2025

Merged

fix: enable last intersection of last step before a via point#3176

karussell merged 8 commits intographhopper:masterfromOlafFlebbeBosch:intersection

Aug 7, 2025

Conversation

Copy link

Contributor

OlafFlebbeBosch commentedJul 4, 2025•
edited
Loading

We noticed that intersections are not populated on the arrival intersection on both OSRM and mapbox directions creating issues to pass VIA points in navigation.

rename instructionJson to stepJson, since it is about the step Json object
Adding an ManeuverType enum to better document the Maneuver we are going to create
Handle ManeuverTypes and TurnTypes differently, since they are different concepts, sharing some values but not all
Fix typo bearingsArray vs bearingsrray
rename isFirstInstructionOfLeg to isDepartInstruction and use maneuverType in the subroutines instead
rename fixFirstIntersectionDetail to fixDepartIntersectionDetail since intersections have to be fixed on every leg
fix one test setup and one test
@boldtrn would you please have a look ?

OlafFlebbeBosch changed the title~~fix: enable last intersection of last step of via points~~fix: enable last intersection of last step before a via points

Jul 4, 2025

OlafFlebbeBosch changed the title~~fix: enable last intersection of last step before a via points~~fix: enable last intersection of last step before a via point

Jul 4, 2025

boldtrn reviewed

Jul 8, 2025

View reviewed changes

navigation/src/main/java/com/graphhopper/navigation/NavigateResponseConverter.java Outdated

		/**
		* fix the first IntersectionDetail.
		* <p>
		* The first Intersection of the first step should only have one "bearings" and one

Copy link

Member

Choose a reason for hiding this comment

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

The comments should be updated as well :)

boldtrn reviewed

Jul 8, 2025

View reviewed changes

navigation/src/main/java/com/graphhopper/navigation/NavigateResponseConverter.java Outdated

		* "out" entry
		*/
		privatestaticvoidfixFirstIntersectionDetail(List<PathDetail>intersectionDetails) {
		privatestaticvoidfixDepartIntersectionDetail(List<PathDetail>intersectionDetails,intpos) {

Copy link

Member

Choose a reason for hiding this comment

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

I don't like the name pos, can you rename this to something that explains what pos is?

boldtrn approved these changes

Jul 8, 2025

View reviewed changes

Copy link

Member

boldtrn left a comment

Choose a reason for hiding this comment

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

I did setup this PR on a test server and tried to use the routing in combination with Maplibre-Navigation-Android. This worked fine as well, so we haven't broken anything here as well.

boldtrn reviewed

Jul 8, 2025

View reviewed changes

navigation/src/main/java/com/graphhopper/navigation/NavigateResponseConverter.javaShow resolvedHide resolved

boldtrn requested changes

Jul 8, 2025

View reviewed changes

Copy link

Member

boldtrn left a comment

Choose a reason for hiding this comment

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

Sorry, I just found this issue that can occur in some situations when using Maplibre Navigation, this can lead to a nullpointer because the modifier is missing.

OlafFlebbeBosch force-pushed theintersection branch fromc5d43e1 to5631847Compare

July 10, 2025 07:42

boldtrn approved these changes

Jul 17, 2025

View reviewed changes

Copy link

Member

boldtrn left a comment

Choose a reason for hiding this comment

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

LGTM, just gave this another try with Maplibre after the latest change.

karussell added the navigation label

Jul 17, 2025

OlafFlebbeBoschand others added6 commits

July 22, 2025 09:50

fix: enable last intersection of last step of via points

0b7671a

fix: accidenteley removed modifier

ee3aa45

Improve ferry instructions (graphhopper#3179)

e5a6601

* announce ferry in instructions* extra instruction sign needed* Update maps to 34acd30e* fix* ensure never NPE* use mode==ferry for mapbox response

fix: enable last intersection of last step of via points

f09e3d2

fix: typo

0877f15

fix: depart and arrival at the same last step at end

86213ce

OlafFlebbeBosch force-pushed theintersection branch from5631847 to86213ceCompare

July 22, 2025 07:52

fix: comment

11e4506

Copy link

ContributorAuthor

OlafFlebbeBosch commentedJul 22, 2025

Rebased and fixed an edge case

karussell added this to the11.0 milestone

Jul 22, 2025

karussell reviewed

Aug 4, 2025

View reviewed changes

navigation/src/test/java/com/graphhopper/navigation/NavigateResponseConverterTest.java

		request.addPoint(newGHPoint(42.505144,1.526113));
		request.addPoint(newGHPoint(42.50529,1.527218));
		request.setProfile(profile);
		request.setProfile(profile).setPathDetails(Collections.singletonList("intersection"));

Copy link

Member

Choose a reason for hiding this comment

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

minor: can you check the formatting here and elsewhere in this file?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

done

chore: formatting using editorconfig

a442760

karussell merged commit5b1f142 intographhopper:master

Aug 7, 2025

Copy link

Member

karussell commentedAug 7, 2025•
edited
Loading

There is a now test failure in master. And if I changenull to "straight":

assertEquals("straight", maneuver.get("modifier").asText());

it runs fine. Is this expected or did I something wrong when merging?

karussell added a commit that referenced this pull request

Aug 7, 2025

test fix (#3176)

0fb184c

Copy link

Member

boldtrn commentedAug 7, 2025

There is a now test failure in master. And if I change null to "straight":

Actually depart usually has no modifier IIRC. I am not sure this will break anything, if there is a modifier - I would not expect it to (at least for Maplibre I would expect the impact to be rather low)

Copy link

ContributorAuthor

OlafFlebbeBosch commentedAug 8, 2025

Sorry, I am not sure why this slipped through. And agree to@boldtrn , it shouldn't have a modifier. Looking into why that happend

Copy link

Member

karussell commentedAug 8, 2025•
edited
Loading

Hm, this is now just as it was before the merge of this PR:

graphhopper/navigation/src/test/java/com/graphhopper/navigation/NavigateResponseConverterTest.java

Line 85 ina148d51

assertEquals("straight",maneuver.get("modifier").asText());

Copy link

Member

boldtrn commentedAug 8, 2025

Hm, this is now just as it was before the merge of this PR

Ah sorry, I must have missed this, OK, if we have always added a modifier here, then this is probably not an issue, so all good from my side then.

Copy link

Contributor

oflebbe commentedAug 8, 2025

Looks like i introduced the change with a previous attempt fixing and somehow the test didnt trigger w/o a mvn clean on my side

Labels

navigation

Movatterモバイル変換

fix: enable last intersection of last step before a via point#3176

fix: enable last intersection of last step before a via point#3176

Uh oh!

Conversation

OlafFlebbeBosch commentedJul 4, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

boldtrnJul 8, 2025

Choose a reason for hiding this comment

Uh oh!

boldtrnJul 8, 2025

Choose a reason for hiding this comment

Uh oh!

boldtrn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

boldtrn left a comment

Choose a reason for hiding this comment

Uh oh!

boldtrn left a comment

Choose a reason for hiding this comment

Uh oh!

OlafFlebbeBosch commentedJul 22, 2025

Uh oh!

karussellAug 4, 2025

Choose a reason for hiding this comment

Uh oh!

OlafFlebbeBoschAug 6, 2025

Choose a reason for hiding this comment

Uh oh!

karussell commentedAug 7, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

boldtrn commentedAug 7, 2025

Uh oh!

OlafFlebbeBosch commentedAug 8, 2025

Uh oh!

karussell commentedAug 8, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

boldtrn commentedAug 8, 2025

Uh oh!

oflebbe commentedAug 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

OlafFlebbeBosch commentedJul 4, 2025•
edited
Loading

karussell commentedAug 7, 2025•
edited
Loading

karussell commentedAug 8, 2025•
edited
Loading