Skip to content
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

Overview asset 2 #13

Merged
merged 27 commits into from
Oct 28, 2021
Merged

Overview asset 2 #13

merged 27 commits into from
Oct 28, 2021

Conversation

DanielJDufour
Copy link
Member

PR to visualize updates to displayOverview work.

Some of the changes are actually from the main branch.

@m-mohr
Copy link
Collaborator

m-mohr commented Oct 26, 2021

Not sure what's going on here, but you seem to really clean things up in addition to adding the overviews, right? Looks awesome!

PS: Chris is about to make some noise about stac-layer tomorrow or so in a blog post, I think...

@DanielJDufour
Copy link
Member Author

Yup. Adding the overview support and sorta fixing the issue with georaster hanging by adding a timeout. Several more check-ins happening asap.

@DanielJDufour
Copy link
Member Author

DanielJDufour commented Oct 27, 2021

@m-mohr , okay. this is ready for your review. Sorry for the delay. There's a few updates.

  • of course the biggest being adding overview support (which you added earlier, but putting it here for completeness sake)
  • displayPreview is enabled, which will look to display preview links or thumbnail assets. it's off by default.
  • issues loading JPG/PNG images whether as tiles or overlays are immediately discovered rather than waiting for the layer to be added to the map. this allows us to proactively respond and return a layer group that is more unlikely to need to call fallbacks
  • code split into more files to ease organization
  • and added more tests!

It might be worth splitting some of the functionality into their own micro packages like imageOverlay, tileLayer, and with-timeout as I could see that functionality being useful in other projects, but I'll let you make that determination.

@DanielJDufour DanielJDufour requested a review from m-mohr October 27, 2021 03:30
@DanielJDufour
Copy link
Member Author

@m-mohr, also if you have time, could you check if this new code works well with stac-browser? I wouldn't want to accidentally break anything!

I could also publish a release candidate to NPM if that makes it easier for you.

@DanielJDufour DanielJDufour changed the title WIP: Overview asset 2 Overview asset 2 Oct 27, 2021
@m-mohr
Copy link
Collaborator

m-mohr commented Oct 27, 2021

Thanks a lot! I'll try to review and test this by the end of the week at the latest.

@m-mohr
Copy link
Collaborator

m-mohr commented Oct 27, 2021

I've tried to build stac-layer on Windows with Node 16.13.0, but can't get it to build.
First I had to make (and comit) two minor changes to make it build on Windows(?) and now I'm getting the following error:

PS C:\Projects\stac-layer> npm run build

> [email protected] build C:\Projects\stac-layer
> webpack

assets by status 1.12 MiB [cached] 1 asset
orphan modules 247 KiB [orphan] 103 modules
runtime modules 1.02 KiB 6 modules
built modules 1.85 MiB [built]
  modules by path ./node_modules/ 1.76 MiB
    modules by path ./node_modules/urijs/src/*.js 98.4 KiB 4 modules
    modules by path ./node_modules/reproject-bbox/ 3.71 KiB
      ./node_modules/reproject-bbox/reproject-bbox.js 3.04 KiB [built] [code generated]
      ./node_modules/reproject-bbox/node_modules/proj4-fully-loaded/proj4-fully-loaded.js 680 bytes [built] [code generated]
  modules by path ./src/ 96.4 KiB
    ./src/index.js + 14 modules 95.8 KiB [built] [code generated]
    ./src/utils/with-timeout.js 640 bytes [built] [code generated] [1 error]
  external {"root":"L","commonjs":"leaflet","amd":"leaflet","commonjs2":"leaflet"} 42 bytes [built] [code generated]

ERROR in ./src/utils/with-timeout.js
Module parse failed: The top-level-await experiment is not enabled (set experiments.topLevelAwait: true to enabled it)
File was processed with these loaders:
 * ./node_modules/babel-loader/lib/index.js
You may need an additional loader to handle the result of these loaders.
Error: The top-level-await experiment is not enabled (set experiments.topLevelAwait: true to enabled it)
    at C:\Projects\stac-layer\node_modules\webpack\lib\dependencies\HarmonyDetectionParserPlugin.js:54:11
    at Hook.eval [as call] (eval at create (C:\Projects\stac-layer\node_modules\tapable\lib\HookCodeFactory.js:19:10), <anonymous>:7:16)
    at Hook.CALL_DELEGATE [as _call] (C:\Projects\stac-layer\node_modules\tapable\lib\Hook.js:14:14)
    at JavascriptParser.walkAwaitExpression (C:\Projects\stac-layer\node_modules\webpack\lib\javascript\JavascriptParser.js:2306:29)
    at JavascriptParser.walkExpression (C:\Projects\stac-layer\node_modules\webpack\lib\javascript\JavascriptParser.js:2236:10)
    at JavascriptParser.walkExpressionStatement (C:\Projects\stac-layer\node_modules\webpack\lib\javascript\JavascriptParser.js:1624:8)
    at JavascriptParser.walkStatement (C:\Projects\stac-layer\node_modules\webpack\lib\javascript\JavascriptParser.js:1551:10)
    at JavascriptParser.walkStatements (C:\Projects\stac-layer\node_modules\webpack\lib\javascript\JavascriptParser.js:1445:9)
    at JavascriptParser.parse (C:\Projects\stac-layer\node_modules\webpack\lib\javascript\JavascriptParser.js:3330:9)
    at C:\Projects\stac-layer\node_modules\webpack\lib\NormalModule.js:1077:26
 @ ./src/utils/create-georaster-layer.js 13:0-44 15:9-20
 @ ./src/index.js 42:0-69 638:19-39 803:19-39 885:19-39 970:19-39

webpack 5.60.0 compiled with 1 error in 29375 ms
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] build: `webpack`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] build script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\Matthias Mohr\AppData\Roaming\npm-cache\_logs\2021-10-27T20_49_50_821Z-debug.log

While I could certainly enable this feature, I don't think it should be required to use stac-layer. I must admit I don't fully understand the line that is erroring (await withTimeout(1000, () => {});) is used for. I removed it and then it builds...

@m-mohr
Copy link
Collaborator

m-mohr commented Oct 27, 2021

I was able to run stac-layer in STAC Browser.
While for example the Planet data works as before, I now get an error for some CBERS data.

Before it worked, see https://radiantearth.github.io/stac-browser/#/external/cbers-stac-1-0-0.s3.amazonaws.com/CBERS4/MUX/001/022/CBERS_4_MUX_20210819_001_022_L2.json

That's the result now:
grafik

I'm not 100% sure yet whether that's an issue on stac-layers side or whether that is due to some CORS issues (I'm testing on localhost instead of on github pages right now).

Overall it seems you didn't break anything, I'd suspect the issue above is CORS. Maybe you can give it a quick try yourself.

Lastly, it indeed does not show the previews anymore, which is good. I don't think I have a public example with an overview though.

Thanks!

@m-mohr m-mohr linked an issue Oct 27, 2021 that may be closed by this pull request
@DanielJDufour
Copy link
Member Author

@m-mohr , regarding the build issue on Windows. That was totally my fault. I left in some test code accidentally. I removed it and committed the fix you identified. Sorry again for that.

@DanielJDufour
Copy link
Member Author

I think there's two things going on with the CBERS data. The first is that, the previous version of stac-layer displayed assets with the key "thumbnail", the new version treats thumbnails as previews and hides them by default. That's why we see something with the old version and no imagery with the new.

Regarding the other issue with the Network Error. I think this is because the assets are using the S3 protocol, like "s3://..." and so we can't fetch them in the browser.

I'm not sure the best way to treat thumbnails. If you'd like them to be treated as overviews, we could definitely update the code to do that.

@DanielJDufour
Copy link
Member Author

Sorry, missed another issue, which is that in the new version no layer even loads. I'll look into that.

@DanielJDufour
Copy link
Member Author

It's a bit odd because the CBERS/thumb test works as intended here:

Screen Shot 2021-10-27 at 7 10 52 PM

@m-mohr
Copy link
Collaborator

m-mohr commented Oct 27, 2021

Oh, indeed, that are thumbnails for CBERS. Sorry, that is my fault, did not realize that. So it should not display anything by default as thumbnails usually don't make a lot of sense on the map, but is also doesn't load the COGs once selected. That's due to s3, I guess?

@DanielJDufour
Copy link
Member Author

Yes, that's correct. But for some reason it appears an error is not being caught. I'm not sure if that's on the stac-layer or stac-browser side.

@m-mohr
Copy link
Collaborator

m-mohr commented Oct 27, 2021

The issue seems to be:

Uncaught TypeError: this._southWest is undefined
    getCenter leaflet-src.js:1187
    wrapLatLngBounds leaflet-src.js:1588
    wrapLatLngBounds leaflet-src.js:3999
    _tileCoordsToBounds leaflet-src.js:11378
    _isValidTile leaflet-src.js:11354
    _update leaflet-src.js:11305
    _onMoveEnd leaflet-src.js:11252
    wrapperFn leaflet-src.js:100
    fire leaflet-src.js:588
    invalidateSize leaflet-src.js:3562
    _resizeRequest leaflet-src.js:4305

I need to check whether that is a STAC Browser issue, but will have to sleep first. ;-)

@DanielJDufour
Copy link
Member Author

@m-mohr , I think the issue is that stac-layer is compiled with a different instance of Leaflet than the one that stac-browser uses, so this line fails, which causes the whole adding to the map to throw an error.

I'll try to help brainstorm solutions. One would be trying to directly import stacLayer from "stac-layer/src/index.js", but I think that would require you to add babel plugins to stac-browser.

Another solution would be to try to override the addTo function and somehow get the instance of Leaflet and the bounds constructor from the map object and then on-the-fly recreate the bounds object using the correct instance of Leaflet. (but I don't think this would work if the user typed map.addLayer(lyr).

But I have to admin I'm a bit perplexed and not sure I have correctly diagnosed the issue. I thought added Leaflet to the externals in the webpack.config.cjs would have fixed this problem.

We could also try adding a new build that is just running babel without all the other webpack stuff. Therefore where a user imports stac-layer they are essentially importing it and running through their own compiler. We could still keep the webpack build for unpkg jsdelivr and other CDNs or people who are loading it through a script tag. But this would require a lot of testing as I'm not sure what different build tools look for with regards to keys in package.json (main vs. browser, etc.). This might be something we'd want to do anyway, because it could allow adopters/users to have smaller builds.

Lastly, we could try to submit a PR to Leaflet to relax the super strict instanceof.

Thoughts?

@m-mohr
Copy link
Collaborator

m-mohr commented Oct 28, 2021

Okay, that makes sense. I often face this issue when linking dev packages locally. Usually that is only a local issue and once stac-layer would be released it's working again. So from my side I think we can simply release after the open PRs have been merged. I expect this to work again and if it is not working, we can look into a fix again and release a patch version.

@DanielJDufour
Copy link
Member Author

Sounds good.

@DanielJDufour DanielJDufour merged commit 4bc3153 into overview-asset Oct 28, 2021
@m-mohr m-mohr deleted the overview-asset-2 branch November 4, 2021 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preview/Thumbnail shown on Map
2 participants