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

Do not ignore layer filter for unmanaged layers #4143

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@elemoine
Member

elemoine commented Sep 21, 2015

PR #3883 made forEachFeatureAtPixel ignore unmanaged layers. This commit reverts that change, and modifies the Select interaction to make sure that the unmanaged layer it uses internally is not filtered out when the user provides a layer filter.

So this PR fixes #3878 in a different way, and it addresses the limitation reported in #2940, where unmanaged layers cannot be filtered out.

@ahocevar, @probins, do you anticipate problems with that approach?

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Sep 21, 2015

Member

Not sure actually. Since we create unmanaged layers for the Draw and Modify interactions as well, these two controls might also need modifications. Since this is a change in behavior, it should also be added to the release notes.

Member

ahocevar commented Sep 21, 2015

Not sure actually. Since we create unmanaged layers for the Draw and Modify interactions as well, these two controls might also need modifications. Since this is a change in behavior, it should also be added to the release notes.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Sep 22, 2015

Member

Thanks for your comments @ahocevar.

Since we create unmanaged layers for the Draw and Modify interactions as well, these two controls might also need modifications.

Not sure.

With the current code, the layer filter is ignored for the Draw and Modify interactions' unmanaged layers. This means that features in these layers will always be detected by the Select interaction.

With my patch, the layer filter will no longer be ignored for the Draw and Modify interactions' unmanaged layers. It means that, if the Select interaction is configured with a layer filter, the Select interaction will not detect features drawn by the Draw and Modify interactions, which I think is fine.

My goal with this patch is to provide more flexibility, where the user can filter out unmanaged layers if that's what he wishes.

Since this is a change in behavior, it should also be added to the release notes.

Sure. I can add that if you think my patch makes sense.

Member

elemoine commented Sep 22, 2015

Thanks for your comments @ahocevar.

Since we create unmanaged layers for the Draw and Modify interactions as well, these two controls might also need modifications.

Not sure.

With the current code, the layer filter is ignored for the Draw and Modify interactions' unmanaged layers. This means that features in these layers will always be detected by the Select interaction.

With my patch, the layer filter will no longer be ignored for the Draw and Modify interactions' unmanaged layers. It means that, if the Select interaction is configured with a layer filter, the Select interaction will not detect features drawn by the Draw and Modify interactions, which I think is fine.

My goal with this patch is to provide more flexibility, where the user can filter out unmanaged layers if that's what he wishes.

Since this is a change in behavior, it should also be added to the release notes.

Sure. I can add that if you think my patch makes sense.

Do not ignore layer filter for unmanaged layers
PR #3883 made `forEachFeatureAtPixel` ignore unmanaged layers. This commit reverts that change, and modifies the Select interaction to make sure that the unmanaged layer it uses internally is not filtered out when the user provides a layer filter.
@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Sep 24, 2015

Member

@ahocevar more opinions about this? As I said I'm happy to add release notes if you agree with the change.

Member

elemoine commented Sep 24, 2015

@ahocevar more opinions about this? As I said I'm happy to add release notes if you agree with the change.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Sep 24, 2015

Member

I'll leave the final judgement to someone else. I really don't understand the implications of this change. All I know is that the old feature overlays could not be filtered out as well, so I tend to lean towards thinking that this proposed change here is not a good one.

Member

ahocevar commented Sep 24, 2015

I'll leave the final judgement to someone else. I really don't understand the implications of this change. All I know is that the old feature overlays could not be filtered out as well, so I tend to lean towards thinking that this proposed change here is not a good one.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Sep 24, 2015

Member

I am just looking at providing more flexibility and making unmanaged layers work similarly to managed layers. People use unmanaged layers because they're always on top, but they don't expect their layer filters to be ignored for unmanaged layers.

Member

elemoine commented Sep 24, 2015

I am just looking at providing more flexibility and making unmanaged layers work similarly to managed layers. People use unmanaged layers because they're always on top, but they don't expect their layer filters to be ignored for unmanaged layers.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Sep 24, 2015

Member

I think we should discourage the use of unmanaged layers just for the sake of being on top. Especially since we can now set a z-index on a layer.

Member

ahocevar commented Sep 24, 2015

I think we should discourage the use of unmanaged layers just for the sake of being on top. Especially since we can now set a z-index on a layer.

@elemoine

This comment has been minimized.

Show comment
Hide comment
@elemoine

elemoine Sep 24, 2015

Member

I think we should discourage the use of unmanaged layers just for the sake of being on top. Especially since we can now set a z-index on a layer.

Also a good point.

Member

elemoine commented Sep 24, 2015

I think we should discourage the use of unmanaged layers just for the sake of being on top. Especially since we can now set a z-index on a layer.

Also a good point.

@probins

This comment has been minimized.

Show comment
Hide comment
@probins

probins Sep 26, 2015

Contributor

I think we should discourage the use of unmanaged layers just for the sake of being on top

