code review 2171045: freetype: add a clip rectangle.

freetype: add a clip rectangle.

[nigeltao, 2010-09-13 08:15:18]

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

I'd like you to review this change.

[r, 2010-09-13 08:16:47]

LGTM

[nigeltao, 2010-09-13 08:21:12]

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

freetype: add a clip rectangle.

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

[rog, 2010-09-13 11:26:50]

i'm a bit concerned by the way that freetype.Context is going.
AFAICS, it is significantly less flexible than it was, as it does
not now provide the capability to draw through a custom Painter.
in code i've previously written, that capability is very useful
(for instance i used it to implement hit testing). i realise that this
is being done to enable glyph caching, but perhaps this could
be done slightly differently (see below).

also, the move from explicitly specifying the point at which
the text is drawn to having the text position implicit in the context
doesn't seem great to me. i'd be happier if the point was explicit
and the new point was returned by DrawText. currently there's not
even a way to get the new point out of Context, although i realise
that's easily fixable.

perhaps there could be two methods on Context,
DrawText and DrawTextPainter:

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

// DrawTextPainter draws the text s at dp using p. It returns
// the next position for text to be drawn at.
func (c *Context) DrawTextPainter(p raster.Painter, dp image.Point, s
string) draw.Point

i removed the error return because passing a nil Font is really no different
to passing a nil anything else - and we don't check that everything is non-nil
at every opportunity; a panic is fine IMHO. that is: the caller of DrawText
can easily check for the error (font != nil) without checking the return
from DrawText every time.

sorry for the late addition; i haven't looked at this stuff in a while
(i'm waiting until it stabilises before updating my canvas stuff).

On 13 September 2010 09:15,  <nigeltao@golang.org> wrote:
> Reviewers: r,
>
> Message:
> Hello r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> freetype: add a clip rectangle.
>
> Please review this at http://codereview.appspot.com/2171045/
>
> 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
> @@ -91,6 +91,7 @@
>        c.SetDPI(*dpi)
>        c.SetFont(font)
>        c.SetFontSize(*size)
> +       c.SetClip(rgba.Bounds())
>        c.SetDst(rgba)
>        c.SetSrc(fg)
>
> Index: freetype/freetype.go
> ===================================================================
> --- a/freetype/freetype.go
> +++ b/freetype/freetype.go
> @@ -56,6 +56,8 @@
>        glyphBuf *truetype.GlyphBuf
>        // pt is the location where drawing starts.
>        pt raster.Point
> +       // clip is the clip rectangle for drawing.
> +       clip image.Rectangle
>        // dst and src are the destination and source images for drawing.
>        dst draw.Image
>        src image.Image
> @@ -228,7 +230,12 @@
>                        return err
>                }
>                c.pt.X +=
> c.FUnitToFix32(int(c.font.HMetric(index).AdvanceWidth))
> -               draw.DrawMask(c.dst, mask.Bounds().Add(offset), c.src,
> image.ZP, mask, image.ZP, draw.Over)
> +               glyphRect := mask.Bounds().Add(offset)
> +               dr := c.clip.Intersect(glyphRect)
> +               if !dr.Empty() {
> +                       mp := image.Point{0, dr.Min.Y - glyphRect.Min.Y}
> +                       draw.DrawMask(c.dst, dr, c.src, image.ZP, mask, mp,
> draw.Over)
> +               }
>                prev, hasPrev = index, true
>        }
>        return nil
> @@ -302,6 +309,11 @@
>        c.pt = pt
>  }
>
> +// SetClip sets the clip rectangle for drawing.
> +func (c *Context) SetClip(clip image.Rectangle) {
> +       c.clip = clip
> +}
> +
>  // TODO(nigeltao): implement Context.SetGamma.
>
>  // NewContext creates a new Context.
>
>
>

[nigeltao_gnome, 2010-09-14 00:59:13]

On 13 September 2010 21:26, roger peppe <rogpeppe@gmail.com> wrote:
> i removed the error return because passing a nil Font is really no different
> to passing a nil anything else - and we don't check that everything is non-nil
> at every opportunity; a panic is fine IMHO. that is: the caller of DrawText
> can easily check for the error (font != nil) without checking the return
> from DrawText every time.

The error isn't necessarily a nil font; it could be a malformed font.
I also can't remember whether a missing glyph is an error or silently
falls back to a default glyph.

I'm still thinking about your Painter suggestion. It was removed
mostly because of glyph caching, but also partly because people found
the number of moving parts confusing. My current thinking is that the
freetype/raster and freetype/truetype packages should provide the
lower-level APIs to let you do whatever you want, and the freetype
top-level package provides a convenience API that makes common tasks
easy without necessarily giving you the kitchen sink. Doing something
with a custom Painter might require using the lower-level APIs.

[r, 2010-09-14 03:26:50]

i agree with the proposal to keep the point as an explicit argument
and return value.

-rob

[rog, 2010-09-14 08:29:39]

On 14 September 2010 01:59, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> On 13 September 2010 21:26, roger peppe <rogpeppe@gmail.com> wrote:
>> i removed the error return because passing a nil Font is really no different
>> to passing a nil anything else - and we don't check that everything is
non-nil
>> at every opportunity; a panic is fine IMHO. that is: the caller of DrawText
>> can easily check for the error (font != nil) without checking the return
>> from DrawText every time.
>
> The error isn't necessarily a nil font; it could be a malformed font.

yes, i'd missed that. i still think that the error return is somewhat
unnecessary. as a caller of DrawText, there's very little i can do
about a malformed font - it's not a show stopper, and i'd like as
much valid text to be displayed as possible.

> I also can't remember whether a missing glyph is an error or silently
> falls back to a default glyph.

i'd prefer the latter option (possibly with a way to log errors when
they happen, to enable debugging of buggy fonts)

the alternative, as i see it, is that every client that draws text
will either display nothing, or quit with an error, when a dodgy
font is encountered - neither behaviour being particularly desirable.

> I'm still thinking about your Painter suggestion. It was removed
> mostly because of glyph caching, but also partly because people found
> the number of moving parts confusing. My current thinking is that the
> freetype/raster and freetype/truetype packages should provide the
> lower-level APIs to let you do whatever you want, and the freetype
> top-level package provides a convenience API that makes common tasks
> easy without necessarily giving you the kitchen sink. Doing something
> with a custom Painter might require using the lower-level APIs.

the problem with this is that freetype.go contains quite a bit of complex
functionality. to use the lower level APIs all that would need to be
duplicated or rewritten, which doesn't seem right to me.

personally, i found the Painter interface quite intuitive,
although i certainly agree that use of it should be optional.