p-lr / MapCompose

A fast, memory efficient Jetpack Compose library to display tiled maps, with support for markers, paths, and rotation.
Apache License 2.0
220 stars 19 forks source link

Lazy loading reuses marker composables with the same ID #71

Closed TimPushkin closed 2 years ago

TimPushkin commented 2 years ago

When using RenderingStrategy.LazyLoading, if I add a marker, let it be displayed for a while, then remove it and add another marker with the same ID, the composable from the removed marker appears on the specified coordinates instead of the new one.

This doesn't happen in any of the following cases:

  1. If I use the default rendering strategy.
  2. If I add and remove the first marker immediately.
  3. If I add a delay between removing the old marker and adding the new one. The higher the delay, the less are the chances for this to happen, but it seems like it should be 10 ms at least.

I reproduced the bug in the demo app here -- on tap A should disappear and B should appear, but in reality, A appears again.

p-lr commented 2 years ago

From my testing, this also happens using the default rendering strategy. This is due to how the library considers a marker to be identical to another. Three parameters are compared: id, x, and y. If a marker with id "id" is removed then added using the same id and position, it's not considered a different marker, even though the composable function associated with it is different. The behavior you observed using a delay can also be explained. When there's a delay between the removal and the addition of B, the compose machinery has the time to see two different values for the list of markers. To my knowledge, there's no way to identify a composable function. Comparison must be done based on other parameters (here, the id and position).

My recommendation is: use a different id for the marker B. If needed, you could even ensure uniqueness for each newly created marker using UUID.randomUUID().

TimPushkin commented 2 years ago

Thanks for the explaination, but I'm experiencing this even when the markers have different coordinates. I actually intended to show this in the demo, but copy-pasted the coordinates accidentally. If you run the demo from this commit, you can see that A and B have different coordinates but A appears instead of B again.

p-lr commented 2 years ago

I could reproduce. For the moment, I can see that an assumption made in the lazy loading and clustering strategies is responsible for that particular behavior. I have a fix, which would make B appear on tap. However, I'm not comfortable fixing that particular case while still requiring the user to provide a different id when the position doesn't change. It simply isn't consistent. So I'm working on a solution which works even when the id does not change.

p-lr commented 2 years ago

I've created the branch try-fix-#71. Could you try it out and tell me if that also fixes issues for you?

TimPushkin commented 2 years ago

Yep, now it updates the composable even when the same coordinates are used, thanks.