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

Winding/Orientation for LineStrings #169

Merged
merged 1 commit into from
Mar 1, 2018
Merged

Winding/Orientation for LineStrings #169

merged 1 commit into from
Mar 1, 2018

Conversation

amandasaurus
Copy link
Member

This is a rework of #134 with the suggested changes.

The Orient trait is very useful for encoding (Multi)Polygons, since you can ensure the data is correctly ordered, but it's not helpful for reading bytes. In that case, you need a way to look at a linestring and see if it's clockwise or counter-clockwise, to see if it's an interior or exterior ring. That's what I've added in this PR.

@amandasaurus amandasaurus mentioned this pull request Sep 23, 2017
Closed
@amandasaurus amandasaurus force-pushed the winding-order branch 2 times, most recently from 3104e2e to 3c70cc6 Compare February 10, 2018 11:00
@amandasaurus
Copy link
Member Author

I've rebased this branch on current master. Is there any feedback? Is this a good feature?

@urschrei
Copy link
Member

I don't think we should extract the shoelace formula into its own public module; can we change Orient::signed_ring_area to be pub(crate) instead? That would make it importable in other modules that need it (area)?

@amandasaurus
Copy link
Member Author

I don't think we should extract the shoelace formula into its own public module

Changed (with a history rewrite)

can we change Orient::signed_ring_area to be pub(crate) instead? That would make it importable in other modules that need it (area)?

I'm not sure what you mean here, there's (now) algorithm::winding_order::twice_signed_ring_area, not Orient::signed_ring_area, and I've changed it to pub(crate)

@urschrei
Copy link
Member

I'm not sure what you mean here

Yeah, I misread that change – sorry about that. This looks good to me now!

@amandasaurus
Copy link
Member Author

Don't merge this now. I've done an explicit panic!() for a scenario that I thought was impossible, and I'm using this code more myself and hitting this scenario.

@amandasaurus
Copy link
Member Author

I have fixed this panic issue, and rebased, and pushed to the branch. Is this all OK and merge-able?

@frewsxcv
Copy link
Member

frewsxcv commented Mar 1, 2018

bors r=urschrei

bors bot added a commit that referenced this pull request Mar 1, 2018
@bors
Merge #169
17ae39c 
169: Winding/Orientation for LineStrings r=urschrei a=rory

This is a rework of #134 with the suggested changes.

The `Orient` trait is very useful for encoding `(Multi)Polygons`, since you can ensure the data is correctly ordered, but it's not helpful for reading bytes. In that case, you need a way to look at a linestring and see if it's clockwise or counter-clockwise, to see if it's an interior or exterior ring. That's what I've added in this PR.
@bors
Copy link
Contributor

bors bot commented Mar 1, 2018

Build succeeded

@bors bors bot merged commit 271a3e8 into georust:master Mar 1, 2018
@amandasaurus amandasaurus deleted the winding-order branch March 1, 2018 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amandasaurus @urschrei @frewsxcv