code review 50910043: freetype/truetype: calculate advance widths correctly.

freetype/truetype: calculate advance widths correctly.

The logic is a little clunky, but as before, let's get it right first,
then get it clean once we have a full battery of tests.

[nigeltao, 2014-01-12 10:07:14]

Hello bsiegert@gmail.com (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/freetype-go

[bsiegert, 2014-01-12 12:09:05]

LGTM modulo these comments.

The logic is sort of hard to follow though, I am hoping for improvements in the
near future :).

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go
File freetype/freetype.go (left):

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go#oldco...
freetype/freetype.go:127: // rasterize returns the glyph mask and integer-pixel
offset to render the
mention additional return value in the docstring.

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go#oldco...
freetype/freetype.go:162: // glyph at the given sub-pixel point. It is a cache
for the rasterize method.
same as above.

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go
File freetype/freetype.go (right):

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go#newco...
freetype/freetype.go:132: *image.Alpha, image.Point, raster.Fix32, error) {
Not a fan of the linebreak and the empty line at the start of a function.

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go#newco...
freetype/freetype.go:135: return nil, image.Point{}, 0, err
Why change the ZP to Point{} here and below? They should be the same, no?

https://codereview.appspot.com/50910043/diff/20001/freetype/truetype/glyph.go
File freetype/truetype/glyph.go (right):

https://codereview.appspot.com/50910043/diff/20001/freetype/truetype/glyph.go...
freetype/truetype/glyph.go:184: if g0+10 <= g1 {
A comment or a constant for the "g0+10" would be nice.

[nigeltao, 2014-01-13 22:57:17]

Submitting...

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go
File freetype/freetype.go (left):

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go#oldco...
freetype/freetype.go:127: // rasterize returns the glyph mask and integer-pixel
offset to render the
On 2014/01/12 12:09:05, bsiegert wrote:
> mention additional return value in the docstring.

Done.

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go#oldco...
freetype/freetype.go:162: // glyph at the given sub-pixel point. It is a cache
for the rasterize method.
On 2014/01/12 12:09:05, bsiegert wrote:
> same as above.

Done.

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go
File freetype/freetype.go (right):

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go#newco...
freetype/freetype.go:132: *image.Alpha, image.Point, raster.Fix32, error) {
On 2014/01/12 12:09:05, bsiegert wrote:
> Not a fan of the linebreak and the empty line at the start of a function.

Heh, I'd actually wouldn't mind seeing that style be gofmt-enforced.

https://codereview.appspot.com/50910043/diff/20001/freetype/freetype.go#newco...
freetype/freetype.go:135: return nil, image.Point{}, 0, err
On 2014/01/12 12:09:05, bsiegert wrote:
> Why change the ZP to Point{} here and below? They should be the same, no?

In retrospect, I think that ZP (and ZR) were a mistake. Using ZP here requires a
load from memory as opposed to immediate zeroes, and strictly speaking, stupid
code can change the value of image.ZP. ZP and ZR were designed very early on in
Go history. Perhaps we didn't have struct literals at the time, or at least
image.Rectangle{} seemed very awkward so ZR was invented (and ZP for
consistency); I can't remember.

https://codereview.appspot.com/50910043/diff/20001/freetype/truetype/glyph.go
File freetype/truetype/glyph.go (right):

https://codereview.appspot.com/50910043/diff/20001/freetype/truetype/glyph.go...
freetype/truetype/glyph.go:184: if g0+10 <= g1 {
On 2014/01/12 12:09:05, bsiegert wrote:
> A comment or a constant for the "g0+10" would be nice.

Done.

[nigeltao, 2014-01-13 22:58:15]

*** Submitted as
https://code.google.com/p/freetype-go/source/detail?r=c2dfc36777ee ***

freetype/truetype: calculate advance widths correctly.

The logic is a little clunky, but as before, let's get it right first,
then get it clean once we have a full battery of tests.

R=bsiegert
CC=golang-codereviews
https://codereview.appspot.com/50910043