Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ShapeSource.onPress unable to return multiple features #611

Closed
baijii opened this issue Jan 12, 2020 · 10 comments · Fixed by #700
Closed

ShapeSource.onPress unable to return multiple features #611

baijii opened this issue Jan 12, 2020 · 10 comments · Fixed by #700

Comments

@baijii
Copy link

baijii commented Jan 12, 2020

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!

@baijii baijii changed the title ShapeSource.onPress event should return multiple features ShapeSource.onPress unable to return multiple features Jan 12, 2020
@mfazekas
Copy link
Contributor

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
Copy link
Contributor

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

@baijii
Copy link
Author

baijii commented Feb 27, 2020

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
Copy link
Contributor

@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
Copy link
Contributor

mfazekas commented Mar 1, 2020

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

@baijii
Copy link
Author

baijii commented Mar 6, 2020

@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
Copy link
Author

baijii commented Mar 6, 2020

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

@mfazekas
Copy link
Contributor

mfazekas commented Mar 6, 2020

@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
Copy link
Author

baijii commented Mar 6, 2020

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

@cglacet
Copy link

cglacet commented Feb 17, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants