View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002657 | FSSCP | HUD | public | 2012-05-23 15:52 | 2012-07-02 01:24 |
| Reporter | MjnMixael | Assigned To | CommanderDJ | ||
| Priority | low | Severity | minor | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.7.2 | ||||
| Summary | 0002657: Alt-Shift-E doesn't actually clear the escort list. | ||||
| Description | Alt-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 Reproduce | Play 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. | ||||
| Tags | No tags attached. | ||||
|
|
|
|
|
I can confirm this bug. |
|
|
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? |
|
|
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. |
|
|
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 |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
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. |
|
|
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). |
|
|
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. |
|
|
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. |
|
|
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 |
|
|
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. |
|
|
Solved the problem with Goober's help. New patch uploaded. Mjn, can haz test pl0x? |
|
|
I would actually default the flag to false, since the original behavior is what happens on level load. |
|
|
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:
|
|
|
Fixed. |
|
|
Works on my end. |
|
|
Fix committed to trunk@8899. |
|
|
Fix committed to fs2_open_3_6_14@8957. |
|
fs2open: trunk r8899 2012-06-22 11:55 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 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 | ||
| 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 |
