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

Support GeometryCollection containing TIN or Triangle geometries #1163

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

rbuffat
Copy link
Contributor

@rbuffat rbuffat commented Nov 18, 2022

This PR adds support for GeometryCollections containing RFC64 geometries.

Should fix #1155 and #1130

fiona/_geometry.pyx Outdated Show resolved Hide resolved
fiona/_geometry.pyx Outdated Show resolved Hide resolved
fiona/_geometry.pyx Outdated Show resolved Hide resolved
fiona/_geometry.pyx Outdated Show resolved Hide resolved
fiona/_geometry.pyx Outdated Show resolved Hide resolved
fiona/_geometry.pyx Outdated Show resolved Hide resolved
@rbuffat
Copy link
Contributor Author

rbuffat commented Nov 20, 2022

@rouault On a feature level there is the possibility of using OGR_F_StealGeometry to take ownership of a geometry. I assume performance-wise OGR_G_Clone will be slower compared to OGR_F_StealGeometry. Is there an equivalent function on a Geometry level?

For a PolyhedralSurface, TIN or Triangle that are not part of a GeoemtryCollection we could just steal the geometry from the feature, but this would need some more refactoring.

Thanks for your review, it is much appreciated!

@rouault
Copy link
Contributor

rouault commented Nov 20, 2022

there an equivalent function on a Geometry level?

you can use:

  • OGRGeometryH OGR_G_GetGeometryRef( OGRGeometryH hGeom, int iSubGeom ) where hGeom is a container geometry, and iSubGeom the index of the sub geometry inside it, to get a reference to the geometry
  • and OGR_G_RemoveGeometry( OGRGeometryH hGeom, int iGeom, int bDelete ) where hGeom is a container geometry, iGeom the index of the sub geometry inside it and bDelete to be set to FALSE, so that the previously get reference isn't destroyed
    The combination of the 2 is the equivalent of stealing a sub-geometry to a container geometry

fiona/_geometry.pyx Outdated Show resolved Hide resolved
@rbuffat
Copy link
Contributor Author

rbuffat commented Dec 4, 2022

I found some time to work on it. But I will need to go over it again to tidy up everything.

cdef int code

cogr_geometry = OGR_F_GetGeometryRef(feature)
code = base_geometry_type_code(OGR_G_GetGeometryType(cogr_geometry))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the code of a geometry is determined two times: here and in build(). This could be optimized by passing the code to build. What is the equivalent in Cython of build(..., code: Optional[int] = None)? Passing an int pointer that is NULL when no code was previously computed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rbuffat don't worry about optimizing that. Getting the code is practically free compared to other stuff we're doing.

@rbuffat
Copy link
Contributor Author

rbuffat commented Dec 8, 2022

