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

Ensure polygons are inside -180/180 range before making stac request #734

Merged
merged 3 commits into from
Nov 9, 2023

Conversation

danielfdsilva
Copy link
Collaborator

See #732

I created a script to cut the polygons that cross the antimeridian and them shift them inside the correct range.
Includes some unit tests for the function.

Copy link

netlify bot commented Nov 6, 2023

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 268e089
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/654cc713a9124000080bb664
😎 Deploy Preview https://deploy-preview-734--veda-ui.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@j08lue
Copy link
Contributor

j08lue commented Nov 6, 2023

When I draw on the left-most or right-most out of four cycles of the Earth across the antimeridian, part of the geometry seems to be there but not visible:

Drawn on left-most cycle:

image

Drawn on right-most cycle:

image

Actually, when I draw on the left-most or right-most map west respectively east of the antimeridian, the polygon seems to be present (analysis option becomes active), but the polygon is not shown on any of the maps.

@j08lue
Copy link
Contributor

j08lue commented Nov 6, 2023

We earlier discussed whether we could just show only one cycle of the Earth. My concern then was, that this would make the map look weird. However, if this causes too much headache, could we perhaps show the whole map (with repetitions), but guide the user only draw within those cycles where we can do the normalization safely?

@danielfdsilva
Copy link
Collaborator Author

@j08lue Addressed.
Turns out that mapbox does not render anything outside the -540/540 range (basically 3 maps). I found an old mapbox-gl draw issue that talks about this, and it is still happening.
I followed one of the comments and limited the mapbox bounds to -540/540, which gives you 3 maps to draw on. This will be fine for 99% of users. If you have a screen that is so wide and narrow, the map will be a bit more zoomed in, but at that point it's on you. 😅

@danielfdsilva
Copy link
Collaborator Author

3 maps on a very wide and narrow window.
image

Full screen on my high-res monitor, with a smaller timeline allows to view the whole map
image

Copy link
Contributor

@j08lue j08lue left a comment

Choose a reason for hiding this comment

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

Awesome, great work handling all these over-the-edge cases... The guardrails keep me from running into the original issue now.

);

// Or 1 in case the largestLng is exactly -180.
const shift = Math.ceil(Math.abs((largestLng + 180) / 360)) || 1;
Copy link
Collaborator

@hanbyul-here hanbyul-here Nov 9, 2023

Choose a reason for hiding this comment

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

It will be an edge case, but 1 degree in Mercator doesn't seem to be that small number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not refer to 1º but to how many times to ship 360º. The result is that the coordinates move to the "repeated map" they are to the correct one (between -180/180)

coordinates: [
[
[
[-170, -20],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you point me where the order of the coordinates changes? I can't wrap my head around it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was also strange to me, but for some reason the turf.js function (intersect and difference), reorder the coordinates when cutting the polygons.

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

Yay test. (Test description can be more helpful! ) I left a concern about an edge case and a question about the order of the coordinates. 🤔 I wonder if there are better names than east and west for coordinates to go over >180, <-180 . Nothing seems to be a blocker

@danielfdsilva
Copy link
Collaborator Author

@hanbyul-here You're right. The messages were absolutely atrocious 😅. I made them more descriptive and will merge. Feel free to suggest further improvements.
I could not thing of better names than west/east, but happy to hear a suggestion.

@danielfdsilva danielfdsilva merged commit 2c3f46d into feature/exploration Nov 9, 2023
7 checks passed
@danielfdsilva danielfdsilva deleted the fix/732-antimeridian branch November 9, 2023 11:51
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