Drawing scanline of an image

Question

This draws scanline of the image to System.Drawing.Graphics. It's quite simple and it's optimized in order to merge neigbour samples with the same color into single rectangle (instead of drawing sample-by-sample in 1x1 rectangles).

I don't like the duplication of this code inside and outside the loop:

updateAlpha(brush, previousColor);
graphics.FillRectangle(brush, previousIndex + positionX, y, i - previousIndex, 1);

Do you know the good way how to follow DRY principle here and to keep code easy to understand? Or this is a kind of perfectionism and all is already fine?

private static void drawGlyphRowToGraphics(int rowIndex, FT_Bitmap glyphBitmap, 
    Graphics graphics, float positionX, float positionY, SolidBrush brush)
{
    float y = rowIndex + positionY;

    byte previousColor = 0;
    int previousIndex = 0;
    for (int i = 0; i < glyphBitmap.Width; ++i)
    {
        byte color = (byte)(255 - glyphBitmap.Buffer[i + rowIndex * glyphBitmap.Width]);
        if (i == 0)
            previousColor = color;

        if (color != previousColor)
        {
            updateAlpha(brush, previousColor);
            graphics.FillRectangle(brush, previousIndex + positionX, y, i - previousIndex, 1);

            previousColor = color;
            previousIndex = i;
        }
    }

    if (glyphBitmap.Width > 0)
    {
        updateAlpha(brush, previousColor);
        graphics.FillRectangle(brush, previousIndex + positionX, y, glyphBitmap.Width - previousIndex, 1);
    }
}

private static void updateAlpha(SolidBrush brush, byte gray)
{
    brush.Color = Color.FromArgb(255 - gray, brush.Color.R, brush.Color.G, brush.Color.B);
}

Answer 1

First off I have to say I found this code very hard to understand. It took me quite some time to finally understand what this code actually does.

First off the function takes 6 arguments. That's way too many. Clearly the positionX and positionY should be combined into one Point object.

But the larger problem is, that you seem to be working at the wrong level of abstraction. The function is supposed to deal with one single row of a FT_Bitmap, but because there is no abstraction of such a single row, it ends up dealing with the whole FT_Bitmap, doing confusing offset calculations all over the place.

What I would do first, is to create a separate GlyphRow class:

public class GlyphRow
{
    private FT_Bitmap bitmap;
    private int offset;

    public int Width;

    public GlyphRow(FT_Bitmap bitmap, int rowIndex)
    {
        this.bitmap = bitmap;
        this.offset = rowIndex * bitmap.Width;
        this.Width = bitmap.Width;
    }

    // Returns the transparency value of a point in position x of our row
    public byte Alpha(x)
    {
        return bitmap.Buffer[offset + x];
    }

}

This will allow us to work with a single row at a time.

Now our function can take just the following arguments:

private static void drawGlyphRowToGraphics(
    GlyphRow row, Graphics graphics, Point position, SolidBrush brush)

And when calling it, we initialize Point and GlyphRow objects:

GlyphRow row = new GlyphRow(glyphBitmap, rowIndex);
Point position = new Point(positionX, rowIndex + positionY);
drawGlyphRowToGraphics(row, graphics, position, brush);

Note that the position is now exactly the position where this GlyphRow should be painted, already offset with rowIndex.

So here's the new simplified drawGlyphRowToGraphics:

private static void drawGlyphRowToGraphics(
    GlyphRow row, Graphics graphics, Point position, SolidBrush brush)
{
    int i = 0;
    while (i < row.Width)
    {
        int length = sameAlphaLength(row, i);
        brush.Color = cloneColorWithAlpha(brush.Color, row.Alpha(i));
        graphics.FillRectangle(brush, position.X + i, position.Y, length, 1);
        i += length;
    }
}

I found the algorithm in original implementation very hard to grasp, so using divide-and-conquer strategy, I created a separate function to calculate the length of a section that has the same transparency value:

private static int sameAlphaLength(GlyphRow row, int start)
{
    byte alpha = row.Alpha(start);
    int length = 1;
    while (start + length < row.Width)
    {
        if (alpha != row.Alpha(start + length))
        {
            return length;
        }
        length++;
    }
    return length;
}

In the original code, at first the gray value was calculated with (255 - alpha), and then passed to updateAlpha, where it was again transformed with (255 - gray). Pretty redundant IMHO. So I'm skipping this whole double-conversion. I also find it better to work with functions without side-effects, so I replaced updateAlpha with a function that creates a new color value from an existing one, which I can then assign to brush:

private static Color cloneColorWithAlpha(Color color, byte alpha)
{
    return Color.FromArgb(alpha, color.R, color.G, color.B);
}

Disclaimer: I've never written a single line of C# before. It probably doesn't compile, not to mention other possible bugs. But hoping to get the general point across.