-
Notifications
You must be signed in to change notification settings - Fork 15
Fix sline_sline_pos #36
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
Conversation
Always connect your pull requests to their relevant issue numbers, please. You can do this by mentioning the issue number in the title or description of the PR. Issue #35 |
Seems like a simple enough change, but I'd like to see a test case added which fails without the PR and passes with the PR. |
b6d7eb0
to
a6bd0ff
Compare
Sure, I added a pair of tests to sql/poly.sql |
Colleagues, I have some doubts about this change. I think, degenerate polygons can be accepted by most of the algorithms. This PR disables some types of polygons that may affect on user experience - if the old version constructed such degenerate polygons, the new version reports an error. Some user datasets may contain degenerate polygons. Explicit error reporting may stop data pipelines to process some datasets. Give me please some more time to think about it. |
If there's a use-case for supporting degenerate polygons, I'd be open to it, but it seems to me that giving correct results is what should matter. Degenerate polygons are not valid polygons, according to the definition given by the OGC Simple Features Specification. See https://cs.stackexchange.com/a/112703. |
Interesting. Thank you for testing that and reporting your findings, @ggnmstr. |
It seems there is a more complex issue with degenerate polygons. Thank you @ggnmstr for your findings. There is the working idea to separate two concepts - storage and algorithms. spoly is a storage for polygon data. The storage may or may not validate the input data because in most cases the storage just used to manipulate data but not to validate it. My position is to allow to keep both valid and invalid data in storage. Data import algorithms just load data into storage objects as is. Some functions to validate and normalize data should be implemented as well. The algorithms assume that the input storage objects contain valid data. Such check is not trivial for polygons. It is the developer responsibility to pass valid data to algorithms. Otherwise, the behaviour is undefined. For most of the algorithms degenerate polygons are ok but it is important to avoid edge crosses that may lead to the wrong results. Overlapping edges may be used to construct polygons with holes (if needed). Now we have some inconsistent behaviour with spoly. Once we can't create degenerate spoly with 4 points and it is the adopted behaviour I think I'm ok to disable creation of all types of degenerate polygons. Later, if we decide to allow degenerate polygons we can change this behaviour. Lets wait for PR #22 and then merge it. |
@ggnmstr Could you please rebase the branch to the newer version and start the commit description from the capital letter? We will merge it if tests are passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ggnmstr Could you, please, start the commit description from the capital letter?
a6bd0ff
to
b6be266
Compare
b6be266
to
5e7111e
Compare
@vitcpp Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the tests are passed, I approve it. One trivial request is to start the PR title with a capical letter. @ggnmstr Thank you!
The problem fixed by this pull request is described in Issue #35