Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Hit test Marker and MarkerViews #9424

Merged
merged 2 commits into from
Jul 7, 2017
Merged

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Jul 5, 2017

WIP, Closes #8159.

This PR adds hit testing to Markers and MarkerViews, we first query the map for nearby markers and on that result we perform rectangular hit testing. If we hit multiple annotation (eg. overlapping), we will make an intersection between the touch target and the bounds of the annotation to determine which annotation should be selected. This PR requires some re factoring before merging.

ezgif com-video-to-gif 61

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Jul 5, 2017
@tobrun tobrun self-assigned this Jul 5, 2017
@tobrun tobrun changed the title [android] - hit test Marker and MarkerViews Hit test Marker and MarkerViews Jul 5, 2017
@tobrun tobrun force-pushed the 8159-hit-testing-annotations branch from b2298c8 to 3847550 Compare July 6, 2017 13:12
@tobrun
Copy link
Member Author

tobrun commented Jul 6, 2017

Code has been re-factored (initial implementation was made for backporting that behaviour).
PR is ready for review.

Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really smart solution, well thought out 👍
Ni details

private boolean isClickHandledForMarker(long markerId) {
boolean handledDefaultClick;
Marker marker = (Marker) getAnnotation(markerId);
if (marker instanceof MarkerView) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we continue evolving AnnotationManager keeping both Markers and MarkerViews, we should think about implementing an Strategy pattern around Marker and MarkerView.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, would love to get in #6912 first as this will changes the whole setup around annotations (this makes 50% of our current java code around annotations obsolete) and would like to avoid doing twice the work.

this.projection = projection;
}

public long execute(MarkerHit markerHit) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should strive to keeppublic methods as clean as possible so they read like pseudocode (no conditionals, fors, etc.). What about extracting some private methods to make it simpler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with

    public long execute(MarkerHit markerHit) {
      resolveForMarkers(markerHit);
      return closestMarkerId;
    }

    private void resolveForMarkers(MarkerHit markerHit){
      for (Marker marker : markerHit.markers) {
        if (marker instanceof MarkerView) {
          resolveForMarkerView(markerHit, (MarkerView) marker);
        } else {
          resolveForMarker(markerHit, marker);
        }
      }
    }

bitmap = marker.getIcon().getBitmap();
hitRectMarker.set(0, 0, bitmap.getWidth(), bitmap.getHeight());
hitRectMarker.offsetTo(
markerLocation.x - bitmap.getWidth() / 2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about extracting these expressions into explaining variables? E.g. averageXXX

@tobrun tobrun force-pushed the 8159-hit-testing-annotations branch from 29cda9c to aaa5518 Compare July 7, 2017 08:57
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@tobrun tobrun merged commit 2876db7 into master Jul 7, 2017
@tobrun tobrun deleted the 8159-hit-testing-annotations branch July 7, 2017 10:28
@tobrun tobrun added this to the android-v5.1.1 milestone Jul 20, 2017
tobrun added a commit that referenced this pull request Jul 20, 2017
* [android] - hit test Marker and MarkerViews

* fixup
tobrun added a commit that referenced this pull request Jul 21, 2017
* [android] - hit test Marker and MarkerViews

* fixup
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants