2007-12-09

Miscellaneous Changes

Yet another update. Any functions that need to check _flags.visible are, where possible, now using isVisible() instead. There are one or two functions that do need direct access to the struct, which defeats my plan to hide away some of the internals of the Gadget class.

The Gadget::focus() function, and any that override it, now check to see if the gadget is already active before running the function. This results in less redraws and less flicker. One more minor change - all resize() functions now respect the parent’s isPermeable() property.

The brief discussion in the comments about C versus C++ reminded me of another basic C++ feature I re-invented the other day. I wanted to add the floodfill code into the SuperBitmap but, at the same time, I didn’t want the popStack(), eraseStack() and pushStack() code bundled into the class - it made more sense to move that into its own class. So, I thought to myself, I need a class that can push values onto a stack and pop them off again. I started designing it in my head, but then: Hang on a mo, that sounds oddly familiar… In the end, I did use the vector class, but kept the pushStack() and popStack() functions within SuperBitmap because they do some processing before adding values into the vector.

Anyway. Another good suggestion from Jeff on SourceForge - VBL events shouldn’t be sent through the whole hierarchy, especially as that would make a complex hierarchy an unnecessary drain on CPU time. Woopsi now implements static registerForVBL() and unregisterFromVBL() methods. Passing a gadget pointer into the first function will make it receive VBL events. Passing a gadget pointer into the second will, naturally enough, prevent it from receiving VBL events. Gadgets are automatically unregistered when they are deleted if the developer has forgotten to unregister them manually, but otherwise all responsibility lies with the developer. I don’t want to automatically unregister them when a gadget is hidden, for example, in case the developer intends the gadget to continue processing in the background.

The Woopsi class now has a static getVBLCount() member, which will return a u32 containing the number of VBLs that have occurred since Woopsi began running. This will eventually wrap around, of course, so it’s down to the app coder to take this into account.

Switching VBL control around caused a bit of a problem, as the deletion queue was processed by each gadget during the VBL. I’ve moved that into static members and functions in the Woopsi class. The method for deleting gadgets remains the same for developers, though - gadget->close(). I’ve refactored gadget->closeChild(gadget), too, so that can now be used as expected to delete a child. Along side those changes, I’ve added gadget->hideChild(gadget) to complement gadget->hide(). Hopefully nothing has broken as a result of these changes.

These changes exposed one stupid bug - the closeChild() and hideChild() functions were setting the _activeGadget pointer to the highest gadget in the stack. This was usually the gadget being closed, so the _activeGadget pointer got trashed as soon as the next VBL ran. Whilst making these changes, I noticed that the GraphicsPort class would happily draw to deleted or invisible gadgets, so I’ve fixed that.

(Oh, and I discovered that F7 in VC++ Express does a make instead of make clean - that’s saving me loads of time.)

EDIT: Last thing today - split Window class into WindowBase, AmigaWindow and SimpleWindow classes.

Comments

Jeff on 2007-12-10 at 04:13 said:

Hmmm, it might be a trivial thing but I’ve have incremented vblCount BEFORE calling the listeners, whereas currently its after

for (u8 i = 0; i vbl();
}

// Increase vbl counter AFTER everyone else had a go
_vblCount++;

I realise that it makes little difference, but personal preference would have the base class do all that it wants to do before calling the others. I can see where vbl() listener might still call getVBLCount() and expect the “current” value rather than what its about to become. Not sure I can justify it really, it just leapt at me as being the wrong way around.

The initial justification for the vbl() count tracking was double-clicking (which I have in my list - if there’s been less than 60 vbl’s since the previous click, its considered a double. I need to tune that constant, and I can see where people might like it configurable but for now its good enough) but I also see a need for it in detecting key-repeat.

My list supports the cross-buttons UP/DOWN to move by one row in the corresponding direction and LEFT/RIGHT as moving 5 rows. (Again, just a preference I’m experimenting with). However, they don’t auto-repeat so holding down the UP doesn’t move up the list like I expected. It looks like PALib doesn’t support key-repeat out of the box, and Woopsi only delivers the press/release events. I think I can track, in the list, when a key is down, and then in my vbl() watch to see if it is staying down - however, I wonder whether that belongs in the Woopsi class instead?

(This is part of my rationale for preferring that the vbl count that the Woopsi has is the same as I have when it calls me - again, its gut feel at this point, not established fact, that its a bad thing)

I was also wondering about how to implement chording - ie, letting the user have two buttons down at the same time. Woopsi currently splits all that stuff out and calls you once for each individual key.

I was thinking that I’d have my app such that holding both triggers simultaneously would make the debug console display. Or return you to the main menu. Etc…

I know I can directly access the PALib Pad structure, but thats a bit gnarly - it would be better to ask the base Woopsi class what the set of held keys is (somehow)

ant on 2007-12-10 at 08:27 said:

Hmm, Woopsi can easily implement a KeysHeld struct, so you could do this:

handleKeyPress(key) { if (key == KEY_CODE_A) && (Woopsi::getKeysHeld()->B) { doSomeStuff() } }

It could also log the frame count when a key is pressed, and re-send that key to the active gadgets as a keyRepeat after a (developer-definable) number of frames. It would keep resending until the key is released.

Supporting double-clicking in Woopsi would be more tricky. I’m not sure how that’s usually handled - would Woopsi send clicks through the hierarchy when it knew that it was either a single- or double-click, or would it just send single clicks and allow the gadget to decide for itself? The former would work something like this:

click() search tree for collision with click if a click has occurred in the last 10 frames if clicked gadget same as the last gadget if click within 5 pixels of last click send double-click else send single-click end else send click to previous gadget end

    remember new gadget
     remember vbl count
   end
end

end

(I think that’s the right number of ends.)

This would switch Woopsi upside-down, to a certain extent. Woopsi would maintain the clicked gadget variable currently handled by each parent.

Jeff on 2007-12-10 at 09:19 said:

At the moment, I think double-clicking is best left to the gadgets themselves, but thats gut-feel, not mystic knowledge. Given that Woopsi keeps track of the vblcounter for you, its relatively trivial to implement in each gadget that cares.

Having said that, its worth revisiting the event models used by OSX and Win32. In both cases, as I recall, you recieve a message which contains the time (vblcount) when the event originated. Perhaps keyPress() should be passing through the vblcounter as well? Nah, thats getting too ‘operating system’ for my taste - the current solution works fine.

What is frustrating, however, is that the most recent upgrade broke me completely, and I haven’t pinned it down yet. Essentially, there are two problems that I can see.

  1. my menu_screen was derived from Screen rather than AmigaScreen. When I put it on-screen, I get the background being drawn, which shows that my ‘solid background gadget’ is being added ok, but the Button I put on does not appear. When I switch it to AmigaScreen, it works fine.

Possibly a problem with rectangle caching, or perhaps the coord transforms (my button is placed using the getClientRect() result.

  1. my file_selector screen creates an instance of Screen and deletes it on exit. On the second call, it locks up. If I scroll the menu screen down, and start the browser, then exit, then try to move the menu screen around, it locks up.

Possibly a problem with double-destruction, or the screen->close() code.

Jeff on 2007-12-10 at 09:34 said:

Ok, problem 1 explained. I was using getTitleHeight() for the height of my button - which comes back as zero. Thats good, an easy fix.

The second problem remains - time to add some dprint()s

ant on 2007-12-10 at 09:44 said:

The vblcount could always be sent as part of the EventArgs struct. That would be quite handy.

Argh - Screen is supposed to be called ScreenBase. No wonder it affected less code than I thought it would. However, as the Screen class can be used as a bare-bones screen, I wonder if “Screen” is more appropriate. Same applies to the renamed “WindowBase” class.

The delete problem might be related to the new deletion system. If you can’t find a problem in your code let me have the source and I’ll see if I can track it down in Woopsi.

Jeff on 2007-12-10 at 10:16 said:

Yes, the deferred deletion problem occurred to me as well. When I start clicking a bit faster, it seems to get a little further along which suggests that its vbl related.

Its in a pretty ugly state at the moment, but essentially you

a) click Browse b) click Cancel [ I add a text box to the middle of the screen, to confirm that the screen exists, and works - it was locking up at this point before I added the status display, whereas now you can …] c) click Browse again d) you are now locked up

