Fixin'

Published November 12, 2007
Advertisement
Today was a day of bug fixes. First off, I got bounty hunting quests working properly, and then I went off to work on tracking down a strange bug I had been encountering.

In Novarunner, all models and sounds and other assets are loaded through the use of an AssetCache object. This guy basically follows this pattern:
public:    T* obtain(string name):        if(name is not in the dictionary of stuff we have):            dictionary[name] = loadAsset        return dictionary[name]protected:    virtual T* loadAsset(string name) = 0;


Now spot the problem with this code:
class ModelCache : public AssetCache {public:    virtual PropaneModel* loadAsset(string name) { ... }};// EntirelyUnrelatedCode.cppvoid letUsLoadSomeFreakingModelsAlready(ModelCache& models) {    // Hey, why the fuck can I invoke a protected method here?!?    PropaneModel* view = models.loadAsset(xmlnode.name);}
Spotted it yet? It took me awhile to figure out, but apparently it's perfectly valid C++ to override a protected method with a public one in the derived class. Neither VS2005 nor GCC 4 managed to come up with a complaint against this usage.

Basically, what my idiocy when I derived ModelCache allowed me to do was directly invoke the loadAsset method, which bypassed keeping a record of the model being loaded previously, so the model would be loaded over and over again because I was never actually using the cache part of the ModelCache.

To make life worse, since I'm working with raw pointers and the consumers of the PropaneModel* expect it to be freed elsewhere (i.e. in ModelCache), the meshes were never actually freed when necessary and we ended up with a pretty juicy runaway memory leak, which I would've noticed earlier if I could get the new Leaks tool in Xcode3 to work properly.

The reason why the loadModel method is public when it should be protected? Because I pulled out the AssetCache abstract class originally from it! [rolleyes] I should now, by law, invoke ApochPIQ's excellent post about frivolous and unnecessary refactoring.

I had assumed that one of the load methods would be protected and thus not allow me to invoke it from another piece of code (so I just picked the first load method that popped up in Code Sense), but since I had 'publicized' it, the 'dumb' load method was accepted instead.

Bork bork bork hoogen floogen

Link of the Day
Check out Ovine by Design and Smila's Exile remake. It's got gameplay thicker than War and Peace and you could spend a long time licking those graphics. Lick them, slave!
Previous Entry An open quest-ion.
0 likes 3 comments

Comments

Tape_Worm
[wow]
IT'S COMPANION CUBE!!
November 13, 2007 01:22 AM
Evil Steve
I have some memory manager code if you want. It'll need a little tweaking to get it to compile on a non-window platform, but should work fine. It provides a replacement new and delete, so it can track leaks.

Link
November 13, 2007 03:58 AM
superpig
Perhaps you could specify the loadAsset function as a template parameter instead? People would then be able to call it completely freely but at least you wouldn't be misleading yourself into thinking you were calling it on the cache object. You'd eliminate the unnecessary virtual-function indirection too.

Or even do it using template specialization...
November 13, 2007 09:38 AM
You must log in to join the conversation.
Don't have a GameDev.net account? Sign up!
Profile
Author
Advertisement
Advertisement