[ImageJ-devel] Crop behavior (was Re: Changes to the overlay support)

Tue Jun 12 14:50:32 CDT 2012

Hi all,

I think this discussion should be on imagej-devel, in case anyone else is
interested, and also for archival purposes, so I have CCed it.

Regarding removal of clipped overlays... the other option is to leave in
any overlay that partially overlaps the cropped region. Our image windows
will become less ImageJ1-like once we fully support multiple datasets in
the same display. There is no reason an overlay cannot occupy a part of the
aggregate coordinate space outside of a particular dataset, after all.

But Barry also has a good point that undo will fix any problems.

Perhaps we could have two separate commands "Quick Crop" (with shortcut
key) and "Crop..." which has the advanced options such as what to do with
questionable overlays.

-Curtis

On Mon, Jun 11, 2012 at 10:32 PM, Barry DeZonia <bdezonia at gmail.com> wrote:

> Lee, after reviewing your comments I think the most straightforward
> approach for crop is to only keep overlays completely contained in the crop
> region. If an important overlay is clipped and removed it should be
> restorable via undo. This of course requires that we have a fully
> functional undo capability.
>
>
> On Thu, May 31, 2012 at 7:41 AM, Lee Kamentsky <leek at broadinstitute.org>wrote:
>
>>  On 5/30/2012 5:44 PM, Barry DeZonia wrote:
>>
>> Thanks for looking at this Lee.
>>
>>  Whether to use your crop image branch or not depends upon how we want
>> crop to behave. Should it maintain all current overlays (picture a full
>> size window with overlay outlines all across it but a small image inside
>> it)? Or do we just want all overlays that are contained in the user
>> specified region? Or all those that are contained or cross the user
>> specified region (and the crossing overlays would be permanently edited to
>> be clipped)? Anyone feel free to chime in here.
>>
>> I am guessing that there's not much use for an overlay that falls
>> completely outside of the cropped region. Overlays that span the clipping
>> region are a problem, especially for display. If they aren't cropped, the
>> window has to expand / scroll to show their edit handles (and calculations
>> of the viewing extent have to be done over the interval of the image and
>> overlays). If the overlays are cropped, then they change shape - difficult
>> to implement and possibly frustrating to many users. I think that users
>> might be a little upset if a cropping cut off a small corner of an
>> important overlay and the overlay was removed because of that - so IMHO
>> that's not a good behavior.
>>
>> Related - an IterableRegionOfInterest.getIterableIntervalOverROI returns
>> an IterableInterval. If you feed it a cropped image (a
>> RandomAccessibleInterval), then the Interval of the IterableInterval should
>> be the intersection of the interval of the cropped image
>> and the IterableRegionOfInterest and the cursor should not throw an
>> exception when it tries to set the position on the RandomAccessible outside
>> of its interval. I'll file a bug for that and assign it to myself.
>>
>>
>>
>> On Wed, May 30, 2012 at 12:57 PM, Lee Kamentsky <leek at broadinstitute.org>wrote:
>>
>>>  Hi Barry,
>>> For AbstractOverlay.min() and .max(), I intended for the interval to be
>>> from the pixel with the minimum coordinate value to the pixel with the
>>> maximum coordinate value (there's reasonable arguments for other
>>> definitions, though). IMHO, the default for min in AbstractOverlay should
>>> probably return the ceiling of the realMin and the max should return the
>>> floor of the realMax so that when you iterate through the pixels, all pixel
>>> coordinates will fall between the min and max. Otherwise, filling in the
>>> unsupported operations with reasonable defaults is a good idea - thanks for
>>> doing that.
>>>
>>> In BinaryMaskOverlay, I think the default constructor has to be there if
>>> deserialization is to work. I guess you need to do things in two steps and
>>> set the context after deserialization. The new format is perfectly ok and
>>> it's ok to break backward compatibility.
>>> BinaryMaskRegionOfInterest.duplicate() looks like it has an error. The
>>> original ROI might have an origin other than 0, so I think you need
>>> something like:
>>> newRoi.move(getRegionOfInterest().getOrigin());
>>> around line # 220
>>>
>>> The comment in CompositeOverlay "I think ROIS must have .duplicate()"...
>>> That's not the worst idea, but it seems to me that you could implement
>>> duplicate() in the abstract base class by serializing yourself and then
>>> deserializing. You can always override it in some class where that's
>>> inefficient, but I'm guessing that the use case for .duplicate() is that
>>> the user is copying a handful of overlays from one place to another - the
>>> overhead for marshalling to and from a byte representation should be
>>> imperceptible. You definitely want to optimize the bitmask overlays to
>>> handle things like many particles.
>>>
>>> CropImage - that really stinks having to delete the overlays and then
>>> recreate them. I am guessing that you did this because JHotDraw doesn't
>>> move the handles for the currently selected object. I made a branch,
>>> "lee_figure_selection_bugfix", and I modified
>>> OverlayFigureView.doUpdateFigure() to deselect the figure, modify it, then
>>> reselect it if appropriate. I changed CropImage to just move the overlays
>>> and it all seems to work. If you all want to take the change, fine, if not,
>>> go ahead and delete the branch.
>>>
>>> Otherwise, thanks for taking over for me... let me know if you have any
>>> questions.
>>>
>>>
>>>
>>> On 5/29/2012 1:32 PM, Barry DeZonia wrote:
>>>
>>> Made a few more commits this morning to improve consistency.
>>>
>>> On Tue, May 29, 2012 at 11:19 AM, Lee Kamentsky <leek at broadinstitute.org
>>> > wrote:
>>>
>>>> On 5/29/2012 11:42 AM, Barry DeZonia wrote:
>>>>
>>>>> Hi guys,
>>>>>
>>>>> FYI on Friday of last week I updated the RegionOfInterest classes to
>>>>> allow them to be moved. You can see the related changes at
>>>>> https://github.com/imagej/imglib/commit/7ad99450980ec0b73e1258ff80e65a23338f11d3
>>>>> .
>>>>>
>>>>> And Lee I also updated the IJ2 Overlay classes to support this kind of
>>>>> functionality. See
>>>>> https://github.com/imagej/imagej/commit/192365bdbc3b77c02b348bfb1be6e8c18e03705d.
>>>>> Note the duplicate() implementation needs to be improved but it is
>>>>> functional.
>>>>>
>>>>> Lee, one thing to note is that the BinaryMaskOverlay's writeExternal()
>>>>> code has been modified. The order of saved items is now different. Since we
>>>>> have yet to release IJ2 I assume I am able to change it. If not we may need
>>>>> to alter the order and do version checking on readExternal(). Please let me
>>>>> know if you think updating this implementation as it has been is
>>>>> problematic.
>>>>>
>>>>  I don't have any dependencies on the format yet, so now is the time to
>>>> change the format, at least for me.
>>>>
>>>>
>>>>> Also if either of you know of ways these changes violate original
>>>>> design constraints of the RegionOfInterest contract let me know. Thanks.
>>>>>
>>>>>  I'll see if I can review the changes today or tomorrow
>>>>
>>>> --Lee
>>>>
>>>
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://imagej.net/pipermail/imagej-devel/attachments/20120612/1158f5c9/attachment.html>