Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Map gets rebuild with every frame while navigating #768

Closed
JohnnyRainbow81 opened this issue Nov 1, 2020 · 28 comments
Closed

Map gets rebuild with every frame while navigating #768

JohnnyRainbow81 opened this issue Nov 1, 2020 · 28 comments

Comments

@JohnnyRainbow81
Copy link

JohnnyRainbow81 commented Nov 1, 2020

As far as I understand flutter, widgets should only rebuild when configuration changes happen.

Maybe this is intentional here because of some internal, non visible magic of the leaflet map implementation. In this case, just let me know if this is wanted. Otherwise - what did I do wrong? My code example shows mostly a c/p version of the official example from https://pub.dev/packages/flutter_map.

The problem is: Rebuilding the FlutterMap also means its children rebuild, in my case, pretty expensive marker widgets, which I didn’t include in the example code (because they re not necessary to prove my point). The performance is quite laggy, so here’s my question:

Is the way it works now intentional or can I avoid the permanent rebuilding or is it a bug in the leaflet framework?

Thank you for your help!

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

class MyApp extends StatelessWidget {
  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
        visualDensity: VisualDensity.adaptivePlatformDensity,
      ),
      home: MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  MyHomePage({Key key, this.title}) : super(key: key);
  final String title;

  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {

  @override
  Widget build(BuildContext context) {
    return Scaffold(
        appBar: AppBar(
          title: Text(widget.title),
        ),
        body: LeafletMap());
  }
}

class LeafletMap extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    print("LeafletMap.build()");
    return FlutterMap(
      options: MapOptions(
        center: LatLng(51.5, -0.09),
        zoom: 13.0,
      ),
      layers: [
        TileLayerOptions(
            urlTemplate: "https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png", subdomains: ['a', 'b', 'c']),
        MarkerLayerOptions(
          markers: [
            Marker(
                width: 80.0, height: 80.0,
                point: new LatLng(51.5, -0.09),
                builder: (ctx) => MyWidget()),
          ],
        ),
      ],
    );
  }
}

class MyWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    print("MyWidget.build()");
    return FlutterLogo();
  }
}

@ibrierley
Copy link
Contributor

ibrierley commented Nov 1, 2020

I think maybe you need to be a bit more specific about what problem you are experiencing.

I tend to think of it more as 'widgets update when state updates', or you ask them to update. Obviously a simplification, I don't tend to think of 'config', but it may be we sort of mean the same thing.

So when there's a map change, there naturally has to be a widget rebuild somewhere (this may be the 'invisible' bit within flutter_map you mean), but I'm not really sure if we're talking about the same thing, or which widgets you are referencing.

So I'd try and add a bit more info, what problem you are experiencing, how you are testing that, and what you expect vs what you experience. What do you mean by 'wrong' in your case, what do you specifically mean by 'frame' here.

@JohnnyRainbow81
Copy link
Author

Hi ibrierley, thank you for the comment. I edited my post, hopefully my problem gets clearer now.

@ibrierley
Copy link
Contributor

I'm just guessing here a bit, so apologies in advance, just thinking out loud.

If there are any child widgets that don't need to get rebuilt, could you make them into const widgets ?
However, I suspect your widgets will need to get rebuilt, as you're talking about Markers. They will need to get repositioned on the screen won't they ?

@JohnnyRainbow81
Copy link
Author

JohnnyRainbow81 commented Nov 2, 2020

Hi,
Well - it's a bit complicated.

First: Yes, you are right. I can use const widgets to avoid rebuilding when navigating the map. This works to a certain degree even with the complex Marker widgets i want to use in my real code, which contain animation, pictures etc. I can navigate the map, the markers move along accordingly, show animation, everything's fine there. And the widgets only rebuild when i rebuild them with new data, which is what i want. Without having constructor values (like see below), I almost got it.

BUT: Since this only works with const widgets, i don't have the possibility to set the Marker widgets' const compile time constructor values, like the key and callback functions, which i really, really need. Please see the code:

for (int i = 0; i <= MAX_MARKERS_IN_SCREEN; i++) {
      Marker m = Marker(
          height: MARKER_HEIGHT,
          width: MARKER_WIDTH,
          point: LatLng(data[i].lat, data[i].lng), 
          builder: (ctx) {
            return const MarkerWidget(
                key: Key(constListOfConstStrings[i]), //Not possible, "i" cannot be const 
                onTappedCallback: widget.onTappedCallback); // "Invalid constant value"
          });
    }

In summary: I cannot initialize the const widget with the values it needs.

Probably this is not a too hard problem, but after messing around for a long time I cannot see it anymore...

Best regards

@Swepilot
Copy link

Swepilot commented Nov 2, 2020

I'm not a coding expert by any means so I might have misunderstood the problem, but have you looked at flutter_map_marker_popup https://github.com/rorystephenson/flutter_map_marker_popup

I'm using this to build markers at runtime. Perhaps that could give you a hint.

@maRci002
Copy link
Contributor

maRci002 commented Nov 2, 2020

Can u try #719 because as far as I remember I took care of that.

I mean in master (latest stable) version gestures.dart uses setState() every time in handleScaleUpdate however #719 gestures.dart uses another logic.

@JohnnyRainbow81
Copy link
Author

@Swepilot I tried the marker_popup and they have the same issue with rebuilding every frame when navigating the map.

@maRci002 thank you. I guess I'll try this now...

@JohnnyRainbow81
Copy link
Author

JohnnyRainbow81 commented Nov 2, 2020

@maRci002
I tried the example app containing your newest code https://github.com/maRci002/flutter_map/tree/feature/gesture-update. Unfortunately for my purposes, the example Marker widgets(FlutterLogos) still rebuild with every frame, so the 'problem' of slow performance with larger Marker widgets still exists.

The only possibility to avoid Marker rebuild per frame is to make the Markers' widget constant. This works fine considering performance, but leads me to the other problem, that i cannot initialize them properly:

//initializing in a for-loop...

Marker marker = Marker(
...
builder: (ctx) {
            return const MarkerWidget(
                key: Key(constListOfConstStrings[i]), //Not possible, "i" from for-loop cannot be const 
                onTappedCallback: widget.onTappedCallback); // "Invalid constant value" > can this callback-Function be made const anyhow?!
          });

Maybe I'm just too stupid to use const correctly. If you see my mistake here or you know a workaround, please let me know.

@ibrierley
Copy link
Contributor

What happens if you create your marker list outside of the build method (i.e in class LeafletMap) , and then just reference it later?
Eg MarkerLayerOptions( markers: myMarkerListFromEarlier )

@JohnnyRainbow81
Copy link
Author

JohnnyRainbow81 commented Nov 2, 2020

@ibrierley I already tried that and the problem of Marker widget rebuilding every frame stays the same.

(Edit) I could maybe get rid of the const-problem in the Key by initialising every Marker widget individually, which means, not in a loop, like:

Marker marker1 = Marker(... builder: (ctx) => const MarkerWidget(key: Key("1"), ....),
Marker marker2 = Marker(... builder: (ctx) => const MarkerWidget(key: Key("2"), ....)

This feels totally stupid, but would at least be a solution for the key.

But there would be still no solution for the callback functions i want to pass in the constructor. I want to give every MarkerWidget the same callback functions, but it doesn't seem to be possible to make functions == const...

@maRci002
Copy link
Contributor

maRci002 commented Nov 2, 2020

@JohnnyRainbow81 rebuilding the layer every frame while panning is the intended behaviour because we have to calculate the new position for the Marker, if we throttle the building time then you will see the marker stays at the same place while map is moving.

Anyway if you cache a Widget in a statefull widget then Flutter will respect that the Widget is the same instance so it won't rebuild it's subchildren. Here is how to do it manually.

#719 just prevents to rebuild the whole FlutterMap because it just updates the layers (the only exception when map is rotating)

@JohnnyRainbow81
Copy link
Author

JohnnyRainbow81 commented Nov 3, 2020

Hi @maRci002 ,
thank you for your explanation and your help.
(Edit) I just deleted this whole posting because of a big flaw in my state handling logic. I hope you didn't already read my post and invested time here, sorry :/

@Swepilot
Copy link

Swepilot commented Nov 3, 2020

How about putting _buildEmptyMarkers in initState instead of in the build method?

@JohnnyRainbow81
Copy link
Author

JohnnyRainbow81 commented Nov 3, 2020

I got it working now! Hooray 🚀 The MarkerWidgets only rebuild when their data changes and not when User is navigating on the map.

On the other hand, one little piece of the code i'm not able to understand 😆

In case you re interested:

MarkerWidget _buildEmptyMarkerWidget() {
    final MarkerWidget m = MarkerWidget(
        number: 1, key: Key("1"), onMarkerTappedCallback: widget.onMarkerTappedCallback); //Probably i don't need the key here anymore
    return m;
  }
 ...

@override
  Widget build(BuildContext context) {

     //Caching one marker (Why the hell is **only one(final!)** marker enough here?!)
    final markerWidget = _buildEmptyMarkerWidget(); 

    return Consumer<MarkerDatas>( //bulk of data arrives for several MarkerWidgets
      builder: (ctx, markerDatas, widget) => FlutterMap
        //other stuff...
         layers: [
                 //other stuff
                  MarkerLayerOptions(
                markers: markerDatas.items.map((data) { 
                  MarkerData markerData = MarkerData(data);  //"MarkerData" is a ChangeNotifier from Provider Package, wrapping data for the MarkerWidget. Necessary to let the data flow down only to the state of the MarkerWidget and no need to rebuild it like MarkerWidget(markerData) 
              return Marker( 
                  point: LatLng(
                    data.place.location.latitude,
                    data.place.location.longitude,
                  ),
                  width: MARKER_WIDTH,
                  height: MARKER_HEIGHT,
                  builder: (ctx) =>
                      ChangeNotifierProvider.value(value: markerData, child: markerWidget)); //MarkerWidget uses a Consumer<MarkerData> to receive new data only in it's state to avoid complete rebuilding of the widget when new data arrives
            }).toList()),
          ],
        );
}

(Edit) Forgot to obfuscate my code :D

@ibrierley
Copy link
Contributor

What bit aren't you able to understand ?

@JohnnyRainbow81
Copy link
Author

JohnnyRainbow81 commented Nov 3, 2020

@ibrierley
Why only one prebuild final Marker is sufficient to show many Markers simultaneously , each with a different state. Doesn't make sense to me yet. Flutter's intertwined widget logic (widgets<>elements<>state<>renderobject) sometimes produces hard to understand results.

@ibrierley
Copy link
Contributor

I'd have to test to be sure, but I "think", your final markerWidget isn't what you think, and has no state. So it's a bit like using an icon or whatever. What has the 'state' is the Marker().
I suspect if you swapped child:markerWidget for child:Icon(...) it would still work fine (I can't see what MarkerWidget is, so I may be talking rubbish :D)

@JohnnyRainbow81
Copy link
Author

@ibrierley The MarkerWidget seems to have state, because my map shows different markers with different data, but i cannot explain why 😃
Your comparison with Icons isn't accurate i'd say, because if you instantiate them like Icon(Icons.smiley), the Icon shows a constant smiley-icon. But my Markers show different data due to their different MarkerWidget-states.

@ibrierley
Copy link
Contributor

What is the different MarkerWidget state that you are seeing, what's different between them, can you make them different colours ? (as mentioned, it's difficult when you can't see the code :)), so it feels like it's slightly heavier work than it needs to be :).

@JohnnyRainbow81
Copy link
Author

Everything is different; the Markers' shape, Text, Images... It somehow works

@ibrierley
Copy link
Contributor

I think what isn't clear, is how/where you are setting those things (shape/text etc), as that's nowhere in the shown code, so feels like the important stuff you keep leaving out, dragging this out somewhat :). I'm a bit wary of spamming the git members here though, so if you want to include that code great, if not, maybe post a minimal example on Stackoverflow or similar.

@JohnnyRainbow81
Copy link
Author

I'd prefer to not post my real code here. But all the changing stuff( different data, images, shape,...) happens in the build()-method of the one final MarkerWidget.

To give you a glimpse:

class MarkerWidget extends StatefulWidget {
  final Key key;
  final Function onMarkerTappedCallback;

  const MarkerWidget({
    @required this.key,
    @required this.onMarkerTappedCallback,
  }) : super(key: key);

  @override
  _MarkerWidgetState createState() => _MarkerWidgetState();
}

class _MarkerWidgetState extends State<MarkerWidget> with TickerProviderStateMixin, AutomaticKeepAliveClientMixin<MarkerWidget> {

//much, much stuff

@override
  Widget build(BuildContext context) {
    super.build(context);
    return  Consumer<Data>(
        key: widget.key,
        builder: (ctx, markerData, ch) {

     //complex widget tree with animations, images etc
}

@ibrierley
Copy link
Contributor

Hmm I think I give up :D. You're now missing what is. There comes a point....

Just be wary of using ChangeNotifierProvider.value() earlier in your code. Eg https://pub.dev/documentation/provider/latest/provider/ChangeNotifierProvider-class.html

"To create a value, use the default constructor. Creating the instance inside build using ChangeNotifierProvider.value will lead to memory leaks and potentially undesired side-effects."

Now, this may be fine, it's getting quite complex, and can't see the code. I'm just leading you to the water, I can't make you drink it. Problem as I've mentioned is just posting bits of code all of the time, and never an actual solution, so you only ever get half baked answers (from me anyway).

I'll leave it there :D.

@JohnnyRainbow81
Copy link
Author

JohnnyRainbow81 commented Nov 3, 2020

Thank you for your time and your efforts:) I'm just happy to have performant markers now, even though it nags me a bit I don't fully get why it works.

Yeah, I know about potential memory leaks with ChangeNotifierProvider.value, but I think they only leak when you instantiate new Notifiers as a value (like ChangeNotifierProvider.value(value: MarkerData()), but here I instantiate them upfront in another place so the value parameter gets an already existing value.

Yes, it's a bit hairy, but I don't see another solution to have performant markers now.

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Mar 21, 2021
@github-actions
Copy link

This issue was closed because it has been stalled for 5 days with no activity.

@Pepslee
Copy link

Pepslee commented Dec 3, 2022

@JohnnyRainbow81
Can you share simple example of marker widget code how you solve the rebuild problem when User is navigationg on the map ?

"I got it working now! Hooray rocket The MarkerWidgets only rebuild when their data changes and not when User is navigating on the map."

@JohnnyRainbow81
Copy link
Author

JohnnyRainbow81 commented Dec 5, 2022

Hi @Pepslee ,
Well, It's been long I touched this project, but maybe a snippet of the build-method where the leaflet-map gets instantiated with its markerOptions might help you.
Unfortunately I cannot remember how this changeNotifierValue (from the Provider package I guess) works in detail and if / how it influences or solves the problem which is described in this thread. At least I left some comments back then. Haha.

Here's my untidy code snippet. I hope it helps you out somehow:

//...other widgets
return FlutterMap(
              mapController: _mapController,
              options: MapOptions(
                onLongPress: (tapPos, latLng) {
                  _onLongPress();
                },
                onTap: (tapPos, latLng){          
                  this.widget.onMapTappedCallback();
                } ,
                controller: _mapController,
                onPositionChanged: (mapPosition, hasGesture) {
                  _center = mapPosition.center;
                  _mapBounds = mapPosition.bounds;
                  _zoom = mapPosition.zoom;
                },
                center: _center,
                zoom: 13,
                maxZoom: 23,
              ),
              layers: [
                TileLayerOptions(
                    maxZoom: 23,
                    urlTemplate: 
                        "http://MY_LEAFLET_URL"?access_token={accessToken}",
                    additionalOptions: {
                      'accessToken':
                          'pk.MY_TOKENxyz1234' /*subdomains: ['a', 'b', 'c']*/,
                      'id': 'mapbox.mapbox-streets-v8',
                    }),
                MarkerLayerOptions(
                    markers: bubbleDatas.items.map((data) {
                  //make (bubbleData and) changeNotifier final to avoid permanent rebuilding per frame
                  final changeNotifier =
                      ChangeNotifierProvider.value(value: data, child: bubbleWidget);
                  return Marker(
                      point: LatLng(
                        data.place.location.latitude,
                        data.place.location.longitude,
                      ),
                      width: BUBBLE_WIDTH,
                      height: BUBBLE_HEIGHT,
                      anchorPos: _anchorPos(data.direction),
                      //hack to avoid permanent rebuilding per frame so
                      //that the bubblewidget(-state) only gets rebuild and not the whole widget
                      builder: (ctx) => changeNotifier);
                }).toList()),
              ],
            );
        
    

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants