geos icon indicating copy to clipboard operation
geos copied to clipboard

Add classes for curved geometry types [WIP]

Open dbaston opened this issue 1 year ago • 5 comments

This PR adds curved geometry types to the GEOS Geometry hierarchy and adds support to the WKTReader for reading them. It is not ready to be merged; I'm just posting it at this stage in case anyone has feedback on the class hierarchy before I continue with implementation.

It makes the following additions to the class hierarchy:

  1. Adds a parent class SimpleCurve to LineString.
  2. Adds a parent class Curve to SimpleCurve.
  3. Adds a parent class Surface to Polygon.
  4. Adds class CircularString with SimpleCurve as a parent
  5. Adds class CompoundCurve with Curve as a parent
  6. Adds class CurvePolygon with Surface as a parent
  7. Adds class MultiCurve with GeometryCollection as a parent
  8. Adds class MultiSurface with GeometryCollection as a parent

It turns out this is pretty close to what OGR does, except that OGR does Surface--> CurvePolygon --> Polygon instead of having both CurvePolygon and Polygon as children of Surface. While it is true that a Polygon is a specialization of a CurvePolygon, setting CurvePolygon as the parent has a downside of making the result of dynamic_cast<CurvePolygon*> less informative, and I didn't see an upside that would outweigh this annoyance. Similarly, I did not make MultiPolygon a subclass of MultiSurface, or MultiLineString a subclass of MultiCurve. But I'm receptive to the argument that it's simpler to just do things the same as OGR.

The QGIS class hierarchy looks to be the same as OGR's, except for the omission of SimpleCurve.

dbaston avatar Feb 14 '24 00:02 dbaston

Looks like you have a winner, @dbaston

pramsey avatar Feb 20 '24 21:02 pramsey

a little helper for myself:

classDiagram
    class GeometryCollection
    class LineString
    class SimpleCurve
    class Curve
    class Polygon
    class Surface
    class CircularString
    class CompoundCurve
    class CurvePolygon
    class MultiCurve
    class MultiSurface

    SimpleCurve <|-- LineString
    Curve <|-- SimpleCurve
    Surface <|-- Polygon
    SimpleCurve <|-- CircularString
    Curve <|-- CompoundCurve 
    Surface  <|-- CurvePolygon
    GeometryCollection  <|-- MultiCurve
    GeometryCollection   <|-- MultiSurface

m-kuhn avatar Feb 28 '24 08:02 m-kuhn

From QGIS side, I don't expect any troubles from the difference, most code works on geometry level (and not individual geos class level), we this is pretty transparent.

@nyalldawson @lbartoletti @elpaso (as last contributors there) do you see any potential problem?

m-kuhn avatar Feb 28 '24 16:02 m-kuhn

do you see any potential problem?

No, looks ok to me.

Glad to have curve support in GEOS! kudos @dbaston

lbartoletti avatar Feb 28 '24 20:02 lbartoletti

Perhaps mark this as Draft?

dr-jts avatar Mar 03 '24 20:03 dr-jts

I think this is ready for review. While large, I think it represents the minimum set of changes that can safely go into master:

  • add curved types
  • add WKT read/write support for use in testing
  • add an implementation of GetEnvelope (since this is eagerly calculated, we can't have it just error out)
  • error out on curved types for all other operations accessible from the C API

dbaston avatar Apr 16 '24 18:04 dbaston