I wired in libfat so it should show the content of “/” if its working - I’m testing it in DesMume so it shows empty normally. However, that bit was all working fine before I upgraded.

http://rapidshare.com/files/75564578/source.zip.html

Jeff on 2007-12-10 at 10:18 said:

Oops, forgot the makefile change. Just add -lfat to the LIBS line.

LIBS := -lfat -lnds9

Jeff on 2007-12-10 at 10:42 said:

Hmm, well, I think I found it.

My List gadget was not calling unregisterFromVBL() before being destructed. When I modified it so its destructor did unregister, it seemed to come good.

Race condition, I guess. I’ll remove more debug code and try it for real in NDS hardware, but I suspect that’s the problem…

Jeff on 2007-12-10 at 10:45 said:

I can see where addToDeleteQueue() does the automatic unregister, but is that the path that a screen would push its gadgets through when it is being closed?

ant on 2007-12-10 at 10:48 said:

Heh, I was just typing this reply:

Bug #1 - not the cause of your problem, but closing a gadget will cause that gadget to delete all of its children. They are just closed using “delete” - they should really be closed using “closeChild()”, because they are not being unregistered from the VBL list. I wonder if that sort of thing is better handled in the destructor?

Jeff on 2007-12-10 at 10:53 said:

I may have spoken too soon - it was all working nicely, I took out all my debug and tried again and its broken again.

I do wonder whether there’s a timing problem still with the unregister being called from a destructor, though its hard to see why. Of course, the delete of the child is happening inside a VBL, isn’t it?

It looks like Woopsi::vbl() calls all the listeners, then processes the deletion queue, so that should be safe.

Jeff on 2007-12-10 at 10:55 said:

