View Issue Details

IDProjectCategoryView StatusLast Update
0002856FSSCPSEXPspublic2013-05-08 00:50
ReporterYarn Assigned ToGoober5000  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Platformx64OSWindows 7 
Product Version3.6.18 
Target Version3.7.0 
Summary0002856: Has-docked-delay always returns false if dockee is destroyed
DescriptionJust as the summary says. This has happened since retail and before revision 9325, but only if the dockee did not self-destruct. A change in revision 9325 also made this happen if the dockee self-destructed. This change causes the failure debriefing to appear in "Return to Babel" (SM3-03) even if the player completes the mission successfully.
Steps To ReproduceIn the attached test mission, an Elysium docks with a Fenris, then undocks after 30 seconds. After the player departs, the debriefing tells whether the Elysium has docked based on what has-docked-delay (with the Fenris being the dockee) returns. During the mission, you can press the following number keys to cause the following events:

1 key: Self-destructs the Fenris
2 key: Self-destructs the Elysium
3 key: Spawns a Moloch to destroy the Fenris

In all FreeSpace 2 versions (including retail), has-docked-delay returns false if the Moloch destroys the Fenris (or the Fenris is destroyed by some means other than self-destructing), regardless of whether the Elysium actually docked. Additionally, in revisions 9325 and later, has-docked-delay returns false if the Fenris self-destructed.
TagsNo tags attached.

Activities

Yarn

2013-04-29 04:37

developer  

DockTest.zip (2,354 bytes)

Goober5000

2013-04-30 02:52

administrator  

2856.patch (932 bytes)   
Index: code/parse/sexp.cpp
===================================================================
--- code/parse/sexp.cpp	(revision 9660)
+++ code/parse/sexp.cpp	(working copy)
@@ -4903,17 +4903,17 @@
 	if (sexp_query_has_yet_to_arrive(dockee))
 		return SEXP_CANT_EVAL;
 
+	Assert ( count > 0 );
+	if ( mission_log_get_time_indexed(LOG_SHIP_DOCKED, docker, dockee, count, NULL) )
+		return SEXP_KNOWN_TRUE;
+
 	if ( mission_log_get_time(LOG_SHIP_DESTROYED, docker, NULL, NULL) || mission_log_get_time(LOG_SHIP_DESTROYED, dockee, NULL, NULL) )
 		return SEXP_KNOWN_FALSE;
 
 	if ( mission_log_get_time(LOG_SELF_DESTRUCTED, docker, NULL, NULL) || mission_log_get_time(LOG_SELF_DESTRUCTED, dockee, NULL, NULL) )
 		return SEXP_KNOWN_FALSE;
 
-	Assert ( count > 0 );
-	if ( !mission_log_get_time_indexed(LOG_SHIP_DOCKED, docker, dockee, count, NULL) )
-		return SEXP_FALSE;
-
-	return SEXP_KNOWN_TRUE;
+	return SEXP_FALSE;
 }
 
 /**
2856.patch (932 bytes)   

Goober5000

2013-04-30 02:55

administrator   ~0014987

Interesting. Revision 9325 only made the behavior consistent; the bug has been around since retail. What happens is that the sexp first checks to see whether the ships exist before checking whether they have docked. If either ship hasn't arrived yet, it returns SEXP_CANT_EVAL. If either ship is destroyed, it returns SEXP_KNOWN_FALSE. Once they have docked, it returns SEXP_KNOWN_TRUE.

The problem is that ships can be destroyed after they have docked, as Yarn said. This problem isn't seen in-mission because the sexp is latched to KNOWN_TRUE as soon as the ship has docked, even if it's destroyed in the very next frame. It only becomes a problem in debriefing (and campaign tree evaluation), because there you are checking a single snapshot, not a constantly changing environment.

I've attached a patch for review, and I'll commit it in a day or so.

niffiwan

2013-04-30 09:07

developer   ~0014996

Umm.. the patch changes "has-docked", but both SM3-03 & the test mission use "has-docked-delay"?

Goober5000

2013-05-01 02:34

administrator   ~0015003

Derp. I forgot that there were two versions of the has-docked sexp.

And there's also has-undocked to worry about as well. So I've gone and consolidated the four sexps into one function. New patch attached.

Goober5000

2013-05-01 03:10

administrator  

2856-v2.patch (6,928 bytes)   
Index: code/parse/sexp.cpp
===================================================================
--- code/parse/sexp.cpp	(revision 9666)
+++ code/parse/sexp.cpp	(working copy)
@@ -4861,7 +4861,6 @@
 		return SEXP_FALSE;
 }
 
-
 /**
  * Return true if the subsystem of the given ship has been destroyed
  */
@@ -4889,63 +4888,6 @@
 }
 
 /**
- * Determine if a ship has docked
- */
-int sexp_has_docked(int n)
-{
-	char *docker = CTEXT(n);
-	char *dockee = CTEXT(CDR(n));
-	int count = eval_num(CDR(CDR(n)));		// count of times that we should look for
-
-	if (sexp_query_has_yet_to_arrive(docker))
-		return SEXP_CANT_EVAL;
-
-	if (sexp_query_has_yet_to_arrive(dockee))
-		return SEXP_CANT_EVAL;
-
-	if ( mission_log_get_time(LOG_SHIP_DESTROYED, docker, NULL, NULL) || mission_log_get_time(LOG_SHIP_DESTROYED, dockee, NULL, NULL) )
-		return SEXP_KNOWN_FALSE;
-
-	if ( mission_log_get_time(LOG_SELF_DESTRUCTED, docker, NULL, NULL) || mission_log_get_time(LOG_SELF_DESTRUCTED, dockee, NULL, NULL) )
-		return SEXP_KNOWN_FALSE;
-
-	Assert ( count > 0 );
-	if ( !mission_log_get_time_indexed(LOG_SHIP_DOCKED, docker, dockee, count, NULL) )
-		return SEXP_FALSE;
-
-	return SEXP_KNOWN_TRUE;
-}
-
-/**
- * Determine if a ship has undocked
- */
-int sexp_has_undocked(int n)
-{
-	char *docker = CTEXT(n);
-	char *dockee = CTEXT(CDR(n));
-	int count = eval_num(CDR(CDR(n)));
-
-	if (sexp_query_has_yet_to_arrive(docker))
-		return SEXP_CANT_EVAL;
-
-	if (sexp_query_has_yet_to_arrive(dockee))
-		return SEXP_CANT_EVAL;
-
-	Assert ( count > 0 );
-	if ( !mission_log_get_time_indexed(LOG_SHIP_UNDOCKED, docker, dockee, count, NULL) ) {
-		// if either ship destroyed before they dock, then sexp is known false
-		if ( mission_log_get_time(LOG_SHIP_DESTROYED, docker, NULL, NULL) || mission_log_get_time(LOG_SHIP_DESTROYED, dockee, NULL, NULL) )
-			return SEXP_KNOWN_FALSE;
-		if ( mission_log_get_time(LOG_SELF_DESTRUCTED, docker, NULL, NULL) || mission_log_get_time(LOG_SELF_DESTRUCTED, dockee, NULL, NULL) )
-			return SEXP_KNOWN_FALSE;
-
-		return SEXP_FALSE;
-	}
-
-	return SEXP_KNOWN_TRUE;
-}
-
-/**
  * Determine if a ship has arrived onto the scene
  */
 int sexp_has_arrived(int n, fix *latest_time)
@@ -5217,73 +5159,50 @@
 	return SEXP_FALSE;
 }
 
-int sexp_has_docked_delay(int n)
+int sexp_has_docked_or_undocked(int n, int op_num)
 {
+	Assert(op_num == OP_HAS_DOCKED || op_num == OP_HAS_UNDOCKED || op_num == OP_HAS_DOCKED_DELAY || op_num == OP_HAS_UNDOCKED_DELAY);
+
 	char *docker = CTEXT(n);
 	char *dockee = CTEXT(CDR(n));
 	int count = eval_num(CDR(CDR(n)));		// count of times that we should look for
-	fix delay = i2f(eval_num(CDR(CDR(CDR(n)))));
-	fix time;
+
 	if (count <= 0)
 	{
-		Warning(LOCATION, "Has-docked-delay count should be at least 1!  This has been automatically adjusted.");
+		Warning(LOCATION, "Has-%sdocked%s count should be at least 1!  This has been automatically adjusted.", (op_num == OP_HAS_UNDOCKED || op_num == OP_HAS_UNDOCKED_DELAY ? "un" : ""), (op_num == OP_HAS_DOCKED_DELAY || op_num == OP_HAS_UNDOCKED_DELAY ? "-delay" : ""));
 		count = 1;
 	}
 
-	if ( mission_log_get_time(LOG_SHIP_DESTROYED, docker, NULL, NULL) || mission_log_get_time(LOG_SHIP_DESTROYED, dockee, NULL, NULL) )
-		return SEXP_KNOWN_FALSE;
-
-	if ( mission_log_get_time(LOG_SELF_DESTRUCTED, docker, NULL, NULL) || mission_log_get_time(LOG_SELF_DESTRUCTED, dockee, NULL, NULL) )
-		return SEXP_KNOWN_FALSE;
-
 	if (sexp_query_has_yet_to_arrive(docker))
 		return SEXP_CANT_EVAL;
 
 	if (sexp_query_has_yet_to_arrive(dockee))
 		return SEXP_CANT_EVAL;
 
-	if ( !mission_log_get_time_indexed(LOG_SHIP_DOCKED, docker, dockee, count, &time) )
-		return SEXP_FALSE;
+	if (op_num == OP_HAS_DOCKED_DELAY || op_num == OP_HAS_UNDOCKED_DELAY)
+	{
+		fix delay = i2f(eval_num(CDR(CDR(CDR(n)))));
+		fix time;
 
-	if ( (Missiontime - time) >= delay )
-		return SEXP_KNOWN_TRUE;
+		if ( mission_log_get_time_indexed(op_num == OP_HAS_DOCKED_DELAY ? LOG_SHIP_DOCKED : LOG_SHIP_UNDOCKED, docker, dockee, count, &time) )
+		{
+			if ( (Missiontime - time) >= delay )
+				return SEXP_KNOWN_TRUE;
+		}
+	}
 	else
-		return SEXP_FALSE;
-}
-
-int sexp_has_undocked_delay(int n)
-{
-	char *docker = CTEXT(n);
-	char *dockee = CTEXT(CDR(n));
-	int count = eval_num(CDR(CDR(n)));
-	fix delay = i2f(eval_num(CDR(CDR(CDR(n)))));
-	fix time;
-	if (count <= 0)
 	{
-		Warning(LOCATION, "Has-undocked-delay count should be at least 1!  This has been automatically adjusted.");
-		count = 1;
+		if ( mission_log_get_time_indexed(op_num == OP_HAS_DOCKED ? LOG_SHIP_DOCKED : LOG_SHIP_UNDOCKED, docker, dockee, count, NULL) )
+			return SEXP_KNOWN_TRUE;
 	}
 
-	if (sexp_query_has_yet_to_arrive(docker))
-		return SEXP_CANT_EVAL;
+	if ( mission_log_get_time(LOG_SHIP_DESTROYED, docker, NULL, NULL) || mission_log_get_time(LOG_SHIP_DESTROYED, dockee, NULL, NULL) )
+		return SEXP_KNOWN_FALSE;
 
-	if (sexp_query_has_yet_to_arrive(dockee))
-		return SEXP_CANT_EVAL;
+	if ( mission_log_get_time(LOG_SELF_DESTRUCTED, docker, NULL, NULL) || mission_log_get_time(LOG_SELF_DESTRUCTED, dockee, NULL, NULL) )
+		return SEXP_KNOWN_FALSE;
 
-	if ( !mission_log_get_time_indexed(LOG_SHIP_UNDOCKED, docker, dockee, count, &time) ) {
-		// if either ship destroyed before they dock, then sexp is known false
-		if ( mission_log_get_time(LOG_SHIP_DESTROYED, docker, NULL, NULL) || mission_log_get_time(LOG_SHIP_DESTROYED, dockee, NULL, NULL) )
-			return SEXP_KNOWN_FALSE;
-		if ( mission_log_get_time(LOG_SELF_DESTRUCTED, docker, NULL, NULL) || mission_log_get_time(LOG_SELF_DESTRUCTED, dockee, NULL, NULL) )
-			return SEXP_KNOWN_FALSE;
-
-		return SEXP_FALSE;
-	}
-
-	if ( (Missiontime - time) >= delay )
-		return SEXP_KNOWN_TRUE;
-	else
-		return SEXP_FALSE;
+	return SEXP_FALSE;
 }
 
 int sexp_has_arrived_delay(int n)
@@ -21707,10 +21626,6 @@
 				sexp_val = sexp_is_subsystem_destroyed(node);
 				break;
 
-			case OP_HAS_DOCKED:
-				sexp_val = sexp_has_docked(node);
-				break;
-
 			case OP_HAS_ARRIVED:
 				sexp_val = sexp_has_arrived( node, NULL );
 				break;
@@ -21719,10 +21634,6 @@
 				sexp_val = sexp_has_departed( node, NULL );
 				break;
 
-			case OP_HAS_UNDOCKED:
-				sexp_val = sexp_has_undocked(node);
-				break;
-
 			case OP_IS_DISABLED:
 				sexp_val = sexp_is_disabled( node, NULL );
 				break;
@@ -21744,8 +21655,11 @@
 				sexp_val = sexp_is_subsystem_destroyed_delay(node);
 				break;
 
+			case OP_HAS_DOCKED:
+			case OP_HAS_UNDOCKED:
 			case OP_HAS_DOCKED_DELAY:
-				sexp_val = sexp_has_docked_delay(node);
+			case OP_HAS_UNDOCKED_DELAY:
+				sexp_val = sexp_has_docked_or_undocked(node, op_num);
 				break;
 
 			case OP_HAS_ARRIVED_DELAY:
@@ -21756,10 +21670,6 @@
 				sexp_val = sexp_has_departed_delay(node);
 				break;
 
-			case OP_HAS_UNDOCKED_DELAY:
-				sexp_val = sexp_has_undocked_delay(node);
-				break;
-
 			case OP_IS_DISABLED_DELAY:
 				sexp_val = sexp_is_disabled_delay(node);
 				break;
2856-v2.patch (6,928 bytes)   

Zacam

2013-05-04 06:33

administrator   ~0015030

Seems to do as advertised on the tin, at least with the supplied test scenario.

niffiwan

2013-05-07 09:08

developer   ~0015038

Tested with SM3-03 and works OK there too. Code looks OK as well, the 4 replaced functions were quite similar so combining them seems like a good idea.

Goober5000

2013-05-08 00:48

administrator   ~0015040

Woot. Committing.

Goober5000

2013-05-08 00:50

administrator   ~0015041

Fix committed to trunk@9675.

Related Changesets

fs2open: trunk r9675

2013-05-07 21:43

Goober5000


Ported: N/A

Details Diff
fix for Mantis 0002856 - combine all the has-[un]docked[-delay] sexp functions and make adjustments for the order of ship checks Affected Issues
0002856
mod - /trunk/fs2_open/code/parse/sexp.cpp Diff File

Issue History

Date Modified Username Field Change
2013-04-29 04:37 Yarn New Issue
2013-04-29 04:37 Yarn File Added: DockTest.zip
2013-04-29 04:47 niffiwan Target Version => 3.7.0
2013-04-30 02:35 Goober5000 Assigned To => Goober5000
2013-04-30 02:35 Goober5000 Status new => assigned
2013-04-30 02:52 Goober5000 File Added: 2856.patch
2013-04-30 02:55 Goober5000 Note Added: 0014987
2013-04-30 09:07 niffiwan Note Added: 0014996
2013-04-30 10:48 Echelon9 Status assigned => code review
2013-05-01 02:34 Goober5000 File Added: 2856-v2.patch
2013-05-01 02:34 Goober5000 Note Added: 0015003
2013-05-01 03:10 Goober5000 File Deleted: 2856-v2.patch
2013-05-01 03:10 Goober5000 File Added: 2856-v2.patch
2013-05-04 06:33 Zacam Note Added: 0015030
2013-05-07 09:08 niffiwan Note Added: 0015038
2013-05-08 00:48 Goober5000 Note Added: 0015040
2013-05-08 00:50 Goober5000 Changeset attached => fs2open trunk r9675
2013-05-08 00:50 Goober5000 Note Added: 0015041
2013-05-08 00:50 Goober5000 Status code review => resolved
2013-05-08 00:50 Goober5000 Resolution open => fixed