Mapbox-gl-native: Problems onMarkerClickListener

[nijs9]

*Platform:Android*
*Mapbox SDK version:6.0.1*

Steps to trigger behavior

1. Add marker to map
2. Attach setOnMarkerClickListener
3. Click marker

Expected behavior

onMarkerClickListener is fired

Actual behavior

Nothing happens on certain devices. I upgraded from the 5.2 version, but since the upgrade to 6.0.1, users with Huawai devices encounter problems when clicking a marker. Nothing happens when clicking a marker. I use this implmentation:

` map.setOnMarkerClickListener(new MapboxMap.OnMarkerClickListener() {
@Override
public boolean onMarkerClick(@NonNull Marker marker) {

                    if (marker.getTitle().equals("node")) {

                        Gson gson = new Gson();
                        Node clickedNode = gson.fromJson(marker.getSnippet(), Node.class);

                        showToast(getString(R.string.same_node));

                        } else {


                            clickedNodeSurpriseMe = clickedNode;
                            showSurpriseMeDialog("error");

                        }

                        return true;

                    }

            });`

[levchukAY]

I have the same problem.
Example: https://drive.google.com/file/d/1p1Rna984VPe07sR8Y6vFXOjdYN4k8TT_/view https://github.com/levchukAY/map-box-marker-sample
The issue is not actual for sdk version 4.

[LukasPaczos]

Thanks for reaching out and providing the examples, really appreciate that!
I was able to reproduce the issue, but it isn't related to marker clicks per say, rather mbgl::Renderer::queryRenderedFeatures which doesn't return any features for annotation layer if a Marker was removed and another was added.

Here is a reproducible example:

mapView.getMapAsync(new OnMapReadyCallback() {                                                      

  private Marker currentMarker;                                                                     

  @Override                                                                                         
  public void onMapReady(MapboxMap mapboxMap) {                                                     
    mapboxMap.addOnMapLongClickListener(point -> {                                                  
      if (currentMarker != null) {                                                                  
        mapboxMap.removeMarker(currentMarker);
        currentMarker = null;                                                                       
      } else {                                                                                      
        currentMarker = mapboxMap.addMarker(new MarkerOptions().position(point).title("My marker"));
      }                                                                                             
    });                                                                                             
    mapboxMap.addOnMapClickListener(point ->                                                        
      Toast.makeText(                                                                               
        SimpleMapActivity.this,                                                                     
        "Features: " + mapboxMap.queryRenderedFeatures(                                             
          new RectF(0, 0, mapView.getHeight(), mapView.getWidth()),                                 
          "com.mapbox.annotations.points")                                                          
          .size(),                                                                                  
        Toast.LENGTH_SHORT).show());                                                                
  }                                                                                                 
});                                                                                                 

[LukasPaczos]

Bisecting reveals that the culprit is https://github.com/mapbox/mapbox-gl-native/pull/10899/commits/7ce8588f4eff7c7f3e1b855e40e24947a2487ee4, this line to be precise. The issue happens only when Placement::updateLayerOpacities executes before Placement::placeLayer, which seems like a race condition, because it never happens when adding the first marker and can be prevented by pausing the GeometryTileWorker thread for a second before returning to the GL thread for any subsequent additions. All based on the reproducible example from https://github.com/mapbox/mapbox-gl-native/issues/11795#issuecomment-393251784.

@ChrisLoer or @ansis, would any of you mind taking this over?

[ChrisLoer]

Thanks for reaching out and providing the examples, really appreciate that!

Yes, thanks everyone for the clear reporting and examples!

I'm trying to get the reproduction case running (it usually takes me a while to get everything working on Android). While I work on that, two notes:

• This might be something partially addressed by the global symbol query changes that we just ported to release-boba but aren't included in 6.0.1 (https://github.com/mapbox/mapbox-gl-native/pull/11952).
• It's an architectural constraint of our symbol querying architecture that newly added symbols won't be included in queryRenderedSymbols results until the next global collision detection/placement happens (up to 300ms). I'm not sure if that explains what we're seeing here.

[ChrisLoer]

OK, I can reproduce locally on top of #11952, and I see now that it's reproducible over intervals much longer than 300ms. Investigating.

[ChrisLoer]

Here's what's causing the problem:

• The marker symbol is added before placement/collision detection can run, but because it allows overlap, it's added to the current placement with a "default opacity" of "show".
• Placement runs and the symbol is added to a new collision index, but no opacities change (placementChanged == false) and no new buckets were loaded during the current frame (symbolBucketsChanged == false) so the new placement isn't committed.
• queryRenderedSymbols runs against the old collision index, so it doesn't see the marker.

You'll notice the problem goes away if you're zoomed in and you move the map to trigger some sort of collision detection.

@ansis, the "don't commit unless opacity changes" logic keeps biting us in symbol querying. IIRC the reason you wanted to do it is that it saves us from a potentially unnecessary update of the opacity buffers. But the opacity updates are relatively cheap and this at most avoids one update every 300ms (and only on a map where nothing interesting is happening symbol-wise). We could add logic to prevent this particular problem, but it feels like it'll just make placement commit logic harder to follow -- I think I'd prefer to just remove the commit-skipping logic entirely.

[yang-owl]

Can we expect this to be fixed in 6.2?

[ChrisLoer]

Can we expect this to be fixed in 6.2?

This doesn't look like it will be a large fix, so I would expect that we get it in a patch or minor release pretty soon.

[ansis]

@ansis, the "don't commit unless opacity changes" logic keeps biting us in symbol querying.
...
I think I'd prefer to just remove the commit-skipping logic entirely.

Yep, this sounds like the right thing to do. It's not worth the complexity it adds. Should I open a pr or do you want to take this?

[ChrisLoer]

I confirmed this is fixed with @ansis's changes in #12076. I got thrown off at first because the box for the query geometry in the repro case didn't actually match the viewport:

 mapboxMap.queryRenderedFeatures(                                             
          new RectF(0, 0, mapView.getHeight(), mapView.getWidth()),                                 
          "com.mapbox.annotations.points")

Height and width are reversed there...

[mahyarv]

@ChrisLoer is there going to be a release with this fix included soon?

[LukasPaczos]

@mahyarv fix for this issue landed with https://github.com/mapbox/mapbox-gl-native/pull/12076 and will be available with v6.2.0 later this week.

[mahyarv]

@LukasPaczos any updates on the release timeline? We have updated to Mapbox 6 in order to use location layer plugin and this issue is blocking us from shipping a new version. Thanks!

[LukasPaczos]

Hey @mahyarv, v6.2.0-beta.3 is out and contains the fix for this issue, would you mind retesting?

[Dozer1170]

Updating my sdk version fixed this issue for me. Thanks!