Gadget::closeChild(…)

            // Ensure gadget knows it is being closed
            if (!gadget->isDeleted()) {
                    gadget->close();
            }

            // Do we need to make another gadget active?
            if (_activeGadget == gadget) {

Doesn’t gadget->close() delete the gadget? The activeGadget checking that follows is probably accessing a slightly dead pointer?

ant on 2007-12-10 at 11:00 said:

Found three stupid bugs. Number 1 - Gadget::hideChild() was checking if a gadget was invisible and then hiding it, instead of hiding visible gadgets. Numbers 2 and 3 - Gadget::closeChild() and hideChild() loop through the gadget vector from back to front, finding the next highest gadget to make active. Unfortunately, I’d put “i++” instead of “i--” in the loop increment, which means the whole thing locked up.

I’ve submitted the changes - see if that fixes it?

ant on 2007-12-10 at 11:03 said:

Gadget->close() doesn’t close the gadget. It marks it as deleted, and calls parent->closeChild. The parent then manages closing everything, as it needs to manage its internal pointers.

One more bug here - a gadget with no parent (ie. Woopsi) will therefore do nothing when woopsi->close() is called. The top-level gadget needs to be able to close itself.

Jeff on 2007-12-10 at 11:03 said:

The behaviour seems to change if there is only one button on the menu screen - it was when I removed the dummy textbox that it broke again. When I add it back in, it seems to work fine.

One other thing which occurred to me was to do with event handlers - my screen is installed as the event handler for the button. I noticed that during close(), gadgets like to raise a notification about the close - thats going to notify my screen while its deleting itself. again, it probably works, but then again, its probably unexpected…

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

Replying to myself - no, the deletion code shouldn’t go in the destructor, because if you’re deleting Woopsi it will try to interact with its own static members and cause chaos. The top-level gadget shouldn’t be closed using woopsi->close(), it should just be deleted. However, the other bug does still stand - a gadget should close() its children instead of just deleting them.

Jeff on 2007-12-10 at 11:14 said:

Ok, I just checked out version 166 and that doesn’t fix the problem, though it does seem to interact with the top screens. When I have the extra gadget on the screen, it works fine.

(Note, my menu screen has a solidcolor background, a button and a dummy text box - if it only has the solidcolor and the button, it locks up)

When its working, every time I come back from the browser (which is closing a screen, the TOP screens change places.

ie, it should be a DebugScreen on top of an AboutScreen on the HARDWARETOP and a MenuScreen on the HARDWAREBOTTOM.

However, the DebugScreen and AboutScreen seem to flip back and forward.

Jeff on 2007-12-10 at 11:19 said:

… the screen flipping, of course, is being caused by Gadget::closeChild() - whilst in a Window, activation needs to flick around the gadgets, and in a Screen, it should flick around the windows, in a Woopsi it shouldn’t reorder the windows!

Jeff on 2007-12-10 at 11:23 said:

… and by windows, of course I mean screens!

Its the “activate the highest gadget that isn’t the one being closed” loop that reordering the screens. I do wonder whether some of those other globals (_activeGadget, etc) aren’t causing the problem.

Ah, what happens if the activeGadget ends up being on the HARDWARETOP? Is it possible that fron then on, clicks in the HARDWAREBOTTOM don’t count?

ant on 2007-12-10 at 11:23 said:

Hmm, run that by me again? I’ll need to check that out.

Fixed a few more bugs. The Gadget destructor now calls closeChild() instead of delete. Gadget::moveToChildHiddenList() and Gadget::moveToDeletedList() were both decreasing the decoration count, but that value was also being decreased in closeChild() and hideChild(), which would screw up anything that tried to access the gadget vectors (and would explain why adding a dummy gadget solved the problem). Give that one a go?

ant on 2007-12-10 at 11:32 said:

And another fix - the hidden gadget vector wasn’t getting deleted.

Jeff on 2007-12-10 at 11:32 said:

In Gadget::closeChild() which is the same as Woopsi::closeChild(), we have the following:

            // Do we need to make another gadget active?
            if (_activeGadget == gadget) {

                    // Try to choose highest gadget
                    if (_gadgets.size() > 1) {
                            for (s16 i = _gadgets.size() - 1; i > -1; i--) {
                                    if (_gadgets[i] != gadget) {
                                            _activeGadget = _gadgets[i];
                                    }
                            }
                    } else {
                            _activeGadget = NULL;
                    }
            }

Now, the _gadgets[] array in a Woopsi is actually an array of Screens. Lets say I just closed Screen[4] - this loop will change _activeGadget to whatever is now the last screen in the _gadgets[] array - which is what causes my top screen flipping, I believe.

Jeff on 2007-12-10 at 11:37 said:

Hmmm, at revison 173, and having the dummy textbox doesn’t fix the problem any more.

I think I’m going to have to sleep on this one…

ant on 2007-12-10 at 11:37 said:

Aha, that makes sense. Woopsi should override that method and not implement that loop, since there’s no visual feedback when a screen is activated anyway. The only problem is if developers allow their users to close a screen and don’t check if they need to pull the top display back down, but that’s not something I can solve automatically without causing unnecessary screen flips for people who want to keep the top display where it is and play with the bottom display.

Jeff on 2007-12-10 at 11:40 said:

Yes. Hmmm. When I remove the List from the screen I’m displaying on the fly, the problem appears to disappear as well - perhaps I’ve goofed some memory management in there somehow. Definitely need to sleep on it.

Jeff on 2007-12-10 at 11:42 said:

As to Woopsi::closeChild(), I don’t think you should flip things from screen to screen, I’m one of those people who wants the top to stay where it is, even if that means the bottom goes black…

I’ll let you know if I managed to identify my List as causing the other problems…

ant on 2007-12-10 at 13:09 said:

Fixed a few more things. Calling child->close() in the destructor caused everything to screw up because it got into a permanent loop. That was the wrong thing to do anyway. Gadgets now correctly:

  • Delete their children.
  • Delete their hidden children
  • De-register their children from the VBL list when they are deleted.

Also, Woopsi should no longer flip screens if you close the last one in the bottom display. Your app is still crashing if I get rid of the status textbox, though. Will keep digging.

EDIT: I removed the status box creation line, but neglected to remove the addGadget() line. All seems to work for me now.

ant on 2007-12-10 at 19:23 said:

Fixed a couple of bugs in those changes. Also renamed the WindowBase class and files back to Window, and renamed the ScreenBase files back to Screen. Didn’t change the name of the Screen class because I forgot to change that in the first place.

Coding late at night seems to produce errors. I must remember this.

Jeff on 2007-12-10 at 21:52 said:

Ok, downloaded the latest and it all seems to work fine. Gotta put in a day at the coalface so I won’t be able to bughunt any more for a while, but it is looking good.