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

Add a feature box selection example #1959

Merged
merged 16 commits into from
Sep 17, 2014
Merged

Add a feature box selection example #1959

merged 16 commits into from
Sep 17, 2014

Conversation

elemoine
Copy link
Member

@elemoine elemoine commented Apr 7, 2014

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.

@fredj
Copy link
Member

fredj commented Apr 7, 2014

LGTM

var map = new ol.Map({
interactions: ol.interaction.defaults({
// disable the DragZoom interaction
shiftDragZoom: false
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 should be able to remove this when #1960 is merged.

@elemoine
Copy link
Member Author

elemoine commented Apr 7, 2014

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@tschaub
Copy link
Member

tschaub commented Apr 7, 2014

Good example. I wonder if we should instead be encouraging people to use a ol.FeatureOverlay for this - it seems weird to have the select interaction not handle any interactions.

Though I know that requires that people also understand styles.

@elemoine
Copy link
Member Author

elemoine commented Apr 7, 2014

Good example. I wonder if we should instead be encouraging people to use a ol.FeatureOverlay for this - it seems weird to have the select interaction not handle any interactions.

Yeah, was thinking about the same. Changing now.

@elemoine
Copy link
Member Author

elemoine commented Apr 7, 2014

Example changed according to @tschaub's suggestions.

@fredj
Copy link
Member

fredj commented Apr 7, 2014

There's something wrong with this example (more likely the data).
If I draw a box on the west of Portugal (in the sea) Russia is selected

@elemoine
Copy link
Member Author

elemoine commented Apr 7, 2014

Yeah saw that. I think it's related to the data (which includes the continents if I remember correctly).

@elemoine
Copy link
Member Author

elemoine commented Apr 7, 2014

#781 might be relevant.

@tschaub
Copy link
Member

tschaub commented Apr 7, 2014

#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.

@fredj
Copy link
Member

fredj commented Apr 7, 2014

Same behavior if a box is between the Sicilia and the Italy (in the sea).
The biggest issue is that when Ukraine is selected, Russia is selected too ...

@twpayne
Copy link
Contributor

twpayne commented Apr 7, 2014

I think the problem is that forEachFeatureInExtent currently only does overlapping extent tests and therefore returns all features whose extents overlap the passed extent. This is quite different from the behaviour desired here.

@fredj
Copy link
Member

fredj commented Apr 8, 2014

data/kml/2012_Earthquakes_Mag5.kml could be used instead.

@elemoine
Copy link
Member Author

elemoine commented Apr 8, 2014

I think the problem is that forEachFeatureInExtent currently only does overlapping extent tests and therefore here.

Are you saying that it's not worth adding an example that does box selection based on forEachFeatureInExtent? What's your opinion?

@twpayne
Copy link
Contributor

twpayne commented Apr 8, 2014

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 forEachFeatureInExtent is not sufficient to implement this though, as it returns "too many" features. It will be necessary to add further code that can determine whether a geometry intersects an extent. The intersection of the geometry's extent with the selection extent is a necessary, but not a sufficient, condition. Effectively, it will be necessary to determine whether points, lines, and polygons intersect an extent.

@elemoine
Copy link
Member Author

elemoine commented Apr 8, 2014

Yep, your comment makes sense @twpayne. I'll see what I can do (when I have some time for that).

@elemoine
Copy link
Member Author

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.

@elemoine
Copy link
Member Author

One bug to look at: when drawing very small boxes in a country the country is not selected.

@elemoine elemoine added this to the v3.0.0 milestone Jun 26, 2014
@ahocevar ahocevar modified the milestones: v3.0.0-gamma.3, v3.0.0 Jun 26, 2014
@elemoine
Copy link
Member Author

elemoine commented Jul 8, 2014

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.

@bartvde
Copy link
Member

bartvde commented Jul 11, 2014

is it easy to show in this example how to combine point selection with box selection?

@elemoine
Copy link
Member Author

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?

@bartvde
Copy link
Member

bartvde commented Jul 11, 2014

sure I can give it a try

@bartvde
Copy link
Member

bartvde commented Jul 11, 2014

confirmed the combination works fine with: h
https://github.com/bartvde/ol3/commit/35e0aae53cdc3e8d4e34e60c91e4ed6d33b47bb9

My guess is that people will want to combine box and click selection often, but feel free to disregard my commit.

@elemoine
Copy link
Member Author

Thanks Bart. I'll merge this. And possibly do a fixup to merge your commit into the existing commit for the example.

@elemoine
Copy link
Member Author

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.

@elemoine
Copy link
Member Author

Things to do for this PR:

  • At this point ol.geom.Polygon and ol.geom.MultiPolygon only implement intersectsExtent. The function needs to be implemented by the other geometry types.
  • Add a forEachFeatureIntersectingExtent function to ol.source.Vector. That function will use forEachFeatureInExtent and call intersectsExtent on the geometry to check that the feature actually intersects the passed extent.

@elemoine
Copy link
Member Author

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

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

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've wondered when to use @function. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

forget it: ol.geom.Geometryis not in the api doc

Copy link
Member

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.

Copy link
Member Author

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.

@elemoine
Copy link
Member Author

Thanks for the good comments. I've addressed them all.

@ahocevar
Copy link
Member

Looks good to me as well.

@fredj
Copy link
Member

fredj commented Sep 17, 2014

+1

elemoine pushed a commit that referenced this pull request Sep 17, 2014
Add a feature box selection example
@elemoine elemoine merged commit dfb2734 into openlayers:master Sep 17, 2014
@elemoine elemoine deleted the feature-box-selection branch September 17, 2014 13:02
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

7 participants