[Webkit-unassigned] [Bug 11929] Gradient SVG animation demonstrates tearing at animation extremes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=11929





--- Comment #12 from Dirk Schulze <krit at webkit.org>  2009-12-03 00:24:44 PST ---
(In reply to comment #11)
> (From update of attachment 44155 [details])
> Good morning Dirk,
> 
> I see some issues with the patch:
> - ChangeLogs are not using the correct style: bug url should sit above
> description, without surrounding brackets (best idea is to use
> "prepare-ChangeLog --bug=<bug-id>", this takes care of using the right style
> for you)

I take care of it in the next patch.

> - The patch can't go in without updated test results (I can help with that, if
> you don't have a mac around)

I updated the results already.

> 
> > -    float fdx = focalPoint.x() - centerPoint.x();
> > -    float fdy = focalPoint.y() - centerPoint.y();
> > +    FloatPoint adjustedFocalPoint = focalPoint;
> > +    adjustedFocalPoint.move(-centerPoint.x(), -centerPoint.y());
> I agree with the first change, using 'FloatPoint adjustedFocalPoint' instead of
> the two adjustedFocusX/adjustedFocusY variables, though the "fdx" ones are
> useful, as they save function call overhead in the sqrt() below, just the name
> is bad. (Re-)Introducing helper variables, also saves you from having to mve
> back the adjustFocalPoint by the centerPoint below the first if() statement.

Well fdx/fdy can be renamed dfx/dfy (d stands for delta). Is this ok for you?

> > +        // The maximum deviation of 0.2% is needed for Cairo, since Cairo
> > +        // is working with fixed point numbers.
> > +        if (focalPoint.x() < centerPoint.x())
> > +            adjustedFocalPoint.setX(cosf(angle) * radius + 0.002f);
> > +        else
> > +            adjustedFocalPoint.setX(cosf(angle) * radius - 0.002f);
> > +        if (focalPoint.y() < centerPoint.y())
> > +            adjustedFocalPoint.setY(sinf(angle) * radius + 0.002f);
> > +        else
> > +            adjustedFocalPoint.setY(sinf(angle) * radius - 0.002f);
> 
> This really worries me. If you really need a Cairo specific workaround, it
> can't go directly into SVGRadialGradientElement. Not sure about the root of
> your problem, and how to fix it in another way, offhand. Please elaborate on
> that.

We do platform specific things everywhere in WebCore (mostly for CG), also for
SVG (see SVGPaintServerGradient as example). The deviation of maximal 0.2% is
not noticable, even if you zoom as far as possible into the document, you won't
see a difference.
I'm fine to make #if PLATFORM(Cairo) around this code, but it is not possible
to move this deviation into the platform specific code in
platform/graphics/cairo. That would mean to move the complete calculation of
adjustedFocalPoint into platform/graphics/* (and replication of the same code
multiple times).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.