CameraUpdateFactory.newLatLngBounds

[tobrun]

Currently not yet implemented:

    /**
     * Returns a CameraUpdate that transforms the camera such that the specified latitude/longitude
     * bounds are centered on screen at the greatest possible zoom level.
     * You can specify padding, in order to inset the bounding box from the map view's edges.
     * The returned CameraUpdate has a bearing of 0 and a tilt of 0.
     *
     * @param bounds
     * @param padding
     * @return
     */
    public static CameraUpdate newLatLngBounds(@NonNull LatLngBounds bounds, int padding) {
        throw new UnsupportedOperationException("Not implemented yet");
    }

    /**
     * Returns a CameraUpdate that transforms the camera such that the specified latitude/longitude
     * bounds are centered on screen within a bounding box of specified dimensions at the greatest
     * possible zoom level. You can specify additional padding, to further restrict the size of
     * the bounding box. The returned CameraUpdate has a bearing of 0 and a tilt of 0.
     *
     * @param bounds
     * @param width
     * @param height
     * @param padding
     * @return
     */
    public static CameraUpdate newLatLngBounds(@NonNull LatLngBounds bounds, int width, int height, int padding) {
        throw new UnsupportedOperationException("Not implemented yet");
    }

We also need to expose one extra for setting content insets as documented in #3583

• Write unit tests for LatLngBounds and LatLng
• Revisit old setVisibleCoordinateBounds and see how we can hook this up with camera api
• Add new CameraUpdate type
• Investigate/patch setVisibleCoordinateBounds directly after onCreate fails #2719 while I'm at it
• make it work with camera api
• refactor camera api to correctly use Factory pattern
• Write unit tests for MapboxMap integration
  • one does not simply write test for something that uses MapView directly.
• Remove old methods + model classes in java
• Remove obsolete methods in jni/c++
  • will be handled in Cleanup MapView and corresponding jni #3791

[tobrun]

While working on the first item, I was able to bring up LatLng code coverage from :

to:

I'm not able to hit 💯 , because of the Parceable interface. This can only be tested on a real device or using a Mockito mock. I might look into the latter sometime. For now continuing with unit tests for LatLngBounds

[tobrun]

While implementing test cases, I have simplified LatLngBounds. We now enforce the usage of a Builder pattern. This will make the API more clear and this will do the heavy work for end developers (calculate NorthEastLatLng and SouthWestLatLng). Also this is in parity with Google.

I was able to get a coverage of:

Not hitting 💯 because of Parcel issue from above.

The Builder pattern mentioned above will look like:

 LatLngBounds bounds = new LatLngBounds.Builder()
             .include(NEW_YORK)
             .include(LOS_ANGELES)
             .build();

[tobrun]

While revisiting the old setVisibleCoordinates method in jni.cpp:

void JNICALL nativeSetVisibleCoordinateBounds(JNIEnv *env, jobject obj, jlong nativeMapViewPtr,
        jobjectArray coordinates, jobject padding, jdouble direction, jlong duration) {
    mbgl::Log::Debug(mbgl::Event::JNI, "nativeSetVisibleCoordinateBounds");
    assert(nativeMapViewPtr != 0);
    NativeMapView *nativeMapView = reinterpret_cast<NativeMapView *>(nativeMapViewPtr);

    jfloat left = env->GetFloatField(padding, rectFLeftId);
    if (env->ExceptionCheck()) {
        env->ExceptionDescribe();
        return;
    }

    jfloat right = env->GetFloatField(padding, rectFRightId);
    if (env->ExceptionCheck()) {
        env->ExceptionDescribe();
        return;
    }

    jfloat top = env->GetFloatField(padding, rectFTopId);
    if (env->ExceptionCheck()) {
        env->ExceptionDescribe();
        return;
    }

    jfloat bottom = env->GetFloatField(padding, rectFBottomId);
    if (env->ExceptionCheck()) {
        env->ExceptionDescribe();
        return;
    }

    jsize count = env->GetArrayLength(coordinates);

    mbgl::EdgeInsets mbglInsets = {top, left, bottom, right};
    mbgl::AnnotationSegment segment;
    segment.reserve(count);

    for (int i = 0; i < count; i++) {
        jobject latLng = reinterpret_cast<jobject>(env->GetObjectArrayElement(coordinates, i));
        jdouble latitude = env->GetDoubleField(latLng, latLngLatitudeId);
        if (env->ExceptionCheck()) {
            env->ExceptionDescribe();
            return;
        }
        jdouble longitude = env->GetDoubleField(latLng, latLngLongitudeId);
        if (env->ExceptionCheck()) {
            env->ExceptionDescribe();
            return;
        }
        segment.push_back(mbgl::LatLng(latitude, longitude));
    }

    mbgl::CameraOptions cameraOptions = nativeMapView->getMap().cameraForLatLngs(segment, mbglInsets);
    if (direction >= 0) {
        // convert from degrees to radians
        cameraOptions.angle = (-direction * M_PI) / 180;
    }
    mbgl::AnimationOptions animationOptions;
    if (duration > 0) {
        animationOptions.duration.emplace(mbgl::Milliseconds(duration));
        // equivalent to kCAMediaTimingFunctionDefault in iOS
        animationOptions.easing = mbgl::util::UnitBezier(0.25, 0.1, 0.25, 0.1);
    }

    nativeMapView->getMap().easeTo(cameraOptions, animationOptions);
}

I'm noticing that it's already using the easeTo from the camera api.

    nativeMapView->getMap().easeTo(cameraOptions, animationOptions);

We need to fix this that we can use all the different ones defined in MapboxMap:

• move
• ease
• animate

We also need to fit this in nicely with a call coming from MapboxMap.

[tobrun]

I was able to resolve #2719, by pushing our OnMapReadyCallback at the end of the message queue:

    /**
     * Sets a callback object which will be triggered when the {@link MapboxMap} instance is ready to be used.
     *
     * @param callback The callback object that will be triggered when the map is ready to be used.
     */
    @UiThread
    public void getMapAsync(@NonNull final OnMapReadyCallback callback) {

        // We need to put our callback on the message queue
        post(new Runnable() {
            @Override
            public void run() {
                callback.onMapReady(mMapboxMap);
            }
        });
    }

Now out the Async in getMapAsync is starting to make more sense.

[tobrun]

Currently I have a working version, but it is not updating the state of CameraPosition. To have this I will have to extract some code form the original setVisibleCoordinateBounds and use that in java instead. So all the methods can use the same moveTo, easeTo and animateTo. After that we can remove all the old methods.

This is the code calculating the center LatLng + corresponding zoom level:

CameraOptions Map::cameraForLatLngs(const std::vector<LatLng>& latLngs, const EdgeInsets& padding) {
    CameraOptions options;
    if (latLngs.empty()) {
        return options;
    }

    // Calculate the bounds of the possibly rotated shape with respect to the viewport.
    PrecisionPoint nePixel = {-INFINITY, -INFINITY};
    PrecisionPoint swPixel = {INFINITY, INFINITY};
    double viewportHeight = getHeight();
    for (LatLng latLng : latLngs) {
        PrecisionPoint pixel = pixelForLatLng(latLng);
        swPixel.x = std::min(swPixel.x, pixel.x);
        nePixel.x = std::max(nePixel.x, pixel.x);
        swPixel.y = std::min(swPixel.y, viewportHeight - pixel.y);
        nePixel.y = std::max(nePixel.y, viewportHeight - pixel.y);
    }
    double width = nePixel.x - swPixel.x;
    double height = nePixel.y - swPixel.y;

    // Calculate the zoom level.
    double scaleX = (getWidth() - padding.left - padding.right) / width;
    double scaleY = (getHeight() - padding.top - padding.bottom) / height;
    double minScale = ::fmin(scaleX, scaleY);
    double zoom = ::log2(getScale() * minScale);
    zoom = util::clamp(zoom, getMinZoom(), getMaxZoom());

    // Calculate the center point of a virtual bounds that is extended in all directions by padding.
    PrecisionPoint paddedNEPixel = {
        nePixel.x + padding.right / minScale,
        nePixel.y + padding.top / minScale,
    };
    PrecisionPoint paddedSWPixel = {
        swPixel.x - padding.left / minScale,
        swPixel.y - padding.bottom / minScale,
    };
    PrecisionPoint centerPixel = {
        (paddedNEPixel.x + paddedSWPixel.x) / 2,
        (paddedNEPixel.y + paddedSWPixel.y) / 2,
    };

    // CameraOptions origin is at the top-left corner.
    centerPixel.y = viewportHeight - centerPixel.y;

    options.center = latLngForPixel(centerPixel);
    options.zoom = zoom;
    return options;
}

[tobrun]

I was able to port above C++ code to a java equivalent. We now should be able to user the Camera API directly and we can manage our CameraPosition state in java.

I'm also going to refactor the current Camera API to abstract away implementation details behind the CameraUpdate interface.Now we are not leveraging the benefits of the factory pattern introduced by CameraUpdateFactory and CameraUpdate.

[tobrun]

Before refactor mentioned above:

After refactor:

The logic for each type of Update is abstracted away in it's own type,
No need for silly if-else constructions. Long live the factory pattern!

[mudar]

related to #3794: the only issue I can think of is handling polylines or polygons that would be partially visible. Not sure how GMaps handles this these days. Thanks!