| Re: [AD] monochrome 8x8 builtin font + upscaling algorithm |
[ Thread Index |
Date Index
| More lists.liballeg.org/allegro-developers Archives
]
On Mon, 09 Jul 2012 17:35:52 +0200, Dennis-Busch@xxxxxxxxxx wrote:
> On 9 Jul 2012 at 14:36, Elias Pschernig wrote:
> > On Mon, Jul 9, 2012 at 12:48 PM, Peter Wang <novalazy@xxxxxxxxxx> wrote:
> > >
> > > Keeping in mind the reason for introducing the builtin font in the first
> > > place (quick debugging) I think there should just be one function that
> > > returns a ready to use font.
> > >
> >
> > True. Just something like this would work I think:
> >
> > ALLEGRO_FONT *al_create_builtin_font(void);
>
>
> So we're back at the beginning. :-)
> Done. New patch is attached. The two information functions were
> unnecessary in that case, so I removed them.
>
> The only public function is now:
> ALLEGRO_FONT *al_create_builtin_font(void);
That looks fine to me.
Since you seem to be sending a few patches, I'm going to nitpick the
patch.
> From c49387b251059ab2af95327f424c1cabda97081b Mon Sep 17 00:00:00 2001
> From: d_busch <d_busch@xxxxxxxxxx>
> Date: Wed, 4 Jul 2012 12:19:32 +0200
> Subject: [PATCH 1/2] simple builtin 8x8 font creation function added to the
> font addon, modified ex_font to demonstrate usage and
> updated font docs
>
Make use of multiple lines in the log message.
> diff --git a/addons/font/CMakeLists.txt b/addons/font/CMakeLists.txt
> index a4e3dd6..7b74dc7 100644
> --- a/addons/font/CMakeLists.txt
> +++ b/addons/font/CMakeLists.txt
> @@ -1,4 +1,4 @@
> -set(FONT_SOURCES font.c fontbmp.c text.c)
> +set(FONT_SOURCES font.c fontbmp.c stdfont.c text.c)
>
> set(FONT_INCLUDE_FILES allegro5/allegro_font.h)
>
> diff --git a/addons/font/allegro5/allegro_font.h b/addons/font/allegro5/allegro_font.h
> index a4acc7c..a65088e 100644
> --- a/addons/font/allegro5/allegro_font.h
> +++ b/addons/font/allegro5/allegro_font.h
> @@ -83,6 +83,7 @@ ALLEGRO_FONT_FUNC(ALLEGRO_FONT *, al_load_bitmap_font_flags, (const char *filena
> ALLEGRO_FONT_FUNC(ALLEGRO_FONT *, al_load_font, (const char *filename, int size, int flags));
>
> ALLEGRO_FONT_FUNC(ALLEGRO_FONT *, al_grab_font_from_bitmap, (ALLEGRO_BITMAP *bmp, int n, int ranges[]));
> +ALLEGRO_FONT_FUNC(ALLEGRO_FONT *, al_create_builtin_font, (void));
>
> ALLEGRO_FONT_FUNC(void, al_draw_ustr, (const ALLEGRO_FONT *font, ALLEGRO_COLOR color, float x, float y, int flags, ALLEGRO_USTR const *ustr));
> ALLEGRO_FONT_FUNC(void, al_draw_text, (const ALLEGRO_FONT *font, ALLEGRO_COLOR color, float x, float y, int flags, char const *text));
> diff --git a/addons/font/stdfont.c b/addons/font/stdfont.c
> new file mode 100644
> index 0000000..a196b17
> --- /dev/null
> +++ b/addons/font/stdfont.c
> @@ -0,0 +1,489 @@
> +/* ______ ___ ___
> + * /\ _ \ /\_ \ /\_ \
> + * \ \ \L\ \\//\ \ \//\ \ __ __ _ __ ___
> + * \ \ __ \ \ \ \ \ \ \ /'__`\ /'_ `\/\`'__\/ __`\
> + * \ \ \/\ \ \_\ \_ \_\ \_/\ __//\ \L\ \ \ \//\ \L\ \
> + * \ \_\ \_\/\____\/\____\ \____\ \____ \ \_\\ \____/
> + * \/_/\/_/\/____/\/____/\/____/\/___L\ \/_/ \/___/
> + * /\____/
> + * \_/__/
> + *
> + *
> + * contains a ported binary version of the Allegro 4 internal standard font
> + * and a function to create an ALLEGRO_BITMAP from the binary data
> + * for use in environments where no external bitmap font or true type font is available
> + *
Use capitals. Try to keep lines short (traditionally < 80 chars) unless
there is good reason.
> + * By Dennis Busch.
> + *
> + * See readme.txt for copyright information.
> + */
We should start referring to LICENSE.txt now.
> +
> +#include "allegro5/allegro.h"
> +#include "allegro5/allegro_font.h"
> +
> +
> +
> +/* adapted from Allegro4 "font.c" (removed unncesseray height and width information and packed them all into a single continuous array) */
Ugh, too long.
> +/* contains the following ranges
> + *
> + * ASCII (0x0020 to 0x007F)
> + * Latin-1 (0x00A1 to 0x00FF)
> + * Extended-A (0x0100 to 0x017F)
> + * Euro (0x20AC)
> +*/
> +static unsigned char _al_rom_font_8x8[] = {
...
> +
> +/* range definitions for the above binary data */
> +static int _al_rom_font_8x8_ranges[] = {
> + 0x00000020, 0x0000007F,
> + 0x000000A1, 0x000000FF,
> + 0x00000100, 0x0000017F,
> + 0x000020AC, 0x000020AC
> +};
> +static int _al_rom_font_8x8_ranges_count = (sizeof(_al_rom_font_8x8_ranges) / sizeof(int)) / 2;
> +
> +
> +
> +/* special version of 'put_pixel' to really replace the target data with the given color */
> +static void _place_color_data_abgr8888_le(ALLEGRO_LOCKED_REGION* region, int x, int y, uint32_t* pixel)
> +{
> + void* target = (uint8_t*)region->data + y * region->pitch + x * region->pixel_size;
> + *(uint32_t*)target = *pixel;
> +}
Generally local definitions do NOT start with underscore, nor _al_.
> +
> +
> +
> +/* Function: al_create_builtin_font_sheet
> + */
> +static ALLEGRO_BITMAP *_al_create_builtin_font_sheet(void)
Delete the Function: header.
> +{
> + int glyph_count = sizeof(_al_rom_font_8x8) / 8;
> + int glyphs_per_row = 32;
> + int needed_rows = (glyph_count / glyphs_per_row) + (((glyph_count % glyphs_per_row) > 0) ? 1 : 0);
> + int ranges = (sizeof(_al_rom_font_8x8_ranges) / sizeof(int)) / 2;
> + ALLEGRO_BITMAP *bmp_glyphs_mem = NULL;
> + ALLEGRO_LOCKED_REGION *bmp_locked_region = NULL;
> + ALLEGRO_BITMAP *bmp_glyphs_vid = NULL;
> + ALLEGRO_BITMAP *bmp_prev_target = NULL;
> + ALLEGRO_BITMAP *bmp_ret = NULL;
> + int i = 0;
> + int j = 0;
> + int k = 0;
> + bool set = 0;
> + int c_col = 0;
> + int c_row = 0;
> + int tx;
> + int ty;
> + uint32_t col_tp = 0x00000000; /* black */
> + uint32_t col_fg = 0xFFFFFFFF; /* white */
> + uint32_t* col_t;
> + uint8_t char_line = 0;
> + uint8_t shifted = 0;
Too many variables. You can definitely move some of these in.
ranges is unused.
bools should be initialised to false.
Premature variable initialisations inhibit the compiler's ability to
warn about variables being used too early. I also find it harder to
read the code as the knee-jerk initialisations hide the true
initialisations.
> +
> + /* putting pixels is much faster on a memory bitmap */
> + al_set_new_bitmap_flags(ALLEGRO_MEMORY_BITMAP);
> +
> + /* create bitmap onto which to render the glyphs */
> + bmp_glyphs_mem = al_create_bitmap((glyphs_per_row * 8 + glyphs_per_row + 1), (needed_rows * 8 + needed_rows + 1));
> +
> + if(bmp_glyphs_mem != NULL) {
Please add spaces after the keyword, around operators, after commas,
etc.
> + /* remember current render target */
> + bmp_prev_target = al_get_target_bitmap();
> +
> + al_set_target_bitmap(bmp_glyphs_mem);
> + al_clear_to_color(al_map_rgba(255,255,0,255));
> + bmp_locked_region = al_lock_bitmap(bmp_glyphs_mem, ALLEGRO_PIXEL_FORMAT_ABGR_8888_LE, ALLEGRO_LOCK_READWRITE);
> +
> + for(i=0; i < glyph_count; i++) { /* for each glyph.. */
Delete redundant comments.
Otherwise they generally belong on the previous line.
You don't need ellipses.
> + c_col = i % glyphs_per_row;
> + c_row = i / glyphs_per_row;
> +
> + for(j=0; j<8; j++) { /* for each of the 8 lines per character.. */
> + char_line = _al_rom_font_8x8[i*8+j];
> + /* decode and draw each of the 8 pixels of the current line.. */
> + for(k=0; k<8; k++) {
> + shifted = (char_line >> (7 - k));
> + set = shifted & 0x01;
> +
> + /* determine target pixel */
> + tx = ( (c_col * 9 + 1) + k );
> + ty = ( (c_row * 9 + 1 + j) );
> +
> + col_t = set ? &col_fg : &col_tp;
> +
> + /* draw target pixel */
> + _place_color_data_abgr8888_le(bmp_locked_region, tx, ty, col_t);
> + }
> + }
> + }
> +
> + al_unlock_bitmap(bmp_glyphs_mem);
> + /* al_save_bitmap("c:/defaultfontX.png", bmp_glyphs_mem); */ /*for debugging*/
Delete.
> +
> + /* blitting of the characters/rendering will be much faster from a video bitmap */
> + al_set_new_bitmap_flags(ALLEGRO_VIDEO_BITMAP);
> + bmp_glyphs_vid = al_clone_bitmap(bmp_glyphs_mem);
> +
> + /* determine whether to return video or memory bitmap version */
> + if(bmp_glyphs_vid != NULL) {
> + al_destroy_bitmap(bmp_glyphs_mem);
> + bmp_ret = bmp_glyphs_vid;
> + }
> + else
> + bmp_ret = bmp_glyphs_mem;
(I find dangling elses ugly.)
> +
> + /* restore previous render target */
> + if(bmp_prev_target != NULL)
> + al_set_target_bitmap(bmp_prev_target);
> + }
> +
> + return bmp_ret;
> +}
> +
> +
> +
> +/* Function: al_create_builtin_font
> + */
> +ALLEGRO_FONT *al_create_builtin_font(void)
> +{
> + ALLEGRO_BITMAP *bmp = NULL;
> + ALLEGRO_FONT *font = NULL;
Don't initialise variables unnecessarily.
> diff --git a/docs/src/refman/font.txt b/docs/src/refman/font.txt
> index b195b70..d27a5e5 100644
> --- a/docs/src/refman/font.txt
> +++ b/docs/src/refman/font.txt
> @@ -280,6 +280,12 @@ ALLEGRO_NO_PREMULTIPLIED_ALPHA
>
> See also: [al_load_bitmap_font], [al_load_bitmap_flags]
>
> +### API: al_create_builtin_font
> +
> +Creates a 1bit monochrome bitmap font containing the characters(8x8 pixels
> +per character) from the builtin font data bits.
> +Returns NULL on an error.
> +
Delete "1bit".
Space required before "(8x8"
The user doesn't know what "builtin font data bits" are, and doesn't care.
I suggestion mentioning debugging, the supported glyph ranges,
and how to destroy the font.
> diff --git a/examples/ex_font.c b/examples/ex_font.c
> index a5e7ac3..9ad0ecc 100644
> --- a/examples/ex_font.c
> +++ b/examples/ex_font.c
> @@ -43,7 +43,8 @@ int main(void)
> {
> ALLEGRO_DISPLAY *display;
> ALLEGRO_BITMAP *bitmap, *font_bitmap;
> - ALLEGRO_FONT *f, *a4f;
> + ALLEGRO_FONT *f, *a4f, *a5fbuiltin;
> +
> int ranges[] = {
> 0x0020, 0x007F, /* ASCII */
> 0x00A1, 0x00FF, /* Latin 1 */
> @@ -84,6 +85,13 @@ int main(void)
> }
> a4f = al_grab_font_from_bitmap(font_bitmap, 4, ranges);
>
> +
> + a5fbuiltin = al_create_builtin_font();
> + if(!a5fbuiltin) {
> + abort_example("Failed to create builtin font.\n");
> + return 1;
> + }
> +
Style.
a4f and a5fbuiltin are pretty ugly variable names.
Peter