Added option to use pxCache and also length check on Markers to avoid crash #1147
Conversation
|
I'm happier with this now, as I think it does help fix crashes when adding markers and the cache hasn't updated. Would be good to get a 2nd glance from someone. |
|
Will see what I can do in the way of testing. |
lib/src/layer/marker_layer.dart
Outdated
| /// Do we want to cache projections for performance, caching can | ||
| /// introduce inaccuracies if the cache is out of date |
There was a problem hiding this comment.
Consider using more 'direct' documentation?
| /// Do we want to cache projections for performance, caching can | |
| /// introduce inaccuracies if the cache is out of date | |
| /// Toggle marker position caching. Enabling will improve performance, but may introduce | |
| /// errors when adding/removing markers. Default is enabled (`true`). |
lib/src/layer/marker_layer.dart
Outdated
| @@ -191,10 +196,30 @@ class _MarkerLayerState extends State<MarkerLayer> { | |||
| var _pxCache = <CustomPoint>[]; | |||
|
|
|||
| // Calling this every time markerOpts change should guarantee proper length | |||
There was a problem hiding this comment.
Should this be a documentation comment (use '///')?
| // Calling this every time markerOpts change should guarantee proper length | |
| /// Calling this every time markerOpts change should guarantee proper length |
|
Thanks, good points! I'm just wondering if it would be more correct (at the expense of optimisation), to store an array of the previous set of markers or similar), check each one to see if its different, if so, update (maybe with an option of cache on/off). If we have a list of X markers (lets say they are taxis moving around a city) where the number of markers doesn't change, I'm not sure the markers would still read an old cached value and be wrong. This fixes additions to marker lists, but not mutations... Or is there a neat way to know if a List has been modified without having to traverse. I suspect this may be microoptimisation though, as I think the original problem was not having to recalculate the projections which is heavier. |
|
That is an interesting point. I'd probably merge this first and look at that later though, because (as you said) that is future optimisation. |
|
Yes, I think I agree. I'm just tied up today, but should get chance to redo the bits you suggested over the weekend. I may also just do a bit of benchmarking with a new option, see how much added time checking the whole list would be vs a length check or redoing all projections. |
Don't merge this without testing/checking. I'd like some thoughts first on whether we want an option to turn caching off, and if using map.var rather than widget.map.var is preferred or not.
There's recently been a crash on MarkerLayer where the pxCache and the Markers didn't match up, after adding a marker after a tap.
This is a suggestion that looks to prevent the crash from happening, making sure we update if the lengths mismatch. Also added the option to turn off pxCaching as it "could" get out of date I think. Default is on.
There may be a better solution.