Introduces GeometryUtil #463

KaiVolland · 2018-03-08T14:11:44Z

FEATURE

Description:

This introduces the GeometryUtil including these methods:

splitByLine: Splits an Polygon-Feature or -Geometry into multiple Features or Geometries
💡 great job @dnlkoch
addBuffer: Adds a Buffer to an Feature or Geometry
mergeGeometries: Merges mutliple Geometries into on MutliGeometry
union: Unions multiple Features or Geometries into one Feature or Geometry
difference: Finds the difference between two polygons by clipping the second polygon from the first.
intersection: Takes two polygons and finds their intersection.

This introduces Turf as a dependency

💡 Splitting a geometry with a hole was not yet tested and might be failing.

This introduces the GeometryUtil including these methods: - splitByLine - addBuffer - mergeGeometries - union Co-authored-by: dnlkoch <koch@terrestris.de>

marcjansen

Very nice work. I was quite chatty in my review but really like this. Take from my comments what you agree with and then please merge. Thanks guys!

marcjansen · 2018-03-08T17:04:34Z

src/Util/GeometryUtil/GeometryUtil.js

+   *
+   * @param {ol.feature | ol.geom.Polygon} polygon The polygon geometry to split.
+   * @param {ol.feature | ol.geom.LineString} lineFeat The line geometry to split the polygon
+   *  geometry with.


Will this also work for multi-geometries?

Does this work with (Multi-)polygons with holes?

Yes it does. I'll add a test.

marcjansen · 2018-03-08T17:10:47Z

src/Util/GeometryUtil/GeometryUtil.js

+      return features;
+    } else {
+      return features.map(f => f.getGeometry());
+    }


We should more clearly document that the return value is dependent on the input values. Thinking again I would rather always return one thing, possibly the features. I think this is a better API. Non-blocking.

marcjansen · 2018-03-08T17:11:51Z

src/Util/GeometryUtil/GeometryUtil.js

+   *
+   * @param {ol.geom.Geometry | ol.Feature} geometry The geometry.
+   * @param {Integer} buffer The buffer to add in meters.
+   * @param {String} projection A projection as EPSG code.


Why would I need that? Documentation enhancement welcome.

marcjansen · 2018-03-08T17:14:08Z

src/Util/GeometryUtil/GeometryUtil.js

+   * Adds a buffer to a given geometry.
+   *
+   * @param {ol.geom.Geometry | ol.Feature} geometry The geometry.
+   * @param {Integer} buffer The buffer to add in meters.


Integer is not a valid type I guess. What happens if a float like number is passed? Can we use this to dissolve a feature with a negative value? Will buffering consider holes in polygons, and how?

marcjansen · 2018-03-08T17:15:09Z

src/Util/GeometryUtil/GeometryUtil.js

+    if (geometry instanceof OlFeature) {
+      return geoJsonFormat.readFeature(buffered);
+    } else {
+      return geoJsonFormat.readGeometry(buffered.geometry);


I think I like one return type better. but this is not blocking this.

Docs are enhanced to clearify why we two different return types.

marcjansen · 2018-03-08T17:16:58Z

src/Util/GeometryUtil/GeometryUtil.js

+      return geoJsonFormat.writeFeatureObject(feature);
+    });
+    if (invalid) {
+      // Logger.warn('Can only create union of polygons.');


Hmm. why not warn? And why don't we iterate over the single parts of the multipolygon in that case?

Are holes in (Multi)polygons also covered here?

@marcjansen I don't think we do log anywhere in react-geo... We just have loglevel as dependency. 🙈

@ahennr Yes. I won't add anymore test as these would be more or less just tests agains turf.js. But it tested it once in a sandbox.

marcjansen · 2018-03-08T17:17:45Z

src/Util/GeometryUtil/GeometryUtil.js

+    if (polygons[0] instanceof OlFeature) {
+      return feature;
+    } else {
+      return feature.getGeometry();


I get it now. the different return types seem to be everywhere.

marcjansen · 2018-03-08T17:20:08Z

src/Util/GeometryUtil/GeometryUtil.spec.js

+       *          |
+       *          +
+       */
+        it('splits the given convex polygon geometry with a straight line', () => {


Indentation is of one space I think. Otherwise impressive ascii art 😎

marcjansen · 2018-03-08T17:20:34Z

src/Util/GeometryUtil/GeometryUtil.spec.js

+       *   |             |
+       *   |             |
+       *   +-------------+
+       */


indentation

marcjansen · 2018-03-08T17:21:33Z

src/Util/GeometryUtil/GeometryUtil.spec.js

+       *       |                   |
+       *       +-------------------+
+       *
+       */


indentation off. probably some more occurences.

ahennr

Nice enhancement @KaiVolland and @dnlkoch 👏

ahennr · 2018-03-09T08:16:26Z

package.json

@@ -69,6 +69,7 @@
    "react": "~16.0"
  },
  "dependencies": {
+    "@turf/turf": "5.1.6",


Does turf have an integrated validation method, or is it possible to use the one of jsts (which is a dependency of turf AFAIK)

I don't think turf.js has a validation method. We could a jsts as an extra dependency if we want to have validation in react-geo. We can't rely on the existence of jsts by using turf.js as the dependency might be remove.

ahennr · 2018-03-09T08:17:20Z

src/Util/GeometryUtil/GeometryUtil.js

+   *
+   * @param {ol.feature | ol.geom.Polygon} polygon The polygon geometry to split.
+   * @param {ol.feature | ol.geom.LineString} lineFeat The line geometry to split the polygon
+   *  geometry with.


Does this work with (Multi-)polygons with holes?

ahennr · 2018-03-09T08:26:24Z

src/Util/GeometryUtil/GeometryUtil.js

+      }
+    });
+    if (mixedGeometryTypes) {
+      // Logger.warn('Can not merge mixed geometries into one multigeometry.');


Can this logger be removed / activated?

I'd like to leave it as is. We may wan't make use of a logger in the near future and this way we don't forget the loggers here.

ahennr · 2018-03-09T08:28:28Z

src/Util/GeometryUtil/GeometryUtil.js

+      return geoJsonFormat.writeFeatureObject(feature);
+    });
+    if (invalid) {
+      // Logger.warn('Can only create union of polygons.');


Are holes in (Multi)polygons also covered here?

ahennr · 2018-03-09T08:33:09Z

src/Util/GeometryUtil/GeometryUtil.js

+   * @returns {ol.geom.Geometry | ol.Feature} A Feature or Geometry with the area
+   *  of polygon1 excluding the area of polygon2.
+   */
+  static difference(polygon1, polygon2, projection = 'EPSG:3857') {


It would be possible to pass linestring geometries (e.g.) as well here, wouldn't it? If true, one could be more general here, e.g. ... Find the difference between two geometries ...

turf.js only supports difference for polygons.

ahennr · 2018-03-09T08:33:32Z

src/Util/GeometryUtil/GeometryUtil.js

+   * @returns {ol.geom.Geometry | ol.Feature} A Feature or Geometry with the
+   * shared area of the two polygons or null if the polygons don't intersect.
+   */
+  static intersection(polygon1, polygon2, projection = 'EPSG:3857') {


See above comment regarding generality.

turf.js only supports difference for intersect.

ahennr · 2018-03-09T08:34:57Z

src/Util/GeometryUtil/GeometryUtil.spec.js

+      });
+    });
+  });
+});


KaiVolland · 2018-03-09T10:33:46Z

I extracted the geometries to a seperate file to increase the readability of the spec file and added some more docs.

…-2.7.4

KaiVolland mentioned this pull request Mar 8, 2018

WIP Introduce split by line #461

Closed

KaiVolland changed the title ~~Introduces GeometryUtil~~ [WIP] Introduces GeometryUtil Mar 8, 2018

Add turf dependency

d8c27a8

KaiVolland force-pushed the geometry-util branch from 290b71e to 5116d98 Compare March 8, 2018 14:19

Introduces GeometryUtil

448c6a4

This introduces the GeometryUtil including these methods: - splitByLine - addBuffer - mergeGeometries - union Co-authored-by: dnlkoch <koch@terrestris.de>

KaiVolland force-pushed the geometry-util branch from 5116d98 to 448c6a4 Compare March 8, 2018 14:49

Introduces intersection method

04e60ca

KaiVolland force-pushed the geometry-util branch from ebb3ad5 to 04e60ca Compare March 8, 2018 15:51

KaiVolland added 2 commits March 8, 2018 17:07

Introduces difference method

64bbfcc

Better docs and more semantic variable names for GeometryUtil

93d2003

KaiVolland changed the title ~~[WIP] Introduces GeometryUtil~~ Introduces GeometryUtil Mar 8, 2018

marcjansen approved these changes Mar 8, 2018

View reviewed changes

ahennr approved these changes Mar 9, 2018

View reviewed changes

Further docs- and test- enhancements for GeometryUtil

c20669e

KaiVolland merged commit 894260b into terrestris:master Mar 9, 2018

KaiVolland deleted the geometry-util branch April 3, 2018 08:28

hblitza pushed a commit that referenced this pull request Oct 19, 2022

Merge pull request #463 from terrestris/dependabot/npm_and_yarn/proj4…

792586f

…-2.7.4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces GeometryUtil #463

Introduces GeometryUtil #463

KaiVolland commented Mar 8, 2018 •

edited

marcjansen left a comment

marcjansen Mar 8, 2018 •

edited

ahennr Mar 9, 2018

KaiVolland Mar 9, 2018

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018

KaiVolland Mar 9, 2018

marcjansen Mar 8, 2018 •

edited

ahennr Mar 9, 2018

KaiVolland Mar 9, 2018

marcjansen Mar 8, 2018

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018

marcjansen Mar 8, 2018

ahennr left a comment

ahennr Mar 9, 2018

KaiVolland Mar 9, 2018

ahennr Mar 9, 2018

ahennr Mar 9, 2018

KaiVolland Mar 9, 2018

ahennr Mar 9, 2018

ahennr Mar 9, 2018

KaiVolland Mar 9, 2018

ahennr Mar 9, 2018

KaiVolland Mar 9, 2018

ahennr Mar 9, 2018

KaiVolland commented Mar 9, 2018

Introduces GeometryUtil #463

Introduces GeometryUtil #463

Conversation

KaiVolland commented Mar 8, 2018 • edited

FEATURE

Description:

marcjansen left a comment

Choose a reason for hiding this comment

marcjansen Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcjansen Mar 8, 2018 • edited

Choose a reason for hiding this comment

marcjansen Mar 8, 2018 • edited

Choose a reason for hiding this comment

marcjansen Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcjansen Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcjansen Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahennr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KaiVolland commented Mar 9, 2018

KaiVolland commented Mar 8, 2018 •

edited

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018 •

edited

marcjansen Mar 8, 2018 •

edited