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

Require a radius parameter other than unlimited when using bearings #2983

Open
TheMarex opened this issue Oct 2, 2016 · 3 comments
Open

Comments

@TheMarex
Copy link
Member

TheMarex commented Oct 2, 2016

Currently every plugin will search the whole StaticRTree until a matching bearing segment is found - no matter how far away it is. For a lot of users this is an unexpected result and can have serious performance implications (even on accident).

Hence to make the user aware that it actually only makes sense to search in a specific search radius, say 100m when using the bearings filter, we should enforce this on the API level.

This means in case of no segment with the given bearing value in the search radius we will return NoSegment which is a better indicator then just returning a coordinate potentially across the Atlantic.

Two options for implementing this:

  1. bearings without setting radius will error (invalid request)
  2. bearings implies that radius=SOME_DEFAULT_VALUE

I would prefer option 1 because it makes it more explicit that we changed the behavior.

@TheMarex TheMarex added this to the 6.0 milestone Oct 2, 2016
@TheMarex TheMarex changed the title Require a radius parameter other then unlimited when using bearings Require a radius parameter other than unlimited when using bearings Oct 2, 2016
@chaupow
Copy link
Member

chaupow commented Oct 13, 2016

I guess in most cases when the segment with a matching bearing is too far away, the range of the bearing is probably too strict/narrow/small. Currently, we also do not recommend any value that should be used as a bearing range in osrm. We should at least recommend some range that makes sense in the osrm documentation (is 90 a good range? and a short sentence explaining range would be nice too...)

per chat with @TheMarex:
It might also make sense to decide on a default range X for bearings and use that default value. Three options for implementing this:

  1. change the bearings option to bearings={value} which works like bearings={value},X
  2. accept bearings in both forms, bearings={value},{range} and just bearings={value} which works like bearings={value},X
  3. leave it as it is

I like option 1 as it makes things easier but I don't know if there are many use cases where people would want to set specific ranges.

Otherwise having a radius definitely sounds good - especially considering performance.

@1ec5, @freenerd, @danpat any opinions on this?

@1ec5
Copy link
Member

1ec5 commented Oct 18, 2016

I don't have a strong opinion on the syntax changes proposed here; libraries such as MapboxDirections.swift rationalize away any syntactic problems anyways. Rather, I think most of the confusion stems from bearing falling into an uncanny valley that's just different enough from the expected behavior, but coupling with a radius seems to address that.

@whytro
Copy link
Contributor

whytro commented Mar 6, 2023

I'd like to take a crack at this if this issue is still open and available.
Unless I missed something, it looks like it should only require changes to the parameters' validation function/help message generation, as well as the corresponding adjustments for each service.
This would be for the first option of "bearings without setting radius will error", with the goal of causing such requests to return an InvalidOptions code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants