Removal of ol.FeatureOverlay #3758

Merged
merged 5 commits into from Jun 10, 2015

Conversation

Projects
None yet
5 participants
@ahocevar
Member

ahocevar commented Jun 3, 2015

With an option to disable the spatial index of ol.source.Vector, and a map option for ol.layer.Layer to render on top of other layers without adding it to the map's layers collection, and a way to manage features of an ol.source.Vector in an ol.Collection, we can get rid of ol.FeatureOverlay.

This simplifies code in many places, and it also improves date line handling, which was not possible with ol.FeatureOverlay.

Fixes #3704.
Fixes #2386.

externs/olx.js
+ * Sets the layer as overlay on a map. The map will not manage this layer in its
+ * layers collection, and the layer will be rendered on top. This is useful for
+ * temporary layers. The standard way to add a layer to a map and have it
+ * managed by the map is use {@link ol.Map#addLayer}.

This comment has been minimized.

@probins

probins Jun 3, 2015

Contributor

to use ...

@probins

probins Jun 3, 2015

Contributor

to use ...

externs/olx.js
+ * Sets the layer as overlay on a map. The map will not manage this layer in its
+ * layers collection, and the layer will be rendered on top. This is useful for
+ * temporary layers. The standard way to add a layer to a map and have it
+ * managed by the map is use {@link ol.Map#addLayer}.

This comment has been minimized.

@probins

probins Jun 3, 2015

Contributor

to use ...

@probins

probins Jun 3, 2015

Contributor

to use ...

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins Jun 3, 2015

Contributor

a quick comment. As mentioned in #3737, a featureoverlay isn't needed in several of the examples if the select interaction is used instead; this also simplifies those examples. #3757 enables that, so it might be better to merge #3757 and change those examples first. Then they would no longer need to be changed in this PR. ISTM this would be better than changing them here, and then changing them again as and when #3757 is merged.

Contributor

probins commented Jun 3, 2015

a quick comment. As mentioned in #3737, a featureoverlay isn't needed in several of the examples if the select interaction is used instead; this also simplifies those examples. #3757 enables that, so it might be better to merge #3757 and change those examples first. Then they would no longer need to be changed in this PR. ISTM this would be better than changing them here, and then changing them again as and when #3757 is merged.

src/ol/source/vectorsource.js
+ */
+ol.source.Vector.prototype.bindFeaturesCollection_ = function(collection) {
+ goog.asserts.assert(goog.isNull(this.featuresCollection_),
+ 'bindFeatuesCollection can only be called once');

This comment has been minimized.

@elemoine

elemoine Jun 3, 2015

Member

Typo.

@elemoine

elemoine Jun 3, 2015

Member

Typo.

externs/olx.js
@@ -2471,6 +2472,14 @@ olx.interaction.DrawOptions.prototype.freehandCondition;
+/**
+ * Wrap the world horizontaly on the sketch overlay. Default is `false`.
+ * @type {boolean|undefined}

This comment has been minimized.

@bartvde

bartvde Jun 9, 2015

Member

horizontaly -> horizontally

@bartvde

bartvde Jun 9, 2015

Member

horizontaly -> horizontally

externs/olx.js
@@ -2588,6 +2598,14 @@ olx.interaction.ModifyOptions.prototype.features;
+/**
+ * Wrap the world horizontaly on the sketch overlay. Default is `false`.

This comment has been minimized.

@bartvde

bartvde Jun 9, 2015

Member

horizontaly -> horizontally

@bartvde

bartvde Jun 9, 2015

Member

horizontaly -> horizontally

externs/olx.js
@@ -2804,6 +2823,14 @@ olx.interaction.SelectOptions.prototype.filter;
+/**
+ * Wrap the world horizontaly on the selection overlay. Default is `true`.

This comment has been minimized.

@bartvde

bartvde Jun 9, 2015

Member

horizontaly -> horizontally

@bartvde

bartvde Jun 9, 2015

Member

horizontaly -> horizontally

src/ol/interaction/drawinteraction.js
- this.overlay_ = new ol.FeatureOverlay({
+ this.overlay_ = new ol.layer.Vector({
+ source: new ol.source.Vector({
+ spatialIndex: false,

This comment has been minimized.

@bartvde

bartvde Jun 9, 2015

Member

shouldn't this be useSpatialIndex ?

@bartvde

bartvde Jun 9, 2015

Member

shouldn't this be useSpatialIndex ?

src/ol/interaction/drawinteraction.js
@@ -674,7 +678,7 @@ ol.interaction.Draw.prototype.abortDrawing_ = function() {
this.sketchFeature_ = null;
this.sketchPoint_ = null;
this.sketchLine_ = null;
- this.overlay_.getFeatures().clear();
+ this.overlay_.getSource().clear();

This comment has been minimized.

@bartvde

bartvde Jun 9, 2015

Member

have you considered using true here for opt_fast ?

@bartvde

bartvde Jun 9, 2015

Member

have you considered using true here for opt_fast ?

This comment has been minimized.

@ahocevar

ahocevar Jun 9, 2015

Member

I can try that. I'll also add a test to make sure that a features collection will stay in sync.

@ahocevar

ahocevar Jun 9, 2015

Member

I can try that. I'll also add a test to make sure that a features collection will stay in sync.

This comment has been minimized.

@bartvde

bartvde Jun 9, 2015

Member

Ah good point, it might no stay in sync in that case, didn't think about that.

@bartvde

bartvde Jun 9, 2015

Member

Ah good point, it might no stay in sync in that case, didn't think about that.

src/ol/interaction/drawinteraction.js
@@ -701,7 +705,9 @@ ol.interaction.Draw.prototype.updateSketchFeatures_ = function() {
if (!goog.isNull(this.sketchPoint_)) {
sketchFeatures.push(this.sketchPoint_);
}
- this.overlay_.setFeatures(new ol.Collection(sketchFeatures));
+ var overlaySource = this.overlay_.getSource();
+ overlaySource.clear();

This comment has been minimized.

@bartvde

bartvde Jun 9, 2015

Member

same question for opt_fast

@bartvde

bartvde Jun 9, 2015

Member

same question for opt_fast

@bartvde

This comment has been minimized.

Show comment
Hide comment
@bartvde

bartvde Jun 9, 2015

Member

I have 2 remaining questions which might be addressed simply by docs:

  1. how does someone remove an unmanaged layer? setMap(null) ?
  2. how is z-index managed if you have more than 1 layer unmanaged?

Since I'm not too familiar with the renderer code, it might be good to have someone more knowledgeable wrt the renderer code look specifically at the rendering changes as well. But I'll leave the call up to you @ahocevar

Member

bartvde commented Jun 9, 2015

I have 2 remaining questions which might be addressed simply by docs:

  1. how does someone remove an unmanaged layer? setMap(null) ?
  2. how is z-index managed if you have more than 1 layer unmanaged?

Since I'm not too familiar with the renderer code, it might be good to have someone more knowledgeable wrt the renderer code look specifically at the rendering changes as well. But I'll leave the call up to you @ahocevar

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 9, 2015

Member

@bartvde:

  1. how does someone remove an unmanaged layer? setMap(null) ?

Exactly. I added this information to the API docs, and also added a unit test to verify that it works properly.

  1. how is z-index managed if you have more than 1 layer unmanaged?

This is indeed the only breaking change after the removal of ol.FeatureOverlay. All feature overlays shared a single replay group, so zIndex was effectively stacked per map. All vector layers, even when unmanaged, are separate, so each layer stacks zIndex separately. I added this information to the upgrade notes.

Member

ahocevar commented Jun 9, 2015

@bartvde:

  1. how does someone remove an unmanaged layer? setMap(null) ?

Exactly. I added this information to the API docs, and also added a unit test to verify that it works properly.

  1. how is z-index managed if you have more than 1 layer unmanaged?

This is indeed the only breaking change after the removal of ol.FeatureOverlay. All feature overlays shared a single replay group, so zIndex was effectively stacked per map. All vector layers, even when unmanaged, are separate, so each layer stacks zIndex separately. I added this information to the upgrade notes.

+ * When set to `false`, the features will be maintained in an
+ * {@link ol.Collection}, which can be retrieved through
+ * {@link ol.source.Vector#getFeaturesCollection}.
+ *

This comment has been minimized.

@probins

probins Jun 9, 2015

Contributor

I would add that the spatial queries for extent/coordinate only work if set to true

@probins

probins Jun 9, 2015

Contributor

I would add that the spatial queries for extent/coordinate only work if set to true

This comment has been minimized.

@ahocevar

ahocevar Jun 9, 2015

Member

Done.

@ahocevar

ahocevar Jun 9, 2015

Member

Done.

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins Jun 9, 2015

Contributor

There's another change that I think should be mentioned in the release notes: if you were using a FO getFeatures returned a collection whereas in source.Vector it returns an array. Apps will have to take this into account. I have always found this a bit confusing. interaction.Select returns a collection too. IMO it would be better if this was consistent, though as these are marked as stable, perhaps it's too late to change.

Contributor

probins commented Jun 9, 2015

There's another change that I think should be mentioned in the release notes: if you were using a FO getFeatures returned a collection whereas in source.Vector it returns an array. Apps will have to take this into account. I have always found this a bit confusing. interaction.Select returns a collection too. IMO it would be better if this was consistent, though as these are marked as stable, perhaps it's too late to change.

ahocevar added some commits Jun 3, 2015

Alternatively manage features in an ol.Collection
ol.layer.Vector can now manage both an RTree and a Collection of features.
The new useSpatialIndex option allows to opt out of RTree management, and
the new ol.Collection type of the features option allows to opt in for
Collection management.
Allow layers that are not managed by the map
When a layer is configured with a map, it will be added on top of other
layers, and not be managed in the map's features collection. The layerState
will have an 'unmanaged' flag for such layers. For vector layers, this flag
is used to not skip any features.
Get rid of ol.FeatureOverlay
This also introduces a wrapX option to the Draw, Modify and Select
interaction.
@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 9, 2015

Member

@probins:

There's another change that I think should be mentioned in the release notes: if you were using a FO getFeatures returned a collection whereas in source.Vector it returns an array. Apps will have to take this into account. I have always found this a bit confusing. interaction.Select returns a collection too. IMO it would be better if this was consistent, though as these are marked as stable, perhaps it's too late to change.

I added that to the upgrade notes. You're right, it would be more consistent to use getFeatures wherever an array is returned, and getFeaturesCollection when a collection is returned. But since getFeatures is marked stable with different return types on ol.interaction.Select and ol.source.Vector, we have to live with this inconsistency for now.

Member

ahocevar commented Jun 9, 2015

@probins:

There's another change that I think should be mentioned in the release notes: if you were using a FO getFeatures returned a collection whereas in source.Vector it returns an array. Apps will have to take this into account. I have always found this a bit confusing. interaction.Select returns a collection too. IMO it would be better if this was consistent, though as these are marked as stable, perhaps it's too late to change.

I added that to the upgrade notes. You're right, it would be more consistent to use getFeatures wherever an array is returned, and getFeaturesCollection when a collection is returned. But since getFeatures is marked stable with different return types on ol.interaction.Select and ol.source.Vector, we have to live with this inconsistency for now.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 10, 2015

Member

@bartvde:

Since I'm not too familiar with the renderer code, it might be good to have someone more knowledgeable wrt the renderer code look specifically at the rendering changes as well. But I'll leave the call up to you

The only change to renderer code is the removal of the map replay group, which was only used by ol.FeatureOverlay. Do you feel comfortable reviewing this, just to verify that I did not remove more or less than necessary. I'm adding an upgrade note about the removed replay as well.

Edit: this replayGroup was never exposed to the API, so there is no need to mention it in the upgrade notes.

Member

ahocevar commented Jun 10, 2015

@bartvde:

Since I'm not too familiar with the renderer code, it might be good to have someone more knowledgeable wrt the renderer code look specifically at the rendering changes as well. But I'll leave the call up to you

The only change to renderer code is the removal of the map replay group, which was only used by ol.FeatureOverlay. Do you feel comfortable reviewing this, just to verify that I did not remove more or less than necessary. I'm adding an upgrade note about the removed replay as well.

Edit: this replayGroup was never exposed to the API, so there is no need to mention it in the upgrade notes.

@bartvde

This comment has been minimized.

Show comment
Hide comment
@bartvde

bartvde Jun 10, 2015

Member

I did review it @ahocevar and it looked good to me.

Member

bartvde commented Jun 10, 2015

I did review it @ahocevar and it looked good to me.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 10, 2015

Member

Thanks @bartvde. Is this good to merge?

Member

ahocevar commented Jun 10, 2015

Thanks @bartvde. Is this good to merge?

@bartvde

This comment has been minimized.

Show comment
Hide comment
@bartvde

bartvde Jun 10, 2015

Member

LGTM

Member

bartvde commented Jun 10, 2015

LGTM

ahocevar added a commit that referenced this pull request Jun 10, 2015

@ahocevar ahocevar merged commit fad3cf9 into openlayers:master Jun 10, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.12%) to 72.5%
Details

@ahocevar ahocevar deleted the ahocevar:remove-featureoverlay branch Jun 10, 2015

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Jun 11, 2015

Member

Shouldn't unmanaged be added to ol.layer.State?

Member

elemoine commented Jun 11, 2015

Shouldn't unmanaged be added to ol.layer.State?

@elemoine elemoine referenced this pull request in camptocamp/ngeo Jun 11, 2015

Closed

ol.FeatureOverlay removal #236

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 11, 2015

Member

Good call @elemoine. I'll create a pull request.

Member

ahocevar commented Jun 11, 2015

Good call @elemoine. I'll create a pull request.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
Member

ahocevar commented Jun 11, 2015

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Jun 11, 2015

Member

Quick note: proviously, all the feature overlays attached to a map were equal from the "feature render order" point of view. It's not the case with "umanaged" vector layers. "Unmanaged" vector layers are ordered: the features of the first "unmanaged" vector layer will be drawn below the features of the second "unmanaged" vector layer, etc.

Member

elemoine commented Jun 11, 2015

Quick note: proviously, all the feature overlays attached to a map were equal from the "feature render order" point of view. It's not the case with "umanaged" vector layers. "Unmanaged" vector layers are ordered: the features of the first "unmanaged" vector layer will be drawn below the features of the second "unmanaged" vector layer, etc.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 11, 2015

Member

@elemoine True. I mentioned that in the upgrade notes.

Member

ahocevar commented Jun 11, 2015

@elemoine True. I mentioned that in the upgrade notes.

@bartvde

This comment has been minimized.

Show comment
Hide comment
@bartvde

bartvde Jun 11, 2015

Member

@elemoine do you know of a use case where this will be problematic? We couldn't think of one.

Member

bartvde commented Jun 11, 2015

@elemoine do you know of a use case where this will be problematic? We couldn't think of one.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Jun 11, 2015

Member

@elemoine do you know of a use case where this will be problematic? We couldn't think of one.

The problem is related to components that create vector layers internally (e.g. select interaction).

Member

elemoine commented Jun 11, 2015

@elemoine do you know of a use case where this will be problematic? We couldn't think of one.

The problem is related to components that create vector layers internally (e.g. select interaction).

@tschaub

This comment has been minimized.

Show comment
Hide comment
@tschaub

tschaub Jun 12, 2015

Member

WOW

Member

tschaub commented Jun 12, 2015

WOW

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Jun 15, 2015

Member

The problem is related to components that create vector layers internally (e.g. select interaction).

In our application we have independent components, each using a FeatureOverlay with a specific style. The components could share an "unmanaged" vector layer, but I wonder how we can make the components use specific styles. We'll probably need to use per-feature styles, which will make our code messier.

Member

elemoine commented Jun 15, 2015

The problem is related to components that create vector layers internally (e.g. select interaction).

In our application we have independent components, each using a FeatureOverlay with a specific style. The components could share an "unmanaged" vector layer, but I wonder how we can make the components use specific styles. We'll probably need to use per-feature styles, which will make our code messier.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 15, 2015

Member

@elemoine As long as your styles don't use zIndex, there should not be a difference between unmanaged vector layers and feature overlays. If you rely on zIndex, you could also leverage an index for distinct feature groups. Assuming you had

var overlay1 = new ol.FeatureOverlay({
  style: /* ... */
});
var overlay2 = new ol.FeatureOverlay({
  style: /* ... */
});

overlay1.addFeature(feature1);
overlay2.addFeature(feature2);

you could now do something like

var layer = new ol.source.Vector({
  source: new ol.source.Vector(),
  style: /* ... */
});
featuresIndex = [{}, {}];
function addFeature(feature, index) {
  layer.getSource().addFeature(feature);
  featuresIndex[index][goog.getUid(feature)] = feature;
}
function removeFeature(feature, index) {
  layer.getSource().removeFeature(feature);
  featuresIndex[index][goog.getUid(feature)] = undefined;
}

addFeature(feature1, 0);
addFeature(feature2, 1);

In the styleFunction you just check for the feature whether it is in featuresIndex[0] or featuresIndex[2], and style it appropriately.

Member

ahocevar commented Jun 15, 2015

@elemoine As long as your styles don't use zIndex, there should not be a difference between unmanaged vector layers and feature overlays. If you rely on zIndex, you could also leverage an index for distinct feature groups. Assuming you had

var overlay1 = new ol.FeatureOverlay({
  style: /* ... */
});
var overlay2 = new ol.FeatureOverlay({
  style: /* ... */
});

overlay1.addFeature(feature1);
overlay2.addFeature(feature2);

you could now do something like

var layer = new ol.source.Vector({
  source: new ol.source.Vector(),
  style: /* ... */
});
featuresIndex = [{}, {}];
function addFeature(feature, index) {
  layer.getSource().addFeature(feature);
  featuresIndex[index][goog.getUid(feature)] = feature;
}
function removeFeature(feature, index) {
  layer.getSource().removeFeature(feature);
  featuresIndex[index][goog.getUid(feature)] = undefined;
}

addFeature(feature1, 0);
addFeature(feature2, 1);

In the styleFunction you just check for the feature whether it is in featuresIndex[0] or featuresIndex[2], and style it appropriately.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Jun 15, 2015

Member

As I said, currently, each component has its own FeatureOverlay and its own style. The components are independent and don't know about each other. It looks to me that, in the code you provided, the code that creates the shared vector layer knows the number of components, and it should know how each component wants to style its features. So, at this point, I honestly don't think this represents a good solution to my problem.

Member

elemoine commented Jun 15, 2015

As I said, currently, each component has its own FeatureOverlay and its own style. The components are independent and don't know about each other. It looks to me that, in the code you provided, the code that creates the shared vector layer knows the number of components, and it should know how each component wants to style its features. So, at this point, I honestly don't think this represents a good solution to my problem.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Jun 15, 2015

Member

Maybe I can build an API/abstraction around your "index" idea. Any consumer of that API would first ask the API for an identifier or something. It will then use that identifier in subsequent operations like setStyle, addFeature, removeFeature and clear. That may work. Maybe that's what you had in mind.

Member

elemoine commented Jun 15, 2015

Maybe I can build an API/abstraction around your "index" idea. Any consumer of that API would first ask the API for an identifier or something. It will then use that identifier in subsequent operations like setStyle, addFeature, removeFeature and clear. That may work. Maybe that's what you had in mind.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 15, 2015

Member

That sounds good @elemoine, much smarter than the app level solution I suggested above. Thanks!

Member

ahocevar commented Jun 15, 2015

That sounds good @elemoine, much smarter than the app level solution I suggested above. Thanks!

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins Jun 18, 2015

Contributor

There's a difference in display behaviour with this change. If you go to http://openlayers.org/en/master/examples/select-features.html and select the hover option, there's an annoying flickering as the cursor passes over the features. This doesn't occur with http://openlayers.org/en/v3.6.0/examples/select-features.html
Is it constantly adding/removing the feature from the selection layer?

Contributor

probins commented Jun 18, 2015

There's a difference in display behaviour with this change. If you go to http://openlayers.org/en/master/examples/select-features.html and select the hover option, there's an annoying flickering as the cursor passes over the features. This doesn't occur with http://openlayers.org/en/v3.6.0/examples/select-features.html
Is it constantly adding/removing the feature from the selection layer?

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Jun 18, 2015

Member

This is a bug. Thanks for reporting @probins. I'll create a separate ticket for that.

Member

ahocevar commented Jun 18, 2015

This is a bug. Thanks for reporting @probins. I'll create a separate ticket for that.

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