Isn't this line a no-op, since you concat'ing the identity transform? + CGContextConcatCTM(qsdo->cgRef, CGAffineTransformMake(1, 0, 0, 1, 0, 0));
Otherwise, looks fine. Cheers, Mike Swingler Apple Inc. On Sep 4, 2012, at 8:53 PM, Scott Kovatch <[email protected]> wrote: > I need one more reviewer/+1... anyone? > > -- Scott K. > > Begin forwarded message: > >> From: Scott Kovatch <[email protected]> >> Subject: Re: Review: Remove private API from graphics code >> Date: September 4, 2012 8:16:03 AM PDT >> To: Phil Race <[email protected]> >> Cc: "[email protected]" <[email protected]>, 2d-dev >> <[email protected]>, "[email protected] OS X" >> <[email protected]> >> >> I posted a new webrev at >> http://cr.openjdk.java.net/~skovatch/7187834/webrev.01/ >> >> In ImageSurfaceData, it looks like we are trying to get back to a pure CTM >> (i.e., no transform of any kind applied.) before we draw the image. We have >> already saved the state, but saving the state doesn't reset it. That's why >> there are extra gyrations to invert and concat the inversion. In >> QuartzRenderer.m the information you found applies, so I'm just using that >> now. >> >> These changes fix the printing bugs in Java2Demo that I saw with the first >> webrev. >> >> -- Scott >> >> >> On Aug 31, 2012, at 11:50 AM, Phil Race <[email protected]> wrote: >> >>> The recommendation is to restore the graphics state rather than inverting :- >>> >>> https://developer.apple.com/library/mac/#documentation/graphicsimaging/conceptual/drawingwithquartz2d/dq_affine/dq_affine.html >>> >>> "Quartz also provides an affine transform function that inverts a matrix, >>> |CGAffineTransformInvert|. Inversion is generally used to provide reverse >>> transformation of points within transformed objects. Inversion can be >>> useful when you need to recover a value that has been transformed by a >>> matrix: Invert the matrix, and multiply the value by the inverted matrix, >>> and the result is the original value. You usually don’t need to invert >>> transforms because you can reverse the effects of transforming the CTM by >>> saving and restoring the graphics state." >>> >>> -phil. >>> >>> On 8/31/2012 11:16 AM, Phil Race wrote: >>>> Scott, >>>> >>>> These files were added by Bino to support printing. Quartz isn't used >>>> except for printing in JDK 7, so as I understand it, testing on-screen in >>>> Java2Demo should not exercise this code. I'm surprised that you saw >>>> it being exercised. Did you do any printing testing ? >>>> >>>> The matrix inversion seems unlikely to be applied to any non-invertible >>>> matrices, so that's fine, but I wonder if you have lost precision here >>>> due to floating point inaccuracies ? >>>> >>>> If you originally had a simple scale or identity, rotated it, and then >>>> applied the inverse to unrotate it, do you really end up with exactly >>>> the same results. The more you do this the more inaccuracies creep in, >>>> which may be part of the reason for the original approach. >>>> I find it a little hard to believe that there isn't a direct public way to >>>> restore a transform. >>>> >>>> The changes for mountain lion are safe for snow leopard I presume? >>>> I believe the builds still happen on snow leopard. >>>> >>>> Also this should have been sent to 2d-dev, not awt-dev. >>>> These files, APIs, and printing are all 2D, not awt. >>>> >>>> -phil. >>>> >>>> I am not sure why they are not used but it >>>> On 8/31/2012 10:45 AM, Scott Kovatch wrote: >>>>> http://cr.openjdk.java.net/~skovatch/7187834/webrev.00/ >>>>> >>>>> This is based on the patch submitted by Marco Dinacci. I had to modify it >>>>> a bit to get it to compile against 7u-dev, but nothing major. >>>>> >>>>> The changes in ImageSurfaceData.h were needed as a result of my use of >>>>> Xcode 4.4.1 on Mountain Lion. >>>>> >>>>> I ran the Java2Demo and don't see any problems. I paid close attention to >>>>> the Images tab, since it looks like this code is heavily used by it. >>>>> >>>>> -- Scott K. >>>>> >>>>> ---------------------------------------- >>>>> Scott Kovatch >>>>> [email protected] >>>>> Santa Clara/Pleasanton, CA >>>>> >>>>> >>>> >>> >> >
