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

Port ISIS to use Geos C-API #3627

Closed
Kelvinrr opened this issue Jan 3, 2020 · 10 comments
Closed

Port ISIS to use Geos C-API #3627

Kelvinrr opened this issue Jan 3, 2020 · 10 comments
Assignees

Comments

@Kelvinrr
Copy link
Collaborator

Kelvinrr commented Jan 3, 2020

ISIS version(s) affected: 3.x.x

Description

Geos C++ API is unstable. Specifically, we run into compatibility issues with co-installing other libraries with ISIS in the same environment.

The GEOs C++ interface changes between minor versions, as of ISIS v3.10, ISIS is tied to Geos 3.7.x C++ API and therefore is inflexible to other libraries requirements (e.g. conda-forge Linux build of GDAL requires GEOS 3.7.x but MacOS requires GEOS 3.6.x). To maximize compatibility, the best approach seems to be to port ISIS to use the GEOS C-API as the C-API is stable between minor versions.

How to reproduce

run conda install gdal on an environment with ISIS installed and you will get environment conflicts. Changing GEOS version will cause library resolution errors in ISIS.

Possible Solution

C-API Code: https://github.com/libgeos/geos/tree/master/capi
geos_c.h.in contains all the documentation available on the C-API.

Some details to take note of (mostly implied in "C-API" but worth explicitly pointing out):

  1. The GEOS C-API has a stand-up/tear-down process. We need to run initGEOS before calling any other C-API function as the handle it returns must be passed in as the first argument in every function. finishGEOS has to be called before the end of the program.
  2. We have access to some basic objects: GEOSGeom_t, GEOSPrepGeom_t, GEOSCoordSeq_t, GEOSSTRtree_t, GEOSBufParams_t. They are basically typedef-struct-ed geos::geom objects. No Polymorphism to differentiate between Geometry types.
  3. Seems most if not all geometry manipulation and IO operations are present including reading writing WKT/WKB
  4. No Exceptions, error codes/null pointers are returned instead.

Fact 1 requires that we need to determine the best location to initialize GEOS + run atexit(finishGeos) or similar.

Fact 2 implies potential API-breaking changes to:

  • RubberBandTool
  • Shape
  • SpatialPlotTool
  • MeasureTool
  • Image
  • MosaicSceneItem
  • HistogramTool
  • SpectralPlotTool
  • HiJitCube
  • InterestOperator
  • PolygonTools
  • GisGeometry
  • Chip
  • ImageOverlap
  • ProcessPolygons
  • StripPolygonSeeder
  • GisTopology
  • PolygonSeeder
  • GridPolygonSeeder
  • LimitPolygonSeeder
  • ProcessGroundPolygons
  • ImageOverlapSet
  • ImagePolygon
  • CsvWriterStrategy
  • CamTools

Additionally, we will need to make a decision about whether we want to encapsulate GEOS geometry objects somehow to re-enable ISIS-flavored polymorphic Geometry or if we want to use low-level Geometries directly. Additionally, we will need an answer to removing geos::geom::GeometryFactory with something similar powered by the C-API.

Fact 4 requires us to consider error-code-to-exception coversions.

Overall, transitioning to the C-API will require us to re-design ISIS's geometry backbone to a ISIS C++ API powered by the GEOS C-API in place of the GEO C++ API.

Alternatively, we can support multiple versions of the C++ API through #defines.

Additional context

@KrisBecker
Copy link
Contributor

@Kelvinrr please check out GisTopology and GisGeometry. Both make strict use of the GEOS C-API. GisTopology is a singleton that handles initialization/destruction of the GEOS system. It also handles reading, writing and conversion of GEOS geometries such as WKT and WKB types. GisGeometry is a generic geometry container that implements many of the GEOS operators.

These classes can provide a starting point for this work. See the Strategy, GisOverlapStrategy, GisUnionStrategy and GisIntersectStrategy classes for how they are used.

@ascbot
Copy link
Contributor

ascbot commented Sep 1, 2020

I am a bot that cleans up old issues that do not have activity.

This issue has not received feedback in the last six months. I am going to add the inactive label to
this issue. If this is still a pertinent issue, please add a comment or add an emoji to an existing comment.

