code review 2208041: freetype: make the point an argument to DrawString, not...

freetype: make the point an argument to DrawString, not part of the
context state. Also rename DrawText to DrawString.

[nigeltao, 2010-09-15 03:41:09]

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

I'd like you to review this change.

[r, 2010-09-15 04:30:12]

LGTM

[nigeltao, 2010-09-15 04:35:39]

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

freetype: make the point an argument to DrawString, not part of the
context state. Also rename DrawText to DrawString.

R=r
CC=golang-dev
http://codereview.appspot.com/2208041

[rog, 2010-09-15 08:27:42]

at the risk of being boring, can i reiterate that i don't think
the source and destination rectangles and the clip rectangle
should be in the context either.

the only reason that they are there, it seems to me, is to save
passing extra arguments to DrawString - but in many cases they
will need to be different each time, which makes one call into four
(or even 6, if you set dst and src back to nil each time to allow them
to be garbage collected)

On 15 September 2010 04:41,  <nigeltao@golang.org> wrote:
> Reviewers: r,
>
> Message:
> Hello r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> freetype: make the point an argument to DrawString, not part of the
> context state. Also rename DrawText to DrawString.
>
> Please review this at http://codereview.appspot.com/2208041/
>
> Affected files:
>  M example/freetype/main.go
>  M freetype/freetype.go
>
>
> Index: example/freetype/main.go
> ===================================================================
> --- a/example/freetype/main.go
> +++ b/example/freetype/main.go
> @@ -104,8 +104,7 @@
>        // Draw the text.
>        pt := freetype.Pt(10, 10+c.FUnitToPixelRU(font.UnitsPerEm()))
>        for _, s := range text {
> -               c.SetPoint(pt)
> -               err = c.DrawText(s)
> +               _, err = c.DrawString(s, pt)
>                if err != nil {
>                        log.Stderr(err)
>                        return
> Index: freetype/freetype.go
> ===================================================================
> --- a/freetype/freetype.go
> +++ b/freetype/freetype.go
> @@ -209,27 +209,27 @@
>        return mask, offset.Add(image.Point{ix, iy}), nil
>  }
>
> -// DrawText draws s at the current point and then advances the point. The
> text
> +// DrawString draws s at p and returns p advanced by the text extent. The
> text
>  // is placed so that the left edge of the em square of the first character
> of s
> -// and the baseline intersect at the point. The majority of the affected
> pixels
> -// will be above and to the right of the point, but some may be below or to
> the
> -// left. For example, drawing a string that starts with a 'J' in an italic
> font
> -// may affect pixels below and left of the point.
> -func (c *Context) DrawText(s string) os.Error {
> +// and the baseline intersect at p. The majority of the affected pixels
> will be
> +// above and to the right of the point, but some may be below or to the
> left.
> +// For example, drawing a string that starts with a 'J' in an italic font
> may
> +// affect pixels below and left of the point.
> +func (c *Context) DrawString(s string, p raster.Point) (raster.Point,
> os.Error) {
>        if c.font == nil {
> -               return os.NewError("freetype: DrawText called with a nil
> font")
> +               return raster.Point{}, os.NewError("freetype: DrawText
> called with a nil font")
>        }
>        prev, hasPrev := truetype.Index(0), false
>        for _, rune := range s {
>                index := c.font.Index(rune)
>                if hasPrev {
> -                       c.pt.X += c.FUnitToFix32(int(c.font.Kerning(prev,
> index)))
> +                       p.X += c.FUnitToFix32(int(c.font.Kerning(prev,
> index)))
>                }
> -               mask, offset, err := c.glyph(index, c.pt)
> +               mask, offset, err := c.glyph(index, p)
>                if err != nil {
> -                       return err
> +                       return raster.Point{}, err
>                }
> -               c.pt.X +=
> c.FUnitToFix32(int(c.font.HMetric(index).AdvanceWidth))
> +               p.X +=
> c.FUnitToFix32(int(c.font.HMetric(index).AdvanceWidth))
>                glyphRect := mask.Bounds().Add(offset)
>                dr := c.clip.Intersect(glyphRect)
>                if !dr.Empty() {
> @@ -238,7 +238,7 @@
>                }
>                prev, hasPrev = index, true
>        }
> -       return nil
> +       return p, nil
>  }
>
>  // recalc recalculates scale and bounds values from the font size, screen
> @@ -303,12 +303,6 @@
>        c.src = src
>  }
>
> -// SetPoint sets the point where DrawText will next draw text.
> -// pt is a raster.Point and can therefore represent sub-pixel positions.
> -func (c *Context) SetPoint(pt raster.Point) {
> -       c.pt = pt
> -}
> -
>  // SetClip sets the clip rectangle for drawing.
>  func (c *Context) SetClip(clip image.Rectangle) {
>        c.clip = clip
>
>
>

[nigeltao_gnome, 2010-09-17 02:30:41]

On 15 September 2010 18:27, roger peppe <rogpeppe@gmail.com> wrote:
> at the risk of being boring, can i reiterate that i don't think
> the source and destination rectangles and the clip rectangle
> should be in the context either.

On second thoughts, I think you're right. Maybe DrawString should be
like draw.DrawMask, and take a dst, dr, src, sp, 'mask' and op. The
difference between them is that DrawString uses a (string,
raster.Point) to define a mask instead of an (image.Image,
image.Point).


> the only reason that they are there, it seems to me, is to save
> passing extra arguments to DrawString - but in many cases they
> will need to be different each time, which makes one call into four
> (or even 6, if you set dst and src back to nil each time to allow them
> to be garbage collected)

It's funny. In the toy text-editor that I'm playing with, the dst and
src are always the same; I set it only once. It's good to have
different users of an API.

[rog, 2010-09-17 09:56:16]

On 17 September 2010 03:30, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> On 15 September 2010 18:27, roger peppe <rogpeppe@gmail.com> wrote:
>> at the risk of being boring, can i reiterate that i don't think
>> the source and destination rectangles and the clip rectangle
>> should be in the context either.
>
> On second thoughts, I think you're right. Maybe DrawString should be
> like draw.DrawMask, and take a dst, dr, src, sp, 'mask' and op. The
> difference between them is that DrawString uses a (string,
> raster.Point) to define a mask instead of an (image.Image,
> image.Point).

that was the idea of my earlier suggestion for the API,
except i added in the clip rectangle, because it seems
difficult to simulate otherwise (without messing with Painters), and
it doesn't make
sense to pass dr, as the extent of the text is calculated by freetype.

i didn't add op, which is probably needed.

how about this?

// DrawText aligns p in dst with sp in src and draws the text s
// onto dst using src and operator op. No pixels are drawn outside clipr.
// It returns the next position for text to be drawn at.
func (c *Context) DrawString(dst draw.Image, p image.Point,
    src draw.Image, sp image.Point, s string, op draw.Op,
    clipr image.Rectangle) draw.Point

> It's funny. In the toy text-editor that I'm playing with, the dst and
> src are always the same; I set it only once. It's good to have
> different users of an API.

in that kind of situation, i usually define a method on a type that holds
the dst and src that calls the original function with dst and src added in.
it's less hassle that way than the other way around.

also doing it this way means that the API isn't inherently thread-unsafe - it'd
be easy to put a mutex around the cache code if desired.

PS i will be sad if Painters are excluded from the freetype API entirely.

[nigeltao_gnome, 2010-09-19 02:39:42]

On 17 September 2010 19:56, roger peppe <rogpeppe@gmail.com> wrote:
> On 17 September 2010 03:30, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
>> On 15 September 2010 18:27, roger peppe <rogpeppe@gmail.com> wrote:
>>> at the risk of being boring, can i reiterate that i don't think
>>> the source and destination rectangles and the clip rectangle
>>> should be in the context either.
>>
>> On second thoughts, I think you're right. Maybe DrawString should be
>> like draw.DrawMask, and take a dst, dr, src, sp, 'mask' and op. The
>> difference between them is that DrawString uses a (string,
>> raster.Point) to define a mask instead of an (image.Image,
>> image.Point).
>
> that was the idea of my earlier suggestion for the API,
> except i added in the clip rectangle, because it seems
> difficult to simulate otherwise (without messing with Painters), and
> it doesn't make
> sense to pass dr, as the extent of the text is calculated by freetype.

Maybe it isn't obvious (it makes sense to me, but I'm not a neutral
witness), but dr acts as the clip.

I guess I should polish up my toy text editor and publish it somewhere.


> how about this?
>
> // DrawText aligns p in dst with sp in src and draws the text s
> // onto dst using src and operator op. No pixels are drawn outside clipr.
> // It returns the next position for text to be drawn at.
> func (c *Context) DrawString(dst draw.Image, p image.Point,
>    src draw.Image, sp image.Point, s string, op draw.Op,
>    clipr image.Rectangle) draw.Point

At least one of those image.Points has to be a raster.Point if you
want to be able to position text at sub-pixel resolution.

http://www.antigrain.com/research/font_rasterization/index.html makes
an argument for sub-pixel positioning.

[rog, 2010-09-19 10:46:20]

On 19 September 2010 03:39, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> On 17 September 2010 19:56, roger peppe <rogpeppe@gmail.com> wrote:
>> On 17 September 2010 03:30, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
>>> On 15 September 2010 18:27, roger peppe <rogpeppe@gmail.com> wrote:
>>>> at the risk of being boring, can i reiterate that i don't think
>>>> the source and destination rectangles and the clip rectangle
>>>> should be in the context either.
>>>
>>> On second thoughts, I think you're right. Maybe DrawString should be
>>> like draw.DrawMask, and take a dst, dr, src, sp, 'mask' and op. The
>>> difference between them is that DrawString uses a (string,
>>> raster.Point) to define a mask instead of an (image.Image,
>>> image.Point).
>>
>> that was the idea of my earlier suggestion for the API,
>> except i added in the clip rectangle, because it seems
>> difficult to simulate otherwise (without messing with Painters), and
>> it doesn't make
>> sense to pass dr, as the extent of the text is calculated by freetype.
>
> Maybe it isn't obvious (it makes sense to me, but I'm not a neutral
> witness), but dr acts as the clip.

yes, i realised that that's how you intended to use dr, but i'm not sure
it works well used like that.

in draw.DrawMask, dr acts not only as the clip rectangle, but
its min point also acts as the alignment point for the source
and mask images. i'm don't think it's working that way above,
which makes it subtly (and perhaps misleadingly) different from DrawMask,
even though it is defined with reference to DrawMask.

in your proposed arguments to DrawString above, i think
you are imagining that sp specifies the intersection of the first em square
and the baseline of the first character (i.e. where the text
will be drawn). but there's a problem: if this is aligned with dr.Min, then dr
cannot be the clip rectangle, because the text can drop below
the baseline. if it is not aligned with dr.Min, then where *is*
the source image going to be aligned? it's important to be
able to do the equivalent of both:

draw.DrawMask(dst, dr, src, sp, mask, sp, draw.Over)
and
draw.DrawMask(dst, dst, src, draw.ZP, mask, draw.ZP, draw.Over)

the first one shows a source image that remains still when the
points drawn at change; the second moves the source image along
with the drawn pixels.

> I guess I should polish up my toy text editor and publish it somewhere.

please do! polished or not...

>> how about this?
>>
>> // DrawText aligns p in dst with sp in src and draws the text s
>> // onto dst using src and operator op. No pixels are drawn outside clipr.
>> // It returns the next position for text to be drawn at.
>> func (c *Context) DrawString(dst draw.Image, p image.Point,
>>    src draw.Image, sp image.Point, s string, op draw.Op,
>>    clipr image.Rectangle) draw.Point
>
> At least one of those image.Points has to be a raster.Point if you
> want to be able to position text at sub-pixel resolution.
>
> http://www.antigrain.com/research/font_rasterization/index.html makes
> an argument for sub-pixel positioning.

i agree. and neither does the comment say where the text will actually
be drawn! (i intended p, not sp, by analogy with DrawMask, where the
second argument specifies the destination).

so we'd have

func (c *Context) DrawString(dst draw.Image, p raster.Point,
          src draw.Image, sp image.Point, s string, op draw.Op,
          clipr image.Rectangle)

however, one tricky issue: what does it mean to allign sp with p
when sp is pixel-based and p is sub-pixel-based?
if sp was a raster.Point, that would imply that we could do
sub-pixel operations in general, which we cannot.

perhaps we could say something like this:
// DrawText draws s at point p in dst using src and clipping
// to clipr. sp in src will be aligned to p in dst, rounded to
// the nearest whole pixel. The point p specifies the intersection
// of the text baseline with the left edge of the em square of
// the first character of the text.

PS another thought: if i wanted to draw one character in a string
in a different colour, would that break any kerning hints?