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.

Comments

Jeff on 2011-04-03 at 21:57 said:

Its possible that the Woopsi variables were static because you were concerned about performance. Making them instance variables of the singleton means that the singleton pointer needs to be dereferenced, which is going to be “slightly” more expensive.

Most of the time, it wouldn’t matter, but when it something like the VBL counter, it might.

Ant on 2011-04-04 at 08:04 said:

I’ve gone through the blog to see if I could work out the reasoning behind the static variables, and I think it was simply a way of accessing them from anywhere in the system. I could always access the VBL count by calling Woopsi::getVBLCount().

However, I’m pretty sure I implemented that before you suggested the static Woopsi singleton and the woopsiApplication define. With those in place, I could access the VBL count by calling woopsiApplication->getVBLCount(), which rendered my version redundant.

In any case, I don’t think the change will be at all significant performance-wise, but it has tidied up the code a little.