Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Xamarin.Forms.Map CameraChanged event #7260

Closed
wants to merge 10 commits into from

Conversation

roubachof
Copy link

@roubachof roubachof commented Aug 23, 2019

Description of Change

Added a "camera changed" event to be notified when the maps is moved or zoomed. With the proper CameraChangedEventArgs and the Camera property, it allows developers to add customs overlays with SkiaSharp, or even hide or show some controls while the map is changing.

Issues Resolved

Collaterally fixes an issue on Android.

API Changes

Added:

  • event EventHandler<CameraChangedEventArgs> CameraChanged;
  • Camera Camera { get; }

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP

Behavioral/Visual Changes

None.

Before/After Screenshots

Not applicable.

Testing Procedure

I tested the 3 platforms with the RunAway! app:

https://github.com/roubachof/SkiaSharpnado/tree/xamarin-maps

in the SessionMap.xaml.cs (naming is bad :):

GoogleMap.CameraChanged += GoogleMapCameraChanged;

private void GoogleMapCameraChanged(object sender, CameraChangedEventArgs e)
{
    Debug.WriteLine($"CameraChanged: pos: {e.Camera.Position.Latitude}, {e.Camera.Position.Longitude}");

    if (!_isCameraInitialized)
    {
        _isCameraInitialized = true;
    }

    if (_drawingCount == 0)
    {
        MapOverlay.InvalidateSurface();
    }
}

Then checked that I can indeed move and zoom the map and the overlay is updated accordingly.

image

image

image

Platforms specific comments

Core

I created a Camera class with Zoom and Position to represent the current view photo settings.

For the Camera property, I wasn't sure how to propagate a change from the renderer to the core class for a read only property.
I reproduced the same pattern that the VisibleRegion property.

Android

I added some code in the OnLayout method to return if the map size is 0, since the MoveToRegion call will fail if it's the case.

iOS

MkMapView has no Zoom property, so I had to implement its own zoom.

UWP

The zoom given by the MapControl is not consistent with the Google Maps and iOS custom zooms. To have the same behavior as the others platforms, I had to compute it manually (it uses the same code as the iOS one).

I added a OnSizeChanged delegate for the initial Camera assignement. It fixes also a scenario when MoveToRegion is called before the layout pass is made, and then the region set by the user is not taken into account.

Issues to solve on UWP before merge

  1. The zoom computation on uwp seems to be a little unaccurate, I don't really understand why. I would need a Bing maps expert for this :)

image

  1. The ActualCameraChanging event seems to be aggressively invoked and seems to take a lot of UI thread time. I tried numerous tactics without much success... It works but it is a bit laggy in my sharpnado example.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

@samhouts samhouts added this to In Review in v4.3.0 Aug 23, 2019
@jsuarezruiz jsuarezruiz added a/maps 🌐 DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. labels Aug 23, 2019
@samhouts samhouts moved this from In Review to In Progress in v4.3.0 Aug 23, 2019
@roubachof roubachof changed the title [Do not merge] Xamarin.Forms.Map CameraChanged event Xamarin.Forms.Map CameraChanged event Aug 26, 2019
@samhouts samhouts added this to In Progress in v4.4.0 Aug 30, 2019
@samhouts samhouts removed this from In Progress in v4.3.0 Sep 3, 2019
# Conflicts:
#	Xamarin.Forms.Maps.Android/MapRenderer.cs
#	Xamarin.Forms.Maps.iOS/MapRenderer.cs
@samhouts
Copy link
Member

samhouts commented Oct 7, 2019

@roubachof What's the status of this PR? Your last comment mentions that there is still work to be done. Is that still the case? Thanks!

@roubachof
Copy link
Author

@samhouts It's fine for iOS and Android, but I would need some help of someone from the uwp team maybe to solve the uwp issues I met... Maybe I should split the uwp version out so you could merge the iOS and Android version?

@samhouts samhouts added the API-change Heads-up to reviewers that this PR may contain an API change label Oct 8, 2019
@samhouts
Copy link
Member

samhouts commented Oct 8, 2019

@roubachof That sounds like a good idea! Thanks!

@roubachof
Copy link
Author

@samhouts removed the UWP implementation.

@samhouts samhouts added this to In Progress in v4.5.0 Nov 2, 2019
@samhouts samhouts removed this from In Progress in v4.4.0 Nov 2, 2019
@samhouts samhouts added p/Android p/iOS 🍎 and removed DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. labels Jan 15, 2020
@samhouts samhouts added this to In Review in v4.6.0 Jan 16, 2020
@samhouts samhouts removed this from In Progress in v4.5.0 Feb 13, 2020
@samhouts samhouts removed this from In Review in v4.6.0 Mar 2, 2020
@samhouts samhouts added this to In Review in vCurrent (4.8.0) Mar 10, 2020
@AlonRom
Copy link

AlonRom commented May 10, 2020

Hi, @samhouts what is the status about this merge?

@nbsoftware
Copy link

Hello.
What's the status of this one?
I'm waiting on a fix for #1852

@AlonRom
Copy link

AlonRom commented Jun 22, 2020

@samhouts @hartez Hi, any status regarding this would be great. thanks

@samhouts samhouts changed the base branch from master to main June 26, 2020 00:15
@baneageorge
Copy link

Hi, any chance to have some progress on this?

@samhouts samhouts added this to In Review in .NET MAUI Backlog Jul 30, 2020
@samhouts samhouts removed this from In Review in vCurrent (4.8.0) Jul 30, 2020
@jfversluis
Copy link
Member

Closing this as we won't be adding anything new to Forms. Adding a tag to put it on the list to look into this for .NET MAUI.

@jfversluis jfversluis closed this Sep 6, 2021
.NET MAUI Backlog automation moved this from In Review to Closed Sep 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

Map renderer giving 0 size with MoveToRegion
9 participants