ImageJ 1.53h breaks line bounds

ctrueden commented 2 years ago

$ mvn -Dij1-patcher.version=1.2.2 -Dij.version=1.53h
...
[ERROR] Failures:
[ERROR]   DefaultRoiConversionTest.testDefaultRoiWrapperLineWithWidth:109 expected:<100> but was:<101>
[ERROR]   IrregularPolylineRoiWrapperTest.testIrregularPolylineRoiWrapperFreelineWithWidth:136 expected:<188.25> but was:<189.25>
[INFO]
[ERROR] Tests run: 260, Failures: 2, Errors: 0, Skipped: 0

Caused by this change i.e. this version of Line.java.

More investigation needed to understand the details, but looks like a new bug in ImageJ as of v1.53h. One would expect the max coordinate of lines to be (r.x + r.width, r.y + r.height) but it seems the logic is now off by one.

rasband commented 2 years ago

Where can I find the source code for DefaultRoiConversionTest and IrregularPolylineRoiWrapperTest?

ctrueden commented 2 years ago

Thanks @rasband for being willing to look at it. I didn't want to report a bug in ImageJ until I knew more about the problem, and I ran out of time to dig, hence this issue.

The code in question is here in this imagej-legacy repository:

And the DefaultRoiWrapper:

https://github.com/imagej/imagej-legacy/blob/729751bf7f8a198e363ef0f8181d68a99e47f0bb/src/main/java/net/imagej/legacy/convert/roi/DefaultRoiWrapper.java#L74-L99

It calls r = roi.getBounds() and then reports the max as r.x + r.width and r.y + r.height respectively, which used to match expectations, but now is off by one as shown above. I briefly looked at the changes to Line linked above, but didn't fully understand the consequences of the + 0.5 change, or even if that was for sure what causes this change in behavior.

We could change the tests here in imagej-legacy so they pass, but it feels wrong for the max to be off by one, when the x1/y1/x2/y2 use double and are therefore not necessarily in integer space.

Any insight you could provide would be very helpful.

rasband commented 2 years ago

I looked into this change, made by Michael Schmid, in ImageJ 1.53h and concluded that it's a bug fix, not a regression. The smallest rectangle that encloses a line is the one returned by getBounds() in the current version of ImageJ. For example, this JavaScript, which creates a line and outlines it, works as expected with 1.53s but not with versions of ImageJ prior to 1.53h.

  img = IJ.createImage("Untitled", "8-bit noise", 200, 200, 1);
  line = new Line(10, 10, 20, 20);
  overlay = new Overlay();
  overlay.add(line);
  img.setOverlay(overlay);
  b = line.getBounds();
  rect = new Roi(b.x,b.y,b.width,b.height);
  img.setRoi(rect);
  img.show();
  IJ.run(img, "To Selection", "");

Screenshot

rasband commented 2 years ago

There is a bug with getBounds() when used with point selections. It creates a rectangle that is one pixel wider and taller than expected.

Here is example macro that reproduces the problem:

  newImage("Untitled", "8-bit ramp", 50, 50, 1);
  run("Add Noise"); //makes the pixels visible
  makePoint(10, 10);
  setKeyDown("shift");
  makePoint(11, 10);
  Roi.getBounds(x, y, width, height);
  print("integer bounds: ",x, y, width, height);
  run("Add Selection...");
  makeRectangle(x, y, width, height);
  run("To Selection");

rasband commented 2 years ago

The ImageJ 1.53s31 daily build fixes the bug that caused getBounds() to return a rectangle one pixel wider and taller than expected with point selections.

The following test macro reproduces the problem. It aborts with the error message "contains() test failed" with versions of ImageJ earlier than 1.53h.

  newImage("Points", "8-bit noise", 50, 50, 1);
  xp = newArray(7,8,9,10,8,8,7);
  yp = newArray(7,6,7,9,10,8,9);
  makeSelection("point large red dot", xp, yp);
  Roi.getBounds(x, y, width, height);
  run("Add Selection...");
  makeRectangle(x, y, width, height);
  run("To Selection");
  for (i=0; i<xp.length; i++) {
     if (!selectionContains(xp[i], yp[i]))
        exit("contains() test failed");
  }

Screenshot

imagej / imagej-legacy

ImageJ 1.53h breaks line bounds #279