2011-04-03

Bug Hunting and Refactoring

I’ve had some time to blast through some Woopsi coding over the last week, so there’s quite a bit to write up here. There’s a mix of new features, bugfixes, refactoring and breaking changes.

First up, attempting to flip a screen from one display to the other, or re-organise the depths of screens in the screen stack, is no longer allowed if there is only one screen in existence. Allowing this to occur was a bug introduced in v0.45 when I added a couple of background screens to the Woopsi gadget by default, to ensure the background was always grey and redrawn correctly.

The font class no longer includes a colour member, nor does it distinguish between full-colour and monochrome fonts. It makes no sense for a font to have a state. When trying to draw text, I want to draw text in colour X using font Y. I don’t want to get font Y, make it colour X, then draw with it. Implementing this has involved making changes throughout Woopsi. Text rendering commands now expect to be given a colour to render with. Related to this, the GadgetStyle class now includes a text colour property, whilst the Gadget class has new getTextColour() and setTextColour() methods.

In addition to removing the colour member from the FontBase class, I’ve removed all other members. The FontBase is now a legitimate, data-free interface. All members that are still required, such as height, have been moved into other classes. This will reduce the amount of redundant data inherited by subclasses, such as Lakedaemon’s libfreetype wrappers.

The font change was so successful that I replicated it with bitmaps. BitmapBase no longer includes any data either. Members have been moved into subclasses.

The overload of Gadget::checkCollision() that checks for collisions with other gadgets will not detect collisions with hidden gadgets. I’ve modified it so that it expects to be passed a const gadget pointer instead of a plain gadget pointer. I’ve also added a shortcircuit to the function so that it detects attempts to check for collisions between a gadget and itself and exits early.

I’ve rewritten the Gadget::swapDepth() method. This is used by screens and windows when their depth gadgets are clicked. If swapDepth() is called on a gadget that is not at the front of the subset of sibling gadgets that it collides with, the gadget will be moved to the front of that subset. If the gadget is at the front, it will be sent to the back of the subset.

Previously, the way the method worked was confusing. Most of the logic for the method was actually contained by the parent gadget in its swapGadgetDepth() method. The swapDepth() method just called the parent method to perform the swap. This had some downsides:

  • Children couldn’t determine how they would swap; it was decided by parents, so it was impossible for a new gadget subclass to swap in a different way to (for example) a window.
  • The Gadget, Screen and Woopsi classes all had their own implementations of the swapGadgetDepth() method that were subtly different but basically achieved the same effect.

I’ve renamed swapGadgetDepth() to changeGadgetDepth() and made it considerably more generic - it will now just move a gadget from one index in the child array to another, ensuring the gadget gets erased and redrawn correctly. The swapGadget() method in the child gadget determines which index the child will swap to, so each gadget can decide how it will depth swap. I’ve also added Gadget::getHighestCollidingGadgetIndex() and Gadget::getLowestCollidingGadgetIndex() to help determine these indices. The swapGadgetDepth() overrides in the Screen and Woopsi classes no longer exist.

Gadget::_decorationCount is an s32 instead of a u8. I don’t know why I’d got that set as a u8, especially when the getters/setters were working with s32s.

The RectCache::markRectDamaged() method had a nasty, obscure bug that could lead to the method getting into an infinite loop. It has a couple of nested loops and it seems I’d got the iterator variables confused at some point, and was using the variable from the first loop to index into the iterated-over array of the second loop. Ooops. Thanks to carpfish for spotting the crash and putting together a test so that I could track down the cause.

The todo list that I put together long before Woopsi v1.0 was released included a question: “Is there a bug in the floodfill?” I noticed that there seemed to be a couple of pixels that weren’t filled in the test I wrote for the WoopsiGfx library. Well, there was a bug in the floodfill - it wasn’t filling upwards correctly due to some utterly bizarre typos in the method. Don’t quite know what I thought I was doing when I wrote that function. It’s fixed now.

Related to the floodfill, the stack functions in the Graphics class that it relies on now expect to be passed a reference to a stack object to work with rather than a pointer.

For some reason, three members of the Woopsi class (the vertical blank count that stores the number of frames elapsed since the app started running, the deleted gadget list and another one that I can’t presently remember) were static. This made no sense, so they are no longer static.

I’ve tested empty ListBox gadgets for the first time and encountered a couple of problems. First of all, the scrollbar went crazy and suggested that there were hundreds of options to choose from; that’s fixed. Clicking the empty space within the ListBox caused it to try to locate a non-existent option in its option list and dereference a null pointer. That’s fixed too.

I’ve stripped out some redundant code. The GadgetFlags enum included a “GADGET_NO_RAISE_EVENTS” flag that wasn’t used anywhere; it’s now been removed. I’ve also removed the Woopsi::goModal() method that only existed because the Woopsi::handleClick() method was badly designed. Redesigning that let me get rid of the goModal() override.

Gadgets included a concept of “close type” which existed solely to allow the close gadgets on windows to close, hide or shelve the window as appropriate. It wasn’t useful in any other situation and wasn’t really useful for windows either, so it’s gone too.

Another pair of useless features were the AmigaScreenFlags and AmigaWindowFlags enums. They were used to send gadget-specific flags to the AmigaScreen and AmigaWindow constructors. However, as there were only two flags in each enum, and as typing the enum value names took longer than just passing true or false a couple of times, I’ve stripped them out and replaced them with booleans instead. This change will break user code, but fixing it just a matter of changing a couple of values when creating AmigaScreen and AmigaWindow objects.

2009-12-15

New Font Utilities

I’ve extended the font creation utility I discussed a few posts ago so that it now creates Font subclasses in addition to the other three. The bmp2font utility is now capable of producing any of the official Woopsi font classes.

This is great, except actually creating one of the bitmaps is a real pain. I have no intention of mucking about with glyph maps unless I really have to, so I set about creating a utility that would convert a Windows font to a bitmap.

Doing so was slightly tricky. Requesting a particular font size in .NET doesn’t guarantee you a font of that size. Due to the spacing around fonts (which .NET doesn’t provide a way of changing) and the way it calculates font sizes, what you get if you use the built-in APIs is a bitmap with a lot of empty space and not a lot of font. This is clearly no use.

Instead, I came up with a solution in which I render each glyph to its own bitmap, calculate the exact size of each glyph and the overall size of the font, then blit all of the bitmaps to a single master bitmap. Each of the glyphs is left-aligned and no space is wasted. This new “font2bmp” utility can turn any Windows font into BMP file with the minimum of hassle and dimensions.

At this point, I realised that I had a utility to convert from a Windows font to a bitmap, and from a bitmap to a Woopsi font, and that it would probably be a good idea to have a utility that could cut out the middleman and go straight from one font format to the other. Refactoring the two projects produced two DLLs that could be used to achieve this.

A short time later and I’ve got the third utility in the font trilogy, “font2font”. This will convert straight from a Windows font to a Woopsi font.

All three utilities are available in the Subversion repository, and will be in the next release. One minor bugfix - Lakedaemon spotted Woopsi allocating heap memory for no real reason when it converted single chars to strings when setting string values. This was rather wasteful. Woopsi now uses the stack instead.

2009-12-13

Creating Fonts and Bitmaps

More work on fonts. Jeff’s solution for creating PackedFont fonts is fantastic - instead of mucking around with u16 arrays, he subclasses the fonts and does all of the work for you. Instead of this:


PackedFont1* myFont = new PackedFont(somearray, somewidth, someheight, blah, blah);

…you have this:


NewTopaz* myFont = new NewTopaz();

So much easier.

I’ve shamelessly stolen this idea and reworked the existing “FontBitPacker” application so that it follows this pattern. I’ve heavily modified it so that the app will convert BMP files to either MonoFont, Font, PackedFont1 or PackedFont16 font classes. As long as they have version 3.5 of the .NET framework, Windows users no longer need to faff about with PAGfx or the bmp2font Python script. The new “bmp2font” .NET console application (which the FontBitPacker has become) will do all of the font conversion work needed for Woopsi.

This pattern seemed like such a good idea I’ve used it for bitmaps, too. Instead of converting BMP files with PAGfx, then using the “all_gfx” files to include the bitmap data before wrapping them up in a BitmapWrapper class, it is now possible to use a new “bmp2bitmap” .NET app to convert straight from a BMP file to a BitmapWrapper subclass.

These new programs have enabled me to delete the PAGfx binary and the files it converted from the Woopsi demo folder. I’ve replaced the converted files with classes converted with bmp2bitmap.

2009-11-10

Bitmaps, Framebuffers, Drawing, and Fonts

The latest set of updates encompasses a substantial amount of fundamental changes within Woopsi. It now includes the following types of bitmap class:

BitmapBase

The most basic bitmap abstract class from which all others inherit. It defines the most fundamental properties of an immutable bitmap.

BitmapWrapper

Extends the BitmapBase with the ability to wrap around a raw u16 array and permits it to be used as a standard bitmap object.

MutableBitmapBase

Extends the BitmapBase with the ability to set the colour of a pixel at a given set of co-ordinates. Can also provide a non-const pointer to the raw u16 array. This is an abstract class from which all mutable bitmaps should inherit.

Bitmap

The standard bitmap object. Can produce a new Graphics object that has the capability of drawing to the Bitmap in the same way that the GraphicsPort draws to a gadget.

FrameBuffer

Almost identical to the Bitmap class, except it accepts a pointer to a pre-existing non-const u16 array in its constructor instead of creating the array internally. Intended as a wrapper for the framebuffer to allow it to be used as a bitmap object.

These changes supercede the changes discussed in the last post.

The drawing functions have been removed from the Bitmap class and split up into a class hierarchy:

GraphicsUnclipped

Contains all of the drawing functions previously stored in the GraphicsPort and Bitmap classes, minus any clipping code. Will draw to any instance of a class that inherits from the mutable bitmap class.