I'd agree with that too. The use cases for unmanaged layers in apps (as opposed to internally in interactions for temporary feature display) have never been all that clear to me. The current examples (adapted from the old FeatureOverlay) were written before the select interaction existed. I think most of them could be changed (and simplified) to use a select interaction for visual highlighting of a particular feature instead.

Like Andreas, I'm not sure what implications this change would have.

Contributor

probins commented Sep 26, 2015

I think we should discourage the use of unmanaged layers just for the sake of being on top

I'd agree with that too. The use cases for unmanaged layers in apps (as opposed to internally in interactions for temporary feature display) have never been all that clear to me. The current examples (adapted from the old FeatureOverlay) were written before the select interaction existed. I think most of them could be changed (and simplified) to use a select interaction for visual highlighting of a particular feature instead.

Like Andreas, I'm not sure what implications this change would have.

@bartvde

This comment has been minimized.

Show comment
Hide comment
@bartvde

bartvde Sep 28, 2015

Member

I'm seeing something weird and I believe related in #4114 where unmanaged layers are not filtered out actually, i.e. the unmanaged layer from the draw interaction is taken into account when the select interaction uses map.forEachFeatureAtPixel

Member

bartvde commented Sep 28, 2015

I'm seeing something weird and I believe related in #4114 where unmanaged layers are not filtered out actually, i.e. the unmanaged layer from the draw interaction is taken into account when the select interaction uses map.forEachFeatureAtPixel

@bartvde

This comment has been minimized.

Show comment
Hide comment
@bartvde

bartvde Sep 28, 2015

Member

So it seems my issue in #4114 could be handled with this PR and then:

map.addInteraction(new ol.interaction.Select({
  layers: function(layer) {
    if (layer instanceof ol.layer.Vector) {
      return layer.getSource().getFeaturesCollection() === null;
    }
  } 
}));
Member

bartvde commented Sep 28, 2015

So it seems my issue in #4114 could be handled with this PR and then:

map.addInteraction(new ol.interaction.Select({
  layers: function(layer) {
    if (layer instanceof ol.layer.Vector) {
      return layer.getSource().getFeaturesCollection() === null;
    }
  } 
}));
@alexbrault

This comment has been minimized.

Show comment
Hide comment
@alexbrault

alexbrault Nov 6, 2015

All I know is that the old feature overlays could not be filtered out as well, so I tend to lean towards thinking that this proposed change here is not a good one.

While the old feature overlays couldn't be filtered out, there was a way to effectively ignore them: the callback received a null layer. As it is, there's no simple way for application code to determine whether the layer was filter-approved or if it's unmanaged.

I'm currently facing a problem where my application includes a Select interaction and a custom interaction using forEachFeatureAtPixel with a layer filter. The custom interaction should only interact with the features in the layers it manages, but unfortunately, it also finds the selected features. I can work around this issue by calling my layer filter myself instead of passing it to forEachFeatureAtPixel, but if this is the solution, it leaves me wondering what's the point of passing a layerFilter if it doesn't really filter out the layers I don't want

alexbrault commented Nov 6, 2015

All I know is that the old feature overlays could not be filtered out as well, so I tend to lean towards thinking that this proposed change here is not a good one.

While the old feature overlays couldn't be filtered out, there was a way to effectively ignore them: the callback received a null layer. As it is, there's no simple way for application code to determine whether the layer was filter-approved or if it's unmanaged.

I'm currently facing a problem where my application includes a Select interaction and a custom interaction using forEachFeatureAtPixel with a layer filter. The custom interaction should only interact with the features in the layers it manages, but unfortunately, it also finds the selected features. I can work around this issue by calling my layer filter myself instead of passing it to forEachFeatureAtPixel, but if this is the solution, it leaves me wondering what's the point of passing a layerFilter if it doesn't really filter out the layers I don't want

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Nov 6, 2015

Member

While the old feature overlays couldn't be filtered out, there was a way to effectively ignore them: the callback received a null layer.

Very good point, thanks for mentioning this. I'm preparing a pull request that brings this behaviour back.

Member

ahocevar commented Nov 6, 2015

While the old feature overlays couldn't be filtered out, there was a way to effectively ignore them: the callback received a null layer.

Very good point, thanks for mentioning this. I'm preparing a pull request that brings this behaviour back.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Nov 22, 2015

Member

With #4450 merged, it finally makes sense to make the proposed change to src/ol/renderer/maprenderer.js. See #4471 for a remaining issue that's fixed with the proposed change.

Member

ahocevar commented Nov 22, 2015

With #4450 merged, it finally makes sense to make the proposed change to src/ol/renderer/maprenderer.js. See #4471 for a remaining issue that's fixed with the proposed change.

@ahocevar

This comment has been minimized.

Show comment
Hide comment
@ahocevar

ahocevar Nov 22, 2015

Member

Updated to apply to current master as #4472.

Member

ahocevar commented Nov 22, 2015

Updated to apply to current master as #4472.

@ahocevar ahocevar closed this Nov 22, 2015

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