TDesjardins / gwt-ol

GWT wrapper for OpenLayers 3+ using JSInterop
Apache License 2.0
70 stars 33 forks source link

Problems with FeatureAtPixelOptions.layerFilter #155

Closed gkresic closed 4 years ago

gkresic commented 4 years ago

Looks a lot like same issue as in #154 regarding ol.layer.Vector not been seen as an instance of ol.layer.Layer by GWT.

This code throws ClassCastException first time filter function is called:

ol.Pixel pixel = ...
ol.Map map = ...
ol.FeatureAtPixelOptions options = new ol.FeatureAtPixelOptions();
options.setHitTolerance(10);
options.setLayerFilter(new ol.GenericFunction<ol.layer.Layer, Boolean>() {
    @Override
    public Boolean call(ol.layer.Layer layer) { // "stepping into" on this line brings us to Cast.castToNative with jsType set to "undefined" which results in ClassCastException
        return true;
    }
});
map.forEachFeatureAtPixel(pixel, (feature, layer) -> false, options);

Experimenting on what I have learned from #154, I modified ol.FeatureAtPixelOptions.setLayerFilter to accept ol.layer.Vector instead of ol.layer.Layer and using this example code, but, as would be expected it didn't go well, too:

ol.Pixel pixel = ...
ol.Map map = ...
ol.FeatureAtPixelOptions options = new ol.FeatureAtPixelOptions();
options.setHitTolerance(10);
options.setLayerFilter(new ol.GenericFunction<ol.layer.Vector, Boolean>() {
    @Override
    public Boolean call(ol.layer.Vector layer) {    // "stepping into" on this line brings us to Cast.castToNative but only when encountering first layer that is not ol.layer.Vector (but, for example, ol.layer.Tile)
        return true;
    }
});
map.forEachFeatureAtPixel(pixel, (feature, layer) -> false, options);

Only way I got it to work was to modify ol.FeatureAtPixelOptions.setLayerFilter to accept non-specialized GenericFunction<?, Boolean> and then to use:

ol.Pixel pixel = ...
ol.Map map = ...
ol.FeatureAtPixelOptions options = new ol.FeatureAtPixelOptions();
options.setHitTolerance(10);
options.setLayerFilter(new ol.GenericFunction<Object, Boolean>() {
    @Override
    public Boolean call(Object layer) { // works
        return true;
    }
});
map.forEachFeatureAtPixel(pixel, (feature, layer) -> false, options);

casting layer to whatever I want.

Interesting fact: #154 is manifesting in both GWT 2.8.2 and 2.9, but this one is problem only in 2.9.

I can try to debug this further, but from this point on, I'll need an assistance from someone more experienced with JsInterop.

TDesjardins commented 4 years ago

@gkresic Thanks for reporting. Which version of gwt-ol and OpenLayers you are using?

gkresic commented 4 years ago

gwt-ol locally built from master branch, OpenLayers is 5.3.0.

gkresic commented 4 years ago

Forgot to mention: in HEAD-SNAPSHOT in master, there is currently version of ol.FeatureAtPixelOptions.setLayerFilter that accepts ol.layer.Vector instead of ol.layer.Layer. However, according to OL docs (and common sense), it should accept ol.layer.Layer. However, as I said, none of that versions works correctly in GWT 2.9.

TDesjardins commented 4 years ago

@gkresic I will have a look. Please note that gwt-ol currently doesn't support GWT 2.9 because GWT 2.9 uses new versions of jsinterop.base (1.0.0) and elemental2 (1.0.0) which are not compatible with GWT 2.8 and older jsinterop.base and elemental2 versions. So you can't have a version that supports both. Because of this I will create a branch for 2.9.0 support. You have to switch then. Please note that this is also true for other libs which uses elemental2 and jsinterop.base.

TDesjardins commented 4 years ago

Forgot to mention: in HEAD-SNAPSHOT in master, there is currently version of ol.FeatureAtPixelOptions.setLayerFilter that accepts ol.layer.Vector instead of ol.layer.Layer. However, according to OL docs (and common sense), it should accept ol.layer.Layer.

Seems I missed this in review. The history says that you contributed this code 😉 Could you make a PR for this?

gkresic commented 4 years ago

Seems I missed this in review. The history says that you contributed this code Could you make a PR for this?

Done: #156

Rather trivial change, I've made it while trying to find root cause for this issue (and forgot about it).

gkresic commented 4 years ago

I will have a look. Please note that gwt-ol currently doesn't support GWT 2.9 because GWT 2.9 uses new versions of jsinterop.base (1.0.0) and elemental2 (1.0.0) which are not compatible with GWT 2.8 and older jsinterop.base and elemental2 versions. So you can't have a version that supports both. Because of this I will create a branch for 2.9.0 support. You have to switch then. Please note that this is also true for other libs which uses elemental2 and jsinterop.base.

Is there any changelog for backward-incompatible changes between:

elemental2-1.0.0-RC1 (GWT 2.8.2) -> elemental2 1.0.0 (GWT 2.9.0) jsinterop-annotations-1.0.2 (GWT 2.8.2) -> jsinterop-annotations 2.0.0 (GWT 2.9.0)

I may try to resolve conflicting changes, but would appreciate any starting pointers.

BTW, this problem with CCE and derived classes is so far only problem I've experienced when switching to GWT 2.9. However, I'm using only small portion of gwt-ol and I didn't test in details.

TDesjardins commented 4 years ago

@gkresic I have created a working gwt-ol branch for GWT 2.9. There are not that much changes. If you like to switch to 2.9 you can now build it locally.

I tried to reproduce the CCE and after some investigations I am sure that this is the same cause like in #154 . The type info (@api) of ol.layer.Layer is missing in OpenLayers in the mentioned versions. If you try to cast to an object to ol.layer.Layer or a subtype (ol.layer.Vector) the CCE occures. Casting to ol.layer.Base is working for example.

Interesting fact: #154 is manifesting in both GWT 2.8.2 and 2.9, but this one is problem only in 2.9.

I guess you are mistaken. I can reproduce this behaviour in 2.8.2 and 2.9. Could you recheck this?

The remaining questions is how to handle this issue because type issues restrict the usage. An idea is to change the type to ol.layer.Base in the filter and the function with a comment that this is a workaround for an OpenLayers bug and that it is solved in newer versions.

gkresic commented 4 years ago

Interesting fact: #154 is manifesting in both GWT 2.8.2 and 2.9, but this one is problem only in 2.9.

I guess you are mistaken. I can reproduce this behaviour in 2.8.2 and 2.9. Could you recheck this?

Yes, I confirm there is a scenario in which behavior changes between GWT 2.8.2 and 2.9.0, but it's probably related to upgraded JDT compiler in GWT 2.9, because there it fails as it should while GWT 2.8.2 silently allow this obviously illegal behavior.

Steps to reproduce:

  1. revert ol.FeatureAtPixelOptions.[get|set]LayerFilter to use ol.layer.Vector instead of ol.layer.Layer
  2. set layer filter using lambda

Second step is important - if you set layer filter using method reference, it fails in both versions of GWT, but lambda version fails only in GWT 2.9.

Code:

ol.Pixel pixel = ...
ol.Map map = ...
ol.FeatureAtPixelOptions options = new ol.FeatureAtPixelOptions();
options.setHitTolerance(10);
options.setLayerFilter(layer -> true);
map.forEachFeatureAtPixel(pixel, (feature, layer) -> false, options);
gkresic commented 4 years ago

The remaining questions is how to handle this issue because type issues restrict the usage. An idea is to change the type to ol.layer.Base in the filter and the function with a comment that this is a workaround for an OpenLayers bug and that it is solved in newer versions.

Ideal solution would be to switch to OpenLayers 6, but guessing from a separate branch that option is not available yet. Meanwhile, I've created PR (#158) which uses ol.layer.Base as you suggested because I can't think of a better solution, either.

TDesjardins commented 4 years ago

I could create a release for OL6. Maybe it's time to switch master to OL6 and keep a branch for older versions. I think the type bug is a good reason for going foward. WDYT?

gkresic commented 4 years ago

I could create a release for OL6. Maybe it's time to switch master to OL6 and keep a branch for older versions. I think the type bug is a good reason for going foward. WDYT?

If support for OpenLayer 6 is adequate, I can't see a reason not to switch to it, together with migration to GWT 2.9 as a dependency.

TDesjardins commented 4 years ago

If support for OpenLayer 6 is adequate, I can't see a reason not to switch to it, together with migration to GWT 2.9 as a dependency.

The ol6 branch is working fine except of unit tests because old JSUnit version causes problems with new browser apis. I would prefer to first release a version based on GWT 2.8 because I suppose that there are a lot of users which won't update to 2.9 in the near future.

TDesjardins commented 4 years ago

The ol6 branch is working fine except of unit tests because old JSUnit version causes problems with new browser apis.

Also the JsUnit tests work right now. See https://travis-ci.org/github/TDesjardins/gwt-ol/builds/699610983