rnmapbox / maps

A Mapbox react native module for creating custom maps
MIT License
2.26k stars 847 forks source link

ShapeSource.onPress unable to return multiple features #611

Closed baijii closed 4 years ago

baijii commented 4 years ago

I have a map with a ShapeSource and multiple Symbols. I'd like to get all features at a given point on press.

With mapboxgl for the web:

const onPress = (e) => {
    const { features } = e
    // features is now an array of features
}

With @react-native-mapbox-gl/maps:

const onPress = async (e) => {
    const feature = e.nativeEvent.payload
    // nativeEvent will only return one feature as payload
    // this feature must not be the nearest best feature

    const features = await map.current.queryRenderedFeaturesAtPoint(feature.geometry.coordinates)
    // as feature must not be the nearest, also this solution won't work
    // fort it just returns one or two arrays at the wrong place
}

Is it possible to get the coordinates of the tap instead of the feature? Maybe someone has a solution for me. Thanks!

mfazekas commented 4 years ago

Can't you use MapView#onPress then you have click coordinates/screen coordinates, and you can query stuff the way you prefer?!

See https://github.com/react-native-mapbox-gl/maps/blob/71501690cc7c27bfd32affeb59e91fd49fd50303/example/src/examples/ShowClick.js#L37-L46

mfazekas commented 4 years ago

I guess it would make sense to return multiple features, but it would be an incompatible change. @mattijsf @kristfal what do you think?!

baijii commented 4 years ago

Thank you for that solution, @mfazekas . But this won't be satisfactory, in my case. I have quite a lot of features, so I don't show all of them at the same time. With your solution I still have to get calculate, which features could be found at a position within the currently set zoom level. That would be very hard for me, as they are actually not attached to the map. Plus, with this won't work if there is a onPress listener attached to the ShapeSource as well.

I switched to cluster={true} in the meantime. I found a compromising workaround: I just zoom in some steps as I click on it.

It would be really great, if I am able to get the all the features of a cluster as I click on it to calculate the bounds I want to zoom in.

Yes, this would be a breaking change, but it would be the right thing to do, as mapboxgl does it this way and I think at least the iOS-SDK as well (correct me if I'm wrong). Also it would be very easy for existing projects to migrate:

const feature = e.nativeEvent.payload

will be

const features = e.nativeEvent.payload
const feature = features[0]

Also they could improve:

const features = e.nativeEvent.payload
const findNearest((features) => {
    // fancy script for finding the nearest feature
})
const feature = findNearest(features)
mfazekas commented 4 years ago

@baijii i'd say that if possible keep e.nativeEvent.payload as a single feature for backward compatibility with a deprecation warning. And e.features should return multiple features. I don't like the nativeEvent or payload.

mfazekas commented 4 years ago

@baijii i've added a draft pr to address: #700 please review.

baijii commented 4 years ago

@mfazekas Thank you very much for your work. I'll try to review this. But I'm quite unexperienced with this. Especially with the native parts.

I tried to check out the branch and start the example project, but I'm unable to do so. On iOS, I get some invalid Podfile errors (probably due use_frameworks! and use_native_modules!). On Android, I can launch the App, but the map is displayed plain black.

I don't think, that is has anything to do with your pull request, as these things haven't changed. Maybe somebody else should review this, or you'll find to help a noob like me 😄.

To me, the code seems legit, so I would be happy with just human testing, if the correct things will be returned in the end. Maybe you can add a test for this, to improve the test coverage a litte.

baijii commented 4 years ago

Oh, I was a bit to late, I suppose, as it is already been merged. Maybe next time. Thanks again.

mfazekas commented 4 years ago

@baijii no worries thanks for the effort. Next time you should be able to just use the branch from the PR in your project. PR #700 says that it's from mfazekas:shapesource-onpress-nativeevent clicking that you see the url of the repo + branch. https://github.com/mfazekas/maps-1/tree/shapesource-onpress-nativeevent

Then in your package.json you can just write "@react-native-mapbox-gl/maps" : "mfazekas/maps-1#shapesource-onpress-nativeevent" then npm install etc. and test the branch in your project.

baijii commented 4 years ago

@mfazekas Thanks, that would have been indeed much easier. I'm looking forward to do so, next time.

cglacet commented 2 years ago

I feel the behavior is still unclear, it seems like onPress returns a list of features from the top layer within a particular source. Underlying layers are ignored from what I can tell.