[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] <AWT Dev> [8] Request for review: 8000629 [macosx] Blurry rendering with Java 7
From:       Jim Graham <james.graham () oracle ! com>
Date:       2013-03-28 20:44:49
Message-ID: 5154ABC1.5000301 () oracle ! com
[Download RAW message or body]

I don't think the constant would add any value and would just cause someone looking \
over the code to wonder what kind of magical value means NO_SCALE when the value is \
so obvious.  I think anyone can realize that 1 or 1.0 means "not scaling" with about \
2 neurons firing.  The mystery created by hiding it behind a constant would just \
cause all sorts of thoughts to arise that really have no value or bearing on the work \
being done...

			...jim

On 3/28/2013 11:34 AM, Denis S. Fokin wrote:
> Hi Sergey,
> 
> The fix looks ok. But I have two ideas.
> 
> 1.)  We use everywhere 1 as a measure of scale.Would not it be better to use a \
> constant at least for integer values? Actually, as far as we dealing with 1.0 we \
> should not have any issues with precision. On the other hand, the widening \
> primitive conversion can lead to a performance impact. But at least for integers we \
> could introduce a constant. 
> CGLLayer.java
> 48     private int scale = 1;
> 
> 
> SurfaceData.java
> 1068         return 1;
> 
> SunGraphics2D.java
> 262         if (devScale != 1) {
> 1639         final double invScale = 1.0 / devScale;
> 
> SurfaceManager.java
> 300             return 1;
> 
> 
> Region.java
> 143         if (sv == 1.0) {
> 381         if ((sx == 1.0 && sy == 1.0) || (this == WHOLE_REGION)) {
> 
> 
> CGraphicsDevice.m
> 328     __block jdouble ret = 1.0f;
> 
> The name for constant is a tricky question. Maybe NOT_SCALED or something like \
> that. 
> 2.)
> 
> I would really like to have a comment here
> 457         return getMaxTextureSize() / (getDevice().getScaleFactor() * 2);
> 
> why not
> 
> CGLGraphicsConfig.java
> 457         return (getMaxTextureSize() / (getDevice().getScaleFactor()) - 1);
> 
> at least a minor comment.
> 
> 
> Thank you,
> Denis.
> 
> 
> 
> 
> On 3/28/2013 3:14 PM, Sergey Bylokhov wrote:
> > On 3/28/13 1:04 PM, Denis S. Fokin wrote:
> > > Hi Sergey,
> > > 
> > > actually, the round function has a little bit more complicated
> > > implementation because of 6430675.
> > Here is the new version:
> > http://cr.openjdk.java.net/~serb/8000629/webrev.07
> > > 
> > > Thank you,
> > > Denis.
> > > 
> > > On 3/27/2013 7:15 PM, Sergey Bylokhov wrote:
> > > > On 3/27/13 4:12 PM, Denis S. Fokin wrote:
> > > > > Hi Sergey,
> > > > > 
> > > > > why we do not use Math.round() here?
> > > > > Region.java:
> > > > > 153         return (int) Math.floor(newv + 0.5);
> > > > Just because it one additional call.
> > > > > 
> > > > > Thank you,
> > > > > Denis.
> > > > > 
> > > > > 
> > > > > On 3/26/2013 7:33 PM, Sergey Bylokhov wrote:
> > > > > > Hello,
> > > > > > Please review the fix for jdk 8.
> > > > > > Change adds initial support of hidpi(mostly on 2d side). In the fix
> > > > > > scale was added to the surface data/CGraphicsDevice /CGLLayer. This
> > > > > > scale factor maps virtual coordinates to physical pixels.
> > > > > > This change doesn't add support of hidpi to aqua l&f and doesn't add
> > > > > > support of dynamic change of scale factor.
> > > > > > 
> > > > > > Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8000629
> > > > > > Webrev can be found at:
> > > > > > http://cr.openjdk.java.net/~serb/8000629/webrev.06
> > > > > > 
> > > > > > --
> > > > > > Best regards, Sergey.
> > > > > > 
> > > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]