Skip to content

Fix polygon validation function (issue #112) #113

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

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Fix polygon validation function (issue #112) #113

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2badc3c
Fix polygon validation function
vitcpp Dec 5, 2023
300ffa8
Increment version patch number up to 1.4.2
vitcpp Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions Makefile
View file
Original file line number Diff line number Diff line change
@@ -34,6 +34,7 @@ DATA_built = $(RELEASE_SQL) \
pg_sphere--1.3.0--1.3.1.sql \
pg_sphere--1.3.1--1.4.0.sql \
pg_sphere--1.4.0--1.4.1.sql \
pg_sphere--1.4.1--1.4.2.sql

DOCS = README.pg_sphere COPYRIGHT.pg_sphere
TESTS = version tables points euler circle line ellipse poly path box \
@@ -205,8 +206,8 @@ endif
pg_sphere--1.3.1--1.4.0.sql: pgs_circle_sel.sql.in pgs_hash.sql.in
cat upgrade_scripts/[email protected] $^ > $@

pg_sphere--1.4.0--1.4.1.sql:
cat upgrade_scripts/$@.in $^ > $@
pg_sphere--1.4.0--1.4.1.sql pg_sphere--1.4.1--1.4.2.sql:
@echo "-- Nothing to upgrade in the schema" > $@

# end of local stuff

2 changes: 1 addition & 1 deletion Makefile.common.mk
View file
Original file line number Diff line number Diff line change
@@ -5,4 +5,4 @@
#----------------------------------------------------------------------------

EXTENSION := pg_sphere
PGSPHERE_VERSION := 1.4.1
PGSPHERE_VERSION := 1.4.2
49 changes: 43 additions & 6 deletions expected/poly.out
View file
Original file line number Diff line number Diff line change
@@ -403,13 +403,17 @@ LINE 1: SELECT spoly '{(0d,0d),(10d,10d),(0d,10d),(10d,0d)}';
^
--- degenerate polygons -----
SELECT spoly '{(0d,1d),(0d,2d),(0d,3d)}';
ERROR: spherepoly_from_array: a line segment overlaps or polygon too large
LINE 1: SELECT spoly '{(0d,1d),(0d,2d),(0d,3d)}';
^
spoly
---------------------------------
{(0d , 1d),(0d , 2d),(0d , 3d)}
(1 row)

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem correct. This spoly and the following one should fail, no? They do not seem to be valid polygons to me.

Copy link
Contributor Author

@vitcpp vitcpp Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

SELECT spoly '{(1d,0d),(2d,0d),(3d,0d)}';
ERROR: spherepoly_from_array: a line segment overlaps or polygon too large
LINE 1: SELECT spoly '{(1d,0d),(2d,0d),(3d,0d)}';
^
spoly
---------------------------------
{(1d , 0d),(2d , 0d),(3d , 0d)}
(1 row)

--- functions
SELECT npoints( spoly '{(10d,0d),(10d,1d),(15d,0d)}');
npoints
@@ -1874,3 +1878,36 @@ SELECT spoly_is_convex(NULL);
f
(1 row)

-- Complex but valid polygon
SELECT '{
(3.30474723646012 , 1.08600456205300),
(3.30341855309927 , 1.08577960186707),
(3.30341054542378 , 1.08578643990271),
(3.30297351563319 , 1.08633534556428),
(3.30357156120003 , 1.08643683957210),
(3.30358891855857 , 1.08643995044436),
(3.30360894676365 , 1.08644306147078),
(3.30361829343581 , 1.08644430596871),
(3.30362630482521 , 1.08644555030213),
(3.30364633346451 , 1.08644866102000),
(3.30365300940335 , 1.08645052692055),
(3.30366102096957 , 1.08645177113937),
(3.30367036769496 , 1.08645363721023),
(3.30367837934959 , 1.08645488137174),
(3.30368906174976 , 1.08645612569695),
(3.30370107936906 , 1.08645799183673),
(3.30370642025712 , 1.08645985750225),
(3.30373179124734 , 1.08646358962156),
(3.30374514456618 , 1.08646545561358),
(3.30410706158729 , 1.08652886672786),
(3.30427803417922 , 1.08655868846497),
(3.30429673329093 , 1.08655930694968),
(3.30432478121775 , 1.08655930174652),
(3.30433278932944 , 1.08655308246640),
(3.30446348355532 , 1.08638330933224)
}'::spoly;
spoly
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
{(3.3047472 , 1.0860046),(3.3034186 , 1.0857796),(3.3034105 , 1.0857864),(3.3029735 , 1.0863353),(3.3035716 , 1.0864368),(3.3035889 , 1.08644),(3.3036089 , 1.0864431),(3.3036183 , 1.0864443),(3.3036263 , 1.0864456),(3.3036463 , 1.0864487),(3.303653 , 1.0864505),(3.303661 , 1.0864518),(3.3036704 , 1.0864536),(3.3036784 , 1.0864549),(3.3036891 , 1.0864561),(3.3037011 , 1.086458),(3.3037064 , 1.0864599),(3.3037318 , 1.0864636),(3.3037451 , 1.0864655),(3.3041071 , 1.0865289),(3.304278 , 1.0865587),(3.3042967 , 1.0865593),(3.3043248 , 1.0865593),(3.3043328 , 1.0865531),(3.3044635 , 1.0863833)}
(1 row)

2 changes: 1 addition & 1 deletion expected/version.out
View file
Original file line number Diff line number Diff line change
@@ -2,6 +2,6 @@
SELECT pg_sphere_version();
pg_sphere_version
-------------------
1.4.1
1.4.2
(1 row)

2 changes: 1 addition & 1 deletion pg_sphere.control
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# pg_sphere extension
comment = 'spherical objects with useful functions, operators and index support'
default_version = '1.4.1'
default_version = '1.4.2'
module_pathname = '$libdir/pg_sphere'
relocatable = true
29 changes: 29 additions & 0 deletions sql/poly.sql
View file
Original file line number Diff line number Diff line change
@@ -650,3 +650,32 @@ SELECT spoly_is_convex(spoly'{(53d 45m 35.0s, 37d 6m 30.0s), (52d 21m 36.0s, 41d
SELECT spoly_is_convex(spoly'{(12d,32d),(34d,12d),(59d,21d),(69d,21d)}');
SELECT spoly_is_convex(spoly'{(12d,32d),(34d,12d),(59d,21d),(34d,40d)}');
SELECT spoly_is_convex(NULL);

-- Complex but valid polygon
SELECT '{
(3.30474723646012 , 1.08600456205300),
(3.30341855309927 , 1.08577960186707),
(3.30341054542378 , 1.08578643990271),
(3.30297351563319 , 1.08633534556428),
(3.30357156120003 , 1.08643683957210),
(3.30358891855857 , 1.08643995044436),
(3.30360894676365 , 1.08644306147078),
(3.30361829343581 , 1.08644430596871),
(3.30362630482521 , 1.08644555030213),
(3.30364633346451 , 1.08644866102000),
(3.30365300940335 , 1.08645052692055),
(3.30366102096957 , 1.08645177113937),
(3.30367036769496 , 1.08645363721023),
(3.30367837934959 , 1.08645488137174),
(3.30368906174976 , 1.08645612569695),
(3.30370107936906 , 1.08645799183673),
(3.30370642025712 , 1.08645985750225),
(3.30373179124734 , 1.08646358962156),
(3.30374514456618 , 1.08646545561358),
(3.30410706158729 , 1.08652886672786),
(3.30427803417922 , 1.08655868846497),
(3.30429673329093 , 1.08655930694968),
(3.30432478121775 , 1.08655930174652),
(3.30433278932944 , 1.08655308246640),
(3.30446348355532 , 1.08638330933224)
}'::spoly;
18 changes: 9 additions & 9 deletions src/line.c
View file
Original file line number Diff line number Diff line change
@@ -496,6 +496,15 @@ sline_sline_pos(const SLine *l1, const SLine *l2)
vector3d_spoint(&p[2], &v[1][0]);
vector3d_spoint(&p[3], &v[1][1]);

/* check connected lines */
if (FPgt(il2->length, 0.0) && (vector3d_eq(&v[0][0], &v[1][0]) ||
vector3d_eq(&v[0][0], &v[1][1]) ||
vector3d_eq(&v[0][1], &v[1][0]) ||
vector3d_eq(&v[0][1], &v[1][1])))
{
return PGS_LINE_CONNECT;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add a test for this change to sline_sline_pos()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Sounds good, @vitcpp.

/* Check, sl2 is at equator */
if (FPzero(p[2].lat) && FPzero(p[3].lat))
{
@@ -520,15 +529,6 @@ sline_sline_pos(const SLine *l1, const SLine *l2)
return PGS_LINE_AVOID;
}

/* check connected lines */
if (FPgt(il2->length, 0.0) && (vector3d_eq(&v[0][0], &v[1][0]) ||
vector3d_eq(&v[0][0], &v[1][1]) ||
vector3d_eq(&v[0][1], &v[1][0]) ||
vector3d_eq(&v[0][1], &v[1][1])))
{
return PGS_LINE_CONNECT;
}

/* Now sl2 is not at equator */

if (FPle(il2->length, seg_length))
1 change: 0 additions & 1 deletion upgrade_scripts/pg_sphere--1.4.0--1.4.1.sql.in
View file

This file was deleted.