-
Notifications
You must be signed in to change notification settings - Fork 29
Read Zarr Mesh Files #8682
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
Read Zarr Mesh Files #8682
Conversation
Hej, I have a high level question regarding:
Does this mean, that the frontend no longer needs to pay attention to the meshfile type? Or is this still relevant and returned by the backend when the meshfiles are requested? |
Correct! And because of that, I removed fileType and path from the ApiMeshFileInfo. formatVersion remains (but only to warn that meshfiles with version <3 are not supported anymore) |
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.
Here are my first comments :)
...ossos-datastore/app/com/scalableminds/webknossos/datastore/explore/PrecomputedExplorer.scala
Outdated
Show resolved
Hide resolved
...s-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/DSFullMeshService.scala
Outdated
Show resolved
Hide resolved
...sos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala
Outdated
Show resolved
Hide resolved
...sos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala
Show resolved
Hide resolved
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.
Here is the rest of the review :)
Thanks for implementing all this so fast 🏎️ 💨
...-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataServiceHolder.scala
Outdated
Show resolved
Hide resolved
...nossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala
Show resolved
Hide resolved
...-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataServiceHolder.scala
Show resolved
Hide resolved
…e/services/BinaryDataServiceHolder.scala Co-authored-by: MichaelBuessemeyer <[email protected]>
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.
Here is the rest of the review :)
Thanks for implementing all this so fast 🏎️ 💨
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.
Here are my testing results:
- I can no longer explore the data gs://fafb-ffn1-20190805/segmentation/ which I think has precomputed meshes.On master (wk.org) this worked. So I thikn this is cause by this PR,
- The rest works very nicely (hdf5 mesh, ad hoc mesh, zarr mesh)
Thanks for the detailed testing! Huh, exploring gs://fafb-ffn1-20190805/segmentation/ on this branch works for me, I can then also load its meshes 🤔 Maybe let’s have a look at this together today |
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.
Nice, looks good. The problem I had with dataset gs://fafb-ffn1-20190805/segmentation/ was that I first explored gs://fafb-ffn1-20190805/section_to_section_xcorr/
and then wanted to add gs://fafb-ffn1-20190805/segmentation/
. But they seem to have incompatible mags:
Found 1 Neuroglancer Precomputed layers at gs://fafb-ffn1-20190805/segmentation/.
Error when exploring as layer set: Could not extract common voxel size from layers <~ voxel sizes for layers are not uniform, got List(VoxelSize((8.0, 8.0, 40.0),nanometer)) <~ invalid mag: (0, 0, 1). Must all be powers of two
But this is currently expected and the same behaviour as on the master. Therefore, it should not block this pr and can be solved in a later pr or deemed to be expected.
Similar to Read Zarr Agglomerate Files #8633 we now support reading mesh files in zarr format. Note that there are also neuroglancerPrecomputed meshes, so this time there are three services the meshFileService delegates to.
The frontend no longer needs to specially handle neuroglancerPrecomputed meshes, since the lookUpMeshFileKey function abstracts from that. Because of that, I also simplified the frontend types.
Note that zarr meshfiles must be registered in the datasource-properties.json, they are not explored.
Also fixed a small error in job result status reporting
Also fixed a small error in mag selection for animation job.
Steps to test:
TODOs:
Issues: