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

base polygon geometry rectangle IDL handling on Rectangle.fromCartesianArray #7533

Merged
merged 6 commits into from
Feb 5, 2019

Conversation

likangning93
Copy link
Contributor

@shehzan10 noticed that geodetic polygons had weird rectangles sometimes when crossing the PM.

Here in master

This PR updates the IDL handling for geodetic polygons to more closely match what Rectangle.fromCartesianArray does, which seems to be correct.

@cesium-concierge
Copy link

Thanks for the pull request @likangning93!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@likangning93
Copy link
Contributor Author

Fixes a bug introduced in this release, so no CHANGES.md update.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 1, 2019

@likangning93 should there be a unit test for this?

@likangning93
Copy link
Contributor Author

There's an IDL test case already, the corner case @shehzan10 found was a false-positive for IDL cross. I can add the cases from RectangleSpec to make sure coverage is similar.

@ggetz
Copy link
Contributor

ggetz commented Feb 1, 2019

Thanks @likangning93 this fixes the issue. We can get it in if you fix up those tests soon.

@likangning93
Copy link
Contributor Author

There's an IDL test case already

Looks like the previous case was also only for rhumb, whoops.

@likangning93
Copy link
Contributor Author

likangning93 commented Feb 1, 2019

Okay looks like eslint 5.13.0 is breaking things everywhere: https://github.com/eslint/eslint/issues things broke due to eslint 5.13.0, holding off on this until next release...

@likangning93
Copy link
Contributor Author

@ggetz @hpinkos updated!

@hpinkos
Copy link
Contributor

hpinkos commented Feb 5, 2019

Looks great, thanks @likangning93!

@hpinkos hpinkos merged commit 44c5a17 into master Feb 5, 2019
@hpinkos hpinkos deleted the fixPolygonRectangleIdl branch February 5, 2019 20:05
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.

4 participants