-
Notifications
You must be signed in to change notification settings - Fork 226
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
Add support for Zarr and N5 in DragAndDrop #175
Conversation
Directories that contain .zgroup or .zarray are processed like files.
Directories that end with .n5 are handled as a special case.
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/fiji-ngff-hackathon-sep-2022-prague-cze/69191/2 |
@ksugar This is a great PR. I would love to be able to open n5 and zarr using drag and drop. I see you have now added those two directory types as options. Would it be possible to make this more of a general feature that would allow anyone to add openers for custom directory types? Currently, custom file types can be added using scijava in IOPlugins by supporting them in the supportsOpen method. See here for an example of how we have done that in our project. These files work through drag and drop using this approach. Sadly this is not the case for directories eventhough we added our custom type (.yama.store). Ideally, all discovered IOPlugins should be checked before opening the folder as an Image Sequence. My comment should not stop merging this PR. I think this should be merged as soon as possible. I just wonder if there could be more general support in the future. |
Hi @karlduderstadt , thank you for your feedback! |
@karlduderstadt wrote:
In Edit → Options → ImageJ2..., do you have "Use SCIFIO when opening files" checked or not? If checked, IOPlugins should be used preferentially. Or if it's not checked, then the original ImageJ I/O routines take priority. |
@ctrueden wrote:
Yes! We always have this turned on so our SCIFIO micromanager reader is used and we can use the infrastructure for translation to OME format, which is really nice. Drag and Drop with files works perfectly. We use that all the time. In fact, somehow even with SCIFIO turned off, the IOPlugin is still checked and Drag and Drop still works. Drag and Drop with directories does not work at all. In fact with SCIFIO turned on, I just discovered this error - imagej/imagej-legacy#284. With SCIFIO turned on or off, our IOPlugin is never checked for directories, only files. In all cases, directories are assumed to be Image Sequences when dragged and dropped in my testing. @ksugar thanks for working on an update! I will see if I can get it and test it out. In that case, would you take advantage of IOPlugins for N5 and Zarr? Do those exist? |
@ctrueden @karlduderstadt I did not know this option! I confirmed that the IOPlugin for Zarr works if this option is enabled. This plugin supports only OME-Zarr directories following the version 0.4 specs.
I think the cause of this issue is in TextIOPlugin. If we change this line as follows, the error will be removed. return loc.getFile().isFile() && textService.supports(loc.getFile()); |
Thanks @karlduderstadt for reporting the bug, and @ksugar for the analysis. This bug was recently fixed in scijava-common (scijava/scijava-common@aa7fe2e), but was not yet released. I am cutting a release now, and will upload it to Fiji later today. The check for |
@ksugar Thanks very much for testing and reporting back about this. Yes, I didn't realize directories are already supported. I had a mistake in my directory detection. With that fixed, it also works really nicely for me with SCIFIO turned on, now that @ctrueden uploaded the patch to scijava-common 🚀. Would it then be an option to have the IOPlugins used for directories even when SCIFIO is turned off? So the user doesn't have to do anything special but it uses the IOPlugin infrastructure. For me this is how it is for files. With SCIFIO on or off, file IOPlugins are working thanks to this fiji/IO@e41e251 commit by @ctrueden sometime ago. This change didn't seem to activate this check for directories. I still get the Image Sequence dialog when I drag and drop directories with SCIFIO turned off. |
For completeness, I should probably mention some other open issues and PRs. As far as I understand, Datasets are opened correctly using uiService.show( , which is called somewhere, using IOPlugins with SCIFIO turned on. So N5 and Zarr datasets open without problems. Unfortunately, other types of Objects are opened, but are never displayed. This was discussed on the forum here - https://forum.image.sc/t/open-a-custom-file-type-via-the-imagejs-open-dialog/41471/8 and I made this issue imagej/imagej2#263 and there is a PR that should fix this imagej/imagej-legacy#253. Since the PR hasn't been merged, I have added this work around in my own IOPlugins: final boolean newStyleIO = optionsService.getOptions(
net.imagej.legacy.ImageJ2Options.class).isSciJavaIO();
if (newStyleIO) uiService.show(archive); to make sure our Objects actually get displayed. You can see here, uiService.show is only called if SCIFIO is turned on. If it is turned off, then somewhere uiService.show is already called, so I don't call it myself to avoid displaying our Objects twice... Sorry for the very complicated explanation. I hope this is not a problem N5 or Zarr. I think sorting this out and doing it correctly is worth the effort. Then you can just add an IOPlugin in the future and it will magically work in Drag in Drop no matter what. Wouldn't that be wonderful? Please let me know, if there is something I can help test further. |
@karlduderstadt I don't know how to do it with adding more bytecode patching to imagej-legacy, because IIRC, the Image Sequence logic happens before |
I guess files have always been assumed to have types and corresponding extensions that define how they should be opened. So it works to start in ImageJ and then move to SCIFIO in case no reader is found when SCIFIO is turned off. However, folders were never assumed to have types. Therefore, it was assumed they should be treated as a folder with an image sequence always in ImageJ and ultimately this logic appears to be the problem. Two possible solutions come to mind:
if (f.isDirectory()) {
if (!(new Opener()).openAndAddToRecent(path))
if (openAsVirtualStack)
IJ.run("Image Sequence...", "open=[" + path + "] sort use");
else
openDirectory(f, path);
} else { Here I am assuming if no handlers are found openAndAddToRecent will return false, but I have not tested this and perhaps there are exceptions to this logic that would need to be addressed. I would just like to add, I don't have strong feelings around this and I don't want to get in the way. Please feel free to reject these ideas and move forward however you like. I am already glad it works generally with SCIFIO turned on now. I only raised these points because it seemed like we might be able to solve this problem once instead of directly adding extensions manually here as needed in the future. |
Thank you @karlduderstadt for the great discussion and suggestions! What I implemented in ksugar@8f65ca8 is something like your second approach. boolean error = (new Opener()).openAndAddToRecent(path, false);
if (!error)
Recorder.recordOpen(path);
else {
if (f.isDirectory()) {
if (openAsVirtualStack)
IJ.run("Image Sequence...", "open=[" + path + "] sort use");
else
openDirectory(f, path);
return;
} else {
IJ.error("Opener", "Unsupported format or file not found:\n"+path);
}
} The full code can be found here. I found that the method Lines 307 to 314 in d9d056a
Here, we can see that the field Lines 167 to 169 in d9d056a
Intuitively I expect |
I confirmed that |
@ksugar I think this is a great solution. I am curious what others think. Now that @rasband fixed issue #177 and the How you have tested it so far? You confirmed your files types open and that folders with image sequences open when there is no recognised type? What else should we make sure to check? What are other possible issues? |
- custom-type directories can be added using scijava in IOPlugins
- the field error remains false if fileType is CUSTOM
@karlduderstadt Thank you for your feedback. I have updated the code based on the latest What I have tested:
|
@ksugar Thanks for updating the PR and testing. Based on your report, everything appears to be working as we hoped! So the only change for the average user who wants to open their Image Sequences is the third one. If they are using Fiji with SCIFIO off, their Image Sequence will open as normal, but they receive a new error they did not get before. The error for the fifth case I also get currently. That is not new. If SCIFIO is on, I get the error when opening Image Sequences, but everything still works normally. Ideally, there shouldn't be errors/warning from directories since they will always be opened as Image Sequences by ImageJ if no IOPlugins are found. This could be achieved by excluding directories from throwing errors/warnings at the location you linked in DefaultIOService as you pointed out. The only drawback is when DefaultIOService might be used in contexts outside of ImageJ/Fiji in which case maybe you do want to see the errors/warnings since Fiji/ImageJ would not catch them and treat them as Image Sequences 🤔 I think @ctrueden will need to weigh-in on this and decide if the errors could be prevented for directories. Otherwise, is there another workaround I am not seeing? Maybe this is what he meant by having our cake and eating it too? The only other option could be to have a new DefaultIOService specific to ImageJ/Fiji and excludes directories from errors.. Seems overly complex... |
I'm sitting with @xulman at a hackathon in Dresden, and we discussed this at length on Zulip here. We decided to try a different approach, that avoids needing to modify the original ImageJ codebase, and will allow both directory-based and file-based Closing this PR in favor of imagej/imagej-legacy#296, where the work has been prototyped, and @xulman will test. Thank you @ksugar and @karlduderstadt for all your effort and thought and discussion, and sorry the work is not ending up merged as-is, but I think this imagej-legacy-based solution will be really nice. Hopefully! 🤞 |
This PR adds exclusion rules for drag-and-drop of directories to support Zarr and N5 formats.
With this change, directories that comply with the following conditions are recognised as Zarr or N5 and its processing is delegated to a custom Opener provided by the plugin.
.zgroup
or.zarray
.n5
Here is a demo how it works with Zarr (https://github.com/xulman/ome-zarr-fiji-ui).
This is a result of collective work of several people noted below, during the Fiji and Zarr hackathon 2022 in Prague, CZE, which was generously supported by the IT4Innovations National Supercomputing Centre, Ostrava, CZE:
The people on-site during the hackathon:
Ko Sugawara, Nils Norlin, Tobias Pietzsch, Christian Tischer, Vladimir Ulman
The people online during the hackathon:
Gabor Kovacs, Isaac Virshup, Luca Marconato, John Bogovic, Josh Moore