View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002856 | FSSCP | SEXPs | public | 2013-04-29 04:37 | 2013-05-08 00:50 |
| Reporter | Yarn | Assigned To | Goober5000 | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Platform | x64 | OS | Windows 7 | ||
| Product Version | 3.6.18 | ||||
| Target Version | 3.7.0 | ||||
| Summary | 0002856: Has-docked-delay always returns false if dockee is destroyed | ||||
| Description | Just 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 Reproduce | In 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. | ||||
| Tags | No tags attached. | ||||
|
|
|
|
|
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; } /** |
|
|
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. |
|
|
Umm.. the patch changes "has-docked", but both SM3-03 & the test mission use "has-docked-delay"? |
|
|
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. |
|
|
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;
|
|
|
Seems to do as advertised on the tin, at least with the supplied test scenario. |
|
|
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. |
|
|
Woot. Committing. |
|
|
Fix committed to trunk@9675. |
| 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 |
