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.

Sign up for GitHub

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

Jump to bottom

Longitude Compare at +180 degrees #179

Merged
merged 6 commits into from
Aug 3, 2018
Merged

Longitude Compare at +180 degrees #179

merged 6 commits into from
Aug 3, 2018

Conversation

jklamer
Copy link
Contributor

@jklamer jklamer commented Jul 17, 2018 •
edited
Loading

All Longitudes are stored as an underlying Angle from [-180,180). But if a location is created at 180 the Longitude representation and functionality remain 180. However when using the underlying comparison methods of the parent Angle class, this distinction is lost as the this.getDm7() methods grabs the raw stored Angle value. So that Longitude(179).isGreaterThan(Longitude(180)) is true.

With this change the range methods will use the asDm7 method, which is overridden by Longitude to return 180 if that is what it is created with.

One oddity this will create Longitude(-180).isGreaterThanOrEqualTo(Longitude(180)) will now return false, but Longitude(-180).equals(Longitude(180)) will return true as before.

EDIT UPDATE:
Reverting to using getDm7 when the classes are different and thus have potentially different asDm7 range /values; for instance Heading.isLessThan(Longitude).

@matthieun
Copy link
Collaborator

Closing, will re-open when #178 is merged. Unfortunately travis checks are not reliable until that one is merged.

@matthieun matthieun closed this Jul 17, 2018
@matthieun matthieun reopened this Jul 17, 2018
@MikeGost MikeGost added the Bug label Jul 17, 2018
matthieun
matthieun requested changes Jul 17, 2018
View reviewed changes
Copy link
Collaborator

@matthieun matthieun left a comment

Choose a reason for hiding this comment

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

I think the statement "Longitude(179).isGreaterThan(Longitude(180)) is true" should be correct, as Longitude(180) in reality is equal to Longitude(-180)

@jklamer
Copy link
Contributor Author

jklamer commented Jul 18, 2018

I disagree, I think by that logic then anything above or at -179.999 is greater than 180. This becomes an issue in Rectangle.overlaps(Rectangle) when one of the sides is at 180, its becomes effectively flipped to -180.

I can add an this.equals(other) || this.asDm7 >= other.asDm7 to remove the equals oddity?

@jklamer
Copy link
Contributor Author

jklamer commented Jul 24, 2018

What is your thinking? I understand wanting to keep the continuity of -180 is 180, but without treating 180 as larger in the range of Longitudes we'll get incorrect results for operations involving Rectangles with their East Side on the antiMeridian (and possibly other places).

matthieun
matthieun requested changes Jul 30, 2018
View reviewed changes
Copy link
Collaborator

@matthieun matthieun left a comment

Choose a reason for hiding this comment

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

Ok, let's move forward with this. However I think we might want more unit tests. @MikeGost any ideas of what else we could add to make sure nothing breaks?

@MikeGost
Copy link
Contributor

Per offline discussion with @matthieun - I would add more tests surrounding bounding boxes and intersection/overlap calls with these. Specific cases to consider are bounding boxes that stretch across the antimeridian and ones that greater than 180 degrees but don't cross the antimeridian.

@jklamer
Copy link
Contributor Author

jklamer commented Jul 31, 2018

I added a test to Rectangle that shows two different cases in which without the changes, they would fail. I confirmed this by reverting my changes and running the test. Let me know what you think. I believe the interactions displayed in the Rectangle test are the desired ones.

@MikeGost
Copy link
Contributor

MikeGost commented Aug 1, 2018

Sorry for nit-picking - can we add more tests. Specifically:

  1. Rectangle greater than 180 degrees in width not crossing anti-meridian
  2. Rectangle greater than 180 degrees in width crossing anti-meridian

@jklamer
Copy link
Contributor Author

jklamer commented Aug 1, 2018

@MikeGost Constructed with forCorners?

@MikeGost
Copy link
Contributor

MikeGost commented Aug 2, 2018

@jklamer - does construction matter in this case? I just wanted to verify backward compatibility across Polygons > 180 degrees and wrapping around meridian.

@MikeGost
Copy link
Contributor

MikeGost commented Aug 3, 2018

Scratch that, @jklamer and I talked offline, not a viable test case. PR looks good!

matthieun
matthieun approved these changes Aug 3, 2018
View reviewed changes
Copy link
Collaborator

@matthieun matthieun left a comment

Choose a reason for hiding this comment

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

Let's roll with that!

@matthieun matthieun merged commit 61447c0 into osmlab:dev Aug 3, 2018
@matthieun matthieun added this to the 5.1.6 milestone Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeGost MikeGost MikeGost approved these changes

@matthieun matthieun matthieun approved these changes

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
5.1.6
Development

Successfully merging this pull request may close these issues.
3 participants
@jklamer @matthieun @MikeGost