View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002667 | FSSCP | SEXPs | public | 2012-06-17 19:44 | 2012-07-02 01:25 |
| Reporter | Yarn | Assigned To | Goober5000 | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Platform | x64 | OS | Windows 7 | ||
| Product Version | 3.6.13 | ||||
| Target Version | 3.6.14 | Fixed in Version | 3.6.14 | ||
| Summary | 0002667: Repeating chained events always repeat | ||||
| Description | In retail FS2 (as well as FS1), repeating chained events don't repeat if the next event of the chain has been triggered. (Note that this does not apply to the first event of a chain, since it is not a chained event.) However, r8603 (which resolved Mantis 0000082) changed this behavior so that such events always repeat, regardless of the status of the next event of the chain. This change potentially breaks missions that rely on retail behavior. One minor example of this is found in the first FS1 training mission. At one point in the mission, the player is supposed to match speed with the instructor. If he doesn't do so, the instructor will say "Match my speed now" and return to the first waypoint. However, because of the change made by r8603, this happens even if the player is matching speed. | ||||
| Steps To Reproduce | The attached mission lets you test this. At 0:02, a message will display. That message should not repeat at 0:15. If it does, that indicates incorrect behavior. | ||||
| Additional Information | This bug is also present in 3.6.14 RC6 (can someone please add that to the Product Version list?). | ||||
| Tags | No tags attached. | ||||
| related to | 0000082 | resolved | Goober5000 | Repeat count sexp does not work reliably |
|
|
|
|
|
It looks like Karajorma's r8603 did a little bit more than fix the bug; it also removed part of the chaining mechanism. The attached patch restores it without reverting the timestamp fix. See if that works. |
|
|
chained.patch (480 bytes)
Index: code/mission/missiongoals.cpp
===================================================================
--- code/mission/missiongoals.cpp (revision 8898)
+++ code/mission/missiongoals.cpp (working copy)
@@ -896,6 +896,10 @@
sindex = -1; // bypass evaluation
}
}
+
+ if ((event < Num_mission_events - 1) && Mission_events[event + 1].result && (Mission_events[event + 1].chain_delay >= 0)){
+ sindex = -1; // bypass evaluation
+ }
}
if (sindex >= 0) {
|
|
|
That's not a good fix at all. This needs a mod.tbl setting that means repeating events can actually repeat the way the FREDder requested them to, same as I did when I fixed arguments to loop in the way most FREDders expect them to. Having events not work properly simply because they didn't work properly in retail is a bad idea. We should give the user the decision about whether or not they want to support the old broken code, not foist the choice on them. Oh and I'm highly suspicious about that patch too. I removed that part of the chaining code for when you have 3 events in a row which are chained. |
|
|
This patch actually fixes the repeat count the way Volition originally intended it, and I've confirmed it in a test. It turns out that none of us, including the SCP coders, really understood the chaining mechanism until Yarn pointed out the behavior of Training Mission 1. The chaining mechanism is something like having a "current event pointer" that always allows a *single event* in the event chain to repeat, but no others. If the next event in the chain becomes true, then *that event* will be allowed to repeat, and the first event will quit repeating. If any future events then become true, then they will be allowed to repeat, and so on. In this case the mechanism really is like a "chain", because it joins all the events into one conceptual group. To see an example of the correct usage of a chained repeat ("correct" in the sense that Volition intended, anyway) take a look at the "Match Speed" event in btm-01 in FS1 or the FSPort. This requires learning about the rare feature of training-msg which allows a secondary message to be played if the event repeats. The event is set to repeat two times, playing both training messages, but it is intended for the second message to *not* play if the player successfully matches speed. This is accomplished by having the repeating event stop repeating if the subsequent event in the chain becomes true. |
|
|
In fact, and in light of this, we now need to re-evaluate whether it's actually correct to swap "timestamp" with "satisfied_time" in your fix for 0000082. This would depend on a precise understanding of the difference between those two fields. |
|
|
Ugh. That's a pretty horrible implementation of the way most people will expect chains to work. Like I said, better to add a mod.tbl setting and let people choose the version they want. |
|
|
It's an eminently reasonable way for chains to work. Just because it doesn't meet your expectations, or CP5670's expectations, doesn't mean it's "horrible". The problem is not that the code is wrong; the problem is that nobody realized what the intended behavior was supposed to be. That said, a mod.tbl setting is a reasonable way to control the behavior of chains. But the default will have to be Volition's old code because existing missions rely on that behavior. |
|
|
That's what I've said from the start. |
|
|
You certainly do a very good job of arguing the opposite of what you want as the default, then. :p |
|
|
Check my first post on the subject. I flat out said we need a mod.tbl setting to allow the V version to be the default. Anyway, I should take this one over. |
|
|
Fix committed to trunk@8901. |
|
|
Fix committed to fs2_open_3_6_14@8959. |
|
fs2open: trunk r8901 2012-06-23 06:31 Ported: N/A Details Diff |
restore retail behavior for chaining by reverting r8603 -- this fixes Mantis 0002667 also add descriptive comment in header file |
Affected Issues 0002667 |
|
| mod - /trunk/fs2_open/code/mission/missiongoals.h | Diff File | ||
| mod - /trunk/fs2_open/code/mission/missiongoals.cpp | Diff File | ||
|
fs2open: fs2_open_3_6_14 r8959 2012-07-01 21:26 Ported: N/A Details Diff |
Backport: Trunk r8901; restore retail behavior for chaining by reverting r8603 -- this fixes Mantis 0002667 also add descriptive comment in header file |
Affected Issues 0002667 |
|
| mod - /branches/fs2_open_3_6_14/code/mission/missiongoals.cpp | Diff File | ||
| mod - /branches/fs2_open_3_6_14/code/mission/missiongoals.h | Diff File | ||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2012-06-17 19:44 | Yarn | New Issue | |
| 2012-06-17 19:44 | Yarn | File Added: ChainedEventTest.fs2 | |
| 2012-06-18 05:24 | Goober5000 | Relationship added | related to 0000082 |
| 2012-06-18 05:31 | Goober5000 | Note Added: 0013671 | |
| 2012-06-18 05:31 | Goober5000 | Assigned To | => Goober5000 |
| 2012-06-18 05:31 | Goober5000 | Status | new => acknowledged |
| 2012-06-18 05:31 | Goober5000 | Target Version | => 3.6.14 |
| 2012-06-18 05:31 | Goober5000 | File Added: chained.patch | |
| 2012-06-18 05:32 | Goober5000 | Note Edited: 0013671 | |
| 2012-06-18 05:44 | karajorma | Note Added: 0013672 | |
| 2012-06-18 05:44 | karajorma | Note Edited: 0013672 | |
| 2012-06-18 05:46 | karajorma | Note Edited: 0013672 | |
| 2012-06-18 06:13 | Goober5000 | Note Added: 0013673 | |
| 2012-06-18 06:15 | Goober5000 | Note Added: 0013674 | |
| 2012-06-18 06:32 | karajorma | Note Added: 0013675 | |
| 2012-06-19 05:13 | Goober5000 | Note Added: 0013676 | |
| 2012-06-20 00:57 | karajorma | Note Added: 0013679 | |
| 2012-06-20 03:47 | Goober5000 | Note Added: 0013681 | |
| 2012-06-21 14:03 | karajorma | Note Added: 0013691 | |
| 2012-06-21 14:05 | karajorma | Assigned To | Goober5000 => karajorma |
| 2012-06-21 14:05 | karajorma | Status | acknowledged => assigned |
| 2012-06-21 14:06 | karajorma | Note Edited: 0013691 | |
| 2012-06-23 10:31 | Goober5000 | Changeset attached | => fs2open trunk r8901 |
| 2012-06-23 10:31 | Goober5000 | Note Added: 0013709 | |
| 2012-06-23 10:31 | Goober5000 | Status | assigned => resolved |
| 2012-06-23 10:31 | Goober5000 | Resolution | open => fixed |
| 2012-06-23 10:32 | Goober5000 | Assigned To | karajorma => Goober5000 |
| 2012-06-23 10:32 | Goober5000 | Fixed in Version | => 3.6.14 |
| 2012-07-02 01:25 | Zacam | Changeset attached | => fs2open fs2_open_3_6_14 r8959 |
| 2012-07-02 01:25 | Zacam | Note Added: 0013770 |
