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.

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

ST_OffsetCurve multiline + tests #224

Closed
wants to merge 16 commits into
base: svn-trunk
from

Conversation

Projects
None yet
3 participants
@Komzpa
Copy link
Member

Komzpa commented Feb 27, 2018

https://trac.osgeo.org/postgis/ticket/2508 now without topologyexception

Komzpa added some commits Feb 27, 2018

int cluster_within_distance(LWGEOM** geoms, uint32_t num_geoms, double tolerance, LWGEOM*** clusterGeoms, uint32_t* num_clusters);
int union_dbscan(LWGEOM** geoms, uint32_t num_geoms, UNIONFIND* uf, double eps, uint32_t min_points, char** is_in_cluster_ret);
int
cluster_intersecting(GEOSGeometry** geoms, uint32_t num_geoms, GEOSGeometry*** clusterGeoms, uint32_t* num_clusters);

This comment has been minimized.

@dbaston

dbaston Feb 27, 2018

Member

Are these related to the ticket?

This comment has been minimized.

@Komzpa

Komzpa Feb 28, 2018

Author Member

removed

@@ -1206,8 +1210,11 @@ lwgeom_sharedpaths(const LWGEOM* geom1, const LWGEOM* geom2)
return result;
}

LWGEOM*
lwgeom_offsetcurve(const LWLINE* lwline, double size, int quadsegs, int joinStyle, double mitreLimit)
static LWGEOM*

This comment has been minimized.

@dbaston

dbaston Feb 27, 2018

Member

Consider removing this prototype

This comment has been minimized.

@Komzpa

Komzpa Feb 27, 2018

Author Member

done

const LWCOLLECTION* col = lwgeom_as_lwcollection(geom);
int i;
result = lwcollection_construct_empty(MULTILINETYPE, srid, is3d, LW_FALSE);
for (i = 0; i < col->ngeoms; i++)

This comment has been minimized.

@dbaston

dbaston Feb 27, 2018

Member

ngeoms is unsigned, right?

This comment has been minimized.

@Komzpa

Komzpa Feb 27, 2018

Author Member

@Algunenano how do you get the integer warnings? I miss them on travis :)

This comment has been minimized.

@Algunenano

Algunenano Feb 27, 2018

Member

I have a pretty big list of flags, but you are probably looking for -Wconversion. Right now it leads to a lot of warnings, but hopefully I'll address most of them in #208

This comment has been minimized.

@Komzpa

Komzpa Feb 28, 2018

Author Member

@dbaston fixed

lwcollection_add_lwgeom(result, tmp);
else
{
const LWCOLLECTION* c2 = lwgeom_as_lwcollection(tmp);

This comment has been minimized.

@dbaston

dbaston Feb 27, 2018

Member

Consider a comment to explain what's happening in this block

This comment has been minimized.

@Komzpa

Komzpa Feb 27, 2018

Author Member

made separate function for collection inline concatenation

if (!result) lwerror("lwgeom_offsetcurve: noding input still does not let it be processed.");
}
}
tmp = lwgeom_make_valid(result);

This comment has been minimized.

@dbaston

dbaston Feb 27, 2018

Member

May need to adjust implicit makevalid based on closure of dev list discussion

This comment has been minimized.

@Komzpa

Komzpa Feb 27, 2018

Author Member

removed makevalid for now

lwgeom_offsetcurve_line(const LWLINE* lwline, double size, int quadsegs, int joinStyle, double mitreLimit);

static LWGEOM*
lwgeom_offsetcurve_line(const LWLINE* lwline, double size, int quadsegs, int joinStyle, double mitreLimit)

This comment has been minimized.

@dbaston

dbaston Feb 27, 2018

Member

lwline_offsetcurve and lwmline_offsetcurve would more closely follow existing conventions, IMO

This comment has been minimized.

@Komzpa

Komzpa Feb 27, 2018

Author Member

renamed

This comment has been minimized.

@dbaston

dbaston Feb 28, 2018

Member

Sorry, maybe this comment was unclear. The convention would be to have a function lwgeom_offsetcurve that takes an argument LWGEOM and uses the switch statement to dispatch to lwline_offsetcurve, lwmline_offsetcurve, lwcollection_offsetcurve, etc. as needed. They type-specific variants can all be public. See for example lwgeom_segmentize2d in the header at line 1481.


LWDEBUGF(3, "Boundary point: %s", lwgeom_to_ewkt(GEOS2LWGEOM(point, 0)));
/*
* Union with first geometry point, obtaining full noding

This comment has been minimized.

@dbaston

dbaston Feb 27, 2018

Member

Could this be replaced with a call to UnaryUnion?

This comment has been minimized.

@Komzpa

Komzpa Feb 27, 2018

Author Member

no. UnaryUnion crashed on regression tests.

Komzpa added some commits Feb 27, 2018

@Komzpa

This comment has been minimized.

Copy link
Member Author

Komzpa commented Feb 28, 2018

@dbaston @Algunenano can you wave if it's ok to commit?

uint8_t is3d = FLAGS_GET_Z(geom->flags);
if (srid == SRID_INVALID) return NULL;

if (geom->type == LINETYPE)

This comment has been minimized.

@Algunenano

Algunenano Feb 28, 2018

Member

The rest of the codebase uses case statements instead of if-defs. Please, consider switching it

This comment has been minimized.

@Komzpa

Komzpa Feb 28, 2018

Author Member

switched to case.


result = lwmline_offsetcurve(geom, size, quadsegs, joinStyle, mitreLimit);
if (!result)
{

This comment has been minimized.

@Algunenano

Algunenano Feb 28, 2018

Member

Is this going to print the lwerror("%s: unsupported geometry type: %s", __func__, lwtype_name(geom->type)); from above?

This comment has been minimized.

@Komzpa

Komzpa Feb 28, 2018

Author Member

in context of postgres, lwerror is pg_error

pg_error(const char *fmt, va_list ap)
which in turn calls ereport(ERROR which in turn cancels query at the spot (see https://www.postgresql.org/docs/10/static/error-message-reporting.html) - so if it prints that, it won't get to this stage.

#endif

col->geoms[col->ngeoms] = (LWGEOM*)geom;
col->ngeoms++;
return col;
}

LWCOLLECTION*
lwcollection_concat_in_place(LWCOLLECTION* col1, const LWCOLLECTION* col2)

This comment has been minimized.

@dbaston

dbaston Feb 28, 2018

Member

Add source comment to document what this function does, and what responsibilities of caller are. (It looks like caller would need to release col2 after calling?)

Komzpa added some commits Mar 5, 2018

@strk strk closed this in bd44549 Mar 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.