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

Fix geojson EllipsoidRhumbLine error for duplicated points #12460

Merged
merged 4 commits into from
Feb 10, 2025

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Feb 3, 2025

Description

Sometimes polygons have duplicated points in their definition. This causes the code we have to subdivide the lines for the EllipsoidRhumbLine arc type to fail. Even if it's not ideal the duplicated points shouldn't cause our render to crash, especially when other applications like geojson.io render correctly and validators show nothing wrong with the GeoJSON.

Note: I'm not 100% confident this fix is done at the right "level" of the code. I was also able to get the failing examples working by removing the error call itself. That said I think it's a valid error if you're using that class directly but in this instance for polygons we simply don't care and can short circuit the function before we even try the code that "fails".

For context I encountered this during work on #12456 with one of the example sources

Issue number and link

Fixes #7864

Testing plan

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@jjspace jjspace requested a review from ggetz February 3, 2025 19:23
Copy link

github-actions bot commented Feb 3, 2025

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Feb 4, 2025

Nice, this is a long running bug that I'm sure many will love to see fixed. Thanks @jjspace!

I just have one comment about how we can make this fix a bit more general-purpose. Please update CHANGES.md and look into adding a unit test at least for this case.

I don't think we have any tests for subdivideRhumbLine specifically, so some additional coverage would also be welcome. 😄

@ggetz
Copy link
Contributor

ggetz commented Feb 10, 2025

All looks good to me! I can confirm several of the example datasets from the original issue are now rendering without error 🎉

@ggetz ggetz enabled auto-merge February 10, 2025 18:14
@ggetz ggetz added this pull request to the merge queue Feb 10, 2025
Merged via the queue into main with commit 7f59971 Feb 10, 2025
4 of 5 checks passed
@ggetz ggetz deleted the fix-rhumb-subdivide branch February 10, 2025 18:14
@ggetz
Copy link
Contributor

ggetz commented Feb 10, 2025

Thanks @jjspace!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GeoJSON crashes in Cesium
2 participants