[iOS - Android] visibleFeaturesAtPoint

[Etienne-io]

Platform:iOS & Android
Mapbox SDK version4.0.0 & 6.0.1:

First, I am sorry cause I can't recreate a simple project that reproduce my issue. I promise that I will keep trying.

I want to precise that I did exactly the same on 3.7 and I don't have any issue with.

Here is what I do :

I create a symbol layer and a symbol source to display POI. In some case, I have to display one thousand of point on a small area.

Thanks to collision, some of my point are not displayed.

User can search for a specific POI. If the user search for a POI that is hidden by collision, I have to "promote" this POI to make it visible.

To promote a POI, I recreate the datasource and I add the promoted POI at start of the features array in order to be sure that collision won't hide it. This is working as expected.

User can click on a POI to select it. When he do it, I do the same promote process to be sure that the POI keep displaying if the user zoom out.

Here come the issue.

I click on a POI. The POI is promote so my symbol data source is updated with my promoted POI in first position.

Now, if I click on another POI, the feature returned by visibleFeaturesAtPoint is not the feature that I click on. (The returned feature may be outside of my screen)

I tried a lot of thing and can't figure out how this happens.

Visually, this happens :

I click on the orange cross on the left. It is selected (and promoted)

Then I click on the brown POI under it on the right, and it's the restroom on the other side of the map that is selected.

To finish, I click again on the brown POI and it correctly selected :

Again, I am sorry to not be able to give you a simple example to reproduce, maybe, someone will have an idea about what happens here.

Etienne

[Etienne-io]

I found something that may help.

Considering an array of feature like :

[ 0, 1, 2, 3, 4, 5 ]

if I promote the feature 5, I obtain the following array :

[ 5, 0, 1, 2, 3, 4 ]

now, if i click on the feature 3, visibleFeaturesAtPoint returns the feature 2.

In the original array, feature 3 is at index 4

In the second array, feature 2 is at index 4

This happens very often with a large number of POI. It happens less with a small number of POI but I can reproduce it with 10 POI and a bit of patience.

[Etienne-io]

I reproduce the same on Android

[asheemmamoowala]

if I promote the feature 5

@emerciercontexeo Can you elaborate (with code preferable) what you are doing to promote a feature. When updating source data, the underlying data structures for visibleFeaturesAtPoint are updated asynchronously and may return different results.

[Etienne-io]

I am working on a small project to repoduce. I will share it with you as soon as possible.

To "promote", I just put the selected feature at the first index of my features array, rebuild NSData and assign it to my source.shape.

[Etienne-io]

https://github.com/emerciercontexeo/QueryVenueLayerIssue/tree/master

(You have to set a mapbox access token to make it works)

Here is an example that reproduce the issue.

Sometime, it works as expected. Most of the time the issue happens.

[julianrex]

Thanks @emerciercontexeo - hopefully this will help us narrow down the issue!

[ChrisLoer]

Hi @emerciercontexeo, I work on the core GL team and @julianrex pointed me towards this issue. I just opened up your project and started looking around. I'm still investigating the timing behavior where the annotation shows up in a different location from what we expect, but I saw you had a comment in the project's README saying: "Symbol with longitude 0.0 produces wrong collision behaviour", and I can at least explain that.

What's happening there is that the symbol with longitude 0.0 is just across a tile boundary from the other symbols with longitude >0. Symbols get placed in tile order first, and then their feature order controls collision detection priority within the tile. So for instance with that "0" symbol, you can always make it show while zoomed out by rotating the map so that the tile it's part of shows up on top.

This behavior changed with "global collision detection" in #10436. Before that (ie before 4.0/6.0), collision detection was done independently for each tile, with a "buffer" of data from nearby tiles pulled in. That buffer is what would have allowed this to work before -- symbol "0" was in the buffer for the tile next to it, so it's early position in the feature order could be counted. The change to global collision detection involved a lot of architectural trade-offs, and as part of that we had to make some compromises about what happened with collision detection and rendering at tile boundaries. I'll have to think about this some more, but I don't think there's an easy way to go back to the old behavior implicitly based on feature order within a source -- we'd have to instead add some sort of explicit "collision detection ordering" control. cc @ansis

Sometime, it works as expected. Most of the time the issue happens.

With your project I get the expected behavior most of the time, but occasionally I see what you describe. I don't have an explanation yet for what you're seeing, but something to keep in mind is that visibleFeaturesAtPoint can be working off potentially old data -- it's asking what was at that point the last time collision detection ran, which won't necessarily reflect what's in the latest data you just sent on the shape source. That said, it looks like it's just trying to get a lat/lng coordinate from what was there before, so it shouldn't care about the re-ordering you're doing (?)... 🤔

[ChrisLoer]

@julianrex It would be a good idea for us to try reproducing this using a build that includes #11742 (it went into release-boba but wasn't in the 4.0.0 release). Although I don't know of a bug that would cause this behavior in the previous behavior, the point of that PR was in part to make it easier to reason about query results in just this kind of race-condition situation.

[Etienne-io]

@ChrisLoer Thanks for the explanation about the longitude 0.0. I wrote it in my readme but I didn't want to open an issue about it. I think it is a low impact behaviour in a real project and I can imagine that you have to make choices to keep performance and behaviour at a high level.

About the visibleFeaturesAtPoint :
I suppose that changing the source data run the collision detection process. With this supposition, data should always be up-to-date.

Unfortunately, I don't have enough time to go deeper in the source of Mapbox Core to find what happened between 3.7/5.5 and 4.0/6.0.
I will try to look at it on my free time but I can't promise anything.

I precise that I am sure that never happens before the last major update of the Core.

In another hand, maybe we could think about a feature that allow developer to set a "collision priority" dynamically to each feature.

[lagst3r]

Hi all,

I am facing the same issue. In my case, I have only 1 marker on the map. When selecting the marker on the map, visibleAnnotationsAtPoint and visibleFeaturesAtPoint returns nil. It only works when I have zoom in the map closer before it returns the marker.

I am working on the iOS version and have rolled back to SDK v3.7.8 and it works perfectly fine. Hope there is a resolution to the issue or a work around for v4.0.x.

[ChrisLoer]

@julianrex It would be a good idea for us to try reproducing this using a build that includes #11742 (it went into release-boba but wasn't in the 4.0.0 release

I just reproduced post #11742, investigating further...

[ChrisLoer]

🤦‍♂️ I built the wrong version of the framework to test #11742. Turns out this is already fixed in master. The bug was subtle and took a long time to figure out:

mapbox-gl-native/src/mbgl/renderer/renderer_impl.cpp

Lines 393 to 411 in c019153

	if (!placement->stillRecent(parameters.timePoint)) {
	auto newPlacement = std::make_unique<Placement>(parameters.state, parameters.mapMode);
	std::set<std::string> usedSymbolLayers;
	for (auto it = order.rbegin(); it != order.rend(); ++it) {
	if (it->layer.is<RenderSymbolLayer>()) {
	usedSymbolLayers.insert(it->layer.getID());
	newPlacement->placeLayer(*it->layer.as<RenderSymbolLayer>(), parameters.projMatrix, parameters.debugOptions & MapDebugOptions::Collision);
	}
	}
	placementChanged = newPlacement->commit(*placement, parameters.timePoint);
	// commitFeatureIndexes depends on the assumption that no new FeatureIndex has been loaded since placement
	// started. If we violate this assumption, then we need to either make CollisionIndex completely independendent of
	// FeatureIndex, or find a way for its entries to point to multiple FeatureIndexes.
	commitFeatureIndexes();
	crossTileSymbolIndex.pruneUnusedLayers(usedSymbolLayers);
	if (placementChanged || symbolBucketsChanged) {
	placement = std::move(newPlacement);
	}

Notice the comment "commitFeatureIndexes depends on the assumption that no new FeatureIndex has been loaded since placement started". The logic above is supposed to synchronously run a full placement, and then the assumption was that committing that new placement would update the CollisionIndex to match all of the latest data committed by commitFeatureIndexes.

The problem is that it's possible for symbolBucketsChanged to be false because the new buckets were added on a frame that wasn't doing placement, and no new buckets have been changed since then, and also possible for placementChanged to be false because there was no change in opacity from the last placement. In that case, we skip the final CollisionIndex commit that happens with placement = std::move(newPlacement); . We knew this logic was confusing and hard to audit and that's part of why we did #11742, but it still took two days to figure out what was going on here. 😓

Good news is this is already fixed! 😄 We just need to get it into a patch release.