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

Speed cannot be 0 for unblocked edge #68

Closed
abhumbla opened this issue Apr 19, 2022 · 18 comments · May be fixed by #145
Closed

Speed cannot be 0 for unblocked edge #68

abhumbla opened this issue Apr 19, 2022 · 18 comments · May be fixed by #145
Assignees
Labels
bug Something isn't working

Comments

@abhumbla
Copy link

error preventing route from being returned. also in the interpolated legs like #67 , so might be fixed upstread
speed-0-unblocked

@abhumbla abhumbla added the bug Something isn't working label Apr 19, 2022
@hcourt
Copy link

hcourt commented Aug 5, 2022

Seen also on #83, #87

@hcourt
Copy link

hcourt commented Aug 5, 2022

@hcourt
Copy link

hcourt commented Aug 5, 2022

Another address I get this for: Wildcat Canyon Park Office, 5755 McBryde Avenue, Richmond, 94805
McBryde Avenue seems bad all over

@graue graue changed the title Speed cannot be 0 for unblock edge Speed cannot be 0 for unblocked edge May 13, 2023
@graue
Copy link

graue commented May 13, 2023

Merged related issues #83, #87, #106 here.

I'm not immediately able to repro the cases described in those issues anymore.

@graue graue closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2023
@graue graue reopened this May 13, 2023
@graue
Copy link

graue commented May 13, 2023

A currently failing case with this error:

From: Fox Oakland Theater
To: Calhoun Terrace
Depart at: 05/19/2023 05:58 PM

@graue
Copy link

graue commented Jan 18, 2024

If I'm remembering correctly from the last time I looked into this, it seemed that in cases where this assertion fails, this other commented-out assertion, which was commented out as a workaround when we were first adapting GraphHopper main, would also fail.

@rsarathy
Copy link

Ran into this error as well. Here's the edge in question:
Point 1: (37.4099564, -121.891259)
Point 2: (37.4101079, -121.8907947)

This edge spans across the Milpitas BART station's platforms (see image).
Screenshot 2024-04-20 at 11 54 05

@rsarathy
Copy link

Ran into this error as well. Here's the edge in question: Point 1: (37.4099564, -121.891259) Point 2: (37.4101079, -121.8907947)

This edge spans across the Milpitas BART station's platforms (see image). Screenshot 2024-04-20 at 11 54 05

This error is reproducible in many ways if you are planning journeys that interchange through the Milpitas Transit Center, e.g. VTA -> BART or vice-versa.

For example,

  • Levi's Stadium to Jack London Square,
  • Embarcadero Plaza to Great Mall (Milpitas), or
  • Baylands Park to James Logan High School

@graue
Copy link

graue commented Apr 20, 2024

So this way.

Would be very helpful if you could narrow down why this is happening. I'd suggest uncommenting the assertion in AbstractWeighting to try to catch the problem closer to the source

@graue
Copy link

graue commented Apr 20, 2024

Sure enough the AbstractWeighting assertion fails on that way:

java.lang.IllegalStateException: Calculating time should not require to read speed from edge in wrong direction. (1826169 - 1826167) (37.4101313,-121.890723,14.081999778747559), (37.4101079,-121.8907947,13.880000114440918), dist: 6.849 Reverse:false, fwd:false, bwd:false, fwd-speed: 0.0, bwd-speed: 0.0
	at com.graphhopper.routing.weighting.AbstractWeighting.calcEdgeMillis(AbstractWeighting.java:72)

The "wrong direction" part of the error is spurious, as the flags show the way can't be used in either direction.

That way has highway=corridor, which is a highway value that the bike router is not supposed to use for routing: The way gets set to Access.CAN_SKIP here, which seems to mean the router can skip considering that way. That line is running because there's no setHighwaySpeed call for "corridor". So, the transfer leg between two public transit routes is including a way that the bike router isn't supposed to use, and then we're crashing trying to calculate how long it takes to travel it.

@graue
Copy link

graue commented Apr 21, 2024

Some non-results: I noticed GraphHopperGtfs#interpolateTransfers, running at graph-cache build time, instantiates a GraphExplorer with isBike false. That alone (and rebuild cache) did not fix the problem.

Another bug is that GraphHopperGtfs looks for a config option called "pt.transfer_profile" which is elsewhere not used and not in our config template or my config. It seems it should be "pt.connecting_profile". That, however, also did not fix the problem.

@graue
Copy link

graue commented Apr 21, 2024

An addPushingSection('corridor'); call in BikeFlagEncoder fixes the specific Milpitas BART case, but I would prefer to properly solve this because there might be other cases. The root problem is that transfer legs between two public transit routes can include ways that the bike router isn't supposed to use.

@abhumbla
Copy link
Author

but I would prefer to properly solve this

would you not consider enumerating these way types a proper solution? have we found any other errors? we can search the logs cc @Andykmcc

@abhumbla
Copy link
Author

GraphHopperGtfs#interpolateTransfers, running at graph-cache build time, instantiates a GraphExplorer with isBike false. That alone (and rebuild cache) did not fix the problem.

Another bug is that GraphHopperGtfs looks for a config option called "pt.transfer_profile" which is elsewhere not used and not in our config template or my config. It seems it should be "pt.connecting_profile". That, however, also did not fix the problem.

An addPushingSection('corridor'); call in BikeFlagEncoder fixes the specific Milpitas BART case,

also would welcome a PR with just these changes go up so we can fix those errors while we search for a solution that prevents the transfer interpolation router from returning those ways in the first place

@graue
Copy link

graue commented Apr 21, 2024

would you not consider enumerating these way types a proper solution?

I would not. When you have pt.connecting_profile: bike2 in your config, interpolated legs should not include ways that are impassable to the bike2 router, but for some reason, they do. It's unambiguously a flaw. And I don't think the bike router should be routing inside buildings; skipping highway=corridor is correct.

@abhumbla
Copy link
Author

And I don't think the bike router should be routing inside buildings

depends how the station is mapped, that may be the only way from the street to the station. or transfers in a big station such as BART to T at Powell (if Muni Metro allowed bikes). I don't really disagree here tho, we should dig through the interpolation code

@graue
Copy link

graue commented Apr 21, 2024

We should get you to the front door of the station building. Realistically, the quality of indoor mapping is far from adequate to do more than that (see also #126).

@abhumbla
Copy link
Author

abhumbla commented Apr 21, 2024

We should get you to the front door of the station building.

that's a separate issue which requires more router changes. I'm not sure it's easy to do either, does OSM have enough info for this? we should be tolerant to weird mapping cases, we're never going to address all of them

@rsarathy rsarathy self-assigned this May 21, 2024
graue added a commit that referenced this issue May 21, 2024
- The config param is called pt.connecting_profile, not
  pt.transfer_profile
- isBike should be true in GraphExplorer constructor

I was hoping these fixes would resolve the issue described in #68, but
unfortunately, they don't, but this still seems to be the correct thing
to do (while having no visible effect AFAIK).
graue added a commit that referenced this issue May 21, 2024
- The config param is called pt.connecting_profile, not
  pt.transfer_profile
- isBike should be true in GraphExplorer constructor

I was hoping these fixes would resolve the issue described in #68, but
unfortunately, they don't, but this still seems to be the correct thing
to do (while having no visible effect AFAIK).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants