Polygon clipping package conversion #170

aardrop · 2024-02-17T00:14:26Z

A dart conversion of polygon_clipping with adopted dart native classes such as Point, flutter's Vector2, SplayTreeMap, and SplayTreeSet.

aardrop · 2024-02-17T00:14:45Z

This initial draft commit aimed to set a baseline direct line-by-line conversion of the original JS package to the best of my ability. There’s a lot of work to be done and no functionality has been tested yet, here’s the state of this draft and what’s left to be decided and completed.

Basic work left to be done:

Debug and test basic functionality
Is the class approach what we want to work with?
Are SplayTreeMap and SplayTreeSet working properly?
How do we want to stage polygon_clipping to expose object files relative to the original project?
Check nullsafety implementation and classes relative to original work?
How do we want to implement points relative to original work? Segment implements a point that also tracks attached events, this is currently being handled with an extension of the original math class.
Do we want to use this BoundingBox class, the one from Turf, or the Rectangle class from dart that has all the prebuilt method that the original project writes and is maintained under the dart repository?
What class structuring can we do to improve performance and readability?
Currently all variable maintain original names for the JS package but does naming convention need to be updated?
With the heavy typing of dart can we leverage the class structure more with things like factory constructors to improve dart specific performance?
Are there more generic JS inputs that could be improved by heavier typing like List<List to Turf’s polygon or List
How do we want to handle the index file?

High Level Work Left to be Done:

test and verify functionality from the bottom up
Verify approach and it’s fit into the turf_dart project
Write benchmarks

lukas-h

The main points from my code review:

–> I didn't take a look at every line of code, but I think I already pointed out a few important things

–> we always need strongly typed parameters and return types of functions (if you have dynamic, consider replacing them with GeoJSONObject [for any GeoJSON object] or GeometryObject [for a concrete Geometry like Point, LineString, Polygon, and the Multi** types])

-> Make sure to always use the types from our library, meaning, coordinates should always use the class Position (which contains a lot of vector operations already (addition, subtraction, multiplication, etc.)) and Point (which is the standard type from the GeoJSON RFC standard to encode geographical points)

-> We already have a sweepline intersections package, please take a look if you can use that instead of implementing all the logic from scratch https://github.com/dartclub/sweepline_intersections/

lukas-h · 2024-02-17T12:45:20Z

lib/src/polygon_clipping/bbox.dart

+class BoundingBox {
+  Point ll; // Lower left point
+  Point ur; // Upper right point
+
+  BoundingBox(this.ll, this.ur);
+}


let's use the turf_dart BBox class here – you can find it in the geojson.dart file

lukas-h · 2024-02-17T12:47:59Z

lib/src/polygon_clipping/bbox.dart

+bool isInBbox(BoundingBox bbox, Point point) {
+  return (bbox.ll.x <= point.x &&
+      point.x <= bbox.ur.x &&
+      bbox.ll.y <= point.y &&
+      point.y <= bbox.ur.y);
+}
+
+BoundingBox? getBboxOverlap(BoundingBox b1, BoundingBox b2) {
+  // Check if the bboxes overlap at all
+  if (b2.ur.x < b1.ll.x ||
+      b1.ur.x < b2.ll.x ||
+      b2.ur.y < b1.ll.y ||
+      b1.ur.y < b2.ll.y) {
+    return null;
+  }
+
+  // Find the middle two X values
+  final lowerX = b1.ll.x < b2.ll.x ? b2.ll.x : b1.ll.x;
+  final upperX = b1.ur.x < b2.ur.x ? b1.ur.x : b2.ur.x;
+
+  // Find the middle two Y values
+  final lowerY = b1.ll.y < b2.ll.y ? b2.ll.y : b1.ll.y;
+  final upperY = b1.ur.y < b2.ur.y ? b1.ur.y : b2.ur.y;
+
+  // Create a new bounding box with the overlap
+  return BoundingBox(Point(lowerX, lowerY), Point(upperX, upperY));
+}


let's take a look if we have similar helper functions from other already implemented functions – but let's leave it like it is for now.

Here I could find a helper for overlapping of bboxes.

turf_dart/lib/src/booleans/boolean_contains.dart

Line 178 in f377e46

bool _doBBoxesOverlap(BBox bbox1, BBox bbox2) {

I see that method, could I add methods and helpers to the existing BBox method to make it more interoperable with the rest of the package's dependency on Positions?

i.e. add factory constructor from Positons, add getter methods for Position 1 and 2 of the List lat lng points described?

One more question on the bounding box, the specification the BBox class references https://datatracker.ietf.org/doc/html/rfc7946#section-5

It shows the convention of the first point being the lower left and the second point being the upper right but the BBox does not require these conditions but this polygon clipping uses this relationship to adjust the bounds to the points of the polygons.

Do you want me to add this to the BBox class, add methods/getters to make BBox work for polygon clipping, or make an extended class with this relationship?

I see that method, could I add methods and helpers to the existing BBox method to make it more interoperable with the rest of the package's dependency on Positions?

i.e. add factory constructor from Positons, add getter methods for Position 1 and 2 of the List lat lng points described?

sounds good – go for it!

Do you want me to add this to the BBox class, add methods/getters to make BBox work for polygon clipping, or make an extended class with this relationship?

let's stay conformant with the RFC and maybe work with an extension or class that extends BBox

Sounds good to clarify the RFC seems to follow the lower left upper right convention, so we want to enforce that?

lib/src/polygon_clipping/geom_in.dart

lukas-h · 2024-02-17T12:50:23Z

lib/src/polygon_clipping/geom_out.dart

+  List<List<List<List<double>>>> getGeom() {
+    List<List<List<List<double>>>> geom = [];
+
+    for (int i = 0, iMax = polys.length; i < iMax; i++) {
+      List<List<List<double>>>? polyGeom = polys[i].getGeom();
+      if (polyGeom == null) continue;
+      geom.add(polyGeom);
+    }
+
+    return geom;
+  }


we should definitely use the turf GeoJSON classes here, like Polygon or MultiPolygon for static types

Agreed, any advice on deliniating the return between multipolygon or polygons? I'm making good progress on testing all the components and working my way up to these here now.

lukas-h · 2024-02-17T12:51:47Z

lib/src/polygon_clipping/index.dart

+dynamic union(dynamic geom, List<dynamic> moreGeoms) =>
+    operation.run("union", geom, moreGeoms);
+
+dynamic intersection(dynamic geom, List<dynamic> moreGeoms) =>
+    operation.run("intersection", geom, moreGeoms);
+
+dynamic xor(dynamic geom, List<dynamic> moreGeoms) =>
+    operation.run("xor", geom, moreGeoms);
+
+dynamic difference(dynamic subjectGeom, List<dynamic> clippingGeoms) =>
+    operation.run("difference", subjectGeom, clippingGeoms);


we should have at least a "bit" of static typization for parameters and return types: we have the type GeoJSONObject for any geojson type (Feature, GeometryCollection, Polygon, Point, etc.) or GeometryObject for a concrete geometry (Point, Linestring, Polygon, Multi**)

lukas-h · 2024-02-17T12:58:35Z

lib/src/polygon_clipping/operation.dart

+  late String type;
+  int numMultiPolys = 0;
+
+  List<dynamic> run(String type, dynamic geom, List<dynamic> moreGeoms) {


we should always have proper typization instead of dynamic, (what I described with GeoJSONObject and GeometryObject)

lukas-h · 2024-02-17T13:02:00Z

lib/src/polygon_clipping/rounder.dart

@@ -0,0 +1,83 @@
+import 'dart:collection';
+import 'dart:math';
+


we already have rounding functions in our helpers file.

Please take a look if that works for you:

turf_dart/lib/src/helpers.dart

Line 81 in f377e46

num round(num value, [num precision = 0]) {

Another thing: Please always Position class for coordinates (and maybe an extension or extend to add new things)

Is it worth running a benchmark on the two methods or should I just use the existing one in helpers?

If the existing one works for this use case, let's use that one. And if you see a need for perf. optimization, you can go for it and propose a rewrite of the round we have

lukas-h · 2024-02-17T13:02:49Z

lib/src/polygon_clipping/segment.dart

+// Give segments unique ID's to get consistent sorting of
+// segments and sweep events when all else is identical
+import 'dart:math';
+


generally: strong types for parameters and return types of functions. Will take a look into this file in a future PR

lukas-h · 2024-02-17T13:04:01Z

lib/src/polygon_clipping/vector.dart

+num crossProduct(Point a, Point b) => a.x * b.y - a.y * b.x;
+
+/* Dot Product of two vectors with first point at origin */
+num dotProduct(Point a, Point b) => a.x * b.x + a.y * b.y;
+
+/* Comparator for two vectors with same starting point */
+num compareVectorAngles(Point basePt, Point endPt1, Point endPt2) {
+  double res = orient2d(
+    endPt1.x.toDouble(),
+    endPt1.y.toDouble(),
+    basePt.x.toDouble(),
+    basePt.y.toDouble(),
+    endPt2.x.toDouble(),
+    endPt2.y.toDouble(),
+  );
+  return res > 0
+      ? -1
+      : res < 0
+          ? 1
+          : 0;


we already have implemented those functions on Positions

I can't seem to find the equivalent of the compareVectorAngles. Do you happen to know which method you are referring to? I see the dot and cross product.

ok, compareVectorAngles is the one function we don't have, but we have most of the others (esp. basic vector arithmetic like + - )

You can just use:
Position(...) + Position(...) – it uses operator overloading in dart

lib/src/polygon_clipping/vector_extension.dart

aardrop · 2024-02-17T15:15:14Z

I'll dive into this today thanks! I'll look at reusing the sweepline though the sweepline from polygon_clipping wasn't very well commented so I still need to understand it's functionality a bit more to understand if they are interchangeable.

aardrop · 2024-02-20T14:26:33Z

The main points from my code review:

–> I didn't take a look at every line of code, but I think I already pointed out a few important things

–> we always need strongly typed parameters and return types of functions (if you have dynamic, consider replacing them with GeoJSONObject [for any GeoJSON object] or GeometryObject [for a concrete Geometry like Point, LineString, Polygon, and the Multi** types])

-> Make sure to always use the types from our library, meaning, coordinates should always use the class Position (which contains a lot of vector operations already (addition, subtraction, multiplication, etc.)) and Point (which is the standard type from the GeoJSON RFC standard to encode geographical points)

-> We already have a sweepline intersections package; please take a look if you can use that instead of implementing all the logic from scratch https://github.com/dartclub/sweepline_intersections/

I looked at the sweepline_intersections package this weekend, and there might be a way to use this, but its logic is different than the one used by polygon_clipping, and as far as I can tell, it wouldn't be an easy swap with the logic already written. I'm happy to switch, but I'd probably need support from someone who understands that package better.

The algorithm used for the intersections and the optimization of construction between the intersections is deeply intertwined between the relationships of the segmenting and reconstruction of polygon_clipping. Because the "intersection" is more than just a point in polygon clipping, returning both the broken segments and their relationships to each other and the optimization of the algorithm. If someone more familiar with that package wants to look at the relationship between SweetEvents, SweepLines, and Segments in this package, I'm happy to change it, but I don't see a good way to implement it.

lukas-h · 2024-02-20T17:21:01Z

written. I'm happy to switch, but I'd probably need support from someone who understands that package better.

We can start with what you already have. Maybe @jsiedentop would be a good conversation partner to talk about different sweepline algo implementations.

jsiedentop · 2024-02-21T07:48:53Z

@lukas-h I'm not yet as deep into the subject as you may think.

I would be fine with using different implementations initially. We could create an issue on the topic so that we can take a closer look later. It seems, that we have potential to improve the sweetline_intersections package to fit our needs in this case.

With good test coverage of the polygon_clipping functionality, we shouldn’t be afraid to change the implementation under the hood someday in the future.

jsiedentop · 2024-02-28T11:16:10Z

@aardrop I'm planning a new release for this week. Any chance to finish this feature or will we save it for the next release?

aardrop · 2024-02-28T21:06:32Z

@aardrop I'm planning a new release for this week. Any chance to finish this feature or will we save it for the next release?

I'd love to finish it this week but I'm just over half way done writing the testing scripts. Unfortunately because it's all interconnected I'm not even certain its functional until I finish these tests. I'd plan on this going in the next one to be safe. If I can pull this together in the next two days I'll let you know!

jsiedentop · 2024-02-28T21:37:38Z

Thanks for the update. Take your time with the testing – there's no rush. I've planned a few smaller topics for the next days, so just wanted to check if we should sync up for the release or if you need more time.

Start draft for polygon clipping package concertion

bcb21ac

lukas-h requested changes Feb 17, 2024

View reviewed changes

aardrop added 4 commits February 19, 2024 15:59

Starting adapt polygon-clipping to Position and BBox

1a9edbe

bounding box method testing for is in bbox and for isOverlapping

c5fe123

remove rounder

15f8fc3

Add tests for flp, orient_2d and vector methods.

5d33eff

This was referenced Mar 1, 2024

Implement difference, union, intersects, inverseDifference, xor, etc. by using poly_bool_dart and turfJS as a blueprint #166

Open

Preparation for release 0.1.0 #138

Open

Begin class testing

36b6cf2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Polygon clipping package conversion #170

Polygon clipping package conversion #170

aardrop commented Feb 17, 2024

aardrop commented Feb 17, 2024

lukas-h left a comment

lukas-h Feb 17, 2024

lukas-h Feb 17, 2024

aardrop Feb 17, 2024

aardrop Feb 17, 2024

lukas-h Feb 17, 2024

aardrop Feb 17, 2024

lukas-h Feb 17, 2024

lukas-h Feb 17, 2024

aardrop Feb 27, 2024

lukas-h Feb 17, 2024

lukas-h Feb 17, 2024

lukas-h Feb 17, 2024

aardrop Feb 17, 2024

lukas-h Feb 17, 2024

lukas-h Feb 17, 2024

lukas-h Feb 17, 2024

aardrop Feb 17, 2024

lukas-h Feb 17, 2024

aardrop commented Feb 17, 2024

aardrop commented Feb 20, 2024

lukas-h commented Feb 20, 2024

jsiedentop commented Feb 21, 2024

jsiedentop commented Feb 28, 2024

aardrop commented Feb 28, 2024

jsiedentop commented Feb 28, 2024

		@@ -0,0 +1,83 @@
		import 'dart:collection';
		import 'dart:math';

Polygon clipping package conversion #170

Are you sure you want to change the base?

Polygon clipping package conversion #170

Conversation

aardrop commented Feb 17, 2024

aardrop commented Feb 17, 2024

lukas-h 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

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

aardrop commented Feb 17, 2024

aardrop commented Feb 20, 2024

lukas-h commented Feb 20, 2024

jsiedentop commented Feb 21, 2024

jsiedentop commented Feb 28, 2024

aardrop commented Feb 28, 2024

jsiedentop commented Feb 28, 2024