remove delay argument from notifyMapChange

[jfirebaugh]

It shouldn't be the responsibility of the view implementation to set up a timer to resend the notification after a delay -- notifications should just be dispatched at the right time.

[incanus]

This is not a delay; it relates to the animation timeouts used. For example, if Cocoa tells the map to move on a 300ms animation, the Map/Transform are authoritative on whether that animation completed or not, and should send the completion notification at the end of the delay.

[incanus]

For context on the need for these, see callbacks like -[MKMapViewDelegate mapView:regionDidChangeAnimated:].

https://developer.apple.com/library/ios/documentation/MapKit/Reference/MKMapViewDelegate_Protocol/index.html#//apple_ref/occ/intfm/MKMapViewDelegate/mapView:regionDidChangeAnimated:

[jfirebaugh]

the Map / Transform are authoritative on whether that animation completed or not, and should send the completion notification at the end of the delay

They should be -- that's what I'm suggesting in this issue. But it doesn't look to me like that's the current behavior.

See, e.g.

mapbox-gl-native/src/mbgl/map/transform.cpp

Lines 286 to 289 in b4dbba9

	view.notifyMapChange(duration != std::chrono::steady_clock::duration::zero() ?
	MapChangeRegionDidChangeAnimated :
	MapChangeRegionDidChange,
	duration);

and

mapbox-gl-native/platform/ios/MGLMapView.mm

Lines 1629 to 1637 in b4dbba9

	if (delay != std::chrono::steady_clock::duration::zero())
	{
	dispatch_after(dispatch_time(DISPATCH_TIME_NOW, std::chrono::duration_cast<std::chrono::nanoseconds>(delay).count()), dispatch_get_main_queue(), ^
	{
	[nativeView performSelector:@selector(notifyMapChange:)
	withObject:@(change)
	afterDelay:0];
	});
	}

[jfirebaugh]

Also MGLMapView shouldn't need to dispatch notifyMapChange manually:

mapbox-gl-native/platform/ios/MGLMapView.mm

Lines 586 to 598 in b4dbba9

	if (duration)
	{
	self.animatingGesture = YES;
	__weak MGLMapView *weakSelf = self;
	[self animateWithDelay:duration animations:^
	{
	weakSelf.animatingGesture = NO;
	[weakSelf notifyMapChange:@(mbgl::MapChangeRegionDidChangeAnimated)];
	}];
	}

[incanus]

Also MGLMapView shouldn't need to dispatch notifyMapChange manually:

It does this because each platform is responsible for the velocity-based deceleration that feels right on that platform, which means differing durations.

[jfirebaugh]

Sure. But the point of this bug is: given an easing operation is started on Map with a particular duration, then Map should have sole responsibility for dispatching notifications of the progress of that easing operation.

Make sense?

[incanus]

Not just the easing duration, but also the timing curve, are done Cocoa-side. This isn't done as an atomic duration on the GL side, but rather a series of sub-durations.

mapbox-gl-native/platform/ios/MGLMapView.mm

Lines 570 to 584 in b4dbba9

	if ( ! CGPointEqualToPoint(velocity, CGPointZero))
	{
	CGFloat ease = 0.25;
	velocity.x = velocity.x * ease;
	velocity.y = velocity.y * ease;
	CGFloat speed = sqrt(velocity.x * velocity.x + velocity.y * velocity.y);
	CGFloat deceleration = 2500;
	duration = speed / (deceleration * ease);
	}
	CGPoint offset = CGPointMake(velocity.x * duration / 2, velocity.y * duration / 2);
	mbglMap->moveBy(offset.x, offset.y, secondsAsDuration(duration));

But you only want one notification.

I'm not saying this is the best/only way, but giving some context for why it is how it is now and why it might be the only way.

[jfirebaugh]

But you only want one notification.

If this is necessary, then the notifications have to be done entirely Cocoa-side, and the MBGLView::notifyMapChange implementation should be a no-op.

The current behavior is that each call to mbglMap->moveBy generates a MapChangeRegion[Will/Did]Change[Animated] notification pair, and then MGLMapView dispatches another MapChangeRegionDidChangeAnimated notification of its own.

[incanus]

the MBGLView::notifyMapChange implementation should be a no-op

FWIW, I introduced this whole system in order to start tapping into render lifecycle events to an extent that we've not been able to before, in order to get at things like -[MKMapViewDelegate mapViewDidFinishRenderingMap:fullyRendered:] that devs need in order to do detailed view interplay with maps.

https://developer.apple.com/library/ios/documentation/MapKit/Reference/MKMapViewDelegate_Protocol/index.html#//apple_ref/occ/intfm/MKMapViewDelegate/mapViewDidFinishRenderingMap:fullyRendered:

Perhaps using it for viewport changes as well is ill-advised. It's sounding like it.

[jfirebaugh]

But you only want one notification.

From the docs, it doesn't sound to me like this is true. Both - mapView:regionWillChangeAnimated: and - mapView:regionDidChangeAnimated: state:

During scrolling, this method may be called many times to report updates to the map position. Therefore, your implementation of this method should be as lightweight as possible to avoid affecting scrolling performance.

[incanus]

I meant one notification in the context of one per set of changes — if you make one move, you may get one notification out of MapKit, but for the equivalent change in GL, you might get a willChangeAnimated:YES because the system knows that you are kicking off an animation viewport change, then get a series of didChangeAnimated: with either YES or NO depending on how well you are tracking the kickoff notification and how the current one relates to it.

Additionally, our raster SDK improved upon Apple's implementation to make this more reliable for the user, posting only a single will and a single did notification (while still offering mapViewRegionDidChange for those who needed continuous updates), plus separated them out as to whether they were gesture- (examples: finger gestures, compass tap, or tracking mode change) or system-induced (e.g. updating user location tracking or a programmatic change related to change of another UI element).

We are basically walking a line somewhere between MapKit and up to / ideally including our raster SDK's behavior here.

[jfirebaugh]

while still offering mapViewRegionDidChange for those who needed continuous updates

I think you meant mapViewRegionIsChanging -- I didn't realize this existed, and now thing make more sense. So the intended series of events:

For an unanimated change:

• RegionWillChange
• RegionDidChange

For an animated change:

• RegionWillChangeAnimated
• RegionIsChanging (repeated)
• RegionDidChangeAnimated

[incanus]

Yeah, exactly — that flow captures it just right. These are things devs want (in addition to rendering start/finish and user interaction indication, which are out of scope here maybe) in order to sync up other UI elements.