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

Allow -1.0 as unlimited for default_radius value #6599

Merged
merged 5 commits into from
May 31, 2023

Conversation

whytro
Copy link
Contributor

@whytro whytro commented Apr 7, 2023

Issue

Relevant to:

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

This is an adjustment to PR #6575 to allow -1.0 to be used as an "unlimited" radius for the default radius flag. In effect, this would allow the user to define an unlimited radius default. This is useful for when the default starting behavior might be changed (ie. with #6572), and allow for the following interaction:

  • -1.0: unlimited radius range default
  • unset: no default radius range, return error when bearings are not accompanied by radiuses
  • 15: default radius range of 15

Currently this PR sets the default value in the engine_config to be -1.0 in order to avoid being a breaking change, so the default behavior is still unlimited if not set.

Tasklist

Requirements / Relations

Related to:

#6575 Add support for a default_radius flag

@whytro whytro marked this pull request as ready for review April 7, 2023 15:59
@danpat
Copy link
Member

danpat commented Apr 7, 2023

It might be nice to align the UI here with what we support on the API - decimal values >= 0, and the string unlimited. It's fine to use -1 internally to represent the unlimited case, but it'll be easier to grok if the command-line options match the values users pass as API parameters.

@whytro
Copy link
Contributor Author

whytro commented Apr 8, 2023

It might be nice to align the UI here with what we support on the API - decimal values >= 0, and the string unlimited. It's fine to use -1 internally to represent the unlimited case, but it'll be easier to grok if the command-line options match the values users pass as API parameters.

@danpat Okay, I've made a change that should allow "unlimited" to be passed and evaluated accordingly for both the default-radius flag and max-matching-radius flags.

Copy link
Member

@SiarheiFedartsou SiarheiFedartsou left a comment

Choose a reason for hiding this comment

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

LGTM
@whytro can you please resolve conflict in CHANGELOG?

@SiarheiFedartsou SiarheiFedartsou merged commit 72da455 into Project-OSRM:master May 31, 2023
@whytro whytro deleted the radius_flag_unlimited branch June 2, 2023 14:14
mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants