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

Don't overzoom large (city-sized) POIs #161

Merged
merged 3 commits into from
Sep 19, 2022

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Sep 17, 2022

This is a followup to #153. Now that you can select localities, the zoom feature feels a little off.

before:

So now, when available, we prefer to use the bbox for fitting rather than the coordinate. This works much better for large POIs (e.g. "Seattle").

after zooming to bbox (first commit):

So zooming in/out to Seattle looks good with the first commit, but zooming into Victrolla loses valuable context of the surrounding neighborhood. This is because, while many small venues are represented as OSM.Nodes, thus lacking a bbox, some (like Vitctrolla) are actually stored as OSM.Ways and thus have a bbox.

So the second commit introduces a maxZoom, ensuring that if we are zooming to a Bbox we never zoom too close.

before second commit (too close to small osm ways)

Screen Shot 2022-09-17 at 10 31 24 AM

after second commit (with max zoom)

Screen Shot 2022-09-17 at 10 30 44 AM

Previously we zoomed to the "point" of any POI to a zoom level that
seemed optimized for building-sized points. For large features, like an
entire city, this meant zooming way in to some seemingly random part of
the city.

Now we prefer the bbox whenever available instead of the point.

With this commit, zooming works reasonably for city-sized objects and
the node-based POIs, which have no bbox such as:

e.g. Overcast Coffee Company - "id": "node/4349270303",

But some small venues are defined as an osm way, and thus have a Bbox. With this change
we're now zooming a little too close (IMO). An example of a too-close zoom:

e.g. Victrola Coffee Roasters - "id": "way/206623301"
Some building POI's are stored as OSM ways, this means they have a bbox,
but using that bbox for zoom results in feeling a bit too close - losing
the valuable context of the surround block.

An example of a too-close bbox zoom that this commit fixes:

Victrola Coffee Roasters - "id": "way/206623301",
Copy link
Member

@ellenhp ellenhp left a comment

Choose a reason for hiding this comment

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

Looks great, I'm going to test this just to make sure it behaves smoothly though. I might get to it in the next hour or so but otherwise it'll be tomorrow sometime. In the future it would be really cool to draw a polyline around admin areas but I'm not sure pelias exposes admin area shapes so that could be hard.

web/frontend/src/pages/PlacePage.vue Show resolved Hide resolved
web/frontend/src/utils/models.ts Outdated Show resolved Hide resolved
@michaelkirk
Copy link
Member Author

michaelkirk commented Sep 19, 2022

In the future it would be really cool to draw a polyline around admin areas but I'm not sure pelias exposes admin area shapes so that could be hard.

Yeah I agree. The existing "pin" UI seems inappropriate for a big 2-D entity like a city.

(edit) I created an issue here: #163

@ellenhp
Copy link
Member

ellenhp commented Sep 19, 2022

Running a build to test this now.

@ellenhp
Copy link
Member

ellenhp commented Sep 19, 2022

Looks great, but I noticed during testing that I think I broke URL rewriting when you search >1 thing back-to-back without pressing the "x" button in the search bar.

@ellenhp
Copy link
Member

ellenhp commented Sep 19, 2022

I need to think a bit harder about how to structure the BaseMap API. I tried to rewrite it partially and I think I made everything worse. 😅

@ellenhp ellenhp merged commit b02eace into headwaymaps:main Sep 19, 2022
@michaelkirk michaelkirk deleted the mkirk/poi-zooms-to-bounds branch November 10, 2022 20:46
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.

2 participants