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

Handle wrapped worlds in Box Selection example #14608

Merged
merged 2 commits into from Mar 26, 2023

Conversation

mike-000
Copy link
Contributor

@mike-000 mike-000 commented Mar 25, 2023

@github-actions
Copy link

📦 Preview the website for this branch here: https://deploy-preview-14608--ol-site.netlify.app/.

@mike-000 mike-000 marked this pull request as ready for review March 25, 2023 21:18
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

This example has a lot of complexity. Previously it was the rotation that made it complicated, now antimeridian handling is added. What if the API of ol/interaction/DragBox had a getGeometries() method, or the dragend event had a geometries property, where one geometry for each world is provided? Then this example would not get even more complicated. Later, this example could be further simplified by using turf for geometry level intersections, then nothing would have to be rotated.

@mike-000
Copy link
Contributor Author

That requirement is not specific to this example or DragBox. A method for converting wrapped geometries into normal world MultiPolygons or MultiLineStrings could be very useful elsewhere. #11681 (comment) has the algorithm for LineStrings, which would need to be adapted for LinearRings. It should also work with any global projection coordinates as splitting a long straight line in EPSG:4326 would produce a kink when displayed in EPSG:3857.

@ahocevar
Copy link
Member

@mike-000 Something like a Geometry#getGeometries(projection) method? That would indeed be nice! The algorithms wouldn't be trivial though, because multiple antimeridian crossings could be possible. But still simpler than a generic split algorithm. However, I think this is beyond the scope of this pull request.

A simple fix for #14608, without making this example more complicated, would be to simply limit panning to one world on the view.

@mike-000
Copy link
Contributor Author

A simple fix for #14608, without making this example more complicated, would be to simply limit panning to one world on the view.

That would just be hiding the issue. It is a legitimate requirement for anyone dealing with the Pacific region.

@mike-000
Copy link
Contributor Author

mike-000 commented Mar 26, 2023

What if the API of ol/interaction/DragBox had a getGeometries() method, or the dragend event had a geometries property, where one geometry for each world is provided

Even with multiple geometries there would still need to be separate feature.getGeometry().intersectsExtent(extent) as we would not want to use the extent of a MultiPolygon, and splitting an extent is not difficult (wrapAndSliceX does it but is not API so not suitable for the example). If it wasn't for the complication of geometry.translate(-world * worldWidth, 0); when handling rotation the multiple feature.getGeometry().intersectsExtent(extent) calls could be done in a single filter operation.

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

I'm not a fan of examples that try to be full featured applications. In my opinion, an example should show a single piece of functionality, which gives it an educational value. It seems you're not sharing this view, and it seems I'm unable to convince you. Anyway, as an appreciation for your efforts, I'm going to merge this now. But I'll continue to remind you.

@ahocevar ahocevar merged commit 5040ef2 into openlayers:main Mar 26, 2023
8 checks passed
@mike-000 mike-000 deleted the wrapped-box branch March 26, 2023 18:31
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.

Box Selection example broken
2 participants