RangeError in marker optimization code

[Xennis]

Dear @TheLastGimbus thanks for adding #826 . I'm one of the maintainer of a flutter_map plugin. I'm getting at

flutter_map/lib/src/layer/marker_layer.dart

Line 214 in 38d9047

_pxCache[i] = pxPoint;

the following error:

RangeError (RangeError (index): Invalid value: Valid value range is empty: 0)

The issue can be reproduce with the example here https://github.com/Xennis/flutter_map_location/tree/pxcache-issue/example

Can you please take a look?

[TheLastGimbus]

Damn, I was afraid that some RandeErrors will come up after messing with lists like that... I will take a look 👍

[TheLastGimbus]

Okay, so I see that:

Markers aren't really touched anywhere on those pages, and setState() isn't called neither...

Then, inside `LayerOptions you have them as mutable:

https://github.com/Xennis/flutter_map_location/blob/e8d67dd7b71593b94481cadc1a1aba0e02eed02b/lib/src/location_options.dart#L36

Then, you add them in initState

https://github.com/Xennis/flutter_map_location/blob/e8d67dd7b71593b94481cadc1a1aba0e02eed02b/lib/src/location_layer.dart#L68-L70

which also changes the final List<Marker> userLocationMarkers on page... wait, it's final there, so what the hell?

Anyway, it looks that the way of adding markers in this plugin is not Flutter-ish at all, at leat to me 🤔 - because of that, MarkerLayer's didUpdateWidget isn't called, and it can't refresh the cache size

We can do two things:

• I can explicitly add widget.markerLayerOptions.markers.length == _pxCache.lenght on every build(), or
• you can change the way markers are changed in LocationPlugin - which I would suggest because such non-excplicit mutating markers that are passed to constructor can cause more trouble in future 😬

[Xennis]

Dear @TheLastGimbus Thanks for you analysis.

Then, inside `LayerOptions you have them as mutable:

https://github.com/Xennis/flutter_map_location/blob/e8d67dd7b71593b94481cadc1a1aba0e02eed02b/lib/src/location_options.dart#L36

Good catch. I can easily change it to final but the list itself will be still mutable. I pushed that change into the branch.

which also changes the final List<Marker> userLocationMarkers on page... wait, it's final there, so what the hell?

It adds/removes elements from the final list. final doesn't mean you can't change the elements of the list.

I would suggest because such non-excplicit mutating markers that are passed to constructor can cause more trouble in future

I would be open for changes 🙂 What is your recommended change here? In general the task: If the user clicks a "get my location" button show the location as marker on the map.

[TheLastGimbus]

Hmm... in my opinion - remove markers field and add maybe something like onMarkersUpdate, or merge it with onLocationUpdate - then user does:

class _PageState extends State<Page> {
  final myMarkers = [marker1, marker2];
  // Idk if your library can give multiple markers or just one
  var locationMarkers = <Marker>[];

  Widget build() {
    // ...
    body: Center(
      child: FlutterMap(
        // ...
        mapController: mapController,
        options: MapOptions(
          plugins: <MapPlugin>[LocationPlugin()],
        ),
        layers: <LayerOptions>[
          MarkerLayerOptions(markers: myMarkers + locationMarkers),
          LocationOptions(
            onLocationUpdate: (LatLngData ld, List<Marker> markers) {
              locationMarkers = makrers;
              setState(() {});
            },
          ),
        ],
      ),
    ),
  }
}

Something like this 🤷

[Xennis]

Thanks a lot you. I'll look into it.

Still might be worth to fix that introduce regression in flutter_map itself if it's possible to run into it. I would guess other plugins are affected as well. For example user_location_layer that use a similar pattern that the markers are passed into the plugin and a marker layer:

https://github.com/igaurab/user_location_plugin/blob/d9fc7a378b7c53e5ae3829eb435ef0bed6757af0/example/lib/main.dart#L25

[TheLastGimbus]

😬

This should not be like this in my opinion...

I can add a quick-fix-lenght-check if you want, but honestly I feel like this should throw an error - to show other plugin makers that they can't do it this way...

[Xennis]

At least for the flutter_map_location plugin it's fixed now. The development branch was successfully updated in Xennis/flutter_map_location#51 (The actual work was done Xennis/flutter_map_location#48 ).

[kuhnroyal]

I run into this in my own code. Any advice how to avoid this?
I basically have a list of markers in the state that starts empty and lazily markers are added after initState or after didUpdateWidget.

[TheLastGimbus]

I think above discussion should help you - basically don't mutate stuff - all arguments to layerOptions should be final- if you want to add some markers lazily, you should update them in some stateful widget and call setState()

[mvarendorff]

Would that mean fully rerendering the entire map including tiles each time there is a change to the markers is a necessity?

Just for reference: We hit this error when trying to add markers after fetching information from the server. We tried to achieve that by having an empty array, populating it with the network response and using the rebuild Stream to update the layer which in turn caused the issue at hand.

[kuhnroyal]

Yea same thing for me, not sure how this discussion helps me :(

[ibrierley]

Just wondering if generatePxCache() could be called if the pxcache list length is a different length to the markerlist ?

[TheLastGimbus]

Would that mean fully rerendering the entire map including tiles each time there is a change to the markers is a necessity?

Yes, altought this shouldn't be a problem. You can probably call setState many many times and map shouldn't visibly lag...

[mvarendorff]

Wonderful, thanks a bunch!

[kuhnroyal]

My problem was List.add calls not being picked up by setState which is obvious in hindsight.
Replaced that with spread operator instead:

setState(() {
  markers = [
    ...markers,
    addNewMarker(),
  ];
});