Annotation tap gesture recognizer queries hard-coded region, ignoring image sizes

[jfirebaugh]

Tapping on a point annotation in the iOS test app seems to open a callout as expected in only 15% or so of taps.

[incanus]

Can you get more specific about how close nearby annotations are visually, what point you are tapping (as logged in debug) and how that compares to the annotation you were trying to tap?

Because annotations are in GL and aren't actual tappable views, we are querying a fuzzy region around the user's tap to try to guess which annotations they are referring to, as well as iterating overlapping annotations like MapKit does to enable selection of obscured markers.

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

Lines 993 to 1117 in 44cb91c

	- (void)handleSingleTapGesture:(UITapGestureRecognizer *)singleTap
	{
	if (singleTap.state == UIGestureRecognizerStateEnded)
	{
	[self trackGestureEvent:MGLEventGestureSingleTap forRecognizer:singleTap];
	CGPoint tapPoint = [singleTap locationInView:self];
	if (self.userLocationVisible && ! [self.selectedAnnotation isEqual:self.userLocation])
	{
	CGRect userLocationRect = CGRectMake(tapPoint.x - 15, tapPoint.y - 15, 30, 30);
	if (CGRectContainsPoint(userLocationRect, [self convertCoordinate:self.userLocation.coordinate toPointToView:self]))
	{
	[self selectAnnotation:self.userLocation animated:YES];
	return;
	}
	}
	// tolerances based on touch size & typical marker aspect ratio
	CGFloat toleranceWidth = 40;
	CGFloat toleranceHeight = 60;
	// setup a recognition area weighted 2/3 of the way above the point to account for average marker imagery
	CGRect tapRect = CGRectMake(tapPoint.x - toleranceWidth / 2, tapPoint.y - 2 * toleranceHeight / 3, toleranceWidth, toleranceHeight);
	CGPoint tapRectLowerLeft = CGPointMake(tapRect.origin.x, tapRect.origin.y + tapRect.size.height);
	CGPoint tapRectUpperLeft = CGPointMake(tapRect.origin.x, tapRect.origin.y);
	CGPoint tapRectUpperRight = CGPointMake(tapRect.origin.x + tapRect.size.width, tapRect.origin.y);
	CGPoint tapRectLowerRight = CGPointMake(tapRect.origin.x + tapRect.size.width, tapRect.origin.y + tapRect.size.height);
	// figure out what that means in coordinate space
	CLLocationCoordinate2D coordinate;
	mbgl::LatLngBounds tapBounds;
	coordinate = [self convertPoint:tapRectLowerLeft toCoordinateFromView:self];
	tapBounds.extend(coordinateToLatLng(coordinate));
	coordinate = [self convertPoint:tapRectUpperLeft toCoordinateFromView:self];
	tapBounds.extend(coordinateToLatLng(coordinate));
	coordinate = [self convertPoint:tapRectUpperRight toCoordinateFromView:self];
	tapBounds.extend(coordinateToLatLng(coordinate));
	coordinate = [self convertPoint:tapRectLowerRight toCoordinateFromView:self];
	tapBounds.extend(coordinateToLatLng(coordinate));
	// query for nearby annotations
	std::vector<uint32_t> nearbyAnnotations = _mbglMap->getAnnotationsInBounds(tapBounds);
	int32_t newSelectedAnnotationID = -1;
	if (nearbyAnnotations.size())
	{
	// there is at least one nearby annotation; select one
	//
	// first, sort for comparison and iteration
	std::sort(nearbyAnnotations.begin(), nearbyAnnotations.end());
	if (nearbyAnnotations == self.annotationsNearbyLastTap)
	{
	// the selection candidates haven't changed; cycle through them
	if (self.selectedAnnotation &&
	[[[self.annotationIDsByAnnotation objectForKey:self.selectedAnnotation]
	objectForKey:MGLAnnotationIDKey] unsignedIntValue] == self.annotationsNearbyLastTap.back())
	{
	// the selected annotation is the last in the set; cycle back to the first
	// note: this could be the selected annotation if only one in set
	newSelectedAnnotationID = self.annotationsNearbyLastTap.front();
	}
	else if (self.selectedAnnotation)
	{
	// otherwise increment the selection through the candidates
	uint32_t currentID = [[[self.annotationIDsByAnnotation objectForKey:self.selectedAnnotation] objectForKey:MGLAnnotationIDKey] unsignedIntValue];
	auto result = std::find(self.annotationsNearbyLastTap.begin(), self.annotationsNearbyLastTap.end(), currentID);
	auto distance = std::distance(self.annotationsNearbyLastTap.begin(), result);
	newSelectedAnnotationID = self.annotationsNearbyLastTap[distance + 1];
	}
	else
	{
	// no current selection; select the first one
	newSelectedAnnotationID = self.annotationsNearbyLastTap.front();
	}
	}
	else
	{
	// start tracking a new set of nearby annotations
	self.annotationsNearbyLastTap = nearbyAnnotations;
	// select the first one
	newSelectedAnnotationID = self.annotationsNearbyLastTap.front();
	}
	}
	else
	{
	// there are no nearby annotations; deselect if necessary
	newSelectedAnnotationID = -1;
	}
	if (newSelectedAnnotationID >= 0)
	{
	// find & select model object for selection
	NSEnumerator *enumerator = self.annotationIDsByAnnotation.keyEnumerator;
	while (id <MGLAnnotation> annotation = enumerator.nextObject)
	{
	if ([[[self.annotationIDsByAnnotation objectForKey:annotation] objectForKey:MGLAnnotationIDKey] integerValue] == newSelectedAnnotationID)
	{
	// only change selection status if not the currently selected annotation
	if ( ! [annotation isEqual:self.selectedAnnotation])
	{
	[self selectAnnotation:annotation animated:YES];
	}
	// either way, we should stop enumerating
	break;
	}
	}
	}
	else
	{
	// deselect any selected annotation
	if (self.selectedAnnotation) [self deselectAnnotation:self.selectedAnnotation animated:YES];
	}
	}
	}

