fleaflet / flutter_map

A versatile mapping package for Flutter. Simple and easy to learn, yet completely customizable and configurable, it's the best choice for mapping in your Flutter app.
https://pub.dev/packages/flutter_map
BSD 3-Clause "New" or "Revised" License
2.73k stars 859 forks source link

[FEATURE] Replace the polylabel dependency with an own implementation #1746

Open josxha opened 9 months ago

josxha commented 9 months ago

What do you want implemented?

The current integration of polylabel is not very well performance optimized. By implementing our own port of polylabel, we can:

What other alternatives are available?

No response

Can you provide any other information?

Current usage of the polylabel package:

LatLng _computePolylabel(List<LatLng> points) {
  final labelPosition = polylabel(
    [
      List<math.Point>.generate(points.length,
          (i) => math.Point(points[i].longitude, points[i].latitude)),
    ],
    // "precision" is a bit of a misnomer. It's a threshold for when to stop
    // dividing-and-conquering the polygon in the hopes of finding a better
    // point with more distance to the polygon's outline. It's given in
    // point-units, i.e. degrees here. A bigger number means less precision,
    // i.e. cheaper at the expense off less optimal label placement.
    precision: 0.000001,
  );
  return LatLng(
    labelPosition.point.y.toDouble(),
    labelPosition.point.x.toDouble(),
  );
}

Severity

Minimum: Not required for my use

mootw commented 9 months ago

i noticed this while working on #1750 and added a comment //TODO does this really need to be changed to a math.Point type?

JosefWN commented 8 months ago

Maybe better to be inspired rather than straight-up porting, perhaps a bit touchy since it's a commercial company, and porting counts as redistribution from a legal point of view. In their licensing terms:

The software and files in this repository (collectively, "Software") are
licensed under the Mapbox TOS for use only with the relevant Mapbox product(s)
listed at [www.mapbox.com/pricing](http://www.mapbox.com/pricing).
...
* Redistributions of source code must retain the above copyright notice,
  this list of conditions and the following disclaimer.
* Redistributions in binary form must reproduce the above copyright notice,
  this list of conditions and the following disclaimer in the documentation
  and/or other materials provided with the distribution.
...

https://github.com/mapbox/mapbox-gl-js?tab=License-1-ov-file#readme

Other than Your Content, all content displayed on the site or accessible
through the Services, including text, images, maps, software or source code,
are the property of Mapbox and/or third parties and are protected by
United States and international intellectual property laws.

https://www.mapbox.com/legal/tos/

Porting would be a violation of the TOS, and even if it weren't, only dependencies are managed automatically by flutter (bundling the licenses with the binary), so it could be a bit cumbersome 😅

I think it's quite a bit different to port things from Mapbox than it is from, say, Leaflet.

JaffaKetchup commented 8 months ago

(It is possible to use LicenseBinding to add licenses to the Flutter app: https://api.flutter.dev/flutter/foundation/LicenseRegistry/addLicense.html)

JosefWN commented 8 months ago

Ah, thanks, didn't know! If we could automate that it would certainly simplify things for the users of flutter_map, I'm still doubtful that porting is allowed within the scope of the TOS though?

josxha commented 8 months ago

I'm still doubtful that porting is allowed within the scope of the TOS though

Good point, in that case we should see that as an encouragement to remove the dependency to the polylabel package anyways.