Skip to content

Conversation

@ReinisSprogis
Copy link
Contributor

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.

@ReinisSprogis ReinisSprogis marked this pull request as draft July 21, 2025 11:58
@ReinisSprogis
Copy link
Contributor Author

I found out that m.key is null for all markers, So as soon as children of Stack changed they all rebuild.
You can see that in this video with highlite repaints how they all repaint.

2025-07-21.14-49-06.mp4

So now with non null key and RepaintBoundary we preserve the state.

2025-07-21.14-50-08.mp4

@ReinisSprogis
Copy link
Contributor Author

Ahh. I missclicked and closed PR. So reopened.
It looks to improve with this fix as there is no more of that jank on ui thread.

2025-07-21.14-51-36.mp4

Unlike before it was lagging as soon as any marker left the screen.

2025-07-21.14-52-57.mp4

But need to figure out how to give it a stable and uniqe Key. If you have any ideas, welcome to contribute.

@ReinisSprogis
Copy link
Contributor Author

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();

@JaffaKetchup
Copy link
Member

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?

@ReinisSprogis
Copy link
Contributor Author

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.

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Jul 22, 2025

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 Stack is smart enough somehow to avoid rebuilding itself and its children when its children don't 'change' (even though the generator does re-run on every frame of panning/zooming/etc.)? Or maybe that's completely wrong (I'm not sure how it would do that necessarily, given the identity of the returned list I think would change on every generation?).

So maybe we need to go a little more complex than Stack?


For example, see #2134.

  • I was originally intending to use it to remove the width & height of Markers, until I realised that was necessary for culling prior to build. But using a RO still means that each marker is built only once across multiple worlds, just rendered multiple times.
  • caveat: even here the RO docs say to use a key potentially - but we can use the Marker itself as the key value.
  • point: when testing right now with the many marker example, culling doesn't seem to be necessary because each marker seems to be built only once ever, not on each rebuild. Need to look into if this is stable in a variety of usage conditions. Maybe just because Marker is const, so an ObjectKey depending on it works correctly - and we also just list out the markers once in the example, then take slices? Idk: more research required.
  • so maybe it fixes this?
  • maybe: if it is indeed only building each marker once across a variety of usage situations (and we're not just lucky with how we've done it) - then maybe width/height for culling is indeed unnecessary and we can remove it?
  • it doesn't yet forward hit testing, this is tricky with how I've done it so far

@ReinisSprogis
Copy link
Contributor Author

Ah yes ObjectKey can be used. #2134 looks good, Not sure I understand it all. I don't have much experiance with custom RenderObjects.
Regarding culling, need to test and see whats better. Maybe checking if outside the screen and toggle visibility in Offstage widget https://api.flutter.dev/flutter/widgets/Offstage-class.html
I think It would be better to rename Marker to MapWidget and WidgetLayer (or similar) as it has nothing to do with Marker as such. If user want's to put marker as child, thats ok.

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Aug 7, 2025

@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 as the ObjectKey(...) works, then I think this could be a very good improvement.

@JaffaKetchup JaffaKetchup changed the title fix: Added value key and RepaintBoundary to preserve Marker state. perf: preserve Marker widget state when list of visible markers changes Aug 7, 2025
@ReinisSprogis
Copy link
Contributor Author

Hi @JaffaKetchup. I am bit busy at the moment too. I can check on this some time next week.

@ReinisSprogis
Copy link
Contributor Author

I don't know why, but seems like performance is overal reduced with this fix. But removes that lag when markers are culled.

@JaffaKetchup JaffaKetchup requested a review from a team September 11, 2025 13:41
@JaffaKetchup
Copy link
Member

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 JaffaKetchup marked this pull request as ready for review November 17, 2025 10:04
Copy link
Member

@JaffaKetchup JaffaKetchup left a 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.

@ReinisSprogis
Copy link
Contributor Author

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
It's a typical use case for me where I want a custom marker shape with Icon and I can dynamically change colors and icons. So I replicated it in flutter_map using Marker widget. This is huge amount of markers and not a typical use case, but might be useful as in like plane or ship tracker style maps. On Raster thread performance is similar but significantly better on UI thread. Towards the end you can see I will toggle on "magic" mode that takes performance to another planet :D. But it has some limitations and still needs work.

@JaffaKetchup
Copy link
Member

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?

@JaffaKetchup
Copy link
Member

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'd be very interested in seeing a plugin - as you said there is not much reward and it takes a lot of effort, but still sharing knowledge is never a bad thing. And ofc if there's something more similar to this PR which can be done (by which I mean changes on a smaller scale), we'd be more than happy to accept another PR or re-open this one.

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.

@ReinisSprogis
Copy link
Contributor Author

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.
Because it is hard to make it as one solution that fits all use cases I decided to split it into multiple different layers and marker types where user can chose what's best for their use case. In the first version I took out rasterized markers as I don't have full confidence in it's stability and compatibility with different map projections. First version will paint CustomPainter graphics on map. So that markers can be draw with Canvas. I tested it with the same setup as many markers demo, by drawing markers as IconData and it seems to perform well, doesn't have that culling lag, UI thread preforms much better but raster is about the same. And created some marker presets so it is easy to drop markers on map. There are some cool features, like zoom aware markers. Can increase marker complexity as zooming in as example. There still are thing I want to implement, but for now I just have to get it out as is and get some feedback, otherwise I never will.

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.

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

Successfully merging this pull request may close these issues.

2 participants