imagej / imagej-common

ImageJ core data model
https://imagej.net/libs/imagej-common
BSD 2-Clause "Simplified" License
9 stars 18 forks source link

DefaultDatasetService creates ImgView with null factory #72

Closed awalter17 closed 6 years ago

awalter17 commented 6 years ago

The DefaultDatasetService creates an ImgView with a null factory, see here. This shouldn't be done, as it can lead to problems later on (i.e. if you try to make a copy of the ImgView).

There will need to be some logic for determining which factory should be created based on the size of the RandomAccessibleInterval.

ctrueden commented 6 years ago

There will need to be some logic for determining which factory should be created based on the size of the RandomAccessibleInterval.

Logic is here. We just need to address the circular dependency somehow.

awalter17 commented 6 years ago

Or should there be Converters for RAI to Img? And the canConvert(...) methods could check the RAI dimensions. And then, we'd need to modify some of the convert ops to just use these converters.

That way there doesn't need to be a circular dependency.

ctrueden commented 6 years ago

After discussing with @awalter17, we agree that converters would be a good basis for convert ops that go from RAI to Img and II to Img. We think we would need six initially: RAIToArrayImg (only matches if element count is less than maxint), RAIToCellImg (but with what cell size?), RAIToListImg, and then those three again for II, and in that order of priority. This would let us implement convert.imgView.

But what about create.img? It would be nice to reuse the logic above, but only for factories. We could make e.g. RAIToImgFactory converters like RAIToArrayImgFactory, which would obviously be lossy. But is this really a "conversion"? Would someone want to pass a RAI as an argument to something that takes an ImgFactory? I don't think so.

awalter17 commented 6 years ago

@ctrueden and @kkangle, I've reproduced the NullPointerException in Java. See the code snippet below.

package net.imagej;

import net.imagej.display.DefaultDatasetView;
import net.imglib2.FinalInterval;
import net.imglib2.RandomAccessibleInterval;
import net.imglib2.img.Img;
import net.imglib2.img.ImgView;
import net.imglib2.type.numeric.integer.IntType;
import net.imglib2.util.ConstantUtils;

import org.scijava.Context;
import org.scijava.display.Display;
import org.scijava.display.DisplayService;

public class Main {

    public static void main(final String[] args) {
        final Context c = new Context();
        final DisplayService display = c.getService(DisplayService.class);
        final RandomAccessibleInterval<IntType> rai = ConstantUtils.constantRandomAccessibleInterval(new IntType(12), 2,
                new FinalInterval(new long[] { 0, 0 }, new long[] { 10, 10 }));
        final Display<?> d = display.createDisplay(rai);
        final DefaultDatasetView datasetView = (DefaultDatasetView) d.get(0);
        final Dataset dataset = datasetView.getData();
        final ImgPlus<?> imgPlus = dataset.getImgPlus();
        final Img<?> img = imgPlus.getImg();

        if (img instanceof ImgView)
            System.out.println("Is factory null? " + (((ImgView<?>) img).factory() == null));

        // throws NPE
        img.copy();
    }

}
awalter17 commented 6 years ago

Can this be closed since 6156a14da88daf110b1366282a7c3ffc02190270 is merged to master? Or are we still considering making converters?

ctrueden commented 6 years ago

I filed #74 to track any future work on the converters.