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

Use JTS geometries in vector package #2932

Merged
merged 24 commits into from Jul 11, 2019

Conversation

jpolchlo
Copy link
Contributor

@jpolchlo jpolchlo commented May 23, 2019

Overview

We are now favoring direct use of JTS geometries for improved interoperability with other projects. Scala wrapper classes for Geometry have been snuffed. Many submodules of geotrellis.vector have also been removed in favor of direct usage of the corresponding JTS functionality. Extension methods and companion objects have been added to maintain a crisp, candy shell around JTS to keep most interactions from messing up your fingers. Import geotrellis.vector._ to access these niceties; if it is required, import org.locationtech.jts.{geom => jts} to prevent namespace collisions.

Note: In the REPL, geometries will need to be constructed via the duplicate definitions in the JTS object to avoid namespace clashes that appear to be buggy behavior on the part of the REPL (that is, use JTS.Point(0,0) to construct a point at the origin in interactive sessions, but in compiled code, Point(0,0) will suffice). See core concepts documentation in the guide for more details.

Closes #2930
Closes #2931
Closes #2955

Signed-off-by: jpolchlo jpolchlopek@azavea.com

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • docs guides update, if necessary
  • New user API has useful Scaladoc strings
  • Unit tests added for bug-fix or new feature

@jpolchlo
Copy link
Contributor Author

In the interest of furthering a new RFP process for major changes or new features that we're trying out, I'd like to post my plans for this work, in case there is useful feedback. This will call out some of the salient features of the experimental code.

Proposed deletions

Here is the subset of the vector package that will be removed:

vector/src/main/scala/geotrellis/vector/
├── Dimension.scala
├── GeometryCollectionBuilder.scala
├── GeometryCollection.scala
├── affine
│   ├── AffineTransformation.scala
│   ├── GeometryCollectionTransformationMethods.scala
│   ├── LineTransformationMethods.scala
│   ├── MultiLineTransformationMethods.scala
│   ├── MultiPointTransformationMethods.scala
│   ├── MultiPolygonTransformationMethods.scala
│   ├── PointTransformationMethods.scala
│   └── PolygonTransformationMethods.scala
├── convexhull
│   └── ConvexHullMethods.scala
├── densify
│   └── DensifyMethods.scala
├── dissolve
│   └── DissolveMethods.scala
├── prepared
│   ├── PreparedGeometryMethods.scala
│   └── PreparedGeometry.scala
└── simplify
    └── SimplifyMethods.scala

The sub-packages to be removed add little if anything in the way of functionality. We will favor using JTS directly for these features. Obviously, the basic geometry types will be removed; however, I have opted to this point to keep the companion objects to aid in object creation. This does create some type weirdness, though, in some cases requiring importing JTS into a jts alias to prevent confusion.

Proposed Preservations

vector/src/main/scala/geotrellis/vector/
├── Extent.scala
├── Feature.scala
├── GeomFactory.scala
├── package.scala
├── Projected.scala
├── Results.scala
├── RobustPredicates.scala
├── SeqMethods.scala
├── SpatialIndex.scala
├── conf
│   └── JtsConfig.scala
├── interpolation
│   ├── EmpiricalVariogram.scala
│   ├── GeoKriging.scala
│   ├── InterpolationVector.scala
│   ├── Kriging.scala
│   ├── LeastSquaresFittingNuggetProblem.scala
│   ├── LeastSquaresFittingProblem.scala
│   ├── LinearSemivariogram.scala
│   ├── NonLinearSemivariogram.scala
│   ├── OrdinaryKriging.scala
│   ├── Semivariogram.scala
│   ├── SimpleKriging.scala
│   └── UniversalKriging.scala
├── io
│   ├── json
│   │   ├── CrsFormats.scala
│   │   ├── FeatureFormats.scala
│   │   ├── GeoJson.scala
│   │   ├── GeoJsonSupport.scala
│   │   ├── GeometryFormats.scala
│   │   ├── Implicits.scala
│   │   ├── JsonCRS.scala
│   │   ├── JsonFeatureCollectionMap.scala
│   │   ├── JsonFeatureCollection.scala
│   │   └── Style.scala
│   ├── package.scala
│   ├── WKB
│   │   ├── Implicits.scala
│   │   ├── WKB.scala
│   │   └── WKBWriter.scala
│   └── WKT
│       ├── Implicits.scala
│       └── WKT.scala
├── mesh
│   ├── HalfEdge.scala
│   ├── HalfEdgeTable.scala
│   └── IndexedPointSet.scala
├── reproject
│   ├── Implicits.scala
│   └── Reproject.scala
├── summary
│   └── polygonal
│       └── PolygonalSummaryHandler.scala
├── triangulation
│   ├── BoundaryDelaunay.scala
│   ├── DelaunayStitcher.scala
│   ├── DelaunayTriangulationMethods.scala
│   ├── DelaunayTriangulation.scala
│   ├── Implicits.scala
│   ├── QuadricError.scala
│   ├── StitchedDelaunay.scala
│   ├── TriangleMap.scala
│   └── TriangulationPredicates.scala
├── util
│   └── Intersection.scala
└── voronoi
    ├── DelaunayMethods.scala
    ├── Implicits.scala
    ├── package.scala
    ├── VoronoiDiagram.scala
    └── VoronoiMethods.scala

Notable here is the maintenance of the Extent type, which is easier to work with in a scala context (e.g., pattern matching). We will provide implicit conversion from JTS's Envelope type to facilitate usage. We're also keeping a few classes that provide enriched functionality, like SpatialIndex. Additionally, we provide a pureconfig-configured GeometryFactory object. This will be used throughout the system to build compatible geometries. Also note that the WKB and WKT objects in io provide thread-safe operations, and so should be kept. Finally, the Intersection class in the util subpackage will be moved up to the root package, and will be heavily modified to provide a type-enriched intersection facility that will be more useful for pattern matching.

@echeipesh echeipesh changed the title [EXPERIMENTAL][WIP] Use JTS geometries in vector package Use JTS geometries in vector package May 29, 2019
@jpolchlo jpolchlo force-pushed the experiment/jts-vectors branch 3 times, most recently from 976b7d2 to 2577afb Compare June 27, 2019 13:36
implicit class ProjectGeometry[G <: Geometry](g: G) {
/** Upgrade Geometry to Projected[Geometry] */
def withSRID(srid: Int) = Projected(g, srid)
}

@typeclass trait MultiGeometry[G]
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't used anywhere but for an import in ProtobufGeom. Do you think this is something important or just left-over at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'd agree that it's mostly vestigial. Not entirely clear what its value was in the first place.

Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
@echeipesh
Copy link
Contributor

@jpolchlo CHANGELOG is the only outstanding thing, it can be [skip ci] tagged

def distanceToInfiniteLine(a: Point, b: Point): Double =
CGAlgorithms.distancePointLinePerpendicular(jtsGeom.getCoordinate, a.toCoordinate, b.toCoordinate)
}
object Point extends PointConstructors
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is actually the last thing we talked about, keeping the geotrellis.vector.jts.Point is the thing that would provide duplicate imports if org.locationtech.jts.geom._ was imported. IIRC our discussion landed on removing these constructors and using only the ones in JTS object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this, and I remember settling on "eh, no, maybe not". The footprint of this "simple" change is wide, necessitating either another import (after we went through the trouble to minimize them), or requiring a clumsy prefix for every geometry creation. It's going to be less common for a user to import org.locationtech.jts.geom._, so I made a note in the docs (https://github.com/locationtech/geotrellis/pull/2932/files#diff-38390c0856b2dfe9eff77f20e8a0d9d1R249) to avoid doing so in a careless fashion. The JTS object feels like a kludge to get around a broken thing, not a thing that I would want to use all the time. I would hope that one day the scala REPL would fix its problems and we can just get rid of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thats not what I recall ending on, but I accept this argument now. The docs certainly help. 👍

Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Copy link
Contributor

@echeipesh echeipesh left a comment

Choose a reason for hiding this comment

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

LGTM, will squash-merge on green CI.

Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
Signed-off-by: jpolchlo <jpolchlopek@azavea.com>
@pomadchin
Copy link
Member

@jpolchlo sweet PR! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants