dartclub / turf_dart

A turf.js-like geospatial analysis library working with GeoJSON, written in pure Dart.
https://pub.dev/packages/turf
MIT License
67 stars 30 forks source link

Polygon clipping package conversion #170

Open aardrop opened 7 months ago

aardrop commented 7 months ago

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

aardrop commented 7 months ago

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:

High Level Work Left to be Done:

aardrop commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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.