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

Add bbox parameter #364

Closed
wants to merge 2 commits into from
Closed

Add bbox parameter #364

wants to merge 2 commits into from

Conversation

hbruch
Copy link
Collaborator

@hbruch hbruch commented Oct 12, 2018

Introduce a bbox to filter results contained in a given bounding box as suggested by #268.

Signed-off-by: Holger Bruch <[email protected]>
Copy link
Collaborator

@karussell karussell left a comment

Choose a reason for hiding this comment

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

This is really nice! And it LGTM

One comment ...

@@ -132,6 +133,11 @@ http://localhost:2322/api?q=berlin&limit=2
http://localhost:2322/api?q=berlin&lang=it
```

#### Filter results by bounding box
```
http://localhost:2322/api?q=berlin&bbox=9.5,51.5,11.5,53.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be important to mention which order the bbox has. Maybe we should also use a different order with left,bottom and right,top points ala latSouth,lonWest,latNorth,lonEast or why did you pick this order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right, this should be documented. Concerning the order: to me this was the one I‘m used to, but Nomination and Overpass both use a different order.
Should I change it?
And before merging, I should add a verification of the lat/lon limits.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I change it?

Up, not necessary. I confused the APIs ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you change the order now? Because I read from the code below that you expect <minLon, maxLon, minLat, maxLat>. My preference would be as documented: <minLon,minLat,maxLon,maxLat>. (Or to be precise: <x1,y1,x2,y2>, Envelope() does not seem to care if it is min,max or max,min.)

Just to mention it, the odd boundingbox format of Nominatim is only used for output. The viewbox input parameter also expects <minLon,minLat,maxLon,maxLat>.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As you have a preference then we should do x1,y1,x2,y2

Copy link
Collaborator

@lonvia lonvia left a comment

Choose a reason for hiding this comment

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

Stupid question: how does this work well together with the location bias? Anything strange we should warn about in the docs?

@@ -132,6 +133,11 @@ http://localhost:2322/api?q=berlin&limit=2
http://localhost:2322/api?q=berlin&lang=it
```

#### Filter results by bounding box
```
http://localhost:2322/api?q=berlin&bbox=9.5,51.5,11.5,53.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you change the order now? Because I read from the code below that you expect <minLon, maxLon, minLat, maxLat>. My preference would be as documented: <minLon,minLat,maxLon,maxLat>. (Or to be precise: <x1,y1,x2,y2>, Envelope() does not seem to care if it is min,max or max,min.)

Just to mention it, the odd boundingbox format of Nominatim is only used for output. The viewbox input parameter also expects <minLon,minLat,maxLon,maxLat>.

@karussell
Copy link
Collaborator

Stupid question: how does this work well together with the location bias?

Should work with it. E.g. it should be still possible to have a bias inside a bounding box.

This was referenced Jun 14, 2019
@lonvia
Copy link
Collaborator

lonvia commented Jun 14, 2019

Closing in favour of #421.

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