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.

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