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

Pass geometry to forEachFeatureAtPixel callback #11781

Merged
merged 3 commits into from
Dec 1, 2020

Conversation

ahocevar
Copy link
Member

@ahocevar ahocevar commented Nov 27, 2020

Working on #11769, I realized that when editing a geometry collection, it is useful to not only get the feature that a hit was detected on, but also the exact geometry from the collection. Since this information is available at the time a hit is detected, this change is only about passing that information all the way to the map's forEachFeatureAtPixel() method.

@@ -538,8 +538,7 @@ class PluggableMap extends BaseObject {
* callback with each intersecting feature. Layers included in the detection can
* be configured through the `layerFilter` option in `opt_options`.
* @param {import("./pixel.js").Pixel} pixel Pixel.
* @param {function(this: S, import("./Feature.js").FeatureLike,
Copy link
Contributor

Choose a reason for hiding this comment

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

The description should be updated to include the third geometry parameter.
The FeatueCallback<T> type is not available for the apidocs, becomes a dead link.

Copy link
Member Author

Choose a reason for hiding this comment

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

I intentionally did not document that third parameter, because I am not sure it is relevant for the API. The reason why I added it was to be able to use the Modify interaction with hit detection and geometry collections after #11781, where this third parameter is used internally.

I did update the type to avoid the dead link, so the documentation should now be the same as before this pull request.

Comment on lines -428 to -432
const featureCallback = function (feature) {
const featureCallback = function (feature, geometry) {
const key = getUid(feature);
if (!(key in features)) {
features[key] = true;
return callback(feature, layer);
Copy link
Contributor

Choose a reason for hiding this comment

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

The forEachFeatureAtPixel callback will only be called with the first detected geometry, but multiple geometries of the same feature could be at that pixel.
This should probably also be documented.

@ahocevar
Copy link
Member Author

I updated the documentation now at least for internal use according to your suggestions. Thanks, @MoonE.

@MoonE
Copy link
Contributor

MoonE commented Nov 30, 2020

If this is used with the new layer option of the Modify interaction MutliGeometries will not be supported because there is just one geometry object. Is this expected?

Copy link
Contributor

@MoonE MoonE left a comment

Choose a reason for hiding this comment

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

If the point in my previous comment is not an issue this looks good to me.

@ahocevar
Copy link
Member Author

ahocevar commented Dec 1, 2020

If this is used with the new layer option of the Modify interaction MutliGeometries will not be supported because there is just one geometry object. Is this expected?

I know it is unfortunate, but it is expected. For geometry collections, this change was a low hanging fruit. For multi geometries, this would be much harder to achieve.

@ahocevar ahocevar merged commit 8ceaadc into openlayers:main Dec 1, 2020
@ahocevar ahocevar deleted the hitdetect-geometry branch December 1, 2020 08:59
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 this pull request may close these issues.

None yet

2 participants