search

imagej / ImageJ

Public domain software for processing and analyzing scientific images
http://imagej.org
Other
513 stars 218 forks source link

Add support for Zarr and N5 in DragAndDrop #175

Closed ksugar closed 1 year ago

ksugar commented 1 year ago

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.

Here is a demo how it works with Zarr (https://github.com/xulman/ome-zarr-fiji-ui). DnD

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

imagesc-bot commented 1 year ago

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

karlduderstadt commented 1 year ago

@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.

ksugar commented 1 year ago

Hi @karlduderstadt , thank you for your feedback! I think the following changes will satisfy the general needs. I need to test it more, but it would be a way to support any custom-type directories. https://github.com/ksugar/ImageJ/commit/8f65ca88dd8181cb4c40779cb6b25c3f87c23444

ctrueden commented 1 year ago

@karlduderstadt wrote:

Ideally, all discovered IOPlugins should be checked before opening the folder as an Image Sequence.

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.

karlduderstadt commented 1 year ago

@ctrueden wrote:

In Edit → Options → ImageJ2..., do you have "Use SCIFIO when opening files" checked or not?

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 - https://github.com/imagej/imagej-legacy/issues/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?

ksugar commented 1 year ago

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 @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.

Drag and Drop with directories does not work at all. In fact with SCIFIO turned on, I just discovered this error - https://github.com/imagej/imagej-legacy/issues/284.

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());
ctrueden commented 1 year ago

Thanks @karlduderstadt for reporting the bug, and @ksugar for the analysis. This bug was recently fixed in scijava-common (scijava/scijava-common@aa7fe2e241c9e5dcca2ffad7945475aca47dc72c), but was not yet released. I am cutting a release now, and will upload it to Fiji later today. The check for loc.getFile().isFile(), while intuitive, is not necessarily good because there could be a TextIOPlugin that actually does handle directories (of text files, presumably), and we would not want to shut it down without giving it a chance to report on its capabilities.

karlduderstadt commented 1 year ago

@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 https://github.com/fiji/IO/commit/e41e251c1bb6451267dd01321728f800074322b2 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.

karlduderstadt commented 1 year ago

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 https://github.com/imagej/imagej2/issues/263 and there is a PR that should fix this https://github.com/imagej/imagej-legacy/pull/253.

Since the PR hasn't been merged, I have added this work around in my own IOPlugins:

https://github.com/duderstadt-lab/mars-core/blob/bfb748ad99ff9787c5369db36281d6d5d2d64888/src/main/java/de/mpg/biochem/mars/molecule/MoleculeArchiveIOPlugin.java#L151-L154

        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.

ctrueden commented 1 year ago

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. ... I still get the Image Sequence dialog when I drag and drop directories with SCIFIO turned off.

@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 HandleExtraFileTypes is given a chance to operate. With the "use SCIFIO" option off, the original ImageJ's built-in file handling sequence takes precedence over HEFT. Whereas with the "use SCIFIO" option on, ImageJ2 will intercept I/O operations and give IOPlugins first dibs. I don't think it's easy to have our cake and eat it too here, is it?

karlduderstadt commented 1 year ago

with the "use SCIFIO" option on, ImageJ2 will intercept I/O operations and give IOPlugins first dibs. I don't think it's easy to have our cake and eat it too here, is it?

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:

  1. Any folder name with an extension, a period somewhere, is not treated as an image sequence, but instead goes through the Opener and HEFT. So instead of adding the specific cases as a list currently (.n5, .zarray, and .zgroup), just check if there is an extension and delegate the extension type checking to the Opener and HEFT. Image Sequence could be added as a fall back after this process so in case no handler is found the expected behavior occurs.
  2. Recognize that how we think of folders has changed over time and flip the order of how folders are handled in ImageJ so that the Opener and HEFT are checked first and Image Sequence is used as a fallback. Something like this between lines 178 and 183: 
    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.

ksugar commented 1 year ago

Thank you @karlduderstadt for the great discussion and suggestions! What I implemented in https://github.com/ksugar/ImageJ/commit/8f65ca88dd8181cb4c40779cb6b25c3f87c23444 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. https://github.com/ksugar/ImageJ/blob/8f65ca88dd8181cb4c40779cb6b25c3f87c23444/ij/plugin/DragAndDrop.java#L177-L208

I found that the method openAndAddToRecent actually returns false if the file was opened successfully.

https://github.com/imagej/ImageJ/blob/d9d056a36871f1c292119c3be67fbb19a9260cf2/ij/io/Opener.java#L307-L314

Here, we can see that the field error becomes true only when there is an error in opening a file, otherwise it remains false. https://github.com/imagej/ImageJ/blob/d9d056a36871f1c292119c3be67fbb19a9260cf2/ij/io/Opener.java#L167-L169

Intuitively I expect openAndAddToRecent returns true if the file was opened successfully as documented. I may be misunderstanding it, but the current implementation appears to be the opposite.

ksugar commented 1 year ago

I confirmed that openAndAddToRecent returns false if the file was opened successfully, and it returns true when an error happens (e.g. file not found). I reported it here. https://github.com/imagej/ImageJ/issues/177#issue-1386769336

karlduderstadt commented 1 year ago

@ksugar I think this is a great solution. I am curious what others think. Now that @rasband fixed issue #177 and the openAndAddToRecent returns true when files are opened matching the documentation, you can update your solution accordingly. Do you want to update the PR to match?

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?

ksugar commented 1 year ago

@karlduderstadt Thank you for your feedback. I have updated the code based on the latest master branch.

What I have tested:

[ERROR] No opener IOPlugin found for FileLocation:file:/home/user/test_directory/. is from https://github.com/scijava/scijava-common/blob/0835207eb8e0bce1180a4b89c94fa5a941e802f3/src/main/java/org/scijava/io/DefaultIOService.java#L89

karlduderstadt commented 1 year ago

@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... but currently there is an error with SCIFIO on, when just opening Image Sequences and I think that should not be the case.

ctrueden commented 1 year ago

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 IOPlugins to take precedence over the original ImageJ logic pipeline, by labeling them as "eager".

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! :crossed_fingers: