Bug in contextPrototype.quadraticCurveTo ?

[Kirill Sorokin]

Hi there!

The current revision -- r35, see [1], has the following implementation
of quadraticCurveTo.

contextPrototype.quadraticCurveTo = function(aCPx, aCPy, aX, aY) {
// the following is lifted almost directly from
// http://developer.mozilla.org/en/docs/Canvas_tutorial:Drawing_shapes
var cp1x = this.currentX_ + 2.0 / 3.0 * (aCPx - this.currentX_);
var cp1y = this.currentY_ + 2.0 / 3.0 * (aCPy - this.currentY_);
var cp2x = cp1x + (aX - this.currentX_) / 3.0;
var cp2y = cp1y + (aY - this.currentY_) / 3.0;
this.bezierCurveTo(cp1x, cp1y, cp2x, cp2y, aX, aY);
};

There is a serious problem with it, as it mixes different cooridnate
systems. currentX_ and currentY_ are from the VML coordinate space,
while aCPx, aCPy, aX and aY come from the original canvas coordinates.
This mixture causes the originally-correct algorithm to produce wrong
results. :(

I had fixed this by reimplementing the method (essentially I added the
pre-conversion step for the latter 4 variables):

contextPrototype.quadraticCurveTo = function(aCPx, aCPy, aX, aY) {
// the following is lifted almost directly from
// http://developer.mozilla.org/en/docs/Canvas_tutorial:Drawing_shapes
//
// ksorokin: the control points are calculated incorrectly, as it
is clear that
// the points come from differnet coordinate spaces

//var cp1x = this.currentX_ + 2.0 / 3.0 * (aCPx - this.currentX_);
//var cp1y = this.currentY_ + 2.0 / 3.0 * (aCPy - this.currentY_);
//var cp2x = cp1x + (aX - this.currentX_) / 3.0;
//var cp2y = cp1y + (aY - this.currentY_) / 3.0;
//this.bezierCurveTo(cp1x, cp1y, cp2x, cp2y, aX, aY);

var _cp = this.getCoords_(aCPx, aCPy);
var p = this.getCoords_(aX, aY);

var cp1x = this.currentX_ + 2.0 / 3.0 * (_cp.x - this.currentX_);
var cp1y = this.currentY_ + 2.0 / 3.0 * (_cp.y - this.currentY_);
var cp2x = cp1x + (p.x - this.currentX_) / 3.0;
var cp2y = cp1y + (p.y - this.currentY_) / 3.0;

this.currentPath_.push({type: 'bezierCurveTo',
cp1x: cp1x,
cp1y: cp1y,
cp2x: cp2x,
cp2y: cp2y,
x: p.x,
y: p.y});
this.currentX_ = p.x;
this.currentY_ = p.y;
};

Just FYI..

Kirill

[erik.ar...@gmail.com]

Thanks

I'll apply your patch when I get back from my vacation unless someone
else beats me to it.

Do you also have a simple test case that we can use to ensure correct behavior?

--
erik

[Erik Arvidsson]

Hi Emil,

Do you mind code reviewing a patch that fixes quadraticCurveTo?

I expanded the test case a bit to expose the bug.

--
erik

[Emil A Eklund]

On Wed, 2008-09-17 at 23:45 -0700, Erik Arvidsson wrote:
> Hi Emil,
>
> Do you mind code reviewing a patch that fixes quadraticCurveTo?
>
> I expanded the test case a bit to expose the bug.

Looks good to me, thanks for fixing it!

--
Emil A Eklund
e...@eae.net

[Erik Arvidsson]

Committed and fixed in revision 37.

--
erik