Return LineString geometries on expansion #4275
Conversation
nilsnolde
force-pushed
the
nn-expansion-linestring
branch
from
052026c
to
a6b3260
Compare
nilsnolde
force-pushed
the
nn-expansion-linestring
branch
from
a6b3260
to
1f42f4c
Compare
src/proto_conversions.cc
Outdated
| @@ -373,7 +373,9 @@ bool Options_ExpansionProperties_Enum_Parse(const std::string& prop, | |||
| {"durations", Options_ExpansionProperties_durations}, | |||
| {"distances", Options_ExpansionProperties_distances}, | |||
| {"statuses", Options_ExpansionProperties_statuses}, | |||
| {"edge_ids", Options::ExpansionProperties::Options_ExpansionProperties_edge_ids}}; | |||
| {"edge_ids", Options::ExpansionProperties::Options_ExpansionProperties_edge_ids}, | |||
There was a problem hiding this comment.
we might want to rename that? previously it was the same field name in the response, but it's the singular version i.e. edge_id instead of edge_ids. would be nicer to have them consistent IMO. now that we're breaking compatibility
There was a problem hiding this comment.
i agree that we can break this, its still beta
There was a problem hiding this comment.
also why n ot use the shorter alias of this property?
There was a problem hiding this comment.
ok, let me get that in real quick. also the shorter version.
| expansion_callback_(graphreader, FORWARD ? meta.edge_id : opp_edge_id, prev_pred, | ||
| "bidirectional_astar", "r", newcost.secs, | ||
| pred.path_distance() + meta.edge->length(), newcost.cost); |
There was a problem hiding this comment.
I also changed all callbacks to use the end of the current edge for cost & distance. Most were doing that already, but not the "reached" ones. That's really confusing when looking at the results.
|
done @kevinkreiser , also renamed |
|
Can we merge here? I need to use matrix expansion on many PRs right now and it's painful to merge this branch around. |
| | `distance` | Returns the accumulated distance in meters for each edge in order of graph traversal. | | ||
| | `duration` | Returns the accumulated duration in seconds for each edge in order of graph traversal. | | ||
| | `cost` | Returns the accumulated cost for each edge in order of graph traversal. | | ||
| | `edge_id` | Returns the internal edge IDs for each edge in order of graph traversal. Mostly interesting for debugging. | | ||
| | `pred_edge_id` | Returns the internal edge IDs of the predecessor for each edge in order of graph traversal. Mostly interesting for debugging. | | ||
| | `edge_status` | Returns the edge states for each edge in order of graph traversal. Mostly interesting for debugging. Can be one of "r" (reached), "s" (settled), "c" (connected). | |
There was a problem hiding this comment.
one day we'll have to make this no longer beta and lock down the api 😄
There was a problem hiding this comment.
I’d be fine with lifting the BETA status actually. I’ve used it now quite a bit. Apart from that one bug where random edges are included for isochrones, which I’d really like to fix first.
|
this looks fine to me! |
fixes #4266
pred_edge_idto the response to figure expansionThe config limit is the same as before: it's derived from the expansion action. We could adapt that logic, but I'd do that in a follow-up PR if it's wanted.
Is based on #4263 , so let's merge that first.