Graphics

Inherits from the GraphicsUnclipped class and adds simple clipping functions that ensure the drawing methods do not exceed the width of the bitmap being drawn to. Typically used when drawing to standard bitmap objects.

GraphicsPort

Inherits from the GraphicsUnclipped class and adds complex clipping functions that ensure the drawing methods do not attempt to draw outside the boundaries of a clipping rect, or array of clipping rects.

These changes mean that the basic drawing functions now only exist in one class, instead of being duplicated (with minor changes) in two places. The rationalisation also means that functions such as dim(), copy() and the XOR drawing methods are now available for use on bitmap objects as well as within gadgets.

In order to achieve this, the DrawBg array that previously allowed access to the framebuffer has been replaced with an array of FrameBuffer objects.

Changing these classes necessitated changes to the font system. All fonts that use unpacked bitmap data now expect to be supplied with a pointer to a bitmap object instead of a raw u16 array. Changing this required a rewrite of the rendering and clipping code in the Font and MonoFont classes.

The two standard fonts, “systemFont” and “tinyFont”, are now available as font global font objects. They are instantiated in the initWoopsiGfxMode() function in woopsifuncs.cpp.

These changes should bring a variety of benefits:

  • Fewer magic numbers flying around as bitmap width/height values are stored within bitmap objects instead of specified as parameters for every method;
  • Font bitmaps can be loaded dynamically from BMP files using BitmapIO;
  • It should be easier to add conditional compilation to the drawing methods in order to optimise SDL screen updates;
  • The drawing functions are no longer duplicated in several classes;
  • Bitmaps and the GraphicsPort have access to the same set of drawing functions.

There are a couple of new issues:

  • Blitting a bitmap to another bitmap seems to result in the occasional set of missing pixels at the start or end of the bitmap, probably resulting from the DMA hardware seeing old data (need to flush);
  • I need to check that the new structure is tidy.

2009-11-08

Bitmaps Refactored

Woopsi supports two different kinds of bitmap data. The first is the usual PAGfx/grit-style data containing BMP files converted to C sourcecode. These are compiled into the final ROM file as const u16 arrays. I’m not sure that the DS has a hardware-enforced region of read-only data, so these arrays might be mutable, but it makes sense to treat them as const as they fall within the address space of the ROM file.

The second type of bitmap data is non-const. Until very recently, this kind of data could only be created by instantiating a Bitmap object and using the built-in drawing tools to doodle on it. However, it is now also possible to load data from the flash cart in the form of BMP files and store it inside a Bitmap object.

Both methods of accessing BMPs have their pros and cons. Embedded bitmaps take up memory even if they are not currently being used and pose a development problem - any changes to a BMP mean that the C code must be regenerated. Non-embedded bitmaps must be loaded from disk and therefore necessitate the inclusion of libfat (extra ROM size), imply more files on disk (install complexity), and are awful for testing (need to look into creating a FAT image at some point).

Regardless of the BMP access method used, the Bitmap object has become far more valuable. The Woopsi API currently uses pointers to raw bitmap data and width/height values for interacting with bitmaps, but it makes much more sense to simply pass around pointers to Bitmap objects. However, because of the two types of bitmap data that exist, this is not as simple as it seems.

The Bitmap object allocates a region of RAM based on the width and height supplied in its constructor. It has methods to change that memory and will delete it when the object is destroyed. This is great for blank bitmaps and loaded bitmaps (they are loaded a byte at a time and drawn to a blank bitmap to prevent memory waste that would occur if loading the entire file at once and trying to discard the unused portions). It isn’t at all good for embedded bitmaps. The memory cannot be deleted and does not need to be instantiated. It cannot be changed, so the drawing tools are useless.

To get around this, Woopsi now contains three Bitmap classes. The most basic is a new “BitmapBase” class. It contains just the absolute minimum functionality needed from a bitmap in Woopsi - width and height members/getters, a method for retrieving the raw u16 array, and a method for getting the pixel at a specific set of co-ordinates. The existing “Bitmap” class inherits from this, but is otherwise unchanged. The third class, “BitmapWrapper”, extends the base class with a constructor that accepts a pointer to a const u16 array as a parameter. It does not allow the data to be altered, nor does it attempt to delete it when the object is destroyed. By using this class, embedded bitmaps can be utilised throughout Woopsi in the place of newly created or dynamically loaded bitmaps.

I’m planning to go through the API and swap all functions that accept a u16 array/width/height combination of parameters to use a single Bitmap pointer instead. This gives rise to another problem - the Bitmap class as it stands is a very heavyweight object. It carries around all of the drawing tools with it.

After putting some thought into this, it seems appropriate to move all of the drawing tools into a separate class. A Bitmap object could produce a new object capable of drawing to it in the same way that a gadget can produce a GraphicsPort. The new class would be essentially the same as the “Graphics” class in Java. It may even be possible, once all of the refactoring is done, to combine the proposed new class with the existing GraphicsPort class (or at least work out a common base class).