View Issue Details

IDProjectCategoryView StatusLast Update
0002657FSSCPHUDpublic2012-07-02 01:24
ReporterMjnMixael Assigned ToCommanderDJ  
PrioritylowSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.7.2 
Summary0002657: Alt-Shift-E doesn't actually clear the escort list.
DescriptionAlt-Shift-E is the keypress designated to 'clear' the escort list. However, tests show that it doesn't clear it, it just hides it. Once a new ship is added to the escort list after using Alt-Shift-E, the escort list reappears with all previous ships still on it.
Steps To ReproducePlay the test mission I've included, it'll tell you what to do via training messages. Following the steps should result in the Orion Class ship being the only one in the escort list, instead all three are.
TagsNo tags attached.

Activities

MjnMixael

2012-05-23 15:52

manager  

EscortTest.fs2 (6,973 bytes)

iss_mneur

2012-06-01 02:22

developer   ~0013622

I can confirm this bug.

CommanderDJ

2012-06-08 15:16

developer   ~0013641

From stepping through the code, this seems to be... very deliberate. Measures were taken to store and restore the escort list to how it was before it was cleared... so I dunno. Does that technically make this a feature request?

Also, was this around before the new HUD code? Does anyone know how it functioned then?

MjnMixael

2012-06-08 16:19

manager   ~0013642

That's... interesting. What good is a 'clear escort list' funtion when as soon as the escort list is restored to the HUD, it still retains all previous data? I find that extremely odd.

What was the commit number for the new HUD code? I can try and make a build from before then and run it to find out.

niffiwan

2012-06-08 23:09

developer   ~0013643

Last edited: 2012-06-08 23:16

The new hud code was committed in r6833, r6830 is the last trunk commit prior to that.

edit: I made binaries for both versions here if it helps:
http://scp.indiegames.us/mantis/view.php?id=2417#c13458

MjnMixael

2012-06-09 00:57

manager   ~0013644

Thanks for that, niffiwan.

Looks like it's the same behavior pre-HUD code... I still think it's odd behavior though, because it essentially means that clear-escort-list key command doesn't do anything ever. It just temporarily hides the escort list depending on the mission and how soon another ship is added to the list via sexps.

Whether it's called a feature or a bug, I'd be interested in a fix for that.

Goober5000

2012-06-09 02:24

administrator   ~0013645

CommanderDJ, are you looking at the right place? Alt-Shift-E corresponds to the ESCORT_CLEAR function, which appears to be designed to remove all ships from the escort list. In fact, it invokes hud_escort_clear_all which is also invoked before the escort list is set up at the beginning of a mission.

I think the actual cause of the bug is that hud_escort_clear_all is missing some important housekeeping code, such as clearing the escort flags of all escorted ships. (Compare the code logic with that in hud_remove_ship_from_escort_index.) The function "accidentally" works at the beginning of the mission because the required housekeeping is redundantly performed elsewhere in the mission load sequence.

CommanderDJ

2012-06-09 03:29

developer   ~0013646

Herp derp, Goober's right. I was looking at the right place, but was doing so at 1am and so totally didn't interpret it correctly :P

Anyway, I'd still like to work on this bug if no one's claimed it yet.

CommanderDJ

2012-06-09 16:26

developer   ~0013647

Okay, so, after much flailing, I have come up perplexed. I've uploaded two patches, one good and one lame. The good one consists of the solution I'd prefer, just moving the Num_escort_ships assignment down a bit and adding a hud_remove_ship_from_escort call to an existing loop. Sadly, this one doesn't work. In Mjn's test mission, it clears the Poseidon but not the Fenris from the list.

Yet if I take out the hud_remove_ship_from_escort call and put it in its own separate loop that iterates through the exact same thing (as in the lame patch), it works. As to why this happens, I haven't been able to figure out yet. It's probably something really simple I'm missing.

Anyway, if someone could confirm these results please?

Personally, I'd rather use the good version (once we figure out why it's not working) so we don't need that extra loop, but if other coders weigh in then I'm happy to introduce that extra loop and use the lame version.

Goober5000

2012-06-09 17:29

administrator   ~0013648

You're not going in the right direction. You need to think smarter, not harder. :) I gave you a hint in parentheses in my previous comment.

Here's another hint: You can solve the problem without an extra loop, and without a call to hud_remove_ship_from_escort (or hud_remove_ship_from_escort_index for that matter).

CommanderDJ