I will post again in five months with another reminder and will close this issue on it's birthday unless it has
some activity.

@ascbot ascbot added the inactive Issue that has been inactive for at least 6 months label Sep 1, 2020
@scsides
Copy link
Contributor

scsides commented Jan 20, 2021

This is being looked at as an import piece for an FY22 task and should not be closed as inactive

@jessemapel jessemapel removed the inactive Issue that has been inactive for at least 6 months label Jan 20, 2021
@github-actions
Copy link

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.

@github-actions github-actions bot added the inactive Issue that has been inactive for at least 6 months label Jul 20, 2021
@AustinSanders AustinSanders removed the inactive Issue that has been inactive for at least 6 months label Aug 5, 2021
@github-actions
Copy link

github-actions bot commented Feb 2, 2022

Thank you for your contribution!

Unfortunately, this issue hasn't received much attention lately, so it is labeled as 'stale.'

If no additional action is taken, this issue will be automatically closed in 180 days.

@github-actions github-actions bot added the inactive Issue that has been inactive for at least 6 months label Feb 2, 2022
@oleg-alexandrov oleg-alexandrov removed the inactive Issue that has been inactive for at least 6 months label May 17, 2022
@oleg-alexandrov
Copy link
Contributor

oleg-alexandrov commented Dec 14, 2022

It is not possible for either ISIS (or ASP) to move to a new version of GDAL because one is held hostage to Geos 3.7.

Current version of Geos is 3.11. I know they are notorious for not keeping compatibility, but is there a chance things got broken in just a few places, and perhaps with a couple of days of work one could upgrade to the latest?

Any hope this will be prioritized somehow? Or at least to be added to some work plan, say, for the current fiscal year?

@AustinSanders
Copy link
Contributor

@oleg-alexandrov I'll add this to the support backlog. No guarantees that it'll be worked during the next sprint, but it will at least get prioritized among other issues during the next prioritization meeting.

@oleg-alexandrov
Copy link
Contributor

oleg-alexandrov commented Dec 29, 2022

Thank you for that.

A couple of notes when it comes to GDAL and Geos. The latest ISIS (7.1.0) depends on very modern PROJ 9.1.0. I built a conda GDAL package (very recent version, 3.5) compatible with these, which still uses Geos 3.7. These will be shipped with the upcoming ASP conda package.

GDAL has the very stable OGR interface for polygons, and it calls Geos under the hood. ASP uses OGR for polygons (also for shapefile i/o). (https://gdal.org/api/ogrgeometry_cpp.html)

I think the community will be very grateful whichever way you proceed on this. The hope is that Geos didn't break the API so bad that maybe catching up with it could be little work. Longer term, you could write your abstraction layer on top of Geos, or make use of GDAL/OGR which would take care of keeping up with Geos.

@dshean
Copy link

dshean commented Dec 29, 2022

Just offering my +1 to prioritize support for latest PROJ/GDAL (and GEOS). ISIS and ASP have fallen pretty far behind on this, and the environment conflicts are going to continue to affect users in both planetary and terrestrial geospatial communities.

@amystamile-usgs amystamile-usgs self-assigned this Feb 28, 2023
@amystamile-usgs
Copy link
Contributor

Current test failures with GEOS 3.9 update:

  1. FunctionalTestFindimageoverlapsNoOverlap
  • REASON: geos::io::WKTWriter write function segfaulting in ImagePolygon::toBlob()
  1. TempTestingFiles.UnitTestImagePolygonCross
  2. isisminer_app_test_gisoverlap
  3. isisminer_app_test_stereopair
  4. isisminer_app_test_stereopair2
  5. pixel2map_app_test_lonboundary
  6. pixel2map_app_test_pole

(will continue to update)

@amystamile-usgs amystamile-usgs moved this to In Progress in FY23 Q2 Support Mar 27, 2023
@amystamile-usgs amystamile-usgs mentioned this issue Apr 5, 2023
12 tasks
@github-project-automation github-project-automation bot moved this from In Progress to Done in FY23 Q2 Support Apr 18, 2023
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

No branches or pull requests

9 participants