[core] Fix collision with content insets

[astojilj]

Viewport center offset usage was wrongly submitted in #14664. It was part of alternative approach that used enlarged viewport. Existing and added tests were not sufficient to spot the regression, since the collision check padding is usually larger than the center offset x and y. Annotation picking has tolerance of only 10 pixels but no annotation integration test was using content insets.

Usage of offset is not needed because posMatrix in e.g. CollisionIndex::projectPoint(const mat4& posMatrix, const Point<float>& point) already incorporates center offset (projection matrix) and the code in current master was just offsetting all by the value.

Modified [ios] MGLAnnotationViewIntegrationTests.testSelectingAnnotationWithCenterOffset to use different insets. It verifies the fix.

Fixes [iOS] Annotations are not selectable (added via iosapp menu) #15106:

In case of #15106, view's original content insets is {top:88, bottom:34}, causing that center offset is {x:0, y:27} and selection with tolerance of 10 wouldn't select annotation.
After tapping the view, so that the header gets removed, view's content insets get changed to {top:44, bottom:34}, center offset is {x:0, y:5} and annotation selection would work, as described in #15106.

Fixes: #15106

[julianrex]

This is labelled as needs backport - why is this? /cc @chloekraw

[1ec5]

Does this fix have any visual impact on client code that remains unchanged but upgrades to include this fix? We should test this change against the iOS navigation SDK to ensure that it doesn’t cause any regressions. The navigation SDK (and applications that integrate it) are heavy users of content insets, particularly in landscape orientation and on CarPlay.

/cc @d-prukop

[chloekraw]

Did this issue also arise on @mapbox/maps-android?

[astojilj]

This is labelled as needs backport - why is this? /cc @chloekraw

@julianrex, @chloekraw,
I have added it because the regressions has the same cause (patch #14664) as the other needs backport patch (#15097).

[astojilj]

Did this issue also arise on @mapbox/maps-android?

@chloekraw,

Yes. Verified that without this patch, Android demo app doesn't show "Center map" annotation title when tapping the symbol. Screenshot taken with the fix integrated.

[astojilj]

Does this fix have any visual impact on client code that remains unchanged but upgrades to include this fix? We should test this change against the iOS navigation SDK to ensure that it doesn’t cause any regressions. The navigation SDK (and applications that integrate it) are heavy users of content insets, particularly in landscape orientation and on CarPlay.

/cc @d-prukop

@1ec5,
only visual impact, with current code without this patch, would be noticeable as items around borders not getting rendered. That would happen only if max(abs(rightPadding - leftPadding), abs(topPadding - bottomPading)) > 200.
This is because viewport rect clipping tolerance (padding) is 100px.

Other than that, it is about annotation selection: on annotation selection, collision rectangle dimensions are 10px. I don't know if we there are other selecting item on tap features, that could be affected.

[pozdnyakov]

LGTM

[friedbunny]

Thanks for digging further into this, @astojilj. I (somewhat unilaterally 😅) tagged this as a beta blocker, since it fixes a significant bug that we’ll need to address before the next release.

I’m concerned that existing tests in core and Android didn’t detect this, and I think this further reinforces the need for comprehensive tests to be written on iOS (#14827).

[julianrex]

This needs a change log.

Not fully tested.

[julianrex]

This fixes #15106, and integration tests pass (except for known, unrelated issues). I also tried additional tests from #15123 - which also passed.

However, we really need those additional tests! This also needs a change log entry.

[friedbunny]

Going to merge this so it can make ios-v5.2.0-beta.1 today, but I hope that we’re able to quickly prioritize #14827 soon. Will add a changelog entry in #15146.