In addition, this heuristic can vary depending on marker image size, as that affects the user's perceived tappable area while not changing the region query (though we do weight the region to account for top-heavy markers).

In short, this has felt good for months now, so I'm curious what changed and what the specific scenario is that is triggering this impression.

[incanus]

More background: #1037 (comment)

[jfirebaugh]

I'm testing on device using the Emerald style and "Add 100 Points", and zooming into an area where there is only one marker. It looks like the problem is that the hitbox for the marker is about half the size of the actual marker in Emerald, so only taps on the lower half of the marker image register.

[incanus]

This is probably a result then of:

1. Insufficient tap-testing with Emerald
2. Emerald's marker being so much bigger than Streets' (and Light/Dark's) - 86 vs. 50px high

Streets:

Emerald:

However, this highlights the difficult with the stopgap method of limiting marker imagery to the sprite sheet while simultaneously trying to minimize communications between the GL sprite parsing layer and the iOS code related to gestures.

Possible solutions:

• Tweak annotation gesture algorithms
• Come up with communications layer to better relate sprite sizing, but we will throw this away in b2 when we support runtime imagery (or will we?)
• Punt for b1, for Emerald
• Size down the Emerald marker imagery and document the limitation for users wanting their own sprite-based imagery /cc @peterqliu

[peterqliu]

I'm happy to size down the marker-- do we have a threshold in mind, both for the resizing and for the documentation?

[1ec5]

We’ve got a hardcoded 40×60 “tolerance” for tap recognition. It looks like the sprite is a bit taller than that.

(Eventually we’ll need to support markers of different sizes and enable MGLMapView to query for those sizes. That’ll really help make #1496 shine.)

[jfirebaugh]

I'm ok with punting on this for b1 for Emerald.

[incanus]

I'm ok with punting on this for b1 for Emerald.

I think this is an ok course of action. So: no further actions here for b1, assigning issue to b2.

[incanus]

Again, b3 will feature point annotation improvements.

[1ec5]

The situation may’ve improved a little at lower zoom levels due to #1827. TransformState::latLngForPixel() was being used by -[MGLMapMap handleSingleTapGesture:] to compute a bounds (centered around the tap) to pass into Map::getAnnotationsInBounds().

[friedbunny]

Fixed tap box blocks large/oversized markers, such as this one:

Red square being an approximation of the allowed tap box. Pisa@3x.png available here.

[1ec5]

MGLMapView:

• Hardcodes the bounding box around the touch point to search within
• Knows the dimensions of every custom marker image, but not the default marker sprite

mbgl::AnnotationManager:

• Searches for annotations that coincide with the bounding box
• Considers shapes to have area but does not consider point annotations to have area (it should)

mbgl::SpriteAtlas:

• Knows the dimensions of the default marker sprite

[incanus]

I think we have all the parts we need here, save a possible radius-based query for annotation proximity (assisted by #1694, I'm sure), then we can:

1. Register a tap gesture.
2. Query a suitable over-large radius for annotation candidates.
3. Iterate those candidates' real regions to determine final candidates.

This is relatively slow, but shouldn't be noticeable for reasonable annotation counts with regard to tap delays. I think?

[ljbade]

@incanus Now we have a fixed size default marker, as well as being able to directly know the size of custom markers from the bitmap, this should be implementable?

I'm going to take a stab on Android.

[ljbade]

@incanus I realised the hit box on Android had not been updated to match the size of the new default marker. Did this get updated on iOS?

[ljbade]

For Android #2876 is the first part, it at least adjusts current hit box to be default marker + 10 dp on each side.

Next will be to change getMarkersInBounds to use the Bitmap size of each marker in mMarkers to determine if they are in the hit box.

[1ec5]

Annotation images frequently have shadows or other ornamentation that shouldn’t be considered part of the hit target. The most natural way to handle this detail on iOS would be to set the image’s alignment rectangle; -[MGLMapView handleSingleTapGesture:] would then filter the results from getPointAnnotationsInBounds() by whether a given annotation’s image’s alignment rect intersects with the tap.

[1ec5]

The OS X port in #3135 has a fix for this issue, but porting the fix back to iOS is blocked by #3159.

[1ec5]

This will be fixed in #3261.