Opened 21 months ago
Closed 4 weeks ago
#34406 closed New feature (fixed)
Add support for curved geometries in GeoDjango
Reported by: | Fabien Le Frapper | Owned by: | David Smith |
---|---|---|---|
Component: | GIS | Version: | dev |
Severity: | Normal | Keywords: | geodjango gdal |
Cc: | Anthony Ricaud, Claude Paroz | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
I tried ingesting curved geometries in a GeometryField
in GeoDjango.
At first I encountered some errors as these are not officially supported in GeoDjango, but I noticed that gdal
is able to go from a geometry to another using converters :
- Currently supported geometries https://github.com/django/django/blob/main/django/contrib/gis/gdal/geometries.py#L727
- Corresponding geometries in
gdal
https://gdal.org/doxygen/ogr__core_8h.html#a800236a0d460ef66e687b7b65610f12a
I successfully ingested the following geometries for a project :
- 9: "CompoundCurve"
- 10: "CurvePolygon"
- 11: "MultiCurve"
- 12: "MultiSurface"
Below is a code snippet I used, subclassing OGRGeometry
and OGRGeomType
in order to bypass existing GeoDjango validation to add support for more geometries :
- It mainly adds keys for new geometries and classes inheriting for
OGRGeometry
to handle these in GeoDjango - It shows how we could make these geometries backward compatible (from curved geometries to more standard geometries) using a simple call to existing gdal methods
Is there a reason why it is not supported at the moment in GeoDjango ?
Would you consider a pull request adding support for these geometries ?
I could suggest a patch based on the snippet above, but I am not sure how to treat the transform
part of it.
Should we keep this part ?
It seems that Postgis can handle these polygons https://postgis.net/docs/using_postgis_dbmanagement.html#CircularString
Here is the full snippet
from django.contrib.gis.gdal.geometries import GEO_CLASSES, OGRGeometry from django.contrib.gis.gdal.geomtype import OGRGeomType from django.contrib.gis.gdal.libgdal import lgdal from django.contrib.gis.gdal.prototypes import ds as capi from django.contrib.gis.gdal.prototypes import geom as geom_api class ExtendedOGRGeometry(OGRGeometry): def __init__(self, geom_input, srs=None): try: super().__init__(geom_input, srs) except KeyError: if ( not isinstance(geom_input, self.ptr_type) and self.geom_type.num not in gdal_transform.keys() ): raise self.__class__ = EXTENDED_GEO_CLASSES[self.geom_type.num] @property def geom_type(self): "Return the Type for this Geometry." return ExtendedOGRGeomType(geom_api.get_geom_type(self.ptr)) class CurvePolygon(ExtendedOGRGeometry): pass class CompoundCurve(ExtendedOGRGeometry): pass class MultiSurface(ExtendedOGRGeometry): pass class MultiCurve(ExtendedOGRGeometry): pass EXTENDED_GEO_CLASSES = { **GEO_CLASSES, 9: CompoundCurve, 10: CurvePolygon, 11: MultiCurve, 12: MultiSurface, } class ExtendedOGRGeomType(OGRGeomType): # Copy paste of original types dictionnary from GeoDjango implementation # https://github.com/django/django/blob/main/django/contrib/gis/gdal/geomtype.py#L9 _types = { 0: "Unknown", 1: "Point", 2: "LineString", 3: "Polygon", 4: "MultiPoint", 5: "MultiLineString", 6: "MultiPolygon", 7: "GeometryCollection", 100: "None", 101: "LinearRing", 102: "PointZ", 1 + OGRGeomType.wkb25bit: "Point25D", 2 + OGRGeomType.wkb25bit: "LineString25D", 3 + OGRGeomType.wkb25bit: "Polygon25D", 4 + OGRGeomType.wkb25bit: "MultiPoint25D", 5 + OGRGeomType.wkb25bit: "MultiLineString25D", 6 + OGRGeomType.wkb25bit: "MultiPolygon25D", 7 + OGRGeomType.wkb25bit: "GeometryCollection25D", # Extended geometry types 9: "CompoundCurve", 10: "CurvePolygon", 11: "MultiCurve", 12: "MultiSurface", } # New bindings to existing GDAL methods force_to_polygon = geom_output(lgdal.OGR_G_ForceToPolygon, [c_void_p]) force_to_multi_polygon = geom_output(lgdal.OGR_G_ForceToMultiPolygon, [c_void_p]) force_to_line = geom_output(lgdal.OGR_G_ForceToLineString, [c_void_p]) force_to_multi_line = geom_output(lgdal.OGR_G_ForceToMultiLineString, [c_void_p]) # The functions below need to be called on the corresponding geometry type # before saving it in the database to prevent errors in geodjango. gdal_transform = { 9: force_to_line, 10: force_to_polygon, 11: force_to_multi_line, 12: force_to_multi_polygon, } def ingest_curved_geometry(geom_ptr): transform = gdal_transform[geom.geom_type.num] geom = ExtendedOGRGeometry(transform(geom_api.clone_geom(geom_ptr))) # FIXME: for a yet unknown reason, the initial SRID is not kept when using ExtendedOGRGeometry geom.srid = 3857 geom.transform(4326)
Change History (20)
follow-up: 3 comment:1 by , 21 months ago
comment:2 by , 21 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 21 months ago
Thanks for your reply.
Do we have existing geodjango features that are gdal-only ?
Would a gdal-only contribution erroring in case of GEOS be accepted ?
Edit : sorry this is my first time in the Django bug tracker, I just noticed that you updated the ticket status. I will work on a PR then.
comment:4 by , 21 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 21 months ago
Description: | modified (diff) |
---|
comment:6 by , 21 months ago
Description: | modified (diff) |
---|
comment:7 by , 21 months ago
Cc: | added |
---|
comment:8 by , 10 months ago
Here is a link to GDAL's RFC for "Curve geometries" RFC 49
This required the following C API changes. I think we should make a decision on each of these (should Django implement them) as part of this ticket.
- OGR_GT_xxxx (for Geometry Type)
- OGRErr OGR_G_ExportToIsoWkb( OGRGeometryH, OGRwkbByteOrder, unsigned char*) -- Proposed in PR
- OGRErr OGR_G_ExportToIsoWkt( OGRGeometryH, char ) -- Proposed in PR
- OGRGeometryH OGR_G_Value( OGRGeometryH, double dfDistance )
- int OGR_G_HasCurveGeometry( OGRGeometryH, int bLookForNonLinear )
- OGRGeometryH OGR_G_GetLinearGeometry( OGRGeometryH hGeom, double dfMaxAngleStepSizeDegrees, char papszOptions)
- OGRGeometryH OGR_G_GetCurveGeometry( OGRGeometryH hGeom, char papszOptions )
- void OGRSetNonLinearGeometriesEnabledFlag(int bFlag)
- int OGRGetNonLinearGeometriesEnabledFlag()
comment:9 by , 10 months ago
Owner: | changed from | to
---|
comment:10 by , 8 months ago
Has patch: | set |
---|
comment:11 by , 7 months ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
Version: | 4.1 → dev |
Setting patch flags per latest reviews from Claude and myself.
comment:12 by , 5 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:13 by , 5 months ago
Just looking at some of the other C API changes that were made in GDAL at the time this was implemented. These two items I suggest that we don't implement.
- void OGRSetNonLinearGeometriesEnabledFlag(int bFlag)
- int OGRGetNonLinearGeometriesEnabledFlag()
The RFC says:
If they don't want to test the geometry type and explicitly calling the conversion function, they can call OGRSetNonLinearGeometriesEnabledFlag(FALSE) (the default value is TRUE, i.e. non-linear geometries can be returned). In which case, they will be transformed into their closest linear geometry, by doing linear approximation, with OGR_G_ForceTo().
Given PR#18007 proposes to implement the geometry curve checking (via has_curve
) and the linear conversion method I think that probably gives us enough coverage here.
comment:14 by , 6 weeks ago
Patch needs improvement: | set |
---|
Setting to patch needs improvement until next week where I'll review answers from David (so this is no longer listed in the patch needing review list).
comment:15 by , 4 weeks ago
Patch needs improvement: | unset |
---|
comment:16 by , 4 weeks ago
Has patch: | unset |
---|
comment:18 by , 4 weeks ago
Cc: | added |
---|
Claude, David: I have merged the latest PR from David and now I wonder what's left to do for this ticket? Could either of you, please, summarize next steps for future contributors?
comment:19 by , 4 weeks ago
Thanks for the patch and the merge!
I'm not sure we should go further with this issue. Maybe OGR_G_Value
? David?
comment:20 by , 4 weeks ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
I think we can close this issue. We could always consider OGR_G_Value
as part of #35058
At first, I'd say that we are open to extend OGR recognition of those types. However, I wonder if the missing GEOS type counterpart may prevent proper usage of those new types in GeoDjango. At the very least, we can recognize these new types and error out with a proper error message if we don't support them in the framework. Accepting on that base.