WIP: Experimental integration of viv loaders #1441
Conversation
modules/tiff/package.json
Outdated
| "@loaders.gl/worker-utils": "3.0.0-alpha.18", | ||
| "fast-deep-equal": "^3.1.3", | ||
| "fast-xml-parser": "^3.16.0", | ||
| "geotiff": "ilan-gold/geotiff.js#ilan-gold/viv_094", |
There was a problem hiding this comment.
@ilan-gold is your geotiff fork still required?
There was a problem hiding this comment.
If you want the abort signals and reliable LZW (de)compression, then yes. I think those are the only two differences.
There was a problem hiding this comment.
I am trying to get the first issue resolved. The second one is a bit hairier because it uses @manzt 's compiled-from-rust lzw decoder as well as a small change to the decompressor function
There was a problem hiding this comment.
Primary issue here is that the decoders for geotiff.js are baked into the library. In Zarr.js we a global registry which allows you to swap in/out different codec implementations, as well as allow for dynamic loading of compression modules (you have the option to only "pay" for the codecs that get used at runtime). In contrast, geotiff JS getDecoder function wraps a complete set of decoders and aren't tree-shakeable. geotiffjs/geotiff.js#172 (comment)
modules/tiff/README.md
Outdated
| @@ -1,7 +1,7 @@ | |||
| # @loaders.gl/textures | |||
| # @loaders.gl/tiff | |||
There was a problem hiding this comment.
Nit: If we have both zarr and tiff loaders in this module, there might be a better name; maybe @loaders.gl/array? We could also move the NPYLoader from its temporary location in @loaders.gl/terrain to this new module.
Edit: Didn't realize NPYLoader now resided in @loaders.gl/textures. Here with TIFF and Zarr might still be a better option.
There was a problem hiding this comment.
I guess one question is; do we care strongly about the bundle size of this module for script usage when it won't be tree shaken?
There was a problem hiding this comment.
Agreed that @loaders.gl/array could be a good move. We might be able to unify interfaces for in-memory multi- dimensional/scale arrays.
There was a problem hiding this comment.
I already split into modules/tiff and modules/zarr
There was a problem hiding this comment.
We might be able to unify interfaces for in-memory multi- dimensional/scale arrays.
Maybe we should spend some time discussing a consistent output format first for zarr, tiff, and npy, similar to what we're doing in #1430
There was a problem hiding this comment.
I don't have a strong preference on whether tiff and zarr should be in one module or two, though I do feel that the NPYLoader is closer to both tiff and zarr than it is to the other special GPU texture formats in @loaders.gl/textures.
There was a problem hiding this comment.
Seems different to me. The tiff and zarr modules provide a tile source interface, whereas the numpy loader decodes a single tile?
modules/tiff/src/typings/geotiff.ts
Outdated
| @@ -0,0 +1,74 @@ | |||
| declare module 'geotiff' { | |||
There was a problem hiding this comment.
Would geotiff.js upstream be interested in a PR to add this to a .d.ts file? I didn't see a relevant issue.
There was a problem hiding this comment.
I'm not sure. The types are relatively incomplete (there is a much larger un-typed API). Maybe worth opening an issue.
There was a problem hiding this comment.
| "dependencies-disabled": { | ||
| "geotiff": "ilan-gold/geotiff.js#ilan-gold/viv_094" | ||
| }, |
There was a problem hiding this comment.
Any docs on this? I can't seem to find any on a quick search
There was a problem hiding this comment.
Docs on what? I just moved it to an unused key - think of it as a comment.
There was a problem hiding this comment.
Ah I thought dependencies-disabled had a specific meaning in package.json, something like installing the dependencies for upstream geotiff below but then replacing the geotiff code with Ilan's fork.
|
@ibgreen I went ahead and stripped out all OME-specific metadata handling from the zarr module. What's the preferred way for me to add/suggest these changes? |
Maybe simplest to make a PR onto this branch? |
A gentle |
|
Moved geotiff (landed) and zarr to separate PRs. |
@ilan-gold @manzt