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

Add classes for curved geometry types #1046

Merged
merged 10 commits into from
Apr 30, 2024
Merged

Add classes for curved geometry types #1046

merged 10 commits into from
Apr 30, 2024

Conversation

dbaston
Copy link
Member

@dbaston dbaston commented Feb 14, 2024

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.

@pramsey
Copy link
Member

pramsey commented Feb 20, 2024

Looks like you have a winner, @dbaston

@m-kuhn
Copy link

m-kuhn commented Feb 28, 2024

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
Copy link

m-kuhn commented Feb 28, 2024

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?

@lbartoletti
Copy link
Contributor

do you see any potential problem?

No, looks ok to me.

Glad to have curve support in GEOS! kudos @dbaston

@dbaston dbaston force-pushed the curves branch 6 times, most recently from c85565c to 9d77ade Compare March 3, 2024 19:02
@dr-jts
Copy link
Contributor

dr-jts commented Mar 3, 2024

Perhaps mark this as Draft?

@dbaston dbaston force-pushed the curves branch 2 times, most recently from f30a751 to e7eb5a2 Compare April 15, 2024 19:22
@dbaston dbaston changed the title Add classes for curved geometry types [WIP] Add classes for curved geometry types Apr 16, 2024
@dbaston
Copy link
Member Author

dbaston commented Apr 16, 2024

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 dbaston merged commit fcaebfc into libgeos:main Apr 30, 2024
29 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants