2018-12-14 20:37 EST


View Issue Details Jump to Notes ] Related Changesets ]
IDProjectCategoryView StatusLast Update
0002657FSSCPHUDpublic2012-07-01 21:24
ReporterMjnMixael 
Assigned ToCommanderDJ 
PrioritylowSeverityminorReproducibilityalways
StatusresolvedResolutionfixed 
Product Version3.7.2 
Target VersionFixed in Version 
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.
Attached Files
  • ? file icon EscortTest.fs2 (6,973 bytes) 2012-05-23 11:52
  • patch file icon mantis_2657.patch (1,750 bytes) 2012-06-21 04:08 -
    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:
    
    patch file icon mantis_2657.patch (1,750 bytes) 2012-06-21 04:08 +

-Relationships
+Relationships

-Notes

~0013622

iss_mneur (developer)

I can confirm this bug.

~0013641

CommanderDJ (developer)

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?

~0013642

MjnMixael (manager)

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.

~0013643

niffiwan (developer)

Last edited: 2012-06-08 19:16

View 2 revisions

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

~0013644

MjnMixael (manager)

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.

~0013645

Goober5000 (administrator)

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.

~0013646

CommanderDJ (developer)

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.

~0013647

CommanderDJ (developer)

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.

~0013648

Goober5000 (administrator)

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).

~0013650

CommanderDJ (developer)

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.

~0013651

Goober5000 (administrator)

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.

~0013665

CommanderDJ (developer)

Last edited: 2012-06-12 07:50

View 2 revisions

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

~0013666

Goober5000 (administrator)

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.

~0013685

CommanderDJ (developer)

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

~0013686

Goober5000 (administrator)

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

~0013688

CommanderDJ (developer)

Fixed.

~0013689

MjnMixael (manager)

Works on my end.

~0013701

The_E (administrator)

Fix committed to trunk@8899.

~0013768

Zacam (administrator)

Fix committed to fs2_open_3_6_14@8957.
+Notes

+Related Changesets

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