Hi all,<div><br></div><div>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.</div><div><br></div><div>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.</div>
<div><br></div><div>But Barry also has a good point that undo will fix any problems.</div><div><br></div><div>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.</div>
<div><br></div><div>-Curtis</div><div><br></div><div><br><div class="gmail_quote">On Mon, Jun 11, 2012 at 10:32 PM, Barry DeZonia <span dir="ltr"><<a href="mailto:bdezonia@gmail.com" target="_blank">bdezonia@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<div class="HOEnZb">
<div class="h5"><br>
<br><div class="gmail_quote">On Thu, May 31, 2012 at 7:41 AM, Lee Kamentsky <span dir="ltr"><<a href="mailto:leek@broadinstitute.org" target="_blank">leek@broadinstitute.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
On 5/30/2012 5:44 PM, Barry DeZonia wrote:
<blockquote type="cite">Thanks for looking at this Lee.
<div><br>
</div>
<div>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.</div>
</blockquote>
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.<br>
<br>
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<br>
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.<br>
<blockquote type="cite">
<div>
<div>
<div>
<br>
</div>
<br>
<div class="gmail_quote">On Wed, May 30, 2012 at 12:57 PM, Lee
Kamentsky <span dir="ltr"><<a href="mailto:leek@broadinstitute.org" target="_blank">leek@broadinstitute.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> Hi Barry,<br>
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.<br>
<br>
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:<br>
newRoi.move(getRegionOfInterest().getOrigin());<br>
around line # 220<br>
<br>
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.<br>
<br>
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.<br>
<br>
Otherwise, thanks for taking over for me... let me know
if you have any questions.
<div>
<div><br>
<br>
<br>
On 5/29/2012 1:32 PM, Barry DeZonia wrote:
<blockquote type="cite">Made a few more commits this
morning to improve consistency.<br>
<br>
<div class="gmail_quote">On Tue, May 29, 2012 at
11:19 AM, Lee Kamentsky <span dir="ltr"><<a href="mailto:leek@broadinstitute.org" target="_blank">leek@broadinstitute.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On 5/29/2012 11:42 AM, Barry DeZonia
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Hi guys,<br>
<br>
FYI on Friday of last week I updated the
RegionOfInterest classes to allow them to
be moved. You can see the related changes
at <a href="https://github.com/imagej/imglib/commit/7ad99450980ec0b73e1258ff80e65a23338f11d3" target="_blank">https://github.com/imagej/imglib/commit/7ad99450980ec0b73e1258ff80e65a23338f11d3</a>.<br>
<br>
And Lee I also updated the IJ2 Overlay
classes to support this kind of
functionality. See <a href="https://github.com/imagej/imagej/commit/192365bdbc3b77c02b348bfb1be6e8c18e03705d" target="_blank">https://github.com/imagej/imagej/commit/192365bdbc3b77c02b348bfb1be6e8c18e03705d</a>.
Note the duplicate() implementation needs
to be improved but it is functional.<br>
<br>
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.<br>
</blockquote>
</div>
I don't have any dependencies on the format
yet, so now is the time to change the format,
at least for me.
<div><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
Also if either of you know of ways these
changes violate original design
constraints of the RegionOfInterest
contract let me know. Thanks.<br>
<br>
</blockquote>
</div>
I'll see if I can review the changes today or
tomorrow<span><font color="#888888"><br>
<br>
--Lee<br>
</font></span></blockquote>
</div>
<br>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote></div><br>
</div></div></blockquote></div><br></div>