search

imagej / imagej-common

ImageJ core data model
https://imagej.net/libs/imagej-common
BSD 2-Clause "Simplified" License
10 stars 18 forks source link

DefaultOverlaySerivce.getSelectionBounds miscalculates height and width #83

Open gselzer opened 5 years ago

gselzer commented 5 years ago

In this method the selection bounds return object is given as a new RealRect(xMin, yMin, xMax - xMin, yMax - yMin) (see constructor). xMin/xMax/yMin/yMax are found by calling realMin(dimension) or realMax(dimension) on the Data object of the input ImageDisplay's DataView or from the ImageDisplay's bounds itself. For example, an image image of size 256/256, xMin/yMin will be 0 and xMax/yMax will be 255. This causes image.width and image.height, according to the above, to be 255, which is wrong, since it causes plugins like this one to not calculate the last column/row (see the driving for-loop here).

imagejan commented 5 years ago

For an image of 256x256 pixels, the real width and height are actually 255.0 (max-min), since pixels are just sample points, not squares, right? Maybe the API should be changed to return a net.imglib2.roi.geom.real.ClosedWritableBox here instead of a RealRect? The closed boundary would make sure that all border pixels are included in the selection.

gselzer commented 5 years ago

@imagejan yeah, I was thinking about that when I wrote the issue. I suppose it depends on how we are defining width/height here, as number of pixels or as a measure of distance (I was naively assuming the former). I just took a look at some of the other plugins relying on this method and some (like this one) assume width/height to be as you suggested, not like Sharpen/Smooth.

I am also hesitant to change the API of this method since it would break all of the plugins that might be calling it. Now that I am doing some more digging, maybe this is more of an issue with Neighborhood3x3Operation, since other plugins that call this method but do not use Neighborhood3x3Operation (like the one linked above) do not look like they have this issue. And, seeing as how some of these plugins using Neighborhood3x3Operation are being converted into Ops anyways (see imagej/imagej-ops#557 and imagej/imagej-ops#556), maybe we should just deprecate these faulty plugins, which would also keep reproducability