Draw selected features only once #1834

Closed
wants to merge 5 commits into
from

3 participants

@ahocevar
OpenLayers member

As discussed in #1691 (comment), this adds a way for the select interaction to keep track of the layers that the selected features belong to. I know this is a lot of synchronization code right now, but it can be simplified once we decide that the select interaction is configured with a feature collection instead of a FeatureOverlay.

@elemoine: I know that the renderGeometryFunction is to go away, so I only made minimal changes related to that. All I needed was the feature as 2nd argument to get more context.

This depends on #1833.

@elemoine
OpenLayers member

I have more comments to make but here's a quick one (before getting on the train): the select interaction is not configured with a feature overlay. The feature overlay is created internally and is private to the interaction. The user can still programmatically select features with interaction.getFeatures().push(feature).

@elemoine elemoine and 1 other commented on an outdated diff Mar 11, 2014
src/ol/interaction/selectinteraction.js
+ }
+ var layerMap = this.layerMap_;
+ this.renderGeometryFunctions_[goog.getUid(layer)] = function(
+ geometry, object) {
+ if (object instanceof ol.Feature) {
+ return layerMap[goog.getUid(object)] !== layer;
+ }
+ return true;
+ };
+ if (goog.array.indexOf(renderGeometryFunctions.getArray(),
+ this.renderGeometryFunctions_) == - 1) {
+ var renderGeometryFunction =
+ this.renderGeometryFunctions_[goog.getUid(layer)];
+ renderGeometryFunctions.push(renderGeometryFunction);
+ }
+ this.layerMap_[goog.getUid(feature)] = layer;
@elemoine
OpenLayers member
elemoine added a line comment Mar 11, 2014

It looks like this won't work if a feature is in several layers. This case should be correctly handled I think.

@ahocevar
OpenLayers member
ahocevar added a line comment Mar 11, 2014

The way the Select interaction works currently also will not select the same feature from multiple layers (L254: if (goog.array.indexOf(features.getArray(), feature) == -1)).

@elemoine
OpenLayers member
elemoine added a line comment Mar 11, 2014

Even if we do not add the feature in the collection multiple times we should skip it in the layers it was selected from. Maybe I overlooked something and your patch already handles this.

@ahocevar
OpenLayers member
ahocevar added a line comment Mar 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine elemoine and 1 other commented on an outdated diff Mar 11, 2014
src/ol/interaction/selectinteraction.js
+ * @private
+ */
+ol.interaction.Select.prototype.handleFeatureAdded_ = function(evt) {
+ var map = this.getMap();
+ if (!goog.isNull(map) && this.handlingBrowserEvent_) {
+ return;
+ }
+ var feature = evt.element;
+ goog.asserts.assertInstanceof(feature, ol.Feature);
+ var extent = feature.getGeometry().getExtent();
+ var layers = map.getLayers().getArray();
+ var matchFeature = function(f) { return f === feature; };
+ var found, i, layer, source;
+ for (i = layers.length - 1; i >= 0; --i) {
+ layer = layers[i];
+ if (layer instanceof ol.layer.Vector && this.layerFilter_(layer)) {
@elemoine
OpenLayers member
elemoine added a line comment Mar 11, 2014

This won't work if the feature is in an ol.layer.Image layer (configured with an ol.source.ImageVector).

@ahocevar
OpenLayers member
ahocevar added a line comment Mar 11, 2014

This is mostly because an ol.layer.Image does not have a renderGeometryFunction. Supporting this will probably make more sense when you are done with your refactoring that makes the renderGeometryFunction go away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine elemoine commented on an outdated diff Mar 11, 2014
src/ol/interaction/selectinteraction.js
+ if (goog.isDef(layer)) {
+ goog.asserts.assertInstanceof(layer, ol.layer.Vector);
+ if (goog.array.indexOf(goog.object.getValues(layerMap), layer) == -1) {
+ layer.getRenderGeometryFunctions().remove(
+ this.renderGeometryFunctions_[goog.getUid(layer)]);
+ delete this.renderGeometryFunctions_[goog.getUid(layer)];
+ }
+ }
+};
+
+
+/**
+ * @param {goog.events.Event} evt Event
+ * @private
+ */
+ol.interaction.Select.prototype.handleFeaturesChanged_ = function(evt) {
@elemoine
OpenLayers member
elemoine added a line comment Mar 11, 2014

Is this listener needed given that the feature overlay is private to the interaction?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elemoine
OpenLayers member

Thanks for tackling this @ahocevar. My impression is that this code is fragile and does not work for all the cases we need to cover. See my comments in the code. I also admit that I don't have better solutions at this point.

@ahocevar
OpenLayers member

@elemoine As discussed before, a much simpler solution would be to configure the Select interaction with a single layer to select from.

ahocevar added some commits Mar 10, 2014
@ahocevar ahocevar Call renderGeometryFunction with opaque data object as 2nd arg 3fee20c
@ahocevar ahocevar Make FeatureOverlay an ol.Observable and trigger change events
This allows us to get notified when e.g. the overlay's features
collection changes.
79fd7bc
@ahocevar ahocevar Do not render selected features on the original layer
By keeping track of the layers of selected features, we can
push a renderGeometryFunction to a layer so selected features
are no longer rendered. This also covers features that are
programmatically added to the selection overlay.
4f9a82c
@ahocevar ahocevar No need to monitor feature collection changes on the overlay
As @elemoine points out correctly, the FeatureOverlay is private
to the Select interaction, so it cannot be accessed by the
application to change its feature collection.
8de03fa
@ahocevar
OpenLayers member

Scratch that. Even in that case (assuming the interaction is configured with a user provided feature collection), features from different layers could be in that collection.

@elemoine
OpenLayers member

For reference, tonio@d184e24 is @tonio's commit that replaces the collection of "render geometry functions" by a collection of "skipped features". It may be a good idea to add that commit into your branch. What do you think?

@elemoine
OpenLayers member

I just realize that there's more in @tonio's branch. See https://github.com/tonio/ol3/compare/skip_features. He's even started modifying the select interaction.

@ahocevar
OpenLayers member

I need this in first. Update that addresses you remaining concerns to come in a few minutes.

@ahocevar
OpenLayers member

@elemoine Updated. This simplifies things quite a bit. If you are happy with the code now we can merge it, and when @tonio is done with the renderGeometryFunction refactoring, I'll be willing to contribute the necessary changes to his branch.

@ahocevar ahocevar referenced this pull request Mar 11, 2014
Merged

Use pointer events #1840

@ahocevar ahocevar added a commit to tsauerwein/ol3 that referenced this pull request Mar 11, 2014
@ahocevar ahocevar Removing code for functionality now handled by #1834 ea03bed
@ahocevar ahocevar added a commit to tsauerwein/ol3 that referenced this pull request Mar 13, 2014
@ahocevar ahocevar Removing code for functionality now handled by #1834 2669356
@ahocevar ahocevar added a commit to tsauerwein/ol3 that referenced this pull request Mar 14, 2014
@ahocevar ahocevar Removing code for functionality now handled by #1834 e28faa3
@tonio tonio commented on the diff Mar 17, 2014
src/ol/interaction/selectinteraction.js
+ var map = this.getMap();
+ if (!goog.isNull(map)) {
+ return;
+ }
+ var feature = evt.element;
+ goog.asserts.assertInstanceof(feature, ol.Feature);
+ var extent = feature.getGeometry().getExtent();
+ var layers = map.getLayers().getArray();
+ var matchFeature = function(f) { return f === feature; };
+ var found, i, layer, source;
+ for (i = layers.length - 1; i >= 0; --i) {
+ layer = layers[i];
+ if (this.layerFilter_(layer)) {
+ source = layer.getSource();
+ if (source instanceof ol.source.Vector) {
+ found = source.forEachFeatureInExtent(extent, matchFeature);
@tonio
OpenLayers member
tonio added a line comment Mar 17, 2014

Then if you skip a feature, you potentially iterate on a very large set of feature.
The purpose of skipping features without recreating a batch (e.g. iterate on every feature) is not really achieved…

I think I would prefer keeping a collection of features to skip on the map, rather that on the layer. I can update my branch in that way if it fit your needs. I think it would cover the majority of the use cases.

@ahocevar
OpenLayers member
ahocevar added a line comment Mar 17, 2014

Sounds good to me, with two things to consider:

  1. It might be better to organize the skipped features as hash rather than a collection.
  2. The map renderer is probably a better place to keep the hash than the map.

@tonio How long until you get your branch merged? I need this issue here addressed as quickly as possible, because we have clients depend on it. So if it's not today or tomorrow, I'd rather get a "go" for this now and change it later to suit your skip features work.

@ahocevar
OpenLayers member
ahocevar added a line comment Mar 18, 2014

@tonio Also note that this only modifies a single feature, and the iteration only happens upon feature selection. For the renderGeometryFunction, a hash of feature ids to skip is kept. So I do not anticipate big changes once your branch is in.

@elemoine Can you please give this another look? This open pull request really blocks us. Thanks!

@elemoine
OpenLayers member
elemoine added a line comment Mar 18, 2014
  1. It might be better to organize the skipped features as hash rather than a collection.

This is what we're envisioning having:

map.getSkippedFeatures().push(feature);

Then, when a render frame comes, we pass the uids to the features to skip to the renderer through the frame state.

var frameState = {
   // ...
   skippedFeatureUids: goog.array.map(
       this.getSkippedFeatures().toArray(), function(feature) { return goog.getUid(feature); }),
   // ...
};
  1. The map renderer is probably a better place to keep the hash than the map.

But we may want to expose the getSkippedFeatures function in the future, for users to be able to indicate the features that should be skipped in the layers.

So I do not anticipate big changes once your branch is in.

Our impression is that all of your code will need to be changed for @tonio's work, that is the reason we're not to inclined to merge this work.

@ahocevar
OpenLayers member
ahocevar added a line comment Mar 18, 2014

No worries, I'll make the necessary changes when @tonio is done. This code is ready and working, and it is no more work later if this is merged now.

@twpayne
twpayne added a line comment Mar 18, 2014

My understanding is that if this PR is merged then it will make @tonio's job much, more difficult. He'll either have tens of complicated conflicts to resolve, or he'll have to revert all the commits in this pull request. To me, it makes much, much more sense to wait for @tonio's PR.

@elemoine
OpenLayers member
elemoine added a line comment Mar 18, 2014

@tonio's branch applies to current master (and his branch does also include changes to the select interaction). If you merge this he could create a new branch, revert your commits, and then add his. But we can avoid this by waiting for @tonio's PR.

@ahocevar
OpenLayers member
ahocevar added a line comment Mar 18, 2014

As I said we need to fulfill a client requirement here. If @tonio can finish and merge his work this week and include code that solves this issue, I will be able to close this.

@twpayne
twpayne added a line comment Mar 18, 2014

Are you able to use a separate branch or tag for your client?

@ahocevar
OpenLayers member
ahocevar added a line comment Mar 19, 2014

What time frame are we talking about? It would not be good if I had to keep a branch up to date with master for several weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@tonio
OpenLayers member

I’m working on it today.

@ahocevar
OpenLayers member
@ahocevar
OpenLayers member

Fixed with #1877.

@ahocevar ahocevar closed this Mar 21, 2014
@ahocevar ahocevar deleted the ahocevar:draw-selected-features-only-once branch Mar 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment