View Issue Details

IDProjectCategoryView StatusLast Update
0002667FSSCPSEXPspublic2012-07-02 01:25
ReporterYarn Assigned ToGoober5000  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Platformx64OSWindows 7 
Product Version3.6.13 
Target Version3.6.14Fixed in Version3.6.14 
Summary0002667: Repeating chained events always repeat
DescriptionIn 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 ReproduceThe 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 InformationThis bug is also present in 3.6.14 RC6 (can someone please add that to the Product Version list?).
TagsNo tags attached.

Relationships

related to 0000082 resolvedGoober5000 Repeat count sexp does not work reliably 

Activities

Yarn

2012-06-17 19:44

developer  

ChainedEventTest.fs2 (3,730 bytes)

Goober5000

2012-06-18 05:31

administrator   ~0013671

Last edited: 2012-06-18 05:32

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.

Goober5000

2012-06-18 05:31

administrator  

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) {
chained.patch (480 bytes)   

karajorma

2012-06-18 05:44

administrator   ~0013672

Last edited: 2012-06-18 05:46

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.

Goober5000

2012-06-18 06:13

administrator   ~0013673

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.

Goober5000

2012-06-18 06:15

administrator   ~0013674

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.

karajorma

2012-06-18 06:32

administrator   ~0013675

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.

Goober5000

2012-06-19 05:13

administrator   ~0013676

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.

karajorma

2012-06-20 00:57

administrator   ~0013679

That's what I've said from the start.

Goober5000

2012-06-20 03:47

administrator   ~0013681

You certainly do a very good job of arguing the opposite of what you want as the default, then. :p

karajorma

2012-06-21 14:03

administrator   ~0013691

Last edited: 2012-06-21 14:06

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.

Goober5000

2012-06-23 10:31

administrator   ~0013709

Fix committed to trunk@8901.

Zacam

2012-07-02 01:25

administrator   ~0013770

Fix committed to fs2_open_3_6_14@8959.

Related Changesets

fs2open: trunk r8901

2012-06-23 06:31

Goober5000


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

Zacam


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

Issue History

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