flutter_map icon indicating copy to clipboard operation
flutter_map copied to clipboard

[BUG] MapState methods do not support rotation

Open Zverik opened this issue 3 years ago • 16 comments

While the _rotation field is stored in MapState, and recently we've got rotation support in MapController(#1115), core methods in MapState do not account for rotation anywhere. For example, layerPointToLatLng produces wrong results when the map is rotated.

Looks like fixing project and unproject would be enough.

Flutter_map 1.1.0.

Zverik avatar Jun 18 '22 14:06 Zverik

A project/unproject shouldn't take into account rotation.

However, in your case, sure you aren't looking for pointToLatLng which does into account rotation ?

ibrierley avatar Jun 18 '22 16:06 ibrierley

Well yes, I meant that if we're not changing these two methods, then we would need to change ~5 other, including pointToLatLng.

Zverik avatar Jun 18 '22 18:06 Zverik

I'd step back a bit and describe what you are trying to achieve and a minimal example. Not all methods should account for rotation, so it's hard to figure if there's any bug or not.

ibrierley avatar Jun 18 '22 22:06 ibrierley

@Zverik Any news? Please try and find a minimal reproducible example, that would help a lot :)

JaffaKetchup avatar Jul 18 '22 09:07 JaffaKetchup

I've looked over the MapState class, and these are the methods that I think need to take rotation into account:

  • getBoundsZoom() (used in getBoundsCenterZoom() and fitBounds(), otherwise bounds don't fit when the map is rotated).
  • getPixelBounds() (used in getBounds(), not sure about this, but might be incorrect when rotated).
  • layerPointToLatLng() (basically same as MapControllerImpl.pointToLatLng, but for plugins).

I've personally encountered issues coming from the first and the last item: fitBounds does not work properly when the map is rotated, and obviously layerPointToLatLng also fails. I wonder why the latter method is different for MapState and MapController.

Zverik avatar Jul 27 '22 06:07 Zverik

Yes, I think some of those may make sense for the bounds ones (or at least make sense to have an option there...it may depend if other code is making allowances elsewhere), needs a bit of digging.

I think the reason layerPoint is specifically called that, (rather than point), is probably that it's expected for it to be in layer coordinate space. I.e if the point came from a rotated layer, rotation shouldn't matter, and I suspect that would break (however as we only seem to use it directly on a center point, maybe that would work accidentally..I don't even know if we need it as it just does an unproject). However, we also have nonrotatedlayers now, just to add to the confusion there :D. I wonder if anyone actually uses that.

ibrierley avatar Jul 27 '22 08:07 ibrierley

What may be useful is to create a minimal typical example that would use these methods, that we can test against, to make sure it works (and any future changes).

ibrierley avatar Jul 27 '22 08:07 ibrierley

nonRotatedLayers and nonRotatedChildren are essential for things like the attribution widget, and I think some plugins make use of it.

JaffaKetchup avatar Jul 27 '22 09:07 JaffaKetchup

Well I made a non-rotated map plugin (this one if you're curious) that adds an element to the map from an on-screen interaction, so I needed to have the transformation :)

Zverik avatar Jul 27 '22 09:07 Zverik

And what about mapControllers pointToLatLng, does that not work ? As mentioned, a minimal example would be useful.

ibrierley avatar Jul 27 '22 09:07 ibrierley

Does a plugin layer have access to a map controller?

Zverik avatar Jul 27 '22 10:07 Zverik

Ok sure, I think that maybe makes more sense then. Some of the methods available there should also probably be exposed to MapState (and there's a few exposed that maybe shouldn't be as well...).

ibrierley avatar Jul 27 '22 14:07 ibrierley

Just out of interest, I've done a PR https://github.com/fleaflet/flutter_map/pull/1325 to add latLngToScreenPoint. However, at the same time I've enabled pointToLatLng to be accesed via state as well as mapController (which should take into account rotation). This could help with one of the issues.

ibrierley avatar Jul 29 '22 16:07 ibrierley

Is this still an issue?

JaffaKetchup avatar Aug 19 '22 15:08 JaffaKetchup

There are some elements, like bounds related ones I suspect are an issue, but there's a separate issue for that, so it could be worth closing this, and tracking the separate issues like that to avoid confusion. (I'm not sure layerPointToLatlng should include rotation as pointToLatLng does that, but maybe that should be a separate issue if necessary).

ibrierley avatar Aug 19 '22 15:08 ibrierley

Hi, I see that you are using Flutter_map 1.1.0. There were some things done with the 3.0.0 release to help better protect MapState accesses and ensure the state stays consistent. I would need to check but setting rotation directly in the MapState should correctly become calculated after setState, either that or you would need to call the rotate method which will run the projection calculations again before continuing. This might be part of the issue. before that the calculations weren't consistently being done and the order wasn't as consistent either.

mootw avatar Sep 10 '22 18:09 mootw

Hi @Zverik, can you try reproducing with the latest release? Many thanks!

JaffaKetchup avatar Oct 10 '22 20:10 JaffaKetchup

In response to this comment: https://github.com/fleaflet/flutter_map/issues/1283#issuecomment-1220824329, I'm going to close this for now. If this specifically is still an issue, please ping and I'll reopen :).

JaffaKetchup avatar Oct 10 '22 20:10 JaffaKetchup