-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
STAC support (a STAC layer group) #14927
base: main
Are you sure you want to change the base?
Conversation
📦 Preview the website for this branch here: https://deploy-preview-14927--ol-site.netlify.app/. |
Impressive work @m-mohr, thanks!! since you're asking for feedback here are a few things that came to my mind when reading your PR. Note that some aspects of the PR may have already been discussed and agreed on so this is just my opinion!
Again, this is just my opinion and not a definitive review. And it's great to see first-class STAC support coming in OpenLayers! |
Thank you for your feedback @jahow, I appeciate it!
This is something we could improve, of course, e.g. by having options like
That's why it is a LayerGroup, of course. If you just want XYZ or GeoTiff or VectorLayer, stac-js makes it very simple to just use those separately, but then you don't need any STAC integration at all in OL, I think. The LayerGroup is meant to make life very easy and give a consistent rendering behavior for STAC. But I can understand that this might be too much magic for the core OL library. In that case an option could also be to just include the change to the GeoTiff in this PR, and then keep the magic/LayerGroup as extension outside of this repo. Also fine with me. Just let me know what the maintainers think about it.
Sure, I've created a stacUtil.js to externalize some of the code. Other than that, I'd need to dig a bit deeper to see what makes sense. If you have something obvious in mind, please let me know.
Yeah, I think it was for debugging purposes at some point. I could add a layer switcher there, which could help visualize the layer tree structure. Not sure whether that's something you would want in an example though? |
a98ec25
to
d183dee
Compare
7e01122
to
f9a1f7b
Compare
src/ol/layer/STAC.js
Outdated
const features = new GeoJSON().readFeatures(geojson, { | ||
featureProjection: 'EPSG:3857', // TODO: revisit this | ||
}); | ||
const source = new VectorSource({features}); | ||
const vectorLayer = new VectorLayer({ | ||
source, | ||
style: getBoundsStyle(this.boundsStyle_, this), | ||
}); | ||
vectorLayer.set('bounds', true); | ||
features.forEach((feature) => feature.set('stac', data)); |
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.
A few thoughts here:
- If you configure the source with a
loader
instead offeatures
, you can use the view projection, which is passed to the loader as 3rd argument. Then you do not need to hard-code the projection. - Instead of spreading metadata across assets that the STAC layer group creates, I'd suggest making them available through the STAC instance (e.g.
getBoundsLayer()
instead of setting abounds
boolean on the layer. Same for thestac
data, which should not be set on each feature.
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.
Thanks, good pointers!
- Switched to use a loader, which seems to work perfectly.
- I removed the stac metadata for the features, this is not needed anymore (due to the change for STAC support (a STAC layer group) #14927 (comment)). I added a
getBoundsLayer()
method, but the metadata on the assets is still valuable due to the nesting of the LayerGroups, I think. In case you go through all layers, it is helpful to be able to retrieve the relevant STAC entities. Using that for example for customized LayerSwitcher or for the "click detection" in stacUtils / the STAC Item Collection example.
dd3bcb8
to
ddc2698
Compare
src/ol/source/GeoTIFF.js
Outdated
@@ -382,7 +384,7 @@ class GeoTIFFSource extends DataTile { | |||
super({ | |||
state: 'loading', | |||
tileGrid: null, | |||
projection: null, | |||
projection: options.projection || null, |
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.
I've extracted the changes in this file to #15036 so that we could also release this PR separately, if required.
@@ -0,0 +1,25 @@ | |||
/** |
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.
This was a shared event between MapboxVector and STAC, but now that MapboxVector was removed I could also keep it outside of the events folder and copy it into stacUtils.js or STAC.js. Whatever you prefer.
Hey there, |
Thanks @m-mohr for your effort on this, and having this as a separate library makes perfect sense! No objections from me on how the website looks. Would you mind creating a pull request that adds your library to the list of 3rd party libraries in Also, to make this more prominent in OpenLayers, I think it would be nice to have an example here that uses your library. |
Thanks, @ahocevar. If you still want STAC be part of OL, I'm also happy to also have it live in OpenLayers directly. Up to the maintainers :-) But I wanted to get this out so people can start using it and give feedback.
Sure, see #15041
I tried this, but couldn't figure out how it works with the dependencies. How can I reference external dependencies? If I just add the ol-stac import, it can't resolve it. Any hints? devDependencies? See #15042 |
This PR proposes to add STAC capabilities to OpenLayers. It adds a STAC LayerGroup.
The layer group creates various layers depending on the given options and what the STAC entity offers, e.g. footprints, COGs and thumbnails. What usually is the source is the stac-js library here.
Examples: https://deploy-preview-14927--ol-site.netlify.app/en/latest/examples/?q=stac
The work is based on PR #14101 from @tschaub (he's aware of this work).
The interface is mostly aligned with the stac-layer library for Leaflet (https://github.com/stac-utils/stac-layer), which is also based on stac-js and offers pretty much the same functionality.
An important question is whether this is something for OL core or whether I should publish this as an extension.
The functionality implemented here includes:
getSourceOptions
(implemented by @tschaub) to provide a function that returns options for the source given item metadata. The Planetary Computer example uses this option to sign the asset URLs before the source is created.Adds aMoved to Allow to set GeoTiff projection #15036projection
option to theGeoTiff
source to allow overriding the auto-detected projection with what STAC providesTodos:
Curious to get thoughts from others and maybe point me to things that can be improved.
PS: The work on this was funded by Planet and EOX.