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

[GoogleCodeExporter]

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.

Original issue reported on code.google.com by reids%co...@gtempaccount.com on 12 Jul 2010 at 8:56

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

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

Original comment by reids%co...@gtempaccount.com on 12 Jul 2010 at 9:07

Attachments:

• TestPPathToImage.java
• TestPPathToImage.png

[GoogleCodeExporter]

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.ja
va
===================================================================
--- 
../../../../workingcopy/piccolo2d.java/core/src/main/java/org/piccolo2d/PNode.ja
va  (revision 1020)
+++ 
../../../../workingcopy/piccolo2d.java/core/src/main/java/org/piccolo2d/PNode.ja
va  (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.getHeig
ht())); 

         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.

Original comment by reids%co...@gtempaccount.com on 12 Jul 2010 at 9:12

Attachments:

• TestPPathToImage-fixed.png

[GoogleCodeExporter]

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.

Original comment by reids%co...@gtempaccount.com on 14 Jul 2010 at 10:00

[GoogleCodeExporter]

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.

Original comment by heue...@gmail.com on 19 Jul 2010 at 9:47

[GoogleCodeExporter]

Original comment by heue...@gmail.com on 27 Aug 2010 at 4:17

• Added labels: Toolkit-Piccolo2D.Java

[GoogleCodeExporter]

Original comment by heue...@gmail.com on 22 Dec 2010 at 2:23

• Added labels: Milestone-1.3.1

[GoogleCodeExporter]

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?

Original comment by allain.lalonde on 1 Mar 2011 at 6:48

[GoogleCodeExporter]

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?

Original comment by atdi...@gmail.com on 2 Mar 2011 at 3:47

[GoogleCodeExporter]

I'm +0.

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

Original comment by heue...@gmail.com on 2 Mar 2011 at 4:25

[GoogleCodeExporter]

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

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

Is the compatibility concern that reids expressed significant?

Original comment by atdi...@gmail.com on 5 Mar 2011 at 9:40

[GoogleCodeExporter]

I'm +0, see

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

Original comment by heue...@gmail.com on 6 Mar 2011 at 3:19

[GoogleCodeExporter]

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

Original comment by atdi...@gmail.com on 6 Mar 2011 at 8:17

[GoogleCodeExporter]

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?

Original comment by atdi...@gmail.com on 20 Mar 2011 at 9:06

[GoogleCodeExporter]

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?

Original comment by atdi...@gmail.com on 20 Mar 2011 at 9:06

[GoogleCodeExporter]

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

Original comment by atdi...@gmail.com on 27 Mar 2011 at 1:06

• Added labels: Milestone-2.0
• Removed labels: Milestone-1.3.1

[GoogleCodeExporter]

Original comment by heue...@gmail.com on 26 Nov 2013 at 9:11

• Added labels: Milestone-2.1
• Removed labels: Milestone-2.0