Missing sources in bdvs

[NicoKiaru]

following this commit (92bacfe), which takes into account the last modifications, I have a problem with the demo named AlphaDemoFull.

(Sorry I do not have tests, but I have plenty of demos that I run and check. I check that everything behaves correctly. Let's say it's 'manual integration tests').

This is how the demo looked just a commit before:

And this is how it looks with the commit 92bacfe:

As you can see, some sources are missing and appear randomly or by block.

I will need to bissect this further, and this'll take a bit of time.

@axtimwalde, @tpietzsch

[NicoKiaru]

With

<bigdataviewer-core.version>10.4.13</bigdataviewer-core.version>
<bigdataviewer-vistools.version>1.0.0-beta-33</bigdataviewer-vistools.version>

and commit e20bebd everything works fine.

With 1.0.0-beta-34 the conflicting VolatileSource shows up.

I continue to dig.

[NicoKiaru]

Ok, if I rename "my" VolatileSource to WrapVolatileSource and use these dependencies:

<bigdataviewer-core.version>10.4.13</bigdataviewer-core.version>
<bigdataviewer-vistools.version>1.0.0-beta-34</bigdataviewer-vistools.version>

It still works fine. (commit e20bebd for this repo, and a modified temp commit for bigdataviewer-playground)

[NicoKiaru]

I can't disantangle everything easily, but the issue shows up when I put the BigWarp 9.1.0 dependency.

I can't work on this before mid next week. So I'm gonna have to keep shadowing BigWarp for the moment.

(@tischi no fix for now)

[axtimwalde]

This is truly strange. To me, this looks as if the mipmap levels don't have the correct transformation, but there is no separate code in sandbox' WrapVolatileSource that does anything to the transformations. It's all the same thing...

[NicoKiaru]

Ah yeah indeed no sub-resolution shows up. I'll keep this in mind, I think there's potentially some threading issues - like a transform object shared between threads while it should be copied.

[NicoKiaru]

This commit : saalfeldlab/bigwarp@86d431e
introduced the change I observe.

I did not look yet what's in it.

(@axtimwale, @bogovicj)

[NicoKiaru]

I thought the tmpSrcTransform introduced the behaviour, and I tried creating a new instance each time it's required, but that's not changing anything. (even it is doesn't fix the issue, is it a good idea to keep one instance per object while your wrote this warning?):

getSource can be called by multiple threads, so need ensure application of the transform is thread safe here by copying

[NicoKiaru]

Ok, I found the issue: I need to adopt the same logic than was has been done in BigWarp in

bigdataviewer-biop-tools/src/main/java/bdv/util/source/alpha/AlphaSourceWarped.java

Lines 56 to 67 in 92bacfe

	public RealRandomAccessible<FloatType> getInterpolatedSource(int t, int level, Interpolation method) {
	RealRandomAccessible<FloatType> sourceRealAccessible =
	getAlpha().getInterpolatedSource(t, level, method);
	if (origin_warped.isTransformed()) {
	AffineTransform3D transform = new AffineTransform3D();
	getAlpha().getSourceTransform(t, level, transform);
	RealRandomAccessible<FloatType> srcRaTransformed = RealViews.affineReal(getAlpha().getInterpolatedSource(t, level, method), transform);
	return (RealRandomAccessible)(origin_warped.getTransform() == null ? srcRaTransformed : new RealTransformRealRandomAccessible(srcRaTransformed, origin_warped.getTransform()));
	} else {
	return sourceRealAccessible;
	}
	}

[NicoKiaru]

It's not sufficient, the change in saalfeldlab/bigwarp@86d431e
wreaks havoc in a lot of my repos regarding:

• concatenation of successive transforms
• making a source that blends many warped source efficiently

So, sorry to ask, but before I dig and spend a lot of time on this: do you have any clear benefit in this change ? Also have you tested whether it behaves correctly if you want to run BigWarp over an already WarpedSource (2xBigWarp). I need this in ABBA to apply successive transforms while keeping the ability to edit and cancel these successive layers.

[NicoKiaru]

I also see that this commit makes 56 additions and 16 deletions, so if it's adding more code, breaks other repos, and if there's not clear benefit (but this I don't know). I'd be relieved if this could be reverted.

[bogovicj]

Thanks for the recipe to reproduce the issue - I'm seeing the same thing and investigating ...

[NicoKiaru]

Thanks for the recipe to reproduce the issue

I'm not sure I provided a good recipe... Don't hesitate if you want to set up a quick zoom meeting and avoid wasting time on some of my undocumented logic.

[bogovicj]

😄 , so far so good. I gave this a try today: the changes: bogovicj@04ddc73

I'm not sure if this is entirely correct (especially not sure if the changes to getSource make sense...) but I at least have a prototype of AlphaSourceWarped that works with BigWarp 9's WarpedSource:

The behavior I get now:

wreaks havoc in a lot of my repos regarding:

• concatenation of successive transforms
• making a source that blends many warped source efficiently

I'm sympathetic to that. Especially if there will be many places and lots of code that's affected by this. Perhaps it would have been better for WarpedSource to stay the way it was, and for this changes with different behavior to go to a new class (RealTransformedSource? see here ). I don't know. Will talk with @axtimwalde this week.

[NicoKiaru]

Thanks for digging through this.

Especially if there will be many places and lots of code that's affected by this.

I know that I have at least another demo that fails:

• https://github.com/BIOP/bigdataviewer-biop-tools/blob/master/src/test/java/fused/CrazyMultireWarpedSourcesFusedDemo.java

It's not self-contained (I'll try to make it self-contained) but it was looking like that:

1487185944595861513-dDk_NQ-dUZRToGU8.mp4

Now the image is mostly black, even with the fix you suggested.

I have another demo where the 'fused sources' are not multi-resolution, and this works fine.

Perhaps it would have been better for WarpedSource to stay the way it was, and for this changes with different behavior to go to a new class.

Thanks for considering it. I'll try to make some simple use-cases that could show clearly where I'm affected. I need to confirm this, but I feel like the new way of specifying the transform is now relative to another referential, and that this may break backward compatibility. Maybe it'll backward compatible in some cases, but for the general case, I'm not sure that the landmark coordinates will still be valid when one switches from BW 8 to BW 9 with the same landmark files.

Also, maybe that's a minor performance hit and it's not very relevant, but you need to compute two extra affine transforms per pixel if I understand correctly.

I'll try keep wou posted by the end of week and hopefully provide useful inputs.

[NicoKiaru]

I've tested a bunch of ways to mess around BigWarp with bigwarp 8 (branch test-bigwarp-bw-compat-8):

• https://github.com/bigdataviewer/bigdataviewer-playground/blob/test-bigwarp-bw-compat-8/src/test/src/sc/fiji/bdvpg/transform/BigWarpDemo2D.java

and the same demo with BigWarp9 (branch test-bigwarp-bw-compat-9):

• https://github.com/bigdataviewer/bigdataviewer-playground/blob/test-bigwarp-bw-compat-9/src/test/src/sc/fiji/bdvpg/transform/BigWarpDemo2D.java

And couldn't find any issue.

So I need to look back at this repo and understand what's the root cause of the issue.

[NicoKiaru]

I think this is fixed now. I'm not completely sure of the initial issue.

I rewrote the test.

The only remaining issue I have is thus: imglib/imglib2-realtransform#45