2007-10-28

As Long as there are no Architectural Mistakes...

One of the things that’s been bugging me about Woopsi is the naming system I used for things-that-happen to gadgets. Not events, strictly; just stuff that happens. A stylus click is a thing-that-happens, as is a vertical blank, as is a keypress and a stylus release.

All of these things-that-happen were dealt with in the gadget classes by “handleX()” functions, where X is the name of the thing-that-happened. In the case of a click, the function was “handleClick(s16 x, s16 y)”.

This naming system did kind of make sense, in that the gadget was being passed an event of some sort (a click), and had to handle it appropriately, either by passing it on to a child or processing it itself. Each “handleX()” function then fired the EventHandler’s “handleX()” function.

The problem with it was that it began to fall apart. The naming scheme just seemed clumsy - why would a function that claimed to be an event handler then send that event to another event handler? It had structural problems, too, in that some gadgets needed to send their events to other event handlers, and some just needed to process them themselves.

Where it really collapsed was when you needed to work with the gadgets via the API. If you wanted to simulate a click, you’d call the “handleClick()” function. There is no click to handle, though. The whole structure has been unsettling me for the past couple of weeks, so I decided I had to improve it.

To that end, I have split all of the “handleX()” functions up into two functions. The first is the “x()” function - “click()”, “release()”, “keyPress()”, etc. If I want to click a button, I call “button->click(x, y)”. It contains all of the code needed to perform the gadget’s click/release/keypress operations. Much more sensible. The second function sends the event to the EventHandler, and follows the naming scheme “raiseXEvent()” - “raiseClickEvent()”, “raiseReleaseEvent()”, “raiseKeyPressEvent()”, etc. The “raiseXEvent()” methods cannot be overridden, whilst the “x()” methods can - this was another problem in the initial design.

This has made the code longer, but considerably tidier. It’s also made the API a hell of a lot easier to follow. Instead of the naming system suggesting that gadgets are handling something that might or not be a real or simulated event, developers can now do things to the gadgets - click them, release them, blur them, etc.

Tied in with this, I’ve made a few other improvements. Clicking a gadget now automatically notifies the gadget’s parent that it has become the clicked gadget; this is important, since we’re now assuming that people can call “gadget->click()” as well as wait for the stylus to hit the screen. The notification automatically bubbles up the hierarchy.

The EventHandler base class no longer defines its event handler methods as pure virtual; they’re just plain old virtual now instead. As the number of events that Woopsi can handle increases, it was getting more and more annoying to have to explicitly override all of the base methods even if I wasn’t going to use all of the events.

I’ve split the TextViewer class into two. One of the classes retains the user interface side of the TextViewer; the other, a “Text” class, now contains all of the text data manipulation code. It handles text wrapping, stores the char array pointer, and can produce useful information about the text - number of lines, maximum line length in pixels, total height in pixels, etc. It means that any text-based gadget (such as the console gadget, when I get around to it) can have the same text manipulation features as the TextViewer.

Gadgets can now be enabled and disabled. By default, they don’t change their appearance, but disabled gadgets stop receiving the majority of events (things like lid close and blur still get sent through) and can’t be interacted with either via the API or the GUI until they’re re-enabled.

Couple of new gadget functions - “resize()” and “moveTo()”. I’ve written a custom resize function for windows (currently only accessible through the API rather than the GUI), so that’s one of the features knocked off the version 2 list. The “moveTo()” function has allowed me to tidy up the window drag code a bit. I’ve created a new event for each of the functions; I’ve also added a “value change” event that gets triggered when a gadget’s value changes.

Lastly, I’ve started looking into requesters. I initially made a Requester base class and inherited from that - the plan was to include an instance of the Window class, populate it with gadgets, and implement the EventHandler class to tie it all together. This turned out to be a mistake, though, as the Requester class couldn’t be added to the screen’s gadget list (obviously enough). I’ve gone down the route of inheriting from the Window class instead, and having the class’ constructor pre-populate it with gadgets.

The requesters should work well. I’m currently working on an “Alert” requester, which consists of a textbox and an “OK” button. I’m trying to get the requester to resize itself based on the size of the text in the textbox - this was the real reason for stripping the text manipulation out of the TextWriter and the addition of the “resize()” method. One unexpected benefit of having requesters that resize based on text size is that they will automatically adjust for different sized fonts.