2012-06-10 01:37

developer   ~0013650

Your hint says to compare hud_escort_clear_all with hud_remove_ship_from_escort_index. Comparison: they're nothing alike. They share no common code at all. From your comments I'd concluded that the former is missing something that the latter has. You say that I can solve the problem without calling the remove functions, but if there already exists a function which appears to perform all the necessary housekeeping to remove a ship from the list, is not the smarter option to simply call that function than copy code over from it?

All that said, I'm not challenging your advice, and I mean no disrespect. I'm just explaining my thought process behind introducing the extra calls. I'll try doing it the way you suggest. Also, if there's a flaw in my thinking above somewhere, I'd like to know where my approach went wrong. I'm still learning, after all.

On topic, I tried clearing escort flags in hud_escort_clear_all, but then that ends up clearing the flags at level load, meaning that any ships the mission designer has added to the list won't actually appear. I'll keep trying.

Goober5000

2012-06-10 05:49

administrator   ~0013651

I'm trying to help you realize the solution yourself, because that is more helpful in the long term than simply giving you the answer. :) In regards to the function comparison, yes, they are different; but you should be able to see that most of the function can be dismissed as irrelevant. Once you focus on the relevant part, you should easily see the difference.

You are actually very close to the solution, but I can show you what you missed if you still need help.

CommanderDJ

2012-06-12 11:49

developer   ~0013665

Last edited: 2012-06-12 11:50

I give up :P
I don't think it's worth me flailing about trying to find the optimal solution when you've already found it. I'll forego this little bit of learning experience. Let's just fix this already :P

Goober5000

2012-06-13 00:41

administrator   ~0013666

Now come on. You all but HAVE the solution, and this ticket isn't a constraint to 3.6.14 anyway. :p

Find me on IRC and I'll walk you through it.

CommanderDJ

2012-06-21 02:31

developer   ~0013685

Solved the problem with Goober's help. New patch uploaded. Mjn, can haz test pl0x?

Goober5000

2012-06-21 02:46

administrator   ~0013686

I would actually default the flag to false, since the original behavior is what happens on level load.

CommanderDJ

2012-06-21 08:08

developer  

mantis_2657.patch (1,750 bytes)   
Index: code/hud/hudescort.cpp
===================================================================
--- code/hud/hudescort.cpp	(revision 8898)
+++ code/hud/hudescort.cpp	(working copy)
@@ -464,12 +464,15 @@
 // ----------------------------------------------------------------------
 // hud_escort_clear_all()
 //
-void hud_escort_clear_all()
+void hud_escort_clear_all(bool clear_flags)
 {
 	int i;
 
 	Num_escort_ships = 0;
 	for ( i = 0; i < Max_escort_ships; i++ ) {
+		if(clear_flags && (Escort_ships[i].objnum >= 0) && (Objects[Escort_ships[i].objnum].type == OBJ_SHIP) && (Objects[Escort_ships[i].objnum].instance >= 0)){
+			Ships[Objects[Escort_ships[i].objnum].instance].flags &= ~SF_ESCORT;
+		}
 		Escort_ships[i].obj_signature = -99;
 		Escort_ships[i].np_id = -1;
 		shield_info_reset(&Escort_ships[i].hit_info);
Index: code/hud/hudescort.h
===================================================================
--- code/hud/hudescort.h	(revision 8898)
+++ code/hud/hudescort.h	(working copy)
@@ -26,7 +26,7 @@
 void	hud_setup_escort_list(int level = 1);
 void	hud_escort_view_toggle();
 void	hud_add_remove_ship_escort(int objnum, int supress_feedback = 0);
-void	hud_escort_clear_all();
+void	hud_escort_clear_all(bool clear_flags = false);
 void	hud_escort_ship_hit(object *objp, int quadrant);
 void	hud_escort_target_next();
 void	hud_escort_cull_list();
Index: code/io/keycontrol.cpp
===================================================================
--- code/io/keycontrol.cpp	(revision 8898)
+++ code/io/keycontrol.cpp	(working copy)
@@ -2479,7 +2479,7 @@
 
 		case ESCORT_CLEAR:
 			control_used(ESCORT_CLEAR);
-			hud_escort_clear_all();
+			hud_escort_clear_all(true);
 			break;
 
 		case TARGET_NEXT_ESCORT_SHIP:
mantis_2657.patch (1,750 bytes)   

CommanderDJ

2012-06-21 08:08

developer   ~0013688

Fixed.

MjnMixael

2012-06-21 11:22

manager   ~0013689

Works on my end.

The_E

2012-06-22 15:55

administrator   ~0013701

Fix committed to trunk@8899.

Zacam

2012-07-02 01:24

administrator   ~0013768

Fix committed to fs2_open_3_6_14@8957.

Related Changesets

fs2open: trunk r8899

2012-06-22 11:55

The_E


Ported: N/A

Details Diff
From CommanderDJ: Fix for Mantis 2657 (Alt-Shift-E does not clear escort list) Affected Issues
0002657
mod - /trunk/fs2_open/code/io/keycontrol.cpp Diff File
mod - /trunk/fs2_open/code/hud/hudescort.h Diff File
mod - /trunk/fs2_open/code/hud/hudescort.cpp Diff File

fs2open: fs2_open_3_6_14 r8957

2012-07-01 21:25

Zacam


Ported: N/A

Details Diff
Backport: Trunk r8899; From CommanderDJ: Fix for Mantis 2657 (Alt-Shift-E does not clear escort list) Affected Issues
0002657
mod - /branches/fs2_open_3_6_14/code/hud/hudescort.cpp Diff File
mod - /branches/fs2_open_3_6_14/code/hud/hudescort.h Diff File
mod - /branches/fs2_open_3_6_14/code/io/keycontrol.cpp Diff File

Issue History

Date Modified Username Field Change
2012-05-23 15:52 MjnMixael New Issue
2012-05-23 15:52 MjnMixael File Added: EscortTest.fs2
2012-06-01 02:22 iss_mneur Note Added: 0013622
2012-06-01 02:22 iss_mneur Status new => confirmed
2012-06-01 02:22 iss_mneur Category user interface => HUD
2012-06-01 02:22 iss_mneur Product Version 3.6.14 RC5 => 3.7.2
2012-06-01 02:22 iss_mneur Steps to Reproduce Updated
2012-06-08 15:16 CommanderDJ Note Added: 0013641
2012-06-08 16:19 MjnMixael Note Added: 0013642
2012-06-08 23:09 niffiwan Note Added: 0013643
2012-06-08 23:16 niffiwan Note Edited: 0013643
2012-06-09 00:57 MjnMixael Note Added: 0013644
2012-06-09 02:24 Goober5000 Note Added: 0013645
2012-06-09 03:29 CommanderDJ Note Added: 0013646
2012-06-09 03:30 CommanderDJ Assigned To => CommanderDJ
2012-06-09 03:30 CommanderDJ Status confirmed => assigned
2012-06-09 16:19 CommanderDJ File Added: mantis_2657_good.patch
2012-06-09 16:19 CommanderDJ File Added: mantis_2657_lame.patch
2012-06-09 16:26 CommanderDJ Note Added: 0013647
2012-06-09 17:29 Goober5000 Note Added: 0013648
2012-06-10 01:37 CommanderDJ Note Added: 0013650
2012-06-10 05:49 Goober5000 Note Added: 0013651
2012-06-12 11:47 CommanderDJ File Deleted: mantis_2657_good.patch
2012-06-12 11:49 CommanderDJ Note Added: 0013665
2012-06-12 11:50 CommanderDJ Note Edited: 0013665
2012-06-13 00:41 Goober5000 Note Added: 0013666
2012-06-21 02:30 CommanderDJ File Deleted: mantis_2657_lame.patch
2012-06-21 02:31 CommanderDJ File Added: mantis_2657.patch
2012-06-21 02:31 CommanderDJ Note Added: 0013685
2012-06-21 02:46 Goober5000 Note Added: 0013686
2012-06-21 08:08 CommanderDJ File Deleted: mantis_2657.patch
2012-06-21 08:08 CommanderDJ File Added: mantis_2657.patch
2012-06-21 08:08 CommanderDJ Note Added: 0013688
2012-06-21 08:18 CommanderDJ Status assigned => code review
2012-06-21 11:22 MjnMixael Note Added: 0013689
2012-06-22 15:55 The_E Changeset attached => fs2open trunk r8899
2012-06-22 15:55 The_E Note Added: 0013701
2012-06-22 15:55 The_E Status code review => resolved
2012-06-22 15:55 The_E Resolution open => fixed
2012-07-02 01:24 Zacam Changeset attached => fs2open fs2_open_3_6_14 r8957
2012-07-02 01:24 Zacam Note Added: 0013768