code review 2275043: freetype: implement stroke for quadratic segments.

freetype: implement stroke for quadratic segments.

Fix bug where the String representation of a Fix32 representing minus
one quarter was "0:064" instead of "-0:064".

[nigeltao, 2010-09-26 14:49:05]

Hello r, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[nigeltao_gnome, 2010-10-01 09:43:27]

On 27 September 2010 00:49,  <nigeltao@golang.org> wrote:
> Description:
> freetype: implement stroke for quadratic segments.
>
> Fix bug where the String representation of a Fix32 representing minus
> one quarter was "0:064" instead of "-0:064".
>
> Please review this at http://codereview.appspot.com/2275043/

Ping.

[rog, 2010-10-01 09:54:35]

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go
File freetype/raster/stroke.go (right):

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:418: // Stroke adds the stroked Path q to p. The
resultant stroked path is typically
perhaps mention that the Path represents a line of the given width here. or does
it?

[rsc1, 2010-10-01 15:54:20]

LGTM

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go
File freetype/raster/geom.go (right):

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go#newco...
freetype/raster/geom.go:270: // AddPath adds the Path q to p.
It's not obvious to me why these methods are 
in this file, when type Path is apparently
defined in another file.

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go#newco...
freetype/raster/geom.go:282: // FirstPoint returns the first point in a
non-empty Path.
s/F/f/

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go#newco...
freetype/raster/geom.go:287: // LastPoint returns the last point in a non-empty
Path.
s/L/l/

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go#newco...
freetype/raster/geom.go:292: // AddPathReversed adds q reversed to p.
s/A/a/

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go
File freetype/raster/stroke.go (right):

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:37: // A RoundCapper adds round caps to a stroked
path.
These comments are written like they describe
interface types, but they are describing variables.
There's not a generic "A RoundCapper": there's just 
this specific one.

// RoundCapper adds round caps to a stroked path.

etc

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:38: var RoundCapper Capper = CapperFunc(func(p Adder,
halfWidth Fix32, pivot, n1 Point) {
I suggest writing these as:

var RoundCapper = CapperFunc(roundCapper)

func roundCapper(...) {

It will make the function names nicer in trace backs.
Also, making RoundCapper a CapperFunc instead of a Capper
means that you can still call it directly (and comparisons
with Capper values now work).

Do you expect to have implementations of Capper with state?
If not, another possibility is to just do

type Capper func(...)

and avoid the interface indirection.

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:82: // AddArc adds a circular arc from pivot+n0 to
pivot+n1 to p. The shorter of
s/A/a/

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:167: // Midpoint returns the midpoint of two Points.
s/M/m/

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:172: // AngleGreaterThan45 returns whether the angle
between two vectors is more
s/A/a/

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:179: // Interpolate returns the point (1-t)*a + t*b.
s/I/i/

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:187: // Curviest2 returns the value of t for which the
quadratic parametric curve
s/C/c/

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:230: // AddNonCurvy2 adds a quadratic segment to the
stroker, where the segment
s/A/a/

[r, 2010-10-01 16:43:20]

i can't say i understand the code on first reading, but it's certainly well
written.  do the algorithms appear somewhere you can cite?

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go
File freetype/raster/geom.go (right):

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go#newco...
freetype/raster/geom.go:282: // FirstPoint returns the first point in a
non-empty Path.
grump.  i think you're right but the insistence that we start with the name
fights the rule that english sentences start with capital letters.  my usual fix
for this quandary is to avoid starting the sentence with a word i want lower
case, but our style fights it. i lose either way.

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go
File freetype/raster/stroke.go (right):

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:8: // A Capper signifies how to begin or end a stroked
path.
you see, here we start the sentence with an article and clarity does not suffer.

this discussion is a side issue for the review, i understand.

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:15: // The CapperFunc type adapts an ordinary function
to be a Capper.
again an article (observation; not a request to change it)

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:88: if r2 < 4096 {
4096 is a magic number

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:163: multiple := Fix32(150 - 22*(d-181)/(256-181))
if 106 gets a name, so does 181. and maybe the name should be longer than 't'.

[r, 2010-10-01 16:43:38]

LGTM

[rsc, 2010-10-01 20:01:33]

> freetype/raster/geom.go:282: // FirstPoint returns the first point in a
> non-empty Path.
> grump.  i think you're right but the insistence that we start with the
> name fights the rule that english sentences start with capital letters.
> my usual fix for this quandary is to avoid starting the sentence with a
> word i want lower case, but our style fights it. i lose either way.

i agree that it's unfortunate, but it's definitely the convention
in the existing go library code.  in my mind it's the opposite
of the "proper names are capitalized even if not at the beginning
of the sentence" rule.

russ

[nigeltao_gnome, 2010-10-02 03:00:11]

On 2 October 2010 02:43,  <r@golang.org> wrote:
> i can't say i understand the code on first reading, but it's certainly
> well written.  do the algorithms appear somewhere you can cite?

The algorithm is basically the same as the one used by the Skia
graphics library. SkStroke.cpp doesn't give any references. I'll ask
the skia-discuss list.

[nigeltao, 2010-10-03 02:53:01]

*** Submitted as
http://code.google.com/p/freetype-go/source/detail?r=55fe78831abb ***

freetype: implement stroke for quadratic segments.

Fix bug where the String representation of a Fix32 representing minus
one quarter was "0:064" instead of "-0:064".

R=r, rsc, rog, nigeltao_gnome
CC=golang-dev
http://codereview.appspot.com/2275043

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go
File freetype/raster/geom.go (right):

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go#newco...
freetype/raster/geom.go:270: // AddPath adds the Path q to p.
On 2010/10/01 15:54:20, rsc1 wrote:
> It's not obvious to me why these methods are 
> in this file, when type Path is apparently
> defined in another file.

Path is defined in this file, on line 178.

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/geom.go#newco...
freetype/raster/geom.go:282: // FirstPoint returns the first point in a
non-empty Path.
On 2010/10/01 16:43:20, r wrote:
> grump.  i think you're right but the insistence that we start with the name
> fights the rule that english sentences start with capital letters.  my usual
fix
> for this quandary is to avoid starting the sentence with a word i want lower
> case, but our style fights it. i lose either way.

I'm painting my bike-shed with lower-case initial letters.

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go
File freetype/raster/stroke.go (right):

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:38: var RoundCapper Capper = CapperFunc(func(p Adder,
halfWidth Fix32, pivot, n1 Point) {
On 2010/10/01 15:54:20, rsc1 wrote:
> Do you expect to have implementations of Capper with state?
> If not, another possibility is to just do
> 
> type Capper func(...)
> 
> and avoid the interface indirection.

It's not implemented, but a MiterJoiner would have a miter limit parameter, so
I'd implement it as something like

type MiterJoiner Fix32

which means I'd prefer to keep Joiner as an interface type rather than a
function type.

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:163: multiple := Fix32(150 - 22*(d-181)/(256-181))
On 2010/10/01 16:43:20, r wrote:
> if 106 gets a name, so does 181. and maybe the name should be longer than 't'.

t (representing tan(π/8)) renamed for tpo8. The other magic numbers are used
only on this line and have an extensive comment immediately above, so I don't
see the need for a named constant.

[nigeltao, 2010-10-03 02:53:29]

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go
File freetype/raster/stroke.go (right):

http://codereview.appspot.com/2275043/diff/2001/freetype/raster/stroke.go#new...
freetype/raster/stroke.go:163: multiple := Fix32(150 - 22*(d-181)/(256-181))
On 2010/10/01 16:43:20, r wrote:
> if 106 gets a name, so does 181. and maybe the name should be longer than 't'.

I've renamed t (representing tan(π/8)) to tpo8. The other magic numbers are used
only on this line and have an extensive comment immediately above, so I don't
see the need for a named constant.