Comments

Jeff on 2007-10-30 at 04:33 said:

Not strictly related to this post, but I think its better to keep more recent conversation closer to the current.

I downloaded the latest sources from sourceforge and had a couple of comments

Firstly, SuperBitmap’s constructor seems a bit redundant and I wonder whether you meant to do what you did. Specifically,

SuperBitmap::SuperBitmap( s16 x, s16 y, u16 width, u16 height, u16* bitmap, u16 bitmapWidth, u16 bitmapHeight, Gadget* parent) : Gadget(x, y, width, height, parent) { _x = x; _y = y; _parent = parent;

It deliberately invokes the Gadget constructor, but then overwrites the _x, _y and _parent values that that constructor had already populated.

Secondly, there are two distinct constructors in Gadget when it seems like one would be fine, if you used default arguments. Rather than doing this:

    Gadget(s16 x, s16 y, u16 width, u16 height);
    Gadget(s16 x, s16 y, u16 width, u16 height, Gadget* parent);

just do:

    Gadget(s16 x, s16 y, u16 width, u16 height, Gadget* parent = NULL);

and then you only need to provide the one variant. You currently use the different constructors to set the _isChild variable differently, but it seems to me that _isChild and (parent!=NULL) are synonyms and could be used interchangable - in fact, if you use one member variable, you give the optimiser more chance to reduce memory loads into registers, since it can load the parent pointer, and then indirect from it if it is non-NULL.

The only reason I bring this one up is that Woopsi is looking very lean and clean at the moment, and it would be a real shame for confusing redundancy (and unnecessary inefficiency) to creep in.

The second thing is to do with the way you’ve set the directory structures up - while you’ve discussed building it into a library in other posts, it seems to me that it might be fairly simple for you to split the sources into two distinct directories, ‘source’ and ‘woopsi’. You should just need to change the Makefile to list the dual sources

(ie

SOURCES := gfx source woopsi data INCLUDES := include woopsi data build

)

and it all should build, and as a bonus, people ;-) can just replace the contents of the source, gfx and data directories with their own content, which should make it a lot easier to update via SVN

It might be worth seperating out the woopsi-mandatory graphic files (like the fonts) from the files specifically for the demo - again, it should be as simple as adding another subdirectory and tweaking the all_gfx.c/h names, so that the source copy of all_gfx.c doesn’t need to explicitly list the woopsi files.

ant on 2007-10-30 at 09:37 said:

I’d got the unnecessary variable re-assignments used in the SuperBitmap class in most of the other gadget constructors, but went through and stripped them out - I must have missed the SuperBitmap. Now fixed (or it will be as soon as I put the code back into SVN).

The optional variable is a good idea - I didn’t know you could do that in C++. Or I might have just forgotten. Anyway, that’s done too.

I’ve also stripped out the _isChild variable - I was only actually using it in two places as I’d already decided that “_parent != NULL” was a better way of doing it.

I am planning to fix the directory structure, but I’ll leave that until I’m ready to release an alpha version of the code. The graphics problem has actually already come up - one of the guys from the PALib forum decided to have a go at integrating the code into one of his applications but got stuck because he had his own “all_gfx” includes - the second set of files was hidden from the compiler by preprocessor. Just as well, really, as the handleClick() to click() and raiseClickEvent() switch would probably have broken his code anyway. That’s another one I’ll fix later - at the moment, because I need to tinker with the glyph fonts with PAGfx every now and again, it’s easier to keep the usual structure than it is to keep renaming things every time I generate a new file.

Off on a tangent, radio buttons are now working. But I’ve somehow broken the TextViewer (again).

Jeff on 2007-10-31 at 00:52 said:

I have to say, I think the use of all_gfx.* (in PALib applications) is about the most stupid architectural decision I’ve ever seen. It saves the user about ten seconds of typing and it complicates everything else enormously. And since you have to edit code somewhere to actually use the graphics, its a false economy. When you open the code to add the PA_CreateSprite or PA_LoadPalette or whatever, add the #include “gfx/mypalette.h” and be done with it.

There’s an awful lot of voodoo in NDS development, from what I can see - people don’t understand how the makefiles actually work, they don’t understand that you can include whatever files you like, you don’t need to use the all_gfx files if you don’t want to.

My (pre-alpha) clone of PAgfx (for osx) doesn’t bother creating all_gfx.[ch] because I just don’t see the point. All you need to do is put ‘gfx’ into your SOURCES list in your Makefile and all your graphics are automatically compiled and linked in, without needing to include all_gfx.c anywhere. Provided, of course, that you don’t put it in your gfx directory as well :-(

Since most of Woopsi’s bitmap files are needed to support specific classes, I would suggest the source file for those classes should #include the graphic file(s) that they need. That has the added bonus (if you go to a library) of making the graphic only get linked into your binary if the code that needs it is.

This last part really only makes sense to people who worry about things at the machine level. The way that .a libraries work is that they contain a set of individual .o files - the standard linker looks at each .o to see if it contains any functions/entry points that it needs. If it needs any part of a .o file, it must include the entire .o file.

By compiling your image .c files individually instead of as part of one massive all_gfx.c you give the dead-code stripping logic a lot more scope to ignore things it doesn’t need. If all images are in all_gfx.o (or included into main.o) then they are guaranteed to be part of your binary whether you use them or not.

PAgfx doesn’t automatically get run by the makefiles anyway, you need to run it yourself by hand. As such, it doesn’t matter which directory you are running it. And it doesn’t mandate that you have to use all_gfx.c

ant on 2007-10-31 at 12:06 said:

All but one of the all_gfx includes in Woopsi are now completely redundant, since I pass around pointers to Font objects instead, which contain a pointer to the bitmap data that they use. The main Woopsi class creates a text font and a glyph font, and passes them to its children as required. Each child passes them to their children, and so on. This means that only the Woopsi class needs any graphics includes. There aren’t any other bitmaps within Woopsi.

Ultimately, I want to do two things:

  • Load font bitmaps from the filesystem using the FAT library;
  • Allow fonts to be set on each gadget individually.

The first item is easy enough, and I’ve made sure that the Font class works with u16* data instead of const u16*, which will hopefully make it ready to have the loader code inserted. The second item is already done, really; I just need to expose the functionality via the API (probably by optional parameters, so that the parent’s font gets inherited by the child if no font is specified when the child is created). This will completely remove the need for any all_gfx includes. Anyone who wanted to use all_gfx instead of DLDI could, however, continue to do so.

I haven’t done this already because only one of the two DS emulators I use emulates the FAT system. I’ve found it’s imperative to test in two emulators because DeSMuME is faster than No$GBA, but DeSMuME has unpredictable behaviour if you have a null pointer bug - most of the time it’ll keep running quite happily, whilst No$GBA and the real hardware crash.

Jeff on 2007-10-31 at 22:26 said:

I don’t understand what advantage working with ‘u16’ instead of ‘const u16’ makes - in fact, it seems to me that its a disadvantage.

Any function declared f(const u16) can be passed a u16 but the converse is not true, a function declared f(u16) cannot be passed a const u16 since the function prototype is declaring “I may change the input data”.

ant on 2007-10-31 at 22:34 said:

Really? Yes, that seems sensible. Nuts, I need to undo that then.

Jeff on 2007-10-31 at 22:34 said:

As to loading bitmaps via libfat, I would strongly recomment not making that a mandatory part of Woopsi - by all means, have some inline helper functions that do the work. But don’t lock the UI classes down to requiring specific STORAGE libraries used.

Thus, I wouldn’t make ‘Font::LoadFromDisk()’ - by all means, define an inline function Font* LoadWoopsiFontFromDisk() but put it in a seperate source file, and don’t refer to it except in the actual main/demo sources. That way, it doesn’t link unless you use it, and if your main uses it, then your main is responsible for linking libfat, not Woopsi.

I would still strongly recommend splitting the Woopsi & Demo sources into seperate directories now before you end up with pathological connections between them that become harder to seperate later on. The best way to ensure you are keeping it clean is to have two demos, not one. Don’t dump all your demonstrations into one binary, build two (with an even mix of Gadgets) so that you ensure that you don’t add fonts that one needs to the other accidentally, etc.