Simplifications

[StephanPreibisch]

Hi @hinerm

thanks for implementing it. I made some simplifications to your add-ons. The method for updating the progress bar and the drawing seemed a bit convoluted and I do not want to have synchronized blocks if not absolutely necessary.

Done:

• Just the first thread updates progress bar and draws, less overhead, could removed all the synchronized blocks and complicated methods
• just tested if to update for both (bar & image) together every 10000 pixels
• remove the progress bar once stitching is done
• added the option of display also to the pairwise stitching
• move the static boolean to the GUI class and the boolean for the current stitching to StitchingParameters

If you have any thoughts let me know. Thanks again, Stephan

[dscho]

Well, now it is simpler, but it is also incorrect. The first thread is actually quite unlikely to finish last. So the progress will indicate 100% done too early.

I suspected exactly such a problem might have been the root of the breakage regarding macro recording.

[StephanPreibisch]

The reason was that an image was displayed too early, I never used a status bar before. Before my simplifications the status bar was actually almost never set to a 100%, it was most of the times stuck at around 90% or so after everything was finished. But it seems to just not matter, nevertheless I fixed that little bug in the process.

[hinerm]

@StephanPreibisch Thank you for simplifying the logic here.

I am not sure about the macro recording, but based on what you're saying I am guessing that in your first implementation of Stitching, you displayed the resultant image immediately and perhaps the plugin returned immediately and let the subordinate threads fill in the image as needed, instead of doing a startAndJoin?

Did you test macro recording with the current display fusion option enabled?

Personally, I don't remember my progress bar not completing, so I'm not sure what was going on there.

Anyway, currently the displayFusion option does not work for me. It pops the blank ImagePlus, but does not fill in fused pixels.

I debugged the code and it's calling updateAndDraw in what seems to be an appropriate way, so I don't know why it's not actually redrawing..

I will test a few scenarios and report back if I can figure out what's going on...

[StephanPreibisch]

Hi @hinerm

I tested everything and your preview works. As expected it pops up an image for every channel/timepoint, displays the pixels and closes it once it is fused. It is the same as before, the only difference is that only thread 1 calls the updateanddraw. Maybe you have a problem with the min/max of the display? Or did I miss anything?

The problem with the progress bar occurred mostly on smaller images, but never mind. It is really just about setting it to >1.00 after the threads are joined. I know that your implementation was more correct, but I feel it is too much overhead for a progress bar and a preview to run all these synchronized blocks. Also all methods run some kind of counter anyways, so I just misused that one instead of counting pixels again.

I did not do any testing on the macro recording. But it also makes not much sense to record a macro that displays intermediate images. Feel free to do it, but I do not feel that this is a critical issue.

Thanks and cheers, Stephan

[hinerm]

Hi @StephanPreibisch

Actually, there was a bug. In computing the time of the last draw, the latest call to System.currentTimeMillis() was always being returned. However, this value should only be returned if the image was actually redrawn.

If you were fusing an image that took more than half a second to perform a fusion step, you wouldn't have seen this bug. Also, it was extremely frustrating to debug; when manually debugging in eclipse, it would hit the breakpoints even in that method and update the image display (because the act of debugging with breakpoints caused the system time to elapse, so it would appear to be operating just fine).

This was also complicated by the fact that the uber simplification commit did not compile for me, and conglomerated all the changes together, so I could not git bisect to find the problem. I had to recreate this commit one change at a time to figure out where the actual error was (because, at a glance, who would've thought it would be a problem with the timing method that worked fine in debugging!). Anyway, not a complaint, just another piece of evidence for why we should avoid uber commits when possible (which I am also guilty of with SCIFIO).

Anyway, please review my PR with the fix and merge if you agree. Thanks again for the refactor, I am glad you were able to get rid of the synchronized blocks, and I think the code will be easier to maintain/understand.