mapbox-maps-android

mapbox-maps-android copied to clipboard

There is an inconsistency between pixelForCoordinate:

  override fun pixelForCoordinate(coordinate: Point): ScreenCoordinate {
    checkNativeMap("pixelForCoordinate")
    val coordinate = nativeMap.pixelForCoordinate(coordinate)
    val screenSize = nativeMap.size
    return if (coordinate.x in 0.0..screenSize.width.toDouble() && coordinate.y in 0.0..screenSize.height.toDouble()) {
      coordinate
    } else {
      ScreenCoordinate(-1.0, -1.0)
    }
  }

and pixelsForCoordinates:

  override fun pixelsForCoordinates(coordinates: List<Point>): List<ScreenCoordinate> {
    checkNativeMap("pixelsForCoordinates")
    return nativeMap.pixelsForCoordinates(coordinates)
  }

The former returns ScreenCoordinate(-1.0, -1.0) if the coordinate is on the map, but not on the screen. The latter returns a coordinate even if it is not on the screen.

This was changed as a "bugfix" (by @pengdev) in https://github.com/mapbox/mapbox-maps-android/pull/1226, following https://github.com/mapbox/mapbox-maps-ios/pull/1195 where @endanke says that "Currently we cannot guarantee that the returned point of MapboxMap.point(for: CLLocationCoordinate2D) is valid outside of the MapView bounds".

However, I tried using pixelsForCoordinates with a very big polygon on the map (using Jetpack Compose with ramani-maps), and it feels like it is working fine (see video below):

https://github.com/mapbox/mapbox-maps-android/assets/2606672/ef3a3a6e-21cb-45dd-9d27-aa0209b29b03

Did something change that would allow reverting https://github.com/mapbox/mapbox-maps-android/pull/1226 now, or am I missing an edge-case where it would fail?

@ank27: FYI

hi @JonasVautherin , Looks like a correct observation, pixelsForCoordinates should also have the same check as pixelForCoordinate. I have created a pr and it will be available in v10.16 release. Thank you 🙂

@ank27 but should it? When does it fail without the check? I did not manage to make it fail (see my video above).

Also how do you then draw a polygon that is part-way out of the screen with the check? :thinking: Or should I just fork mapbox-maps-android to remove the check for my use-case?

Also how do you then draw a polygon that is part-way out of the screen with the check

hi @JonasVautherin , it will still be drawn but the part of the polygon that is out of the screen rect will not be visible. layers (polygon) are drawn on the map, it doesn't need to be in visible rect. (see the video for ref)

The fix I implemented was related to having consistent results from mapboxMap.pixelForCoordinate and mapboxMap.pixelsForCoordinates when checking the screen coordinate for the points lie outside of the visible rect.

device-2023-08-17-134815.webm

Right. Yeah sorry I did not express myself clearly.

How do I get a pixel coordinate that is not (-1, -1) when the point is not on the screen, say if I want to make some computation in pixels instead of LatLng on a polygon that is not fully in the boundaries of the screen?

To me it seems like the choice to return (-1, -1) is arbitrary (at least I don't understand when the actual value is wrong, given that I use it in my computations), and it looks like some kind of workaround for something that could just be isOnScreen(pixel), in case the user needs to know whether that pixel is on the screen of not.

What am I missing?

To me it seems like the choice to return (-1, -1) is arbitrary

well, its not arbitrary. ScreenCoordinate is corresponds to one pixel on the user's screen and for any feature that lie outside of the screen pixel, we are returning (-1,-1).

In case you want to know if the specific feature is rendered, you can call queryRenderedFeature with RenderedQueryGeometry and you can find all the features that are currently rendered on the screen.

In case you want to know if the specific feature is rendered

I want to know the coordinate of the feature in pixels, as the function name suggests (pixelForCoordinate). The function is not called (pixelForCoordinateOnlyWhenAppearingOnTheScreen) :see_no_evil:.

In my case, I want to do computations with those coordinates, in pixels: because it's harder with maps coordinates, where I would need to know the projection used by MapBox (otherwise my computations will be distorted e.g. close to the poles).

To me, the solution is to have pixelForCoordinate() return the pixels w.r.t. the map (as the name of the function suggests), and to add e.g. a function to check if the pixel coordinate is on the screen (isRenderedOnScreen(pixel)), such that the user can check that too. With the current implementation, this information is lost in the (-1, -1).

Does that make sense?

@ank27 pixelsForCoordinate is working for me without the check in the current production SDK, but pixelForCoordinate returning (-1,-1) breaks a feature.

I'm displaying an arrow at the edge of the screen that points towards a target off the screen. As the map center or rotation changes, the arrow moves around the edges of the screen to keep pointing at the target. Like a quest pointer in a video game:

The placement + rotation of the arrow depends on getting an off screen coordinate sometimes. Works well without the check that will be added to pixelsForCoordinate in v10.16. Would it be possible to continue to have a function that does return an off screen coordinate?

Thanks

@ank27: looking here, it seems that the inconsistency has been "fixed" by breaking the API again (shouldn't it appear in the release notes together with the other breaking changes?).

So now it is consistent, but it breaks my app (to the point where I cannot use Mapbox anymore). As I mention above, unless there is a good reason for doing that clamping (which I don't see because it works without it, and apparently it works for @JDM8 as well), then the API should not do the clamping (and instead just provide .clampScreenCoordinate() as a helper for those who want it).

@ank27 is there any kind of willingness in keeping support for that feature (that used to work but was broken in the recent releases), or not?

i think clampScreenCoordinate should be an option instead of being the default

Agreed. We still haven't found an alternative, so right now our use-case is just broken. An example where it was used is the "interactive polygon" in Ramani-Maps, where the more complex interactive shapes require us to do some geometry in pixels.

I agree with what people said in the second part of the thread. Both pixelForCoordinate and pixelsForCoordinates should return the correct value even the point is outside the screen boundary, at least as an option. I drew a line on the map. Now if one end is moved outside the screen, I can no longer draw the line because (-1,-1) is returned.

I'm migrating a project from Google maps to Mapbox... this restriction is going to be really painful as a core feature was heavily relying on Google maps ability to return pixel positions outside of the screen :(

@claudioredi: if that's a blocker for Mapbox, you may want to try MapLibre which supports it (we need that in Ramani-Maps).