Replace our polygon code with the JTS implementation #1853
Conversation
…ap) and add to LM (block_area for big map)
| @@ -399,7 +404,10 @@ public static PointList fromLineString(LineString lineString) { | |||
| } | |||
|
|
|||
| public LineString toLineString(boolean includeElevation) { | |||
| GeometryFactory gf = new GeometryFactory(); | |||
| return toLineString(new GeometryFactory(), includeElevation); | |||
There was a problem hiding this comment.
Wouldn't it be better to create a factory for 2D and 3D each and keep them?
There was a problem hiding this comment.
Have done this (one static factory). Or why would we want 2?
There was a problem hiding this comment.
GeometryFactory FAC_2D = new GeometryFactory(new PackedCoordinateSequenceFactory(DOUBLE, 2));
GeometryFactory FAC_3D = new GeometryFactory(new PackedCoordinateSequenceFactory(DOUBLE, 3));includeElevation would just toggle which one of the two is being passed to the function. No need to create new objects here.
There was a problem hiding this comment.
Have adapted the current code (via a single factory). Or will there be something different with your proposed change?
There was a problem hiding this comment.
You can drop all new PackedCoordinateSequence(...) calls:
2D_FAC.createPolygon(coordinates);There was a problem hiding this comment.
Hmmh, maybe you mean something else? There is just one call and it is:
return factory.createLineString(new PackedCoordinateSequence.Double(coordinates, includeElevation ? 3 : 2));
There was a problem hiding this comment.
Ah, nice that we're taking this on.
Ultimately (long term, new code), I would recommend we stop wrapping the JTS classes, and migrate to them. The appeal is that it's not only a library, but a language -- intersects, contains etc. have precise definitions that can be googled. If it's possible to not hide them away behind wrappers, that clarity would propagate to the code where these data types are used. (One wouldn't have to ask oneself if our wrapper uses the word slightly differently.)
| // If they are not collinear, they must intersect in exactly one point. | ||
| return true; | ||
| // for estimation use bounding box as reference: | ||
| return getBounds().calculateArea() * envelope.getArea() / polygon.getGeometry().getArea(); |
There was a problem hiding this comment.
Do you think we could just remove this method, and its definition from Shape? It isn't used generically anywhere in the code, so we could easily avoid giving an "approximate" or fudged implementation here.
There was a problem hiding this comment.
Unfortunately it is used in GraphEdgeIdFinder.parseBlockArea
There was a problem hiding this comment.
Ok, wasn't used before (seems like a bug), but is now :)
I.e. for smaller polygons edgeIds should be used and for bigger polygons we expect that the edgeId lookup is slower than calling the polygon.intersect for every edge of the traversed graph.
👍 |
|
@easbar can you have a short look into my change in Measurement (regarding QuerySetting). Do you agree roughly :) ? Should I do this in a separate PR? |
Yes putting all these booleans in a config object is a good idea imo. Right now the biggest advantage is that we only have to specify the non-default values (all the booleans that should be true instead of false)? |
Yes (and you can better see what changes to the "default") |
I would like to have this before #1838.
This PR does the following:
Shape.intersects(PointList)method. Now not just a single point of an edge is checked for intersection but the whole point list. Still there is more work (later) Reimplement block_area with LocationIndexTree instead of graph traversal #1324.