Static TextureHandles cause crashes on close

#1 Mark,

You are correct. Unless the TextureHandle system was to use a smart-reference pointer similar to the template "template class SimObjectPtr" for SimObjects, you can't do this.

A quick glance shows that this would only show up via the reference counter in the TextureObject which would trigger the "AssertISV" call which you should see as this is an "In Shipping Version" assert. I'm actually suprised you got all the way to the TextureHandles' destructor.

I will mention this to Ben Garney though.

Thanks for the heads-up.

- Melv.

#2 Hmm... This is an interesting one. I am about to leave town for the weekend; I'll review this more fully when I get back. Thank you both for the intelligent discussion and analysis of this issue!

#3 Thanks Ben.

- Melv.

#4 This seems to be a rather dubious double-use of resource management. It seems as if TextureHandle is operating under a pseudo-RAII mechanism (except that it doesn't get it's texture in its constructor). At the same time, however, there is a management scheme where some piece of TextureHandle is managed explicitly by an external object, such that, once this external object is asked to do so, all TextureHandle objects are immediately rendered unusable. Of course, since the TextureManager didn't actually know about the TextureHandle objects, it couldn't let them know that the memory that they were looking at was gone, so they're not simply incomplete, but actually broken.

Both methods of resource management are valid (though I tend to prefer RAII to a manager object where possible), but I'm not sure why both methods are being used on the same objects. If the TextureManager is responsible for destroying something that TextureHandle's use, then the TextureHandle objects shouldn't be trying to clean up themselves at all. If the TextureHandle objects themselves are supposed to take care of themselves, then they should use RAII and there's no need for a TextureManager object.

#5 It seems to me that creating a global texture handle is not a good idea. I'm thinking the fix for this would be to simply disallow it, and add an assert in the TextureHandle constructor/set method that blows up if you try to set/create/delete when the texture manager is not active.

Thoughts?

#6 @Smaug: There will always be a TextureManager of some sort - textures must be tracked properly so that they can be zombified and resurrected.

Meanwhile, it's not feasible to track every handle for a given texture, either.

What should happen is that all objects should destroy their TextureHandles when they are destroyed - and having a global TextureHandle breaks that. TextureHandles are designed to be composited into other objects, not used stand-alone.

#7 Ok, I added a flag indicating whether the texture manager is active, and some asserts in the texture handle lock/unlock code. (Issue #196.)

#8 I solved the problem via a Vector of "Living Texture Handles". Seems to work, only took about 20 lines:

in gTexManager.h:

class TextureHandle;
typedef Vector<TextureHandle *> TextureHandleVector;

struct TextureManager
{
          ...
         static TextureHandleVector LivingTextures;
          ...
};

Then of course in gTexManager.cc

TextureHandleVector TextureManager::LivingTextures;

"addup()" is called in every TextureHandle constructor (not shown here - too many of them)

void TextureHandle::addup()
{
	TextureManager::LivingTextures.push_back(this);
}

TextureHandle::~TextureHandle()
{ 
	
	unlock(); 
	TextureHandleVector::iterator ii = TextureManager::LivingTextures.begin();
	while(ii != TextureManager::LivingTextures.end())
	{
		if(*ii == this)
		{
			TextureManager::LivingTextures.erase(ii);
			break;
		}

		ii++;
	}
}

void TextureHandle::KillObjectRef()
{
	object = NULL;
}

Added the following to destroy, I am assuming that all texture objects are eventually going t be destructed, so I take advantage of the time to clean out the vector.

void TextureManager::destroy()
{
   TextureDictionary::destroy();

   AssertFatal(sgEventCallbacks.size() == 0,
               "Error, some object didn't unregister it's texture event callback function!");

   while(TextureManager::LivingTextures.size())
   {
		TextureHandleVector::iterator ii = TextureManager::LivingTextures.begin();
		TextureHandle * th  = *ii;
		TextureManager::LivingTextures.erase(ii);
		th->KillObjectRef();
   }

	
}

--Mark

#9 @Mike: That seems like a fairly heavy weight solution, especially since TextureHandles should never be allocated on the fly, and the TextureObjects are already getting deleted earlier on... Is there some benefit I'm missing?

#10 I have to agree with Ben on your solution to this, Mark.

However, as to a more reasonable solution, I would say that TextureHandles should not own their textures. The destructor of TextureHandle should not try to delete the texture object on destruction if the TextureManager is going to do it for them. The most they should do is notify the TextureManager that they are being destroyed. If the TextureManager has been deactivated, then it will simply do nothing.

#11 The TextureObjects are refcounted, so it makes sense for them to be deleted when all of the references are gone.

The main purpose of the TextureManager's final deletion is to make sure that eveything that can be released, is released.

TextureObjects cannot exist without a valid TextureManager, as it manages resources central to their existence (notionally, the OpenGL texture state).

#12 I don't see how any of these statements affect my solution.

TextureObjects are still reference counted, except that now the creation and destruction of references is handled by the TextureManager. Other objects, like TextureHandle's can get references to them, but the destructor does not directly delete the object. It simply tells the TextureManager to do so. Since the TextureManager knows when it is active and not active, it can simply ignore calls to delete a reference to a TextureObject if it is not active.

Problem solved. The code is made bulletproof; there no longer needs to be a statement in the docs about TextureHandle or any other class that gets a hard-reference to TextureObjects in order to prevent an unsuspecting user from creating a global or something for that object.

Personally, I'm not a big fan of globals. So I tend to think that Mike is doing some pretty shady things in his code to begin with. However, I also prefer code to be sufficiently bulletproof to handle unpleasant uses.

#13 I think what I'm getting at is that adding the extra level of indirection to deal with allocating/destroying textures with an inactive texturemanager is a bit silly, when all of the current Torque games get along just fine without ever having an issue. I could add an if(TextureManager::isActive() call, but I think a debug assert is just as good in terms of preventing bone-headedness.

The other thing is that, when the TextureManager isn't active, there aren't any TextureObjects to have references (or not) to.

#14 I definitely am doing shady things in my code... ;>

#15

Quote:all of the current Torque games get along just fine without ever having an issue.

Nonsense. Just because someone can ship a game on an existing engine doesn't mean that the engine shouldn't be made more robust. And you have to admit, this is a lack of robustness. Maybe not the most egregious case, but it is here before you and it has a pretty simple fix to it.

Quote:I think a debug assert is just as good in terms of preventing bone-headedness.

It's only a question of "bone-headedness" if the class is clearly labled as being a class that should not be destroyed after the TextureManager has been deactivated. And, even if it is, it is an unnecessary source of fragility.

All I'm asking for is to have the code made more inheriently robust and less fragile. The more classes that exist that have need of specific usage patterns, the more likely it is to cause other problems. Particularly if the class itself doesn't have to have this kind of usage pattern. Or, if the class itself has an asymmetric usage pattern like TextureHandle. You can construct it without the TextureManager just fine, but you can't delete it without the TextureManager. I don't know about you, but when I am developing/debugging, I don't frequently test my game's exit conditions; I just press the stop button on the debugger. It would clearly be better to get these kinds of errors sooner, rather than theoretically having dozens of thse TextureHandles lying around that need to be substantially moved.

I don't see making the engine code more robust as being unreasonable to ask.

#16 Well, right now in SVN trunk (after I tested my changes and made some tweaks) it either silently does nothing (if you try to unlock a texture when the TextureManager is inactive), or it asserts with an informative error message (if you try to lock a texture when the TextureManager is inactive, which should not ever happen).

To me this seems like it will work to gently remind anyone who does something boneheaded (since you can't MAKE a textureobject without an active texturemanager, and the texturemanager remains active continuously during all rendering activity), and silently prevents potentially dangerous activities (in the case of objects trying to release textures after the texturemanager has been destroyed).

So now if you make a global and try to set it to something pre-TM init, it will say, "I need a TM to do this!" which seems pretty robust to me, and you can delete/unset them anytime you want.

This seems, to me, to fulfill your request to make this part of the engine code more robust. Do you disagree?