-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add a feature box selection example #1959
Conversation
LGTM |
var map = new ol.Map({ | ||
interactions: ol.interaction.defaults({ | ||
// disable the DragZoom interaction | ||
shiftDragZoom: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should be able to remove this when #1960 is merged.
Now rebased onto the branch attached to #1960 (which should be merged first). |
<link rel="stylesheet" href="../resources/bootstrap/css/bootstrap.min.css" type="text/css"> | ||
<link rel="stylesheet" href="../resources/layout.css" type="text/css"> | ||
<link rel="stylesheet" href="../resources/bootstrap/css/bootstrap-responsive.min.css" type="text/css"> | ||
<title>Simple example</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "Box Selection Example" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
Good example. I wonder if we should instead be encouraging people to use a Though I know that requires that people also understand styles. |
Yeah, was thinking about the same. Changing now. |
Example changed according to @tschaub's suggestions. |
There's something wrong with this example (more likely the data). |
Yeah saw that. I think it's related to the data (which includes the continents if I remember correctly). |
#781 might be relevant. |
I thought so as well :) Would be nice to use data that doesn't surprise us (or our users). I think it would be worth confirming that the bbox handling for these geometries is correct as well. But I'm also not volunteering to do this immediately, so I don't think it should block getting this in. |
Same behavior if a box is between the Sicilia and the Italy (in the sea). |
I think the problem is that |
|
Are you saying that it's not worth adding an example that does box selection based on forEachFeatureInExtent? What's your opinion? |
I think it is worth adding such an example: selection of features by bounding box is a useful and worthwhile function. I'm just trying to point out that |
Yep, your comment makes sense @twpayne. I'll see what I can do (when I have some time for that). |
For information I've started working on adding ol.geom.Polygon#intersectsExtent and ol.geom.MultiPolygon#intersectsExtent. My implementation is based on algorithms found in the JTS library. And this is what I now use in the example: dragBox.on('boxend', function(e) {
// features that intersect the box are added to the feature ovelay
var geometry = dragBox.getGeometry();
var extent = geometry.getExtent();
vectorSource.forEachFeatureInExtent(extent, function(feature) {
var geometry = /** @type {ol.geom.MultiPolygon} */ (feature.getGeometry());
if (geometry.intersectsExtent(extent)) {
featureOverlay.getFeatures().push(feature);
}
});
}); The example looks better now. I need to make a few changes before declaring this reviewable. |
One bug to look at: when drawing very small boxes in a country the country is not selected. |
I've resumed the work on this. My feature-box-selection branch adds ol.geom.flat.intersects functions for testing if a geometry and an extent intersect. It also adds a box-selection example that allows selecting features and displaying information by drawing boxes on the map. The box-selection example can be tested at http://erilem.net/ol3/feature-box-selection/examples/box-selection.html. I'd like to review the code again before requesting reviews from others. |
is it easy to show in this example how to combine point selection with box selection? |
Using a select interaction should work as expected? Do you want to give it a try? |
sure I can give it a try |
confirmed the combination works fine with: h My guess is that people will want to combine box and click selection often, but feel free to disregard my commit. |
Thanks Bart. I'll merge this. And possibly do a fixup to merge your commit into the existing commit for the example. |
So yesterday we said that ol.source.Vector#forEachFeatureInExtent will call intersectsExtent on the geometry. This means that we'll need a new function that just does the bbox comparison. That function will be internal and used by the vector renderer. That function could be named forEachFeatureOverlappingExtent. I find the term "overlapping" fuzzy enough here. |
Things to do for this PR:
|
This is now ready for review. The new example is available for testing at http://erilem.net/ol3/feature-box-selection/examples/box-selection.html. |
* Test if the geometry and the passed extent intersect. | ||
* @param {ol.Extent} extent Extent. | ||
* @return {boolean} `true` if the geometry and the extent intersect. | ||
* @api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@function
should be added, otherwise jsdoc treats the function as a attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've wondered when to use @function
. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forget it: ol.geom.Geometry
is not in the api doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may forget to add the @function
when we pass the function @api
so it makes sense to always add the annotation.
If possible, having the build process check for the presence of the annotation would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed elsewere ol.geom.Geometry
should be exported and in the API documentation. Users indeed expect to be able to geometry instanceof ol.geom.Geometry
.
So I'll add the @function
annotation.
Includes functions for testing if an extent and a geometry intersect.
Thanks for the good comments. I've addressed them all. |
Looks good to me as well. |
+1 |
Add a feature box selection example
This PR adds a "feature box selection" example. This example combines the use of a DragBox interaction and a Select interaction to select features by drawing boxes on the map.