Interesting feature, thank you! I can imagine that this can be useful for other (analyzing) use cases too.

Do you have an example JSON output of the path detail?

And would it make sense to list all roads of the junction with some path details like road_class? (or even return all specified path details? And then in+out average_speed?

Do we need the location in this path detail or better also use the index into the points list as we do this for instructions too?

Thanks for your feedback. Here is a sample from the intersection json:

  "intersection": [
    [
      0,
      1,
      {
        "entry": [
          true
        ],
        "bearings": [
          109
        ],
        "location": [
          8.947839452785571,
          48.533868607925086
        ],
        "out": 0
      }
    ],
    [
      1,
      2,
      {
        "entry": [
          true,
          true,
          false
        ],
        "bearings": [
          21,
          184,
          289
        ],
        "in": 2,
        "location": [
          8.9480157,
          48.5338281
        ],
        "out": 0
      }
    ],
    [
      2,
      3,
      {
        "entry": [
          true,
          true,
          false
        ],
        "bearings": [
          33,
          89,
          201
        ],
        "in": 2,
        "location": [
          8.9480786,
          48.5339366
        ],
        "out": 0
      }
    ],
    [
      3,
      4,
      {
        "entry": [
          true,
          false
        ],
        "bearings": [
          49,
          213
        ],
        "in": 1,
        "location": [
          8.948174,
          48.534033
        ],
        "out": 0
      }
    ]
  ]

And would it make sense to list all roads of the junction with some path details like road_class? (or even return all specified path details? And then in+out average_speed?

At least for the navigation rerouting case, I am not sure if this makes sense. For data analysis, this could make sense.

Do we need the location in this path detail or better also use the index into the points list as we do this for instructions too?

We could do this, but this will become really tricky I think. I think the simplification happens after the calculation of the PathDetails, so at this point we don't know the intervals yet and we would have to calculate them similarly to how we do it for the instructions. So this would be quite complicated to do?

Here is a sample from the intersection json

Thanks!

The path details in general include the interval for which they exist (from,to both inclusive). I would have expected the interval to have the same start and end index for this intersection list. Then it is also clear for which intersection is meant per path detail.

Is the bearing relative to the "current" edge and the from is the index of this intersection (of the path detail interval)?

And can you describe the entry, in and out JSON entries? Not sure I understand them. Do they include the current edge too? Can we avoid that the JSON meaning is different to the Java meaning? (in&out seems to be ints and in Java they are booleans)

Why is entry singular and bearings plural?

Do we need the location in this path detail...

We could do this, but this will become really tricky I think.

Why should we include the location at all? Due to the path detail interval, which should work with the simplification, we already have the location (?)

tmpEdge = edgeIter.detach(false);

Why is detaching the edge state necessary here?

// Special case for the first intersection. Mapbox expects only the start edge to be present, so we skip

This special case should be better done in the navigate converter (?)

The path details in general include the interval for which they exist (from,to both inclusive). I would have expected the interval to have the same start and end index for this intersection list. Then it is also clear for which intersection is meant per path detail.

You mean to have PathDetails with from=to, only at the intersection?

Sure this can be done, then we would probably need to add empty PathDetails in between the intersections?

Is the bearing relative to the "current" edge and the from is the index of this intersection (of the path detail interval)?

It is absolute. 0 => north.

(in&out seems to be ints and in Java they are booleans)

Yes, this is a trick I use to sort the entries by bearing. I created a small inner class that implements Comparable. This is translated to "Json" here.

I explained the format in the class header, do you think more is necessary? https://github.com/graphhopper/graphhopper/pull/2590/files#diff-cddef4461eced91b655f853965fa82475b82e6b566ce6359c54b30d8cebd57daR36

Why is entry singular and bearings plural?

Entry is an int, the index in the bearings array. So we have bearings of all edges, but only one of them are the entry.

Why should we include the location at all? Due to the path detail interval, which should work with the simplification, we already have the location (?)

I built it with the conversion for the NavigateResponse in mind and there it is easier to have the location :). It is not really an argument for the PathDetail, so I can change this if you like?

Why is detaching the edge state necessary here?

Ah, I think you are right, we could simply read the values from the Iterator. I think I had to use detach in an earlier version and haven't reverted that 👍

This special case should be better done in the navigate converter (?)

Yes 👍

Sure this can be done, then we would probably need to add empty PathDetails in between the intersections?

The API would allow us to omit this which would make slightly more sense, but path simplification might not work and in general it is probably a good idea to always cover the entire path and include an "empty" path detail.

Entry is an int, the index in the bearings array. So we have bearings of all edges, but only one of them are the entry.

Sorry I don't understand what you mean here. If there are multiple edges that we can use as entry (your above examples shows e.g. "entry": [true,false]), then can't we name this entries? Your javadoc comment says: "entry contains an array of the edges at that intersection" and also see:

bearings.add(intersectionValues.bearing);
entry.add(intersectionValues.entry);

Also the name for List<IntersectionValues> intersections = new ArrayList<>(); is a bit confusing because we collect the IntersectionValues only for a single intersection.

It is not really an argument for the PathDetail, so I can change this if you like?

Yes would be better without location.

// The in edge is always false, this means that u-turns are not considered as possible turning option

Why not?

Sorry I don't understand what you mean here. If there are multiple edges that we can use as entry (your above examples shows e.g. "entry": [true,false]), then can't we name this entries?

Ah sorry, you are right, I mixed entry with in and out :). For the naming I did follow the schema that is already consumable by the Maplibre Nav client, which is based on the Mapbox Directions API. So I followed the naming, we can choose completely different names and do the whole conversion in the navigation converter?

// The in edge is always false, this means that u-turns are not considered as possible turning option
Why not?

I think the idea is that for rerouting it wouldn't be a common turn? We can change that if you want??

Sure this can be done, then we would probably need to add empty PathDetails in between the intersections?

I was wrong here, we can't do that with PathDetails, as a PathDetail has at least the lenght of one edge. And every edge change creates a new PathDetail. But we can omit the location 👍.

Why is detaching the edge state necessary here?

I think the reason for detaching was that it wasn't possible to compare edgeIds otherwise. The problem is that we can have VirtualEdgeIteratorStates and we have the method:

    private int edgeId(EdgeIteratorState edge) {
        if (edge instanceof VirtualEdgeIteratorState) {
            return GHUtility.getEdgeFromEdgeKey(((VirtualEdgeIteratorState) edge).getOriginalEdgeKey());
        } else {
            return edge.getEdge();
        }
    }

Here we have a check for VirtualEdgeIteratorState, this does not work for VirtualEdgeIterator(note: != VirtualEdgeIteratorState). Do you have an idea how we can do this without detaching?

I updated the PR. I removed the location from the PathDetails and renamed entry to entries on the GraphHopper side. The navigation endpoint still has to use entry, so it is a bit inconsistent now :( .

But we can omit the location +1.

Thanks 👍 ... and is it possible to move this special case to the /navigate endpoint?

// Special case for the first intersection. Mapbox expects only the start edge to be present, so we skip

The navigation endpoint still has to use entry, so it is a bit inconsistent now :(

As the format is otherwise also completely different I wouldn't call it inconsistent :)

Here we have a check for VirtualEdgeIteratorState, this does not work for VirtualEdgeIterator(note: != VirtualEdgeIteratorState). Do you have an idea how we can do this without detaching?

Oh, ugly indeed. You could call "detach" only if it is a VirtualEdgeIterator :D

Edit: can you also have a look if a response with this path detail is parsable by our Java client? e.g. serialized back into a map? I think atm we can only handle primitives.

Thanks 👍 ... and is it possible to move this special case to the /navigate endpoint?

Ah sorry, I forgot about that one :). I deleted it for now, I think this should still work for the navigation client, if we see issues, we can always introduce special handling :).

Oh, ugly indeed. You could call "detach" only if it is a VirtualEdgeIterator :D

I actually thought about this as well. VirutalEdgeIterator is not a public class (only package visibility). If you want to, I ca have a try?

Edit: can you also have a look if a response with this path detail is parsable by our Java client? e.g. serialized back into a map? I think atm we can only handle primitives.

That is actually a really good point. It did not work. I added special handling for Map values and it should work now.

@boldtrn can you review my comments? It seems close to merging :)

Sorry I think your reply got lost in my emails somehow, I will update the PR so we can merge this :).

I updated the PR. Sorry again for the delay. Let me know if this works for you, or if you need anything else.

Thanks!