1.1.3: t2dPath crash problem

#1 You may:
a) do not detach and delete any of it inside onReachNode callback. Just make new function and schedule it
b) make sure if you create the t2dPath yourself(i.e. from script) to call %yourscenegraph.performPostInit();

#2 Thanks for the suggestions.

Unfortunately, I just tried them both and I'm having the same crash problem.

It appears that the problem is happening as a result of the sprites being on the t2dPath. I also removed the onReachNode callback completely and then I placed a trigger over the last node and called my delete function from there and the same crash occured.

I also just did a test without the t2dPath, using t2dTriggers to both guide and delete the sprite and it worked perfectly for an indefinite period of time. This leads me to believe that it's something with the way t2dPath object detaches objects that's causing the problem.

At any rate, even though it'll be a bit clumsier, I think I'll go with using triggers instead of paths since this is too big of a crash risk even if I somehow get it to work correctly.

#3 I just want to say, that i have not any problem you mention. A t2dPath works good. It is better find issue, than try to hide the problem. It saves you from making it twice. Good luck

#4 Well, seems like that is the same problem i am facing too. unfortunately for my case, i will have to use paths because of th curvy surfaces. tried all sorts of ways to prevent the crash but no luck. any advice is appreciated

#5 The problem is outside the t2dPath. Well, don't do anythig in a onReachNode callback, this is unsafe place (especialy for attached objects)
Check all script with you path and attached objects.

#6 I wish I could tell you that I found a solution but unfortunately I haven't. I've been continuing to use triggers although a path would be far superior for my needs as well so I'd love to see this addressed by GG.

I've downloaded the 1.5 beta but I haven't tested out the path functionality yet to see if it's been improved (though the update notes didn't indicate that it had been addressed.) However, I may give that a try this weekend to see if the results are any better.

Please keep me posted as well if you find a solution.

#7 I am also seeing this problem. I figured I'd drop my two cents in here, seeing as how I have the pro version of the engine; I've managed to trace through some of the code and actually watch it as it crashes.

Basically, I'm creating several sprites in script and attaching them to a path, each separated by a short delay. I let them run around a bit, and then I detach the first sprite. That seems to work fine. After that, I delete that first sprite with a safeDelete call. What happens for me is that one of the *other* sprites becomes detached from the path. If I delete that one, another one becomes detached. If I keep at it enough the engine eventually crashes.

From what I've been able to tell, the cause behind the sprites getting detached from the path (beyone the voluntary first one) has something to do with the t2dPath::preIntegrate method. The first couple of lines of that method contain a check to make sure that the object on the path is not null. (The object is referenced through a smart pointer of some sort.) It appears that that check is triggering; somehow, the act of deleting the first sprite also manages to delete some portion of one of the other ones still on the path. The path class detects that and removes the damaged sprite.

Digging a bit more, I noticed a pattern: the sprite that is getting damaged just happens to be the one that is sitting in the same position within the path's attached object list as the one that was initially detached. When I started, it was always the last added sprite that was getting kicked. In the t2dPath class, the detachObject call uses a method called erase_fast, which removes an entry from the list by overwriting it with the last entry in the list and shrinking the list size by one. Hmmm. I went into the code and modified it to use the slower erase method instead, which deletes by shifting all of the list entries ahead of the deleted item down one location. After that change, it was always the second sprite that got involuntarily detached.

The crash comes as a side effect of all of this. After several iterations of detaching and deleting things, the program hit an access violation in the preIntegrate method. For whatever reason, the pointer that was getting set to null before, that time had gotten set to 0xcfcfcfcf. (Or something like that; an obvious uninitialized memory value.) The path calculation code blew up trying to access a position get method through a bad pointer.

Unfortunately, that's about as far as I've been able to get. I can't find any fields in the sprite class (or related parent classes) that would keep a residual pointer to the path class; there is one, but it seems to be getting correctly cleared when the initial detach occurs. Something is still keeping a pointer to the memory location where that first sprite *was*, so that when it gets deleted, whatever is there now is getting clobbered. It's hard to tell where the problem is, tho. There are a couple of layers of indirection here; the path class keeps a Vector of PathedObject objects, which are wrappers to scene objects. Then those PathedObjects themselves have SimObjectPtrs to the scene object; that seems to be some sort of smart pointer class. Perhaps one of those isn't giving up the address of what they're pointing to when they should. Or something isn't properly telling the smart pointer that what it's pointing to is being referenced elsewhere and shouldn't be deleted.

I'm still researching the problem, but I could use some help; I'm stuck at the moment.

#8 Ok.
There is a simple workaround, instead changing of any code is just schedule the detach & delete operation. I know, that AV is annoying, but you get more portable game code. Since I've change many versions of TGB(T2D)
from first betas to 1.1.3. Its a difficult to syncronize all the chages I made.
Anyway, if you want to fix t2dPath you should look at the attach/detach code more closely. And do all your deeds implicit, don't trust safeDelete(), it's better to use schedule.

