Skip to content
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

[BUG] MapState methods do not support rotation #1283

Closed
Zverik opened this issue Jun 18, 2022 · 18 comments
Closed

[BUG] MapState methods do not support rotation #1283

Zverik opened this issue Jun 18, 2022 · 18 comments
Labels
bug This issue reports broken functionality or another error

Comments

@Zverik
Copy link
Contributor

Zverik commented Jun 18, 2022

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 Zverik added bug This issue reports broken functionality or another error needs triage This new bug report needs reproducing and prioritizing labels Jun 18, 2022
@ibrierley
Copy link
Contributor

ibrierley commented Jun 18, 2022

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 ?

@Zverik
Copy link
Contributor Author

Zverik commented Jun 18, 2022

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

@ibrierley
Copy link
Contributor

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.

@JaffaKetchup
Copy link
Member

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

@Zverik
Copy link
Contributor Author

Zverik commented Jul 27, 2022

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.

@ibrierley
Copy link
Contributor

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
Copy link
Contributor

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).

@JaffaKetchup
Copy link
Member

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

@Zverik
Copy link
Contributor Author

Zverik commented Jul 27, 2022

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 :)

@ibrierley
Copy link
Contributor

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

@Zverik
Copy link
Contributor Author

Zverik commented Jul 27, 2022

Does a plugin layer have access to a map controller?

@ibrierley
Copy link
Contributor

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
Copy link
Contributor

Just out of interest, I've done a PR #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.

@JaffaKetchup
Copy link
Member

Is this still an issue?

@ibrierley
Copy link
Contributor

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).

@mootw
Copy link
Contributor

mootw commented Sep 10, 2022

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.

@JaffaKetchup
Copy link
Member

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

@JaffaKetchup
Copy link
Member

In response to this comment: #1283 (comment), I'm going to close this for now. If this specifically is still an issue, please ping and I'll reopen :).

@JaffaKetchup JaffaKetchup added non-fatal and removed needs triage This new bug report needs reproducing and prioritizing labels Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue reports broken functionality or another error
Projects
None yet
Development

No branches or pull requests

4 participants