-
Notifications
You must be signed in to change notification settings - Fork 55
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
Ol v6141 #1513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested mainly the cursor hover thing and it fails for image layers that are not tiled.
I have only tested basic featureInfo on the simplest setup with one WMS layer and one WFS layer hosted on my local server, and it seems to work as intended by popping up a info window if there is a feature there and not create a remote request unless there is a symbol there.
As a consequence this PR will affect the way featureInfo works as the previous implementation very often created a remote request to a layer even if there were no pixels visible at that pixel as several layers shared canvas, which in effect could give you hits in layers with invisible symbology. That is no longer possible if someone exploited that flaw. Also the previous implementation had a pixel tolerance, which this implementation lacks. It may not affect anything as when I last tested i couldn't make the tolerance setting do any difference.
src/featureinfo.js
Outdated
const layers = viewer.getQueryableLayers().filter(layer => layer instanceof TileLayer); | ||
for (let i = 0; i < layers.length; i += 1) { | ||
const layer = layers[i]; | ||
if (layer.getData(evt.pixel) instanceof Uint8ClampedArray && layer.getData(evt.pixel)[3] > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If renderMode is "image" for a WMS layer test will fail. Don't know why anyone would not tile an image layer, but it should test for ImageLayer as well and possibly test for BaseTileLayer instead of TileLayer. Apparently getData actually exists on all layers, but documentation states it only exist on ImageLayer and BaseTileLayer so checking for its existance is useless.
} | ||
viewer.getQueryableLayers().forEach(layer => { | ||
if (layer.getData(pixel) instanceof Uint8ClampedArray && layer.getData(pixel)[3] > 0) { | ||
const item = getGetFeatureInfoRequest({ layer, coordinate }, viewer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little risky here. getData exists on all layer types, but is only documented on ImageLayer and BaseTileLayer. Calling it on a feature layer is harmless now (it returns null), but as it is not documented it could break.
Also calling getData two times is painful on my eyes, but it will not have any greater impact on performance.
Now I've added support for ImageLayers as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with LMSearch plugin since I was a bit worried that the new OL version might cause a problem, but it seems to work fine so no worries.
Fixes #1423
Fixes #1308
Due to a soon to be deprecated method I made some changes in files modified by #1292 so please @steff-o have a look at the changes.