Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#212 closed defect (fixed)

compound curve in a curve polygon is flaky

Reported by: robe Owned by: mleslie
Priority: medium Milestone: PostGIS 1.4.0
Component: postgis Version: 1.4
Keywords: Cc:

Description

Okay this is nice -- I can now do this:

SELECT ST_GeomFromEWKT('CURVEPOLYGON(COMPOUNDCURVE(CIRCULARSTRING(0 0,2 0, 2 1, 2 3, 4 3),(4 3, 4 5, 1 4, 0 0)))');

but when I do this: SELECT ST_AsEWKT(ST_GeomFromEWKT('CURVEPOLYGON(COMPOUNDCURVE(CIRCULARSTRING(0 0,2 0, 2 1, 2 3, 4 3),(4 3, 4 5, 1 4, 0 0)))'));

I get CURVEPOLYGON();

and this: SELECT ST_CurveToLine(ST_GeomFromEWKT('CURVEPOLYGON(COMPOUNDCURVE(CIRCULARSTRING(0 0,2 0, 2 1, 2 3, 4 3),(4 3, 4 5, 1 4, 0 0)))'));

gives me

ERROR: Invalid ring type found in CurvePoly?. CONTEXT: SQL function "st_curvetoline" statement 1

This is testing against my PostGIS 1.4b build on windows.

Change History (14)

comment:1 Changed 8 years ago by robe

When I do this

SELECT ST_AsEWKT(ST_GeomFromEWKT('CURVEPOLYGON((-1.5 -0.85, 0 5, 4.5 5.5, 5.6 2, 4 -1, -1.5 -0.85), COMPOUNDCURVE(CIRCULARSTRING(0 0,2 0,2 1, 2 3, 4 3),(4 3, 4 5, 1 4, 0 0)))'));

I get this:

CURVEPOLYGON((-1.5 -0.85,0 5,4.5 5.5,5.6 2,4 -1,-1.5 -0.85),)

comment:2 Changed 8 years ago by robe

Owner: changed from pramsey to mleslie

comment:3 Changed 8 years ago by mleslie

Status: newassigned

Split the ST_CurveToLine into a ticket of it's own to avoid confusing myself.

http://trac.osgeo.org/postgis/ticket/213

comment:4 Changed 8 years ago by mcayland

Milestone: postgis 1.4.1postgis 1.4.0
Resolution: fixed
Status: assignedclosed
Version: 1.4

Yup, this appears to work for me - marking as resolved.

ATB,

Mark.

comment:5 Changed 8 years ago by robe

Resolution: fixed
Status: closedreopened

The above seems to work for me too now so much better. However -- what am I doing wrong here?

This works -- I can have an outer ring that is a compound curve and an inner ring that is a regular closed line:

SELECT ST_GeomFromEWKT('CURVEPOLYGON(COMPOUNDCURVE(CIRCULARSTRING(0 0,2 0, 2 1, 2 3, 4 3),(4 3, 4 5, 1 4, 0 0)), (1.7 1, 1.4 0.4, 0.70 0.40, 0.37 0.97, 0.70 1.54, 1.34 1.53, 1.7 1))');

But when I attempt a curved polygon with a compound curve outer ring and a circular inner ring, can't get it to work.

Here is my simple ring -- seems valid produces a nice cirle as far as I can tell

SELECt ST_CurveToLine('CIRCULARSTRING(1.7 1, 1.4 0.4, 1.7 1)');

So replacing my above with my ring: SELECT ST_GeomFromEWKT('CURVEPOLYGON(COMPOUNDCURVE(CIRCULARSTRING(0 0,2 0, 2 1, 2 3, 4 3),(4 3, 4 5, 1 4, 0 0)), CIRCULARSTRING(1.7 1, 1.4 0.4, 1.7 1) )');

I get this error: ERROR: geometry requires more points HINT: "...CIRCULARSTRING(1.7 1, 1.4 0.4, 1.7 1)" <-- parse error at position 126 within geometry Or do we not support yet curved polygon with compoundcurve and circular rings?

Mark L did you say you wanted this to be patched in 1.3 as well? I can't speak for 1.3.7, but for 1.3.6 -- it does not support creating a compound curve in a curved poly . Gives "parse error invalid geometry" and since 1.3 is in bug fix mode, I don't think we should bother patching it since doesn't look like the functionality even made it partly in there.

comment:6 Changed 8 years ago by mcayland

If it helps, I remember I found a couple of typos in the curve support a while back related to number of points. The minimum number of points in a curve should be 3, however in some places the check was incorrectly coded as 4 :(

If you add a 4th point to your example, does it then work?

ATB,

Mark.

comment:7 Changed 8 years ago by robe

On further inspection, this appears to be a bug in our parser. Observe I can make this more complicated construct without a problem.

SELECT ST_GeomFromEWKT('CURVEPOLYGON(COMPOUNDCURVE(CIRCULARSTRING(0 0,2 0, 2 1, 2 3, 4 3),(4 3, 4 5, 1 4, 0 0)), CIRCULARSTRING(1.7 1, 1.4 0.4, 1.6 0.4, 1.6 0.5, 1.7 1) )');

comment:8 Changed 8 years ago by mleslie

That's not a typo. It's wrong, but intentional. I just didn't clue to the fact that while a linestring needs four points to have area, a circular string can do it in three. I've committed to 1.4 at r4244, but haven't changed on trunk yet, as I need to run to a meeting.

comment:9 Changed 8 years ago by mleslie

As for porting to 1.3, I do consider this a bug, but as I worked on it last night I found I was making far too many non-bug changes. So I agree, I'm not going to be porting any of this back to 1.3.

I believe there's still an issue with point and ring counts when using compound curves. I'll have a look at that when I get a few minutes.

comment:10 Changed 8 years ago by mleslie

ST_NumPoints should now behave properly. In the case of both compound curves and curved polygons it will return null instead of treating them as collections. In the case of a circular string it will return the number of points.

Committed to 1.4 at r4245.

This and the fix above were ported to trunk at r4246.

comment:11 Changed 8 years ago by robe

Mark -- don't we use ST_NPoints function for checking (or whatever function is behind that)? I thought ST_NumPoints because its OGC defined (though we should change to make the same but that's another argument) doesn't work for most things anyway though I presume its supposed to work for circular strings and compound curves because they fit under the OGC Curve hierarchy (same family as linestring). Just wondering what that has to do with this ticket item.

comment:12 Changed 8 years ago by mleslie

Resolution: fixed
Status: reopenedclosed

ST_NumPoints is defined in SQL-MM as well, and similarly to it's definition in SF-SQL. Basically it only works for linestrings and circularstrings. For collections we return the number of points of the first linestring (or circstring). We were doing this as well for curvepolygons, and skipping compound curves. It was just a mess really. Should be predictable and correct now, if not actually useful.

It was another case of compound curve being flaky in curve polygon (behaving inconsistently with line and circ), at least until I sorted out the expected behaviour. It could have been it's own bug really.

comment:13 Changed 8 years ago by robe

Mark L, I'm still a bit confused by your statement. My above curved polygon works now by the way so we should keep this closed.

If the sql-mm specs say that for a mulilinestring or compound curve, it should return the count of points of the first element, then shouldn't this return 5 rather than null?

SELECT ST_NumPoints(ST_GeomFromEWKT('COMPOUNDCURVE(CIRCULARSTRING(0 0,2 0, 2 1, 2 3, 4 3),(4 3, 4 5, 1 4, 0 0))'));

I personally don't care since I just assume ST_NPoints and ST_NumPoints behave like ST_NPoints since the whole ST_NumPoints being different from ST_NPoints is just a nuisance, counterintuitive to what people expect, and doesn't even behave consistently across spatial databases to be (in practice not portable). I don't care if we are extending the spec in this case. So I apologize for arguing a point I care less about.

comment:14 Changed 8 years ago by mleslie

I agree that ST_NumPoints is a pain, but it's the 'standard' one so it would be nice to comply even if it's not used.

It's not actually defined for curve, just linestring and circular string. And compound curve isn't intended to be treated like a collection object. For example, it doesn't have an ST_NumGeometries/ST_GeometryN, but instead has an ST_NumCurves/ST_CurveN. Hmm, it looks like we haven't implemented that yet.

Hmm... ST_NumPoints isn't defined for multilinestring or multicurve either, so it should actually be returning null for those. We were traversing multilinestrings and geomcollections looking for the first linestring, so I followed suit and extended it for multicurve.

So that begs the question, do we leave this now and have consistency, removed the collection handling and have strict compliance, or remove the multicurve handling and have as much compliance as we can without breaking things. I'm not overly fussed about it.

Note: See TracTickets for help on using tickets.