code review 1592044: A a function NewPainter to draw from an arbitrary sourc...

A a function NewPainter to draw from an arbitrary source image to an arbitrary destination.

[rog, 2010-06-11 13:18:24]

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

I'd like you to review this change.

[nigeltao, 2010-06-16 02:25:18]

I'm not sure that this is the right approach. Calling draw.DrawMask on each span
could be relatively expensive. It might be better to accumulate all the spans
into one mask image (whether that's an image.Alpha16 or a new Image type
implemented as a run of spans) and then just do one draw.DrawMask(..., mask,
...) call.

I'm sounding like a stuck record, but let me think about it...

http://codereview.appspot.com/1592044/diff/7001/8001
File freetype/raster/paint.go (right):

http://codereview.appspot.com/1592044/diff/7001/8001#newcode259
freetype/raster/paint.go:259: // slow but comprehensive.
Doc comments should start with a noun, and be complete sentences.

http://codereview.appspot.com/1592044/diff/7001/8001#newcode286
freetype/raster/paint.go:286: draw.Color(a | a<<8 | a<<16 | a<<24),
draw.Color should be considered deprecated. Please use an image.ColorImage of an
image.Alpha16Color instead.

[rog, 2010-06-16 07:48:48]

On 16 June 2010 03:25,  <nigeltao@golang.org> wrote:
> I'm not sure that this is the right approach. Calling draw.DrawMask on
> each span could be relatively expensive. It might be better to
> accumulate all the spans into one mask image (whether that's an
> image.Alpha16 or a new Image type implemented as a run of spans) and
> then just do one draw.DrawMask(..., mask, ...) call.

yes, i'm sure there are lots of possible optimisations,
but for the moment, can we start with an implementation
that just works? it's still useful even if slow; for
instance i'm using it once to create a source image that
will be used many times.

> http://codereview.appspot.com/1592044/diff/7001/8001#newcode286
> freetype/raster/paint.go:286: draw.Color(a | a<<8 | a<<16 | a<<24),
> draw.Color should be considered deprecated. Please use an
> image.ColorImage of an image.Alpha16Color instead.

if it had been there, i'd have used it! thanks.

[nigeltao, 2010-06-16 23:25:54]

On 2010/06/16 07:48:48, rog wrote:
> yes, i'm sure there are lots of possible optimisations,
> but for the moment, can we start with an implementation
> that just works? it's still useful even if slow; for
> instance i'm using it once to create a source image that
> will be used many times.

Can you describe your use case? I'm having trouble imagining when I'd want to
e.g. draw some text with a lolcat image source. Sure, patterned text is a great
technical demo but I don't see the utility. Then again, I'm a grumpy old stodge
who doesn't see the point of partially transparent xterms either.

For the use cases I do have (e.g. text editor), the existing RGBAPainter works
for me. If you want to do custom painting then you can write your own Painter in
your own package without having to touch the freetype/raster package, but at the
moment I suspect it's rare enough (and that there's an existing alternative
approach of painting to an intermediate image.Alpha mask) that means I hesitate
to put it into freetype/raster.

Finally, if your destination images are always image.RGBA, it might be better to
simply adapt RGBAPainter to have an image.Image source instead of just an
image.Color source, rather than adding a slow genericImagePainter type.

[rog, 2010-06-17 08:47:12]

On 17 June 2010 00:25,  <nigeltao@golang.org> wrote:
> On 2010/06/16 07:48:48, rog wrote:
> Can you describe your use case? I'm having trouble imagining when I'd
> want to e.g. draw some text with a lolcat image source. Sure, patterned
> text is a great technical demo but I don't see the utility. Then again,
> I'm a grumpy old stodge who doesn't see the point of partially
> transparent xterms either.

it's the destination i'm concerned with, not the source.

my use case is that i've been trying to put together a
framework for composing arbitrary graphical widgets,
and that framework does not specify that the image
being drawn onto must be an image.RGBA.

this means that the widgets themselves are written
to be agnostic about what they're drawing onto,
which means that the package as a whole becomes
more flexible.

it means that, for instance, i can use the same machinery
to draw onto a high quality RGBA64 image if i wish;
or i could extract the transparency properties of the
resulting image by drawing onto an Alpha.

yes, i could create a custom "general" painter that
does a switch on image type and invokes the appropriate
specialised raster painter, but it seems to me that this
is useful functionality, and should live in raster itself.


here's a more specific example, which presupposes CL 1636044:

in this framework, each widget knows how and where to draw itself.
it implements a Draw method:

    Draw(dst draw.Image, clipr draw.Rectangle)

it also implements a Bbox method, to enable
the caller to find out the bounding box of the widget:

    Bbox() draw.Rectangle

i wish to implement scrolling in this framework, so that
an arbitrary widget can be panned about in the
window, with only some sub-rectangle of the widget visible
at any one time. one way of doing this would be
to create a temporary image of size widget.Bbox().Max,
have the widget draw into that, then copy to the destination.
this could be very wasteful of space, however,
and there's an alternative: create an image that only
provides backing for some sub-rectangle of its purported size.
i wrote a function to do that (only possible with CL 1636044):

// SliceImage returns an image which is a view onto a portion of img.
// The returned image has the specified width and height,
// but all draw operations are clipped to r.
// The origin of img is aligned with p. Where img
// overlaps with r, it will be used for drawing operations.
//
func SliceImage(width, height int, r draw.Rectangle,
    img draw.Image, p draw.Point) draw.Image

given this, scrolling becomes straightforward - the widget thinks
it's drawing at its usual position, but the actual draw operations
are translated into the actual destination window coordinates.

another example that uses the same mechanism allows an
image "snapshot" to be taken of an image, that can be later
used as a cache source for actually drawing it (that's the
example i alluded to in my previous email).

> (and that there's an existing alternative approach of painting to an
> intermediate image.Alpha mask)

this can be very expensive when there are many
not affected by the draw operation (for example a long diagonal line).
the overhead is definitely user-perceptible.

footnote: (sorry about the length of all this). the original
plan 9/inferno primitives allowed an image to have an
arbitrary origin - if all the draw and image interfaces were
changed to allow this, then some of my need for this
change would go away, but i think it's justified anyway
for the reasons mentioned earlier.

thanks for bearing with me here.

[nigeltao_gnome, 2010-06-18 01:15:14]

On 17 June 2010 18:47, roger peppe <rogpeppe@gmail.com> wrote:
> footnote: (sorry about the length of all this). the original
> plan 9/inferno primitives allowed an image to have an
> arbitrary origin - if all the draw and image interfaces were
> changed to allow this, then some of my need for this
> change would go away, but i think it's justified anyway
> for the reasons mentioned earlier.

I'm not sure if it belongs in the image.Image interface, but an origin
and bounds might be worth putting into the struct types like
image.RGBA. Currently we don't have a good way to detect overlapping
RGBA images if their slices are sliced from the same backing array,
and we probably need that if you want to use draw.Draw to copy pixels
around when scrolling.

[nigeltao_gnome, 2010-07-15 06:41:51]

On 18 June 2010 11:15, Nigel Tao <nigel.tao.gnome@gmail.com> wrote:
> I'm not sure if it belongs in the image.Image interface, but an origin
> and bounds might be worth putting into the struct types like
> image.RGBA.

This is http://codereview.appspot.com/1849041