There are currently a bunch of redeclared warnings that are suppressed when the compilation is successful. I will need to have a look at them.

      warning: fiona/gdal.pxi:138:4: 'OGRErr' redeclared
      warning: fiona/gdal.pxi:138:4: 'OGRErr' redeclared
      warning: fiona/_env.pxd:5:30: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:138:4: 'OGRErr' redeclared
      warning: fiona/gdal.pxi:141:4: 'OGRwkbGeometryType' redeclared
      warning: fiona/gdal.pxi:89:4: 'OGRErr' redeclared
      warning: fiona/gdal.pxi:138:4: 'OGRErr' redeclared
      warning: fiona/gdal.pxi:142:8: 'wkbUnknown' redeclared
      warning: fiona/gdal.pxi:143:8: 'wkbPoint' redeclared
      warning: fiona/gdal.pxi:144:8: 'wkbLineString' redeclared
      warning: fiona/gdal.pxi:145:8: 'wkbPolygon' redeclared
      warning: fiona/gdal.pxi:146:8: 'wkbMultiPoint' redeclared
      warning: fiona/gdal.pxi:147:8: 'wkbMultiLineString' redeclared
      warning: fiona/gdal.pxi:148:8: 'wkbMultiPolygon' redeclared
      warning: fiona/gdal.pxi:149:8: 'wkbGeometryCollection' redeclared
      warning: fiona/gdal.pxi:150:8: 'wkbCircularString' redeclared
      warning: fiona/gdal.pxi:151:8: 'wkbCompoundCurve' redeclared
      warning: fiona/gdal.pxi:152:8: 'wkbCurvePolygon' redeclared
      warning: fiona/gdal.pxi:153:8: 'wkbMultiCurve' redeclared
      warning: fiona/gdal.pxi:154:8: 'wkbMultiSurface' redeclared
      warning: fiona/gdal.pxi:155:8: 'wkbCurve' redeclared
      warning: fiona/gdal.pxi:156:8: 'wkbSurface' redeclared
      warning: fiona/gdal.pxi:157:8: 'wkbPolyhedralSurface' redeclared
      warning: fiona/gdal.pxi:158:8: 'wkbTIN' redeclared
      warning: fiona/gdal.pxi:159:8: 'wkbTriangle' redeclared
      warning: fiona/gdal.pxi:160:8: 'wkbNone' redeclared
      warning: fiona/gdal.pxi:161:8: 'wkbLinearRing' redeclared
      warning: fiona/gdal.pxi:162:8: 'wkbCircularStringZ' redeclared
      warning: fiona/gdal.pxi:163:8: 'wkbCompoundCurveZ' redeclared
      warning: fiona/gdal.pxi:164:8: 'wkbCurvePolygonZ' redeclared
      warning: fiona/gdal.pxi:165:8: 'wkbMultiCurveZ' redeclared
      warning: fiona/gdal.pxi:166:8: 'wkbMultiSurfaceZ' redeclared
      warning: fiona/gdal.pxi:167:8: 'wkbCurveZ' redeclared
      warning: fiona/gdal.pxi:168:8: 'wkbSurfaceZ' redeclared
      warning: fiona/gdal.pxi:169:8: 'wkbPolyhedralSurfaceZ' redeclared
      warning: fiona/gdal.pxi:170:8: 'wkbTINZ' redeclared
      warning: fiona/gdal.pxi:171:8: 'wkbTriangleZ' redeclared
      warning: fiona/gdal.pxi:172:8: 'wkbPointM' redeclared
      warning: fiona/gdal.pxi:173:8: 'wkbLineStringM' redeclared
      warning: fiona/gdal.pxi:174:8: 'wkbPolygonM' redeclared
      warning: fiona/gdal.pxi:175:8: 'wkbMultiPointM' redeclared
      warning: fiona/gdal.pxi:176:8: 'wkbMultiLineStringM' redeclared
      warning: fiona/gdal.pxi:177:8: 'wkbMultiPolygonM' redeclared
      warning: fiona/gdal.pxi:178:8: 'wkbGeometryCollectionM' redeclared
      warning: fiona/gdal.pxi:179:8: 'wkbCircularStringM' redeclared
      warning: fiona/gdal.pxi:180:8: 'wkbCompoundCurveM' redeclared
      warning: fiona/gdal.pxi:181:8: 'wkbCurvePolygonM' redeclared
      warning: fiona/gdal.pxi:182:8: 'wkbMultiCurveM' redeclared
      warning: fiona/gdal.pxi:183:8: 'wkbMultiSurfaceM' redeclared
      warning: fiona/gdal.pxi:184:8: 'wkbCurveM' redeclared
      warning: fiona/gdal.pxi:185:8: 'wkbSurfaceM' redeclared
      warning: fiona/gdal.pxi:186:8: 'wkbPolyhedralSurfaceM' redeclared
      warning: fiona/gdal.pxi:187:8: 'wkbTINM' redeclared
      warning: fiona/gdal.pxi:188:8: 'wkbTriangleM' redeclared
      warning: fiona/gdal.pxi:189:8: 'wkbPointZM' redeclared
      warning: fiona/gdal.pxi:190:8: 'wkbLineStringZM' redeclared
      warning: fiona/gdal.pxi:191:8: 'wkbPolygonZM' redeclared
      warning: fiona/gdal.pxi:192:8: 'wkbMultiPointZM' redeclared
      warning: fiona/gdal.pxi:193:8: 'wkbMultiLineStringZM' redeclared
      warning: fiona/gdal.pxi:194:8: 'wkbMultiPolygonZM' redeclared
      warning: fiona/gdal.pxi:195:8: 'wkbGeometryCollectionZM' redeclared
      warning: fiona/gdal.pxi:196:8: 'wkbCircularStringZM' redeclared
      warning: fiona/gdal.pxi:197:8: 'wkbCompoundCurveZM' redeclared
      warning: fiona/gdal.pxi:198:8: 'wkbCurvePolygonZM' redeclared
      warning: fiona/gdal.pxi:199:8: 'wkbMultiCurveZM' redeclared
      warning: fiona/gdal.pxi:200:8: 'wkbMultiSurfaceZM' redeclared
      warning: fiona/gdal.pxi:201:8: 'wkbCurveZM' redeclared
      warning: fiona/gdal.pxi:202:8: 'wkbSurfaceZM' redeclared
      warning: fiona/gdal.pxi:203:8: 'wkbPolyhedralSurfaceZM' redeclared
      warning: fiona/gdal.pxi:204:8: 'wkbTINZM' redeclared
      warning: fiona/gdal.pxi:205:8: 'wkbTriangleZM' redeclared
      warning: fiona/gdal.pxi:206:8: 'wkbPoint25D' redeclared
      warning: fiona/gdal.pxi:207:8: 'wkbLineString25D' redeclared
      warning: fiona/gdal.pxi:208:8: 'wkbPolygon25D' redeclared
      warning: fiona/gdal.pxi:209:8: 'wkbMultiPoint25D' redeclared
      warning: fiona/gdal.pxi:210:8: 'wkbMultiLineString25D' redeclared
      warning: fiona/gdal.pxi:211:8: 'wkbMultiPolygon25D' redeclared
      warning: fiona/gdal.pxi:212:8: 'wkbGeometryCollection25D' redeclared
      warning: fiona/gdal.pxi:523:36: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:525:23: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:526:26: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:527:25: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:528:37: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:530:30: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:531:9: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:532:28: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:533:36: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:534:30: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:535:15: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:536:29: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:537:37: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:538:27: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:539:21: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:540:21: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:541:21: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:542:30: Function signature does not match previous declaration
      warning: fiona/gdal.pxi:544:21: Function signature does not match previous declaration

@sgillies
Copy link
Member

sgillies commented Dec 9, 2022

@rbuffat the immediate cause of those warnings is that _geometry.pyx imports gdal.pxi and at https://github.com/Toblerity/Fiona/pull/1163/files#diff-71c8a7a6ae4cca6dd2e3e2e0e98232b4c2decc714d27ed6bc972c0ff5cc66608R11 _err.pyx is imported and gdal.pxi is re-imported. We're redeclaring, but since we're not redefining we're okay. No bugs have arisen because of this. We can try to restructure in the future, but it's fine to ignore it for now. I'm going to merge this.

(By restructuring, it looks like we need to deprecate including gdal.pxi and start cimport'ing GDAL types and functions, but that can wait until after 1.9b1).

@sgillies sgillies merged commit 1e0065d into Toblerity:maint-1.9 Dec 9, 2022
sgillies added a commit that referenced this pull request Dec 9, 2022
@sgillies sgillies added this to the 1.9 milestone Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants