-
-
Notifications
You must be signed in to change notification settings - Fork 893
perf: preserve Marker widget state when list of visible markers changes
#2131
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
Conversation
|
I found out that m.key is null for all markers, So as soon as children of Stack changed they all rebuild. 2025-07-21.14-49-06.mp4So now with non null key and RepaintBoundary we preserve the state. 2025-07-21.14-50-08.mp4 |
|
Ahh. I missclicked and closed PR. So reopened. 2025-07-21.14-51-36.mp4Unlike before it was lagging as soon as any marker left the screen. 2025-07-21.14-52-57.mp4But need to figure out how to give it a stable and uniqe Key. If you have any ideas, welcome to contribute. |
|
Prehaps can add UniqueKey but then cannot make Marker const Marker({
Key? key,
required this.point,
required this.child,
this.width = 30,
this.height = 30,
this.alignment,
this.rotate,
}) : key = key ?? UniqueKey(); |
|
This is a nice fix, thanks. But I would first like to find out when this issue was introduced, since I don't remember it always being there. Is this ready for review? |
|
Hi. Not yet. Still need to figure out what key to use. Right now, the key is based on the marker’s location, but if two markers share the same location, it throws a “Duplicate keys found” exception. As I mentioned, I’m not sure what the best key would be in that case. One option might be to use a UniqueKey when creating each marker, which would avoid the duplicate issue, but then I wouldn’t be able to declare the Marker as const. That could reduce some of the performance optimizations Flutter provides. |
|
I don't know if it's possible, but that might suggest using keys isn't the best way around this. At the moment, we run a function/generator to return widgets. I guess So maybe we need to go a little more complex than For example, see #2134.
|
|
Ah yes ObjectKey can be used. #2134 looks good, Not sure I understand it all. I don't have much experiance with custom RenderObjects. |
|
@ReinisSprogis For the moment, #2134 is nice but low priority for me right now (I've got limited time for the next while). I think that's the "correct" way to do things - moreso the "right" way to support multi-worlds (as it just renders multiple times instead of building multiple times) - but it's a lot to figure out and I'm struggling with the rotation stuff & coordinate maths (as I mentioned there). It's close to working, but not there just yet. + research is needed into culling need. Therefore, for the time being, this might be worth perusing if you're still interested. Renaming stuff is not possible for the time being. If a user wants to overlay a widget more precisely, there's a plugin for that which we could consider incorporating in the future, but the marker API & usage is pretty standardised across mapping libraries and so doesn't need changing. But if using the |
Marker widget state when list of visible markers changes
|
Hi @JaffaKetchup. I am bit busy at the moment too. I can check on this some time next week. |
|
I don't know why, but seems like performance is overal reduced with this fix. But removes that lag when markers are culled. |
|
Hey, unfortunately I haven't had much time to review this in any detail and I'm away for a while soon, so I'm not sure when I'll get round to reviewing this, but I'll try not to leave it too long! |
JaffaKetchup
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ReinisSprogis, I'm really sorry for taking so long.
As you said, unfortunately I think this worsens the performance in most cases. I'm really not sure why this is - but it is significant. Could it be the RepaintBoundary?
I do think this idea could work well though.
|
Hi! @JaffaKetchup No worries. I have been busy myself. I too suspect it's RepaintBoundry. Not sure what can be done about it. Widgets are just not a good way to draw markers to be honest. Adding/removing thousands of widgets to widget, element and render tree I think is what slows it all down. The CircleLayer was not performant enough either. I ended up making a custom solution that covers all my needs. I use it in production as it is good enough, but there still are some bugs and things I would want to change if I publish as a package. But I have been bit hesitant to release it as package also because this puts a responsibility for little to no reward and I have not much experience with maintaining one. Maybe this could be the "reward". It is very hard to make a "one fits all" marker solution. But I think I am on the right track with this. You can check it out here. I uploaded as unlisted. https://www.youtube.com/watch?v=xLGqlNFpHOE |
|
Hey, I've had a look at the video, and yeah it does look like the UI thread is doing a lot better. The tiled display mode looks very interesting as well - that could definitely be a good option for some people. Although does it only support static markers? |
|
For the time being, I'm going to close this PR - but that's not to say we're not interested in improving the marker performance. I am wondering whether there's something simpler we can do to prevent this issue of lagging when markers are entering and exiting the viewport - at the moment we're using a generator because it was easy to implement, but I guess the problem is it has to regenerate all the existing markers for every new marker. Maybe the key is to use keys similar to this PR, but maybe not at the widget level. The polygon/line layers use the same approach as is currently used with markers, but I guess we don't see it there because there's usually fewer polygons? Or maybe I just need to have a dig. Anyway, thanks for this PR and working on this for your own project at least - and interesting to see where it's got you! Maybe once the modern tile layer is done, using tiles for things like this could be a little easier. |
|
Hi. You are correct. It is static markers only, unless there are only few hundred of them, so that it stays performant enough to "re-rasterize" marker animation or counter rotate. It was bit tricky to make it work, but nice thing is for many worlds it does not repaint multiple times, can just reuse same render from main world. It is possible to add or remove a single(or multiple) markers it just re-renders affected tiles, but cannot animate as it flickers if there are too many markers. And hit testing is also fast even with millions of markers. But yeah... that still need some work for public use. Meanwhile I decided to release it as a marker plugin for flutter_map. Nearly done. Just need finishing touches and understand all the package publishing thing. About this issue, when testing my plugin I found out that if I draw IconData in CustomPainter and call tex.layout() in it, the performance significantly degrades. Similarly to this. It could be why culling is causing lag with Icon when in example with just drawing painted Container it was ok. Maybe it calls .layout every time when culling is happening (or repaint) as it adds and remove widget from widget tree. I'm not sure how to resolve it though. |
Added ValueKey and RepaintBoundary to preserve Marker state. m.key was always null. However, might be that implementation can produce duplicate Keys if coordinates of the two markers are the same. Need a better sollution for key, but this is start.