#9 Ahhh... I think I figured it out. I tried scheduling the detach and delete as you suggested, but that didn't seem to help. So I took your other advice and started looking really closely at the attach and detach code.

The apparent problem seems to be that Vectors and SimObjectPtrs don't mix. The t2dPath keeps a list of the attached objects in a Vector array. PathedObject is a container that holds a single SimObjectPtr. SimObjectPtr is a smart pointer... actually, too smart for its own good, here. Not only does it keep a pointer to the scene object, but it also makes sure that the scene object has a pointer *back* to the SimObjectPtr. My guess is that the problem is in the way the Vector class manages its entries: the method that erases an entry in the vector simply calls memmove. While very efficient, this doesn't update the *back* pointers that point the scene objects to the SimObjectPtrs. The result is that the pointers get out of sync, so that when someone goes to delete one of the previously-attached objects, it ends up clearing out some other entry in the path's attached array.

In t2dPath.cc, in the detachObject method, I changed this:

if (object)
{
(*i).mObject->setLinearVelocity(t2dVector(0, 0));
(*i).mObject->setAttachedToPath(NULL);
}
mObjects.erase_fast(i);
break;

to this:

if (object)
{
(*i).mObject->setLinearVelocity(t2dVector(0, 0));
(*i).mObject->setAttachedToPath(NULL);
clearNotify(object);
}
int nEndIndex = mObjects.size() - 1;
(*i) = mObjects[nEndIndex];
mObjects[nEndIndex].mObject = NULL;
mObjects.decrement();
break;

And it appears to work. (It survives quick testing, anyway... I have yet to thoroughly put it through its paces.) Essentially, I'm manually performing the same operation that erase_fast does, except that I'm taking extra care with the mObject pointers. Granted, there is probably a better way to it, but the gist of it is to make sure that the smart pointers are properly handled despite being in a Vector.

Oh, and the clearNotify addition is just an extra touch. I don't really know if it's absolutely necessary, but it does seem to help. The attachObject code calls a t2dPath method called deleteNotify, which sets up some pointers to the attached scene object; I figured calling the clearNotify method to clean up those pointers would be good housekeeping.

Now if I could just get one of the devs in here to look at my fix and tell me if I'm doing something right. :)

#10 Sounds good. I am really have no time to check this, but if I try I post a comment here.
Why GG are reinventing a wheel and don't use the STL is stil a question :)
Anyway thanks for your research, Bryan.

#11 @Bryan:

You can simply set (*i).mObject = 0 and it will take care of the rest. This has been fixed for the next release.

[edit]

Forgot to mention that you have to change the code in t2dPath::onRemove() as well or it will enter an infinite loop waiting for the object list to be cleared.

replace

while(mObjects.size())
      detachObject(mObjects.first().mObject);

with

Vector<PathedObject>::iterator i;

   for( i = mObjects.begin(); i != mObjects.end(); i++ )
      if( !(*i).mObject.isNull() )
         (*i).mObject = 0;

   mObjects.clear();

#12 Actually, I tried that at one point, and it didn't work. Making sure the SimObjectPtrs are updated is only half of the problem. The other part is that the object references in the SimObjects' notify lists aren't updated when the Vector class' erase_fast method does its memmove call.

[edit]

I'll give that a try.

#13 I understand that, I meant instead of all the "manually performing the same operation erase_fast does".

The SimObjectPtr::operator=() will unregister references if you set it to 0 and the mem move will not matter and new objects are constructed in place when added.


Vector and its realloc has been responsible for quite a few bugs that only happened in the off chance that the vector hit the 16 element limit and realloced objects instantiated by value, sometimes right out from under the objects calling the code - boom.

#14 I understand about setting mObject to null to unregister the object that's getting detached. My concern is over the last scene object in the attach list; it's not getting updated with a new referencing object address when the SimObjectPtr that points to it is memmove'd to fill in the hole left by the detached object.

(Sorry for being nitpicky; I'm new to using the engine and I want to be sure I understand the details.)

#15 Heh, no need to be sorry, you are perfectly correct, I was just looking at it in the other direction ;p

It indeed bad form to keep anything that needs a copy constructor, assignment operator, etc, etc in a vector by value as well as anything that passes references to itself, hence my comment in the last paragraph.

I missed that part as on my side I have changed mObjects to a vector of pointers instead so don't have that issue.

[edit]

There are other issues with the code as well. When deleting an object it will call the onDeleteNotify() method on the pathed node but by that time the mObject SimObjectPtr ref has already been zeroed by the processNotifications() call and so it won't find the PathedObject to remove. I added code to save the object id and use that for lookups instead to make sure it all worked. A lot of code changes so it would be wise to wait for the next release or if you are really pressed for time, post in here and I will mail you the updated code.