ShapeFactory and some builders

[dsmiley]

This moves shape construction from SpatialContext to a new ShapeFactory interface.
And it formalizes polygon construction and other shapes without requiring JTS.

[dsmiley]

This is a work-in progress but substantial progress. Tests don't pass. Please review; push back on what you don't like.

• New ShapeFactory interface, with sub-interfaces for builders for everything except point, rect, and circle. ShapeFactory is itself a factory and not a builder because I wanted consumers to create some basic shapes without having to create something stateful (e.g. a point, for example). Granted that concern I have is mostly about a point; I wouldn't mind so much a transient builder object for anything else. Perhaps then Points could be created directly off of SpatialContext? Or is what I have fine? The other rationale for the current approach is that it makes the usage simple IMO.
• I converted GeoJSONReader but nothing else. It's great to see we can have one reader and not a JTS specific subclass :-) This should follow suit with the other formats eventually.

My thoughts for the road ahead:

• Interim goal is to be back-wards compatible for a very short while, and hence keep methods on SpatialContext to delegate out, and to mark deprecated. The LocationTech transition can be the big 1.0 and remove deprecated stuff, and to move things and rename things.
• I think it's clear we need a Spatial4j LineString and Polygon interface, even if initially it's just extends Shape without adding anything. This means JtsGeometry would get some new subclasses for each of these impls that if nothing else provide a marker interface and allows typed API usage.
• ShapeCollection should become an interface and be typed. Likewise, a JtsGeometry subclass implementing it for type safety. Perhaps name this MultiShape to more clearly align with OGC names; the difference being in Spatial4j, "Geometry" is "Shape".
• The JtsSpatialContextFactory should have a flag to choose wether to use a Geometry based Multi shape or wether to use Spatial4j's current ShapeCollection implementation.
• Convert WKT & Polyshape IO formats.
• note there's a TestShapes2D.testBufferedLineString test that fails in this branch for nothing to do with these changes -- basically JtsGeometry needs to handle the "empty" case in it's constructor better.

[jdeolive]

Not much to add here except I really like the general direction and I think the api looks good.. Likely won't have much actual useful feedback (which I am sure would all be minor) until I actually start to use the new apis, but from what I can gleen from the diff this is looking great.

[codecov-io]

Current coverage is 75.40%

Merging #130 into master will increase coverage by +0.96% as of f907f63

@@            master   #130   diff @@
=====================================
  Files           48     48       
  Stmts         3393   3420    +27
  Branches       884    867    -17
  Methods          0      0       
=====================================
+ Hit           2526   2579    +53
- Partial        252    256     +4
+ Missed         615    585    -30

Review entire Coverage Diff as of f907f63

---

Uncovered Suggestions

1. +0.38% via ...o/GeoJSONWriter.java#79...91
2. +0.30% via ...ShapeCollection.java#192...201
3. +0.27% via ...e/io/ParseUtils.java#51...59
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

[dsmiley]

Ok; so far GeoJSONReader & reading WKTReader have moved over. Reading PolyshapeReader has not yet. @jdeolive may I suggest you give that a shot? Ok I just did; I'm hopeful to get some help here, and in the process getting your input.

I admit that the builder for the Multi* shapes is maybe a little weird in that, say for a MultiPolygonBuilder, you call add(PolygonBuilder) instead of an add(Polygon) both because there is no Polygon Spatial4j shape (yet) and more importantly because even if there was one, we don't want the overhead of a JtsGeometry wrapper that would only be removed before adding to a MultiPolygon. There may be other ways I could have done this (much more builder like, like how creating a Polygon w/ holes is) but I fear it would complicate the Java generics quite a bit. Feel free to take a stab at it if you so choose; I fooled with it for a bit before giving up.

The only nocommit is related to the "norm" methods (moved to ShapeFactory) that were documented as only applying to WKTReader. Indeed only WKTReader calls them but that's only because it used to be the only reader; the intention is that all readers (other than a Binary one) should call those methods. The idea being that there may be some normalization (e.g. rounding or correcting) to be done before verifyX is called, which can throw. The simplest thing is just to keep them, but also ensure we add them to GeoJSONReader & PolyshapeReader -- it's simple enough. Figuring out when Spatial4j should normalize or verify stuff, and when it shouldn't is debatable.

[jdeolive]

Hey @dsmiley, sorry for the late response, been on the road the last few days. I'll pull this down later today and take a shot at PolyshapeReader.

[jdeolive]

Hey @dsmiley. Took a crack at porting PolyshapeReader. All went smooth for the most part. The only hitch is that the PolyshapeReader has an override for the JTS version that tries to build the specific types of homogeneous geometry collection if it can. You mention in your original comments perhaps adding a flag to Jts context factory to control this behaviour. +1 on that idea. I can take a crack at adding that logic, although I wouldn't be able to get to it until next week.

[dsmiley]

Sounds good to me Justin. And I think you for forgot to add calls to the norm() methods.

[jdeolive]

Ahh, right. Missed that. Norm() method calls added. As I went through I was wondering if that is something that could potentially be integrated into the builder classes and happen automatically?

[dsmiley]

I know what you mean; there seems to be room for improvement. Certainly when reading from "external" data (e.g. WKT, GeoJSON, or whatever) you'd want -- it's what it was intended for... but if you're reading data that has already been normalized (some binary serialized format) or on the fly generation of rectangles (e.g. that Lucene spatial does when traversing a grid) it's unnecessary overhead.

[jdeolive]

Hey @dsmiley , I spec'd out an initial approach for adding the logic to use JtsGeometry vs ShapeCollection to JtsShapeFactory. Here are my thoughts/findings.

First was I notice that there is already a useJtsMulti flag available. I was thinking it made sense to use that. Or do you think this deserves a different flag?

Second, is that ShapeFactory.multiShape(List) returns a ShapeCollection, meaning we can't return a JtsGeometry from it. From your previous comments it looks like your thinking is to make ShapeCollection an interface and adding a implementation for jts. I think that makes sense so unless you object I'll go that route.

[dsmiley]

RE useJtsMulti: sure, we can always do this when useJtsMulti. If one day we want to sometimes not do it despite useJtsMulti being true, we can then add a separate option.

RE ShapeFactory.multiShape(List): Hmmm. I'd rather try and get this feature branch done without messing with the types, and leave that for a separate issue (following the org.location.spatial4j / LocationTech official release). Perhaps we could either punt on this feature or only do it when we use the MultiShapeBuilder since it returns a Shape?

[jdeolive]

Fair enough, happy to punt on that approach for now and go with your suggestion of just making the change in the builder only for now. Should have a patch for you to review tomorrow.

[jdeolive]

Ok, just submitted #131 which will merge the changes we discussed into this branch. Let me know what you think.

[dsmiley]

I enhanced the javadocs further. If you think it's mergeable I'll do that and get on the org.locationtech package rename.

[jdeolive]

+1 on merge.