Fix polygon validation function (issue #112) #113
Conversation
Some previous changes to improve the behaviour in case of degenerate poygons introduced a bug in the polygon validation function. The changes are rolled back.
An extra change is introduced as well: generate empty upgrade scripts without the need to create *.sql.in files in upgrade_script subdirectory.
| --------------------------------- | ||
| {(0d , 1d),(0d , 2d),(0d , 3d)} | ||
| (1 row) | ||
|
|
There was a problem hiding this comment.
This doesn't seem correct. This spoly and the following one should fail, no? They do not seem to be valid polygons to me.
There was a problem hiding this comment.
@esabol I rolled back the changes in PR #36 because it broke the creation of valid polygons. I looked at the issue where we decided to consider degenerate polygons as invalid, but I can't remember why we decided to so, unfortunately. Probably, due to OGC specification of valid polygons. This bug actually makes pgsphere unusable in production, but invalidation of degenerate polygons don't produce any problems. Despite of OGC specification of valid polygons, in practice, we may consider support of genenerate polygons as well to make pgsphere more usable.
P.S. Polygons with intersecting edges are really invalid and can not be used in calculations. But degenerate polygons do not break algorithms.
P.P.S I remembered our discussions. The key point was that not all types of degenerate polygons were supported. To make the consistent behaviour we decided to disable support of degenerate polygons. But the proposed and merged patch breaks the support for valid polygons and it should be fixed asap. I propose to fix it and think how to improve that function one more time.
There was a problem hiding this comment.
I'd rather see the validation of polygons fixed instead of disabled. I disagree that the current version is "unusable". We've been using this version in production, and we have not encountered any issues with our polygons, but I guess we've just been lucky.
There was a problem hiding this comment.
@esabol I understand the problem with sline_sline_pos. I'm going to propose another solution but it takes more time to implement and test. The core reason is in using of spoint_at_sline in sline_sline_pos before checking for connectivity of adjacent segments. In past, the connectivity check was prior to the calling of spoint_at_sline.
I believe it is reasonable to apply this PR and increment patch number, because the big is severe. But next commit we improve sline_sline_pos. It more risky and needs more testing.
| { | ||
| return PGS_LINE_CONNECT; | ||
| } | ||
|
|
There was a problem hiding this comment.
Would it be possible to add a test for this change to sline_sline_pos()?
There was a problem hiding this comment.
@esabol I think, the added polygon allows to test this branch in the code. Otherwise, it goes down in the function and returns wrong relationship. The current change just rolls back the old commit. I propose to fix degenerate polygons later, in another commit.
No description provided.