Skip to content
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

DefaultDatasetService creates ImgView with null factory #72

Closed
awalter17 opened this issue Feb 23, 2018 · 6 comments
Closed

DefaultDatasetService creates ImgView with null factory #72

awalter17 opened this issue Feb 23, 2018 · 6 comments

Comments

@awalter17
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@ctrueden
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants