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

Support unwrapped bbox values in changeset history queries #5473

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AntonKhorev
Copy link
Collaborator

@AntonKhorev AntonKhorev commented Jan 6, 2025

Fixes #3423.

Issue: Only changesets from that very narrow gap are shown.

How narrow this area is depends on zoom and map view width. It's an area between wrapped left and (independently) wrapped right longitude bounds because:

The bug is caused by Leaflet's method .wrap()

It's not Leaflet's method. Maybe it existed previously in Leaflet and was removed, but we added it again in f612f57. Implementing wrap on L.LatLngBounds by wrapping its corners independently is not going to give correct results in a general case.

Bit more sophisticated fix is to add client-side bbox validation where too large bboxes would be clipped from -180/180 meridian.

It's not difficult to write the logic without clipping. Unfortunately BoundingBox.from_bbox_params insists on clipping, that's why I'm not using BoundingBox.

is this a bug or an intentional feature to reduce server load?

I don't think that the current behavior reduces server load. The more changesets you filter out, the longer you have to scan the table. If the current implementation filters out changesets that shouldn't have been filtered out, it slows down the server.


This PR fixes only how changesets are selected by the /history page based on the current view area, which gets translated into the bbox parameter. It doesn't change how bboxes are displayed, which needs further fixing. When you zoom out, you can see the same place on the map several times. If there's a changeset, it also needs to be rendered several times. That usually doesn't matter much because you can see one of its renderings. But that doesn't work if you zoom in close to the date line. Only one side is going to be visible on the map. On the other hand the list in the sidebar should be correct.

image
image

@tomhughes
Copy link
Member

Isn't the real issue here that our wrap is just wrong when the bounding box spans the whole world, and that if the minimum and maximum longitude are more than 360 degrees apart it should just result 0 to 360 as the wrapped result?

@AntonKhorev
Copy link
Collaborator Author

Do you want the longitude span check on the javascript side? Something like this before sending the bbox:

if (max_lon - min_lon >= 360) {
  min_lon = -180;
  max_lon = 180;
}

This will eliminate one out of three cases here. But how would you modify wrap to produce a valid bbox in cases like min_lon = 170; max_lon = 190?

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.

Not all changesets shown in history view on low zoom levels
2 participants