Polyobj_OR_MoveToSpot Crash/Lockup

[Graf Zahl]

Thanks for this analysis. I expected something along these lines.

[Tapwave]

appears to be line 666 of po_man.cpp

[Edward-san]

Hmm, I did this change:

Code: Select all

Index: src/po_man.cpp
===================================================================
--- src/po_man.cpp	(revision 3399)
+++ src/po_man.cpp	(working copy)
@@ -618,7 +618,7 @@
 bool EV_MovePolyTo(line_t *line, int polyNum, int speed, fixed_t targx, fixed_t targy, bool overRide)
 {
 	int mirror;
-	DMovePolyTo *pe;
+	DMovePolyTo *pe = NULL;
 	FPolyObj *poly;
 	TVector2<double> dist;
 	double distlen;
@@ -652,6 +652,8 @@
 	{
 		pe->StopInterpolation();
 	}
+    delete pe;
+    pe = NULL;
 
 	while ( (mirror = poly->GetMirror()) )
 	{
@@ -677,6 +679,8 @@
 		{
 			pe->StopInterpolation();
 		}
+        delete pe;
+        pe = NULL;
 	}
 	return true;
 }

Now in debug mode I have a failed assertion when I push the button:

Code: Select all

zdoom: /media/27522720-b102-4f44-b9ed-8a24d4ef0738/edward-san/zdoom/trunk/src/dthinker.cpp:229: virtual DThinker::~DThinker(): Assertion `NextThinker == __null && PrevThinker == __null' failed.

seems like 'pe' can't be deleted normally. How is it done?

[Graf Zahl]

pe->Destroy() instead of delete pe;

In general any thinker object in the game may never plainly be 'delete'd.
You have to take care of internal references somehow and the destructor is not the correct spot for this. The object first needs to be garbage collected to ensure that it isn't referenced anymore.

[Edward-san]

After that change the freeze is still there but it's a lot easier to stop the application. I still wonder why there's the loop (surely there's a missing assumption in the while loop).

[edit]Interestingly, 'mirror' oscillates between 2 and 1 ...

[Blzut3]

The loop is there to potentially allow more than 2 polyobjs to be chained together. (Polyobj 1 has polyobj 2 as a mirror and polyobj 2 has polyobj 3 as a mirror for example.) However it is common practice for mirror polyobjs to have the source polyobj as it's mirror creating a cycle. This normally isn't a problem as ZDoom will break the loop when it hits a polyobj that's already active, but when using the override functions it just cycles indefinitely.

I think this also points out another minor bug in the non-override functions. Since it breaks on the first mirror to have an active thinker. You could possibly jam the non-override functions when used on a non cyclic set of polyobjs. Haven't actually tried this though.

[randi]

Fixed.

However it is common practice for mirror polyobjs to have the source polyobj as it's mirror creating a cycle.

I don't know if this statement is actually true now or not, but common practice in Hexen was that only one polyobject was marked with a mirror, so you wouldn't get cycles like this.