Opened 3 years ago

Closed 3 years ago

Last modified 8 months ago

#4461 closed defect (fixed)

ST_AsTWKB doesn't always remove duplicate points

Reported by: nicklas Owned by: nicklas
Priority: medium Milestone: PostGIS 3.0.0
Component: postgis Version: master
Keywords: Cc:

Description

In this tweet gabrielroldan says that ST_AsTWKB doesn't remove repeated points https://twitter.com/gabrielroldan/status/1152379542801174528

and he is right for some cases. If the writer haven't yet collected enough points for what is needed for that geometry type it doesn't remove the point even if there is many more points after.

The function actually needs one extra before it starts removing.

Example

select st_astwkb('linestring(1 1, 2 2, 2 2, 3 1)'::geometry);

The fix is very small, but might be a braking change for someone.

The proposed svn diff:

svn diff
Index: liblwgeom/lwout_twkb.c
===================================================================
--- liblwgeom/lwout_twkb.c	(revision 17618)
+++ liblwgeom/lwout_twkb.c	(working copy)
@@ -112,7 +112,8 @@
 	int64_t nextdelta[MAX_N_DIMS];
 	int npoints = 0;
 	size_t npoints_offset = 0;
-
+	int max_points_left = pa->npoints;
+	
 	LWDEBUGF(2, "Entered %s", __func__);
 
 	/* Dispense with the empty case right away */
@@ -173,8 +174,11 @@
 		/* Skipping the first point is not allowed */
 		/* If the sum(abs()) of all the deltas was zero, */
 		/* then this was a duplicate point, so we can ignore it */
-		if ( i > minpoints && diff == 0 )
+		if ( diff == 0 &&  max_points_left > minpoints )
+		{
+			max_points_left--;
 			continue;
+		}
 
 		/* We really added a point, so... */
 		npoints++;
Index: liblwgeom/measures3d.c
===================================================================
--- liblwgeom/measures3d.c	(revision 17618)
+++ liblwgeom/measures3d.c	(working copy)
@@ -1508,7 +1508,6 @@
 			pl->pv.z += vp.z / vl;
 		}
 	}
-
 	return (!FP_IS_ZERO(pl->pv.x) || !FP_IS_ZERO(pl->pv.y) || !FP_IS_ZERO(pl->pv.z));
 }

Any thoughts before I commit?

Change History (7)

comment:1 by nicklas, 3 years ago

Owner: changed from pramsey to nicklas

comment:2 by Algunenano, 3 years ago

Any thoughts before I commit?

 +	int max_points_left = pa->npoints;

This should use the same type as pa→npoints (uint32_t).

Index: liblwgeom/measures3d.c
===================================================================
--- liblwgeom/measures3d.c	(revision 17618)
+++ liblwgeom/measures3d.c	(working copy)
@@ -1508,7 +1508,6 @@
 			pl->pv.z += vp.z / vl;
 		}
 	}
-
 	return (!FP_IS_ZERO(pl->pv.x) || !FP_IS_ZERO(pl->pv.y) || !FP_IS_ZERO(pl->pv.z));
 }

This part of the patch is unrelated.

Can you also add tests around this, please?

comment:3 by nicklas, 3 years ago

Resolution: fixed
Status: newclosed

In 17654:

Remove duplicate points with ST_AsTWKB also when apearing before min npoints Closes #4461

comment:4 by nicklas, 3 years ago

Thanks @Algunenano. A 4 row patch and I miss the correct type :-)

comment:5 by pramsey, 3 years ago

In 17656:

ST_AsTWKB doesn't always remove duplicate points
References #4461

comment:6 by pramsey, 3 years ago

In 17657:

ST_AsTWKB doesn't always remove duplicate points
References #4461

comment:7 by Aurélien Mino, 8 months ago

From my understanding of the patch, the duplicated point is removed for any linestring. However the TWKB specification only talks about this optimization for linearring of polygons.

EDIT: I was wrong, I misunderstood the optimization done in postgis (removing duplicated consecutive points). I thought the optimization was to remove last point of a linear ring.

Last edited 8 months ago by Aurélien Mino (previous) (diff)
Note: See TracTickets for help on using tickets.