migrate latlng to use record type (feedback requested)

[mootw]

This is a potential proposal to removing dependency on latlong2. Mainly it restricts bounds to +-180, +-90 which makes wrapping maps impossible in practice. A record type makes it easy to work with lat lon points across libraries without having a central "lat lon class" dependency. There are potential performance gains to be had from converting lat, lon pairs to a record type instead of an object at scale, but they are likely minimal due to classes being efficient.

currently, this does not completely remove the latlong2 dependency, as it is still used internally in fm for distance calculations in 2 places but this is theoretically easy to fix in the future.

this is a proof of concept and i intend to create an issue outlining all of the reasons to migrate away from latlong2 and propose some options including a fm fork that fixes the issues with it, alternative base class, or using an existing "Point" class like math.Point or Vector2 from vector_math.

some of the issues that would be able to be resolved by moving away from latlong2:

removing bounds: jifalops/dart-latlong#11
#468
#1338
fixes #1452
#1582
maybe fixes #1717 (do not know if there are more issues with the gesture system past the latlong2 limitation)

I have created a PR on upstream jifalops/dart-latlong#13 to also see if an upstream fix is possible too

[bramp]

This looks cool, can you help me understand two of your statements?

A record type makes it easy to work with lat lon points across libraries without having a central "lat lon class" dependency.

How is it easier? If both cases (class Vs record) you have to convert your type into whatever type the external library is using.

There are potential performance gains to be had from converting lat, lon pairs to a record type instead of an object at scale,

I tried to research the performance/memory layout of records, but didn't find anything useful. Surely a record is more expensive (in ram) than a object, since it has to carry around extra type information, where a class just carries a pointer to the class definition.

I'm curious to learn your reasoning. Thanks

[josxha]

A record type makes it easy to work with lat lon points across libraries without having a central "lat lon class" dependency.

@mootw I understand the reasoning that if all are using a record a base package like latlong2 would not be needed. However, I have a few concerns:

• (3, 2) != (x: 3, y: 2) therefore every other package would need to use the exact same record delaration. This kind of defets the idea to have no base class.
• Some beginners could have problems using records since they are more or less an advanced language feature.
• While we can use a typedef as parameter or return types, we can't use the typedef when we create a new record. That way it could be argued that we would decrease type safetly.

I tried to research the performance/memory layout of records, but didn't find anything useful. Surely a record is more expensive (in ram) than a object, since it has to carry around extra type information, where a class just carries a pointer to the class definition.

@bramp I did some performance and memory testing and want to share the results:

• I found no difference in memory consumption between record instances and an instances of an immutable class.
• Threre is an obvious memory when using constants.
• Immutable classes required by far less time to get created than a records.
• There was no performance difference between the LatLng class and a minimal alternative class.

Therefore I'd prefer to keep a immutable class for latitude/longitude.

---

I used this to test memory (running in profile mode):

Click here to view

import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatefulWidget {
  const MyApp({super.key});

  @override
  State<MyApp> createState() => _MyAppState();
}

class _MyAppState extends State<MyApp> {
  late final List<dynamic> data;

  @override
  void initState() {
    const m = 100000000;

    data = List.generate(m, (_) => const (23, 53));
    // total: 768.6 MB, _Record 32 B

    //data = List.generate(m, (_) => (23, 53));
    // total: 3.7 MB, _Record 3.0 GB

    //data = List.generate(m, (_) => const LatLng(23, 53));
    // total: 768.1 MB, LatLng 32 B

    //data = List.generate(m, (_) => LatLng(23, 53));
    // total: 3.7 GB, LatLng 3.0 GB

    //data = List.generate(m, (_) => const MyLatLng(23, 53));
    // total: 768.6 GB, MyLatLng 32 B

    //data = List.generate(m, (_) => MyLatLng(23, 53));
    // total: 3.7 GB, MyLatLng 3.0 GB

    super.initState();
  }

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(home: Scaffold(body: SizedBox.shrink()));
  }
}

@immutable
final class MyLatLng {
  final double lat;
  final double long;

  const MyLatLng(this.lat, this.long);

  @override
  bool operator ==(Object other) =>
      other is MyLatLng && lat == other.lat && long == other.long;

  @override
  int get hashCode => Object.hash(lat, long);
}

And this to test performance (compile first to use the prevent JIT compilation):

Click here to view

void main() async {
  await Future.delayed(const Duration(seconds: 1));
  final sw = Stopwatch();
  sw.start();
  const m = 300000000;

  final name = 'MyLatLng';
  List.generate(m, (i) => MyLatLng(i.toDouble(), (m - i).toDouble()));
  // result: 82.256s, 81.344s

  /*final name = 'LatLng';
  List.generate(m, (i) => LatLng(i.toDouble(), (m - i).toDouble()));*/
  // result: 71.43s, 81.228s

  /*final name = 'record';
  List.generate(m, (i) => (i.toDouble(), (m - i).toDouble()));*/
  // result: 142.514s, 156.227s

  sw.stop();
  print('[$name]     ${sw.elapsedMilliseconds / 1000}s');
}

final class MyLatLng {
  final double lat;
  final double long;

  const MyLatLng(this.lat, this.long);

  @override
  bool operator ==(Object other) =>
      other is MyLatLng && lat == other.lat && long == other.long;

  @override
  int get hashCode => Object.hash(lat, long);
}

[bramp]

Thanks. It seems Classes and Record are very similar (if not identical) from a memory perspective. Thus I agree the benefits of a class outweigh a record in this case.

@josxha you inspired me to learn more here. Firstly I tested named records (lat: 23, lng: 53), and I got similar memory usage.

However, This may be off topic now, but I dug deeper. Reading sdk/runtime/vm/object.h, runtime/vm/raw_object.cc and related files, and looking at ASM output (dart -print-flow-graph-optimized --disassemble-optimized). I discovered the following. This depends on your architecture, but assume arm64.

All records and class instances are objects. In memory a object is a 64 bit tag, followed by the various fields. The tag contains information about the instance size, class (for this object), GC information, etc. The object is always padded up to the nearest 128 bits. So if a object has no fields, it still takes 64 bits for the tag, and 64 bits padding.

A record (e.g (1.0, 2.0)), is a instance of the dart Record class, with 1 field for the "shape" of the record, and then fields for each value. The shape is 64 bits, and contains information about the number and name of the fields. Again, the full object is always padded up to the nearest 128 bits.

Finally the List will take 64 bits per entry, for the pointer to the Instance/Record.

     Instance     Record     List
0    ┌──────┐    ┌──────┐    ┌───┬───┬───┬───┬───┬───────────┐
     │ Tag  │    │ Tag  │    │ 0 │ 1 │ 2 │ 3 │ 4 │...        │
8    ├──────┤    ├──────┤    └───┴───┴───┴───┴───┴───────────┘
     │Field1│    │Shape │
16   ├──────┤    ├──────┤
     │Field2│    │Field1│
24   ├──────┤    ├──────┤
     .Paddin.    │Field2│
32   ........    └──────┘

So I took your example, and tweaked it a little:

@immutable
final class Three {
  final double a;
  final double b;
  final double c;

  const Three(this.a, this.b, this.c);

  @override
  bool operator ==(Object other) => other is Three && a == other.a && b == other.b && c == other.c;

  @override
  int get hashCode => Object.hash(a, b, c);
}

// Classes Zero, One, Two, Four hidden for brevity.

void main(List<String> arguments) {
  const m = 100000000;
  final data = List.generate(m, (n) => Three(1, 2, 3), growable: false);
}

and ran tests like so dart compile exe bin/record_size.dart && /usr/bin/time -l bin/record_size.exe, which prints out the "peak memory footprint".

Total (bytes)	Size per instance	Code
2,471,483,712	24	Empty()
2,471,483,712	24	One(1)
4,098,762,176	40	Two(1, 2)
4,093,027,648	40	Three(1, 2, 3)
5,699,432,768	56	Four(1, 2, 3, 4)
5,698,105,600	56	Five(1, 2, 3, 4, 5)

and

Total (bytes)	Size per record	Code
4,095,354,176	40	(1, 2)
5,699,350,848	56	(1, 2, 3)
5,699,252,544	56	(1, 2, 3, 4)
7,337,361,024	72	(1, 2, 3, 4, 5)
7,420,051,968	72	(1, 2, 3, 4, 5, 6)

So to conclude this. A LatLng class, with two fields takes: pointer (8 bytes) + object tag (8 bytes) + field1 (8 bytes) + field2 (8 bytes) + 8 bytes of padding = 40 bytes. Where a 2 field record takes: pointer (8 bytes) + object tag (8 bytes) + shape (8 bytes) + field1 (8 bytes) + field2 (8 bytes) + no padding = 40 bytes.

A final side note. If you define the classes, but never access the fields (e.g for example not having a hashCode method), then dart will optimise away those fields, leaving your class with zero fields. It does not do this for records (I think because records always add a hashCode method).

[ignatz]

This is a really fun thread, thanks everyone. I didn't know about the "shape" member. I totally believe the findings, it just defeated my intuition. I always considered records to be syntactic sugar around class definitions that implement the "Record" interface, as such I would expect them to have the same memory layout as any other subclass 🤷‍♀️

A final side note. If you define the classes, but never access the fields (e.g for example not having a hashCode method), then dart will optimise away those fields, leaving your class with zero fields. It does not do this for records (I think because records always add a hashCode method).

Yes, that's a general problem with tree-shaking. Since hashCode is a virtual method that can be called through a base Object reference it's undecidable at compile time whether the members will be accessed. If the method wasn't virtual, the method and members could be shed (if not used).

Therefore I'd prefer to keep a immutable class for latitude/longitude.

There's a variety of arguments but with regards to performance, immutability is the enemy in garbage collected languages. If the coordinates were mutable they could be pooled and recycled putting less pressure on GC.

Just my 2c, latlong2 is used in many geospatial dart libraries. While that's by itself not a good reason to use it, if we'd change the coordinate type now, it would be a very invasive migration and I would end up writing a lot of glue code for my application (which most certainly would drive up the memory consumption :) ).

Personally, I like that LatLng2 validate the ranges. It has shown me quite a few bugs in my code. I don't understand why this would prevent us from wrapping maps. If that's the main reason to move away from latlong2, is there something else one could do?

[JaffaKetchup]

I don't understand why this would prevent us from wrapping maps.

It gets difficult to handle the 'iterations'/'versions'(?) of maps if we restrict to the standard -180 - 180. We'd have to have some sort of extra indicator with every LatLng indicating which 'version' it lies in. I would guess, maybe there's another way I'm not thinking of.

Personally, I like that LatLng2 validate the ranges.

Yes, I like this as well. If we stop validating ranges, we might be running into Issues with people asking why maths is breaking down or stuff isn't appearing correctly.

Just my 2c, latlong2 is used in many geospatial dart libraries. While that's by itself not a good reason to use it, if we'd change the coordinate type now, it would be a very invasive migration and I would end up writing a lot of glue code for my application (which most certainly would drive up the memory consumption :) ).

Another concern of mine. I would much prefer to inherit the library (into the org) as the maintainer appears to not be maintaining it anymore - but I think we've tried in the past to get in touch. Equally however, depending on abandoned libraries isn't a great plan for a growing library. I think migration and using existing APIs is the biggest issue here, not necessarily what the replacement could look like.

As a side note, if we were to do this, an auto migration tool could be helpful. Search/replace would probably get most of the way, but not all of the way.

[ignatz]

It gets difficult to handle the 'iterations'/'versions'(?) of maps if we restrict to the standard -180 - 180. We'd have to have some sort of extra indicator with every LatLng indicating which 'version' it lies in.

Shouldn't the answer be: all? Just thinking out loud: if I have a point LatLng(42, 23) and I'm zoomed out so that we see N "versions" of that point, I would expect to see any marker, polyline, polygone associated with that point N times. I certainly wouldn't expect additional map-space coordinates like LatLng(42+180, 23+360) to spring into existence.

What I would expect is that any transformation from map into screen coordinates would become a 1:N mapping, i.e.:

Point<double> project(LatLng);

would become

List<Point<double>> project(LatLng);

As a side note, if we were to do this, an auto migration tool could be helpful. Search/replace would probably get most of the way, but not all of the way.

Right, it would still leave you with the glue code to other dependencies.

I would much prefer to inherit the library (into the org) as the maintainer appears to not be maintaining it anymore - but I think we've tried in the past to get in touch

That would be ideal. Arguably, latlong2 is relatively self-contained, so as long as we don't require any changes it sort-of-fine until we do :). Maybe, pub.dev has a process for adopting packages?

[JaffaKetchup]

List<Point> project(LatLng);

Would that not be an infinite list? There are infinitely many versions of the map.

[ignatz]

Screen coordinates are very finite. It's just an API sketch but I would expect any such API to return a list of points on screen that correspond to the given map coordinate.

[JaffaKetchup]

Good point.

Maybe, pub.dev has a process for adopting packages?

It doesn't look like it, and that's probably for the best. (I wonder what happens when a Google Account associated with an upload is deleted for inactivity 🤔 - but we can't exactly wait 2 years to find out :D.)

[josxha]

Shouldn't the answer be: all? Just thinking out loud: if I have a point LatLng(42, 23) and I'm zoomed out so that we see N "versions" of that point, I would expect to see any marker, polyline, polygone associated with that point N times. I certainly wouldn't expect additional map-space coordinates like LatLng(42+180, 23+360) to spring into existence.

I agree, I think the reasoning behind removing the -90/90 and -180/180 limits were so that e.g. the track of airplains can cross them. But I think some logic that checks if a line jumps to the other side and then drawing the line where the shorter distance is should solve this issue. Having those asserts in latlong2 is correct too since coordinates outside of these limits aren't valid latitude longitude coordinates.

There's a variety of arguments but with regards to performance, immutability is the enemy in garbage collected languages. If the coordinates were mutable they could be pooled and recycled putting less pressure on GC.

In my opinion it should be fine to have LatLng in public APIs since most LatLng instances shouldn't chance every frame. Internally however we could remove the usage of Point and newly created LatLng instances all together. This should reduce a huge amount of load on the GC.
If we return screen points or coordinates back to the user however I think we should create those LatLng/Point instances unless it fires on every frame.

[josxha]

How about if we add an abstraction widget between the top level FlutterMap widget and the map layers, e.g. FMWorld.

Scope of the FlutterMap widget:

• keep track about the TileProviders and logic that needs to be shared between every world map.
• initialize and remove FMWorld instances and keeps track about their screen offset.
• handles user gestures, MapController, MapCamera

Scope of each FMWorld widget:

• check what tiles are visible
• request tile from the central TileProvider(s)
• Lines across FMWorld borders would be drawn in both instances to an point outside of the canvas.

This is an very simple illustration. In case the map is zoomed in, there are only one (or at max two) FMWorld instances. This would need to be adjusted to the current FlutterMap class hirarchy.

[ignatz]

I agree, I think the reasoning behind removing the -90/90 and -180/180 limits were so that e.g. the track of airplains can cross them. But I think some logic that checks if a line jumps to the other side and then drawing the line where the shorter distance is should solve this issue. Having those asserts in latlong2 is correct too since coordinates outside of these limits aren't valid latitude longitude coordinates.

I appreciate that you're grounding us with a real use-case. I think the airplane example is a great one. (In your later post you have the other example where the world is shown 3 full times. That one is interesting as well but probably not as relevant in practice and has different requirements since markers, polylines, polygons, ... would actually be drawn 3 times.

In the airplane example the map is wrapping but there's only ever one version of everything. That means we should/might be able to tackle this case w/o changing APIs and therefore API consumers like the PolylineLayer et al. Imagine

Point project(LatLng)

would simply return the screen coordinate for LatLng that is on screen rather than the one that is off screen (which it does today). All layers should automatically do the right thing (probably I'm missing some spherical corner cases).

In my opinion it should be fine to have LatLng in public APIs since most LatLng instances shouldn't chance every frame. Internally however we could remove the usage of Point and newly created LatLng instances all together. This should reduce a huge amount of load on the GC.
If we return screen points or coordinates back to the user however I think we should create those LatLng/Point instances unless it fires on every frame.

I agree with you that this would be the ideal. I was mostly playing devils advocate. I don't think performance is everything and immutability has other nice properties.

Even today, FlutterMap should not allocate too many new LatLngs internally. There are some use cases but often it's just a few points (e.g. when you translate a tap in screen coordinates to a marker placement location on the map). For cases that tend to have the most points, like large numbers of Poly(gon|line)s, FlutterMap will typically just take the user-provided/allocated points (that as you say don't necessarily change very often) and convert the point in many intermediate points and ultimately screen coordinates and that on every frame. If we wanted to minimize allocations the meat is in all the non-map-space-coordinates.

[joosakle]

From my point of view the problem with the current use of LatLng is that the -90/90 and -180/180 validation makes using the library for non-geographic maps more difficult. My organization uses the library for floor-plan maps with custom CRS, where coordinates are in millimeters. Migrating from Leaflet-based JS app was very straightforward otherwise, but I had to fork FlutterMap simply to get rid of the LatLng coordinates validation, which didn't exist in Leaflet implementation.

[JosefWN]

I feel your pain @joosakle. On paper flutter_map suports custom CRS, but in practise it's pretty hairy because parts of the code, especially the layers, are not written with them in mind. My suggestion is that you use Point for your custom CRS rather than LatLng, although that also necessitates that you fork/re-implement all layers you want to use.

I would argue that LatLng could extend Point, but that any methods in flutter_map should rely on solely on Point. Enforcing LatLng inputs everywhere favors a very ellipsoid-centric approach to development, which unintentionally introduces assumptions entirely excluding other CRS, like in the case of @joosakle. All CRS, especially local CRS, don't necessarily map to lat/lng (i.e. there is no way to project between the two). In my case it's quite common that the input is planar in CRS X and the output is planar in CRS Y (so an affine transform which doesn't involve lat/lon).

I think the library proj4dart is a pretty good example of how projections are usually handled in CRS-agnostic libraries, not only in Dart but also in other languages. Their library isn't designed around Point inputs and outputs just for fun, but out of necessity. If flutter_map was designed this way, it would open up for a range of new possible use-cases (which would mean that I could retire my fork and work more actively on this repo instead).

I suspect that it wouldn't be a huge undertaking to make this switch. It's a slightly more abstract way of thinking, which some developers seem uncomfortable with for some reason, but it would also help reduce the occurrence of incorrect assumptions in the code base caused by the very same comfort that introduced the enforcement of LatLng in the first place. The fact that a custom CRS can be provided to flutter_map but that the library itself is actively preventing the user from using said CRS without forking the library also comes across as a bit contradictory.

For those that feel this train of thought is very foreign or even wrong, I think the picture would be a bit clearer after a briefer on basic projection mathematics. Coordinates are a pretty central component of the library after all, and by knowing the gist of how projections work, it will also be easier to see the implications of decisions regarding how the code should be structured.

[JosefWN]

@joosakle, technically you don't have to fork the repo, but it's hairy (and I admittedly ended up forking it too). Here is a proposed ugly hack for your floor plan maps.

First, pick a planar CRS, I will pick EPSG:3413 because I use that a lot. It can be broken down into a number of projection parameters in the proj4 string:

"+proj=stere +lat_0=90 +lat_ts=70 +lon_0=-45 +x_0=0 +y_0=0 +datum=WGS84 +units=m +no_defs +type=crs"

The default unit for the coordinate system is meters (+units=m), easting (x) and northing (y).

The CRS is centered at the North Pole (+lat_0=90), where it is centered on Earth doesn't really matter to you, but we need a reference we can use to project your planar coordinates to lat/lon. The longitude (+lon_0=-45) on the pole will determine the rotation of your axes, which isn't really relevant in your case, so we can leave it be.

Next, we'll pick a reasonable latitude of true scale (+lat_ts). If you make an orthogonal projection of the northern hemisphere of the Earth's surface onto a plane, you can think of this as the latitude where the plane intersects the Earth, i.e. where there is no/minimal projection error. The farther away you go from the latitude of true scale, the more distorted your map becomes.

Relative to the size of the northern hemisphere, your building is probably small, so we will pick a latitude really close to the north pole, such as +lat_ts=89.99. This forms a ~1000 meter radius around the pole, so you will be plotting in a region of the CRS where there is minimal distortion.

False easting/northing (+x_0=0, +y_0=0) are set to zero, meaning that we have offset the origin with 0 meters in x and y respectively. In other words, Point(0, 0) will correspond to LatLng(90, ..).

The +no_defs prevents any legacy default settings from being loaded, it's typically included for backwards compatibility with older Proj4 parsers.

Now you should be able to create the CRS:

Proj4Crs.fromFactory(
  code: 'Dummy',
  proj4Projection: proj4.Projection.add(
      'Dummy',
      '+proj=stere +lat_0=90 +lat_ts=89.99 +lon_0=-45 +x_0=0 +y_0=0 +datum=WGS84 +units=m +no_defs +type=crs',
    ),
);

Make Points and (un)project them into spherical coordinates:

Point p = Point(4, 8)
LatLng ll = crs.projection.unproject(p);

Where p is 4 meters from the origin in the x direction, and 8 meters from the origin in the y direction (just an example).

When this is passed into the layer, the layer will then internally project this point back again:

Point p = crs.projection.project(ll); // Point(4, 8)

Going back to my original rant, my point is that if you have a planar projected coordinate system, especially a local coordinate system which don't need projections (or technically even a CRS), from a flutter_maps internals point of view you are good to go, but instead you have to:

1. Read up on planar projections and construct a dummy proj4 string minimizing the local projection distortions
2. Unproject the planar coordinates to spherical coordinates
3. (in the layer) Project the spherical coordinates back to the very same planar coordinates again

All of this just to pass a coordinate to a layer. It incurs a cost in developer time/barrier for new developers, a performance cost (a lot of points projected/unprojected per frame adds up) and it just doesn't make much sense code-wise from a planar CRS point of view (especially in the case of simple local CRS like this).

Additionally, even if you do use LatLng, keeping a list of LatLng within the layer and projecting them for every frame is needless. Just move the projection step (essentially one line of code) out of the layer, project only if need be, and pass Points to the layer. You would only need to reproject if your List<LatLng> changes (panning, zooming etc. does not necessitate a reprojection).

[joosakle]

@JosefWN Thank you for the idea and great explanations in the other issue! It's very helpful for me and others who aren't experienced with mapping libraries. I really need to drive deeper into the topic.

[monsieurtanuki]

If part of the initial question is how to create LatLng beyond +-180, +-90, the simplest solution is to use a constructor that does not control the bounds: fromJson.

Something like

class MyLatLng extends LatLng {
  MyLatLng(final double latitude, final double longitude)
      : super.fromJson({
          'coordinates': [longitude, latitude]
        });
}

final LatLng test = MyLatLng(92, -182);
print('test: $test');
// test: LatLng(latitude:92.0, longitude:-182.0)

[JaffaKetchup]

Just as an update, @mootw pointed out the other day internally that jifalops/dart-latlong#11 has finally been merged! This opens the door to some of these changes without necessarily moving away from latlong2.

[joosakle]

The latlong_bounds class of this project still has the coordinate restrictions in place. I suppose they should be removed from it too, now that latlng2 package doesn't have them either?