search

Kitware / itk-vtk-viewer

2D / 3D web image, mesh, and point set viewer using itk-wasm and vtk.js
https://kitware.github.io/itk-vtk-viewer/
BSD 3-Clause "New" or "Revised" License
210 stars 64 forks source link

feat(toMultiscaleChunkedImage): Support passing a Zarr store #391

Closed thewtex closed 3 years ago

oeway commented 3 years ago

Hi @thewtex This is awesome, I am testing it now.

I noticed that you have these lines in the extractScaleInfo function:

const zattrs = await store.getItem('.zattrs')
const multiscales = zattrs.multiscales
const name = multiscales[0]['name']
const datasets = multiscales[0].datasets

Are you assuming the getItem will return an json object directly? Shouldn't it always be an ArrayBuffer? I think that will simplify the implementation of store and make it key-agnostic. For example, this is a reference store implementation: https://gist.github.com/oeway/d8b5bad5cbb7bc234a87d2e8948d9164#file-vizarrreferencestore-imjoy-html-L49-L70

thewtex commented 3 years ago

Are you assuming the getItem will return an json object directly?

Yes, a json object or an arraybuffer.

Shouldn't it always be an ArrayBuffer?

This makes things more complex and adds unnecessary conversions, correct? Why force JSON data into an ArrayBuffer? And then convert it back? With Zarr, we know that some keys are always JSON and some are always ArrayBuffer's.

thewtex commented 3 years ago

While it seems to add unnecessary overhead, I'll change the behavior to be consistent with the Zarr Python spec and behavior, which is always using bytes for values.

oeway commented 3 years ago

Yes, I agree that will be some overhead, but perhaps it doesn't matter that much because those are mostly for meta information which often very small.

I was about to say the same, since the Zarr store in Python also return bytes.

thewtex commented 3 years ago

@oeway thanks for the review and suggestion. All ArrayBuffer values is now implemented. Please take a look.

oeway commented 3 years ago

Hey, thanks a lot! Super exciting to try it!

The PR looks good! I just trie to test it but don't have the right data format yet.

thewtex commented 3 years ago

Thanks! With the foundation down, I'll continue with the other store implementations :-)

github-actions[bot] commented 3 years ago

:tada: This PR is included in version 11.4.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: