PNode.toImage is not centered and can create cropped images for PNodes with non-integral (x,y) bounds

[mro]

Originally reported on Google Code with ID 181

What steps will reproduce the problem?
1. Run the attached file TestPPathToImage, which creates a PNode with non-integral
(x,y) coordinates and convents it to an image.

What is the expected output? What do you see instead?
The output should be a red circle with black outline centered in the image contents.
 Instead this test program yields an image that is offset by about 1 pixel down and
to the right, which crops the bottom and right edges.

Reported by reids%colorado.edu@gtempaccount.com on 2010-07-12 20:56:10

[mro]

Here is the test case program and resultant output image file.

Reported by reids%colorado.edu@gtempaccount.com on 2010-07-12 21:07:36

---

• Attachment: TestPPathToImage.java
• Attachment: TestPPathToImage.png
  

[mro]

I isolated the problem and identified a potential solution. The problem was being caused
around line 2845 in PNode, where the node bounds is expanded to integer dimensions
for rendering to a buffered image. Only the width and height should be rounded to integer,
but the x and y are also being rounded. This is problematic because (for the node defined
in TestPPathToImage) the node bounds are (x,y,w,h) = (-2.5,-2.5,105,105), so this is
rounded to (-3,-3,105,105), so instead of setting the pnode to appear at the origin,
it is off by (0.5,0.5). I tried solving this problem by expanding only the (w,h) to
be integer, and letting the (x,y) remain floating. This yielded the correct behavior;
see attached file TestPPathToImage-fixed.png.  Here's a minimal patch that resolves
the problem:

Index: ../../../../workingcopy/piccolo2d.java/core/src/main/java/org/piccolo2d/PNode.java
===================================================================
--- ../../../../workingcopy/piccolo2d.java/core/src/main/java/org/piccolo2d/PNode.java  (revision
1020)
+++ ../../../../workingcopy/piccolo2d.java/core/src/main/java/org/piccolo2d/PNode.java  (revision
)
@@ -2854,7 +2854,7 @@
         g2.setClip(0, 0, imageWidth, imageHeight);

         final PBounds nodeBounds = getFullBounds();
-        nodeBounds.expandNearestIntegerDimensions();
+        nodeBounds.setSize(Math.ceil(nodeBounds.getWidth()),Math.ceil(nodeBounds.getHeight()));

         final double nodeWidth = nodeBounds.getWidth();
         final double nodeHeight = nodeBounds.getHeight();

I haven't tested the behavior outside of TestPPathToImage, and I'm not sure how many
incompatibilities this fix will cause in existing programs.  For instance, applications
may be working around this problem currently, and may obtain the wrong behavior after
this is fixed in piccolo.

Reported by reids%colorado.edu@gtempaccount.com on 2010-07-12 21:12:04

---

• Attachment: TestPPathToImage-fixed.png
  

[mro]

The fix described in Comment 3 is not general; for instance, applying that solution
to a stroke with an even number of pixels for width ends up cropping the bottom and
right side.  Inspection of that image shows that the problem with bounds are due to
the anti-aliasing.  When anti-aliasing is disabled, the fix proposed in comment 3 works
properly.  When anti-aliasing is enabled, there is some semi-transparent gray matter
to the top, left, right and bottom of the image, and both solutions (that is, with
or without the fix in comment 3) end up cropping some of it.  The underlying problem
is that piccolo nodes do not account for the extra width created by anti-aliasing.
 The heuristic of always adding a 1-pixel padding around every pnode is unappealing
because it would cause non-anti-aliased (or anti-aliased nodes where there is no extended
semitransparent gray matter) to not fill the extent of the allocated image.

Reported by reids%colorado.edu@gtempaccount.com on 2010-07-14 22:00:51

[mro]

I have also noticed this problem.  As a workaround, I usually add the padding when calculating
an image size and then use the PNode.toImage(BufferedImage, Paint) method.

Reported by heuermh on 2010-07-19 21:47:47

[mro]

Reported by heuermh on 2010-08-27 16:17:52

• Labels added: Toolkit-Piccolo2D.Java

[mro]

Reported by heuermh on 2010-12-22 02:23:19

• Labels added: Milestone-1.3.1

[mro]

Can we assume that the patch proposed in comment #3 is a step in the right direction?
That is is better than the current behaviour, though obviously not a complete solution?

Reported by allain.lalonde on 2011-03-01 18:48:00

[mro]

Allain, I'll +1 to that.

As for solving the anti-aliasing size problem... this is just an off-the-cuff idea,
but if there's not a way to predetermine the anti-aliasing size increase, maybe oversizing
the image at first and then doing something like an "auto-crop" a la Photoshop would
work?

Reported by atdixon on 2011-03-02 03:47:27

[mro]

I'm +0.

If we apply the fix, should it also be applied to PNode.print(Graphics, PageFormat,
int) ?

Reported by heuermh on 2011-03-02 16:25:43

[mro]

Michael, Are you +0, or +1? :)

Yes, I would think is should apply to print(...), too.

Is the compatibility concern that reids expressed significant?

Reported by atdixon on 2011-03-05 21:40:38

[mro]

I'm +0, see

Expressing Votes: +1, 0, -1, and Fractions
http://www.apache.org/foundation/voting.html

Reported by heuermh on 2011-03-06 03:19:27

[mro]

Ah, I see, so make me a +0.5.

Reported by atdixon on 2011-03-06 08:17:06

[mro]

I'm changing my vote and recommending we move this ticket to a future release (2.0)
and intend to implement a fully correct solution.

Given that there is a workaround for 1.3, I don't want to cause any regression for
people that have implemented workarounds that depend on the behavior of 1.3. Especially
since the proposed fix here is not a full fix and will still require workaround.

Can I get votes on this recommendation?

Reported by atdixon on 2011-03-20 21:06:07

[mro]

I'm changing my vote and recommending we move this ticket to a future release (2.0)
and intend to implement a fully correct solution.

Given that there is a workaround for 1.3, I don't want to cause any regression for
people that have implemented workarounds that depend on the behavior of 1.3. Especially
since the proposed fix here is not a full fix and will still require workaround.

Can I get votes on this recommendation?

Reported by atdixon on 2011-03-20 21:06:08

[mro]

Moving to 2.0. (Just to note: got a +1 for moving to 2.0 on developer thread http://goo.gl/qXt7P)

Reported by atdixon on 2011-03-27 01:06:49

• Labels added: Milestone-2.0
• Labels removed: Milestone-1.3.1

[mro]

Reported by heuermh on 2013-11-26 21:11:16

• Labels added: Milestone-2.1
• Labels removed: Milestone-2.0