[OpenJDK 2D-Dev] X11 uniform scaled wide lines and dashed lines; STROKE_CONTROL in Pisces

[Jim Graham]

Hi Denis,

Looks like some great new work here!  I'll try to keep the "pie in the 
sky" suggestions down now so we can get this in soon...

On 10/18/2010 2:19 PM, Denis Lila wrote:
> Hi Jim.
>
>> I'm just now getting down to the nitty gritty of your webrevs (sigh).
>
> Thanks. I hope it's not too bad.

The code was great - what sucked was all of the cobwebs on my trig and 
curve math neurons.

>> PiscesRenderingEngine.java:
>> line 296 - is there an "epsilon" that we can use?  "==" with floating
>> point often fails with calculations.

I was thinking maybe something more like the ULP stuff you did in one of 
the other files.  I don't think 2 non-equal fp values can be subtracted 
and produce a value that is as small as MIN_VALUE unless you are 
subtracting 2 extremely tiny numbers.

>> line 338 - null here too

If this is now line 341 you still use "at" which might be a non-null 
identity transform.  I'd just use "null" as some shapes might try to do 
some work if they get a non-null identity transform, but null pretty 
much tells them "it's identity".

> I turned LengthComputer into an iterator. I think it's much cleaner now.
> There's no longer any of that "scale every t in the array so that they become
> valid parameters of the right subdivided curve".
> It also uses less memory - just limit+1 curves. I guess I am clever enough ;)
> (though unfortunately not clever enough to have thought of the idea myself).

Interesting solution.  I like it.

line 248,251 - I thought it was a bug that you used 2 when I thought you 
should use 0, but it turns out that it doesn't matter because the last 
point of left is the first point of right.  So, I'm not sure why you use 
2, but it isn't a bug.  However...

You only need the array to be 8+6 if you take advantage of that shared 
point and store the 2 halves at "0..type" and "type-2 .. 2*type-2". 
Just a thought.  No real bug here.

> I found a problem with Dashing though. Curves like moveTo(0,0);
> curveTo(498,498,499,499,500,500); are not handled well at all.
> http://icedtea.classpath.org/~dlila/webrevs/noflatten2/webrev/ is the link
> with the new webrev. I have fixed this problem by doing binary search on
> the results of the flattening. I really don't like this solution because
> it does *a lot* more subdivisions than just flattening.

Ah, I get it now.  Hmmm.  We can leave it for now, but I'm pretty sure 
we can detect cases like this because the sides of the control polygon 
are not relatively equal and only do the recursion if the control 
polygon indicates some amount of acceleration is happening.  Leave it 
for now and make a mental note of this for later.  Also, if there is 
acceleration then I think you could just solve either the X or the Y 
cubic for the necessary point (xs = interp(x0,x1,len/leafLen), solve for 
xs).

One simplification to your binary search - since we know the length is 
relatively close to chord length, just compute the point on the curve at 
t and then use the distance formula to the start point to compute the 
arc length - no subdividing needed, just an eval and a linelen, and 
bsBuf goes away...

			...jim

> Regards,
> Denis.
>
> ----- "Jim Graham"<james.graham at oracle.com>  wrote:
>
>> HI Denis,
>>
>> On 10/6/2010 1:36 PM, Denis Lila wrote:
>>>
>>> webrev:
>>> http://icedtea.classpath.org/~dlila/webrevs/noflatten/webrev/
>>
>> TransformingPolyIOHelper should be in its own file - we consider more
>> than one class per file to be bad form these days, especially if the
>> class is used outside of that file.
>>
>> I'm a little troubled by how the PolyIOHelper fits into the design.
>> It's odd to talk to the same object for both input and output.  I have
>> some ideas there, but I think I'll leave it for a followon email and
>> effort.
>>
>> Dasher.java:
>>
>
>>
>>      boolean needsMoveto;
>>
>> in moveTo and pathDone:
>>       if (firstSegBuf is not empty) {
>>           output moveto(sx,sy)
>>           output firstSegs
>>       }
>>       needsMoveto = true;  // not needed in pathDone
>>
>> in goTo() {
>>       if (ON) {
>>           if (starting) {
>>               store it in firstSegBuf
>>           } else {
>>               if (needsMoveto) {
>>                   output moveto(x0,y0);
>>                   needsMoveto = false;
>>               }
>>               send it to output
>>           }
>>       } else {
>>           starting = false;
>>           needsMoveto = true;
>>           // nothing goes to output
>>       }
>> }
>>
>> and in ClosePath:
>>       lineToImpl(sx, sy, LINE);
>>       if (firstSegBuf is not empty) {
>>           if (!ON) {  // Or "if (needsMoveto)"
>>               output moveTo(sx, sy)
>>           }
>>           output firstSegs
>>       }
>>
>> I don't see a need for firstDashOn or fullCurve
>>
>>
>> Stroker.java:
>>
>> line 129 - proof that miterLimit does not need to be scaled... ;-)
>>
>> I'm going to send this buffer of comments off now and continue on with
>>
>> Stroker.java in a future email...
>>
>> 			...jim