View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001982 | FSSCP | gameplay | public | 2009-08-23 00:08 | 2014-06-30 15:47 |
| Reporter | FUBAR-BDHR | Assigned To | Sushi_CW | ||
| Priority | normal | Severity | feature | Reproducibility | always |
| Status | assigned | Resolution | reopened | ||
| Product Version | 3.6.11 | ||||
| Fixed in Version | 3.6.11 | ||||
| Summary | 0001982: Ships with no afterburners call afterburner_start and afterburner_stop | ||||
| Description | Found this the other night and was discussing it with Wanderer. Adding it here so it doesn't get forgotten about. Basically saw a bunch of warnings about ships not having afterburners when I ran into this crash: http://scp.indiegames.us/mantis/view.php?id=1980 so I checked to see what was causing them. Basically there is no check for ships actually having afterburners before afterburner_start is called and a time delay is set in several functions. This results in afterburner_stop being called which throws the warning. It probably wasn't much of an issue in the past but with the new glide features afterburners are used by the AI a lot more. I believe this is called in 14 places that would need updated. There is also a reference to it in the multiplayer packet code but that should never be hit if checked in the first place. | ||||
| Additional Information | 3.6.11 r5524. I don't have the exact line number but it's easy to find. Just do a search for "does not have afterburner capability". | ||||
| Tags | No tags attached. | ||||
|
|
Wouldn't it be smarter to just have Afterburner_start do a quick exit if the ship doesn't actually have an afterburner? :D |
|
|
That's what it does now but the routines that call it set a timer which causes the call of afterburner_stop. Basically they are all kind of like this: if ((dist_to_enemy < 400.0f) && ai_maybe_fire_afterburner(Pl_objp, aip)) { afterburners_start(Pl_objp); aip->afterburner_stop_time = Missiontime + 3*F1_0; } A check in ai_maybe_fire_afterburner would be another idea but not all of the routines call that either. |
|
|
I've changed things a little so that ai_maybe_fire_afterburner only evaluates to true if the ship actually has afterburners in the first place. See if that fixes anything. :) |
|
|
There are a number of places that use afterburner without ai_maybe_fire_afterburner, so it's not a comprehensive fix. afterburners_start silently bails if it is called on a ship with no afterburner. afterburners_stop is the one that complains, then bails (without breaking anything) To be honest, I'd just as soon remove the warning from afterburners_stop. I don't think it serves any useful purpose at all, and IMHO it makes more sense for the "doesn't have afterburners" case to be entirely encapsulated in afterburners_start and afterburners_stop. So that's my suggestion: just remove the useless warning. If/when we do remove the warning, however, we should remove the check that Kara put in ai_maybe_fire_afterburner just to keep things clean. |
|
|
Well if the timer for afterburner_stop wasn't set in the first place then it wouldn't get called. Isn't calling 2 routines unnecessarily extra overhead to begin with? Also are any of the places where the AI attempt to use afterburners being done instead of another maneuver (slide for instance) thus making ships without afterburners "dumber" then those with them? |
|
|
karajorma told me to fix this bug. I would like to propose a fix. (BTW: kara, now you can assign this bug to me, if you want.) An AI could have different strategies when the controlled ship has afterburners or not. I think an IA should never try (and fail) to use afterburners on a ship which is not equipped because it encourages the IA to pilot every ship the same way. That's why I propose to continue the karmajora correction by adding ai_maybe_fire_afterburner call every time it's necessary. Here is my proposal: 1- afterburners_start modified bails with a warning message "Ship type %s does not have afterburners". The check is moved before the snd_play statement. The warning will help to detect earlier, abusive call to afterburner_start. Note that when a player start the afterburner on an unequipped ship,afterburners_start is never called, so there will be no warning logged. 2- afterburners_stop modified to bails with a warning message "Ship type %s does not have afterburners" according to Sushi_CW comment. 3- IA never start afterburner when ai_maybe_fire_afterburner return 0. Theses modifications causes IA to use less the afterburners. For instance ia of class 0 never use the afterburner. 3.1 - evade_ship() modified to check ai_maybe_fire_afterburner 3.2 - ai_big_strafe_glide_attack() modified to check ai_maybe_fire_afterburner (I have an svn patch available, if needed.) I don't know how to be sure the change in IA behavior doesn't introduce new bugs. What do you think about it ? I addition, I don't know how to reproduce the incident on my computer in order to check the correction. I definitely need help on that point. Thanks for your help. |
|
2009-11-10 22:57
|
0001982 .patch (3,489 bytes)
Index: code/ai/aibig.cpp
===================================================================
--- code/ai/aibig.cpp (revision 5643)
+++ code/ai/aibig.cpp (working copy)
@@ -1417,7 +1417,7 @@
if (aip->submode_parm1 == 1) {
accelerate_ship(aip, 1.0f);
//Use afterburners if we have them and are pointed the right way
- if (dot_to_goal > 0.99f) {
+ if (dot_to_goal > 0.99f && ai_maybe_fire_afterburner(Pl_objp, aip)) {
afterburners_start(Pl_objp);
aip->afterburner_stop_time = Missiontime + 3*F1_0;
}
Index: code/ai/aicode.cpp
===================================================================
--- code/ai/aicode.cpp (revision 5643)
+++ code/ai/aicode.cpp (working copy)
@@ -5057,15 +5057,17 @@
} else
accelerate_ship(aip, (float) (Game_skill_level+2) / (NUM_SKILL_LEVELS+1));
- if ((Missiontime - aip->submode_start_time > F1_0/2) && (sip->afterburner_fuel_capacity > 0.0f)) {
- float percent_left = 100.0f * shipp->afterburner_fuel / sip->afterburner_fuel_capacity;
- if (percent_left > 30.0f + ((Pl_objp-Objects) & 0x0f)) {
- afterburners_start(Pl_objp);
+ if (ai_maybe_fire_afterburner(Pl_objp, aip)){
+ if ((Missiontime - aip->submode_start_time > F1_0/2) && (sip->afterburner_fuel_capacity > 0.0f)) {
+ float percent_left = 100.0f * shipp->afterburner_fuel / sip->afterburner_fuel_capacity;
+ if (percent_left > 30.0f + ((Pl_objp-Objects) & 0x0f)) {
+ afterburners_start(Pl_objp);
- if (aip->ai_profile_flags & AIPF_SMART_AFTERBURNER_MANAGEMENT) {
- aip->afterburner_stop_time = (fix) (Missiontime + F1_0 + static_randf(Pl_objp-Objects) * F1_0 / 4);
- } else {
- aip->afterburner_stop_time = Missiontime + F1_0 + static_rand(Pl_objp-Objects)/4;
+ if (aip->ai_profile_flags & AIPF_SMART_AFTERBURNER_MANAGEMENT) {
+ aip->afterburner_stop_time = (fix) (Missiontime + F1_0 + static_randf(Pl_objp-Objects) * F1_0 / 4);
+ } else {
+ aip->afterburner_stop_time = Missiontime + F1_0 + static_rand(Pl_objp-Objects)/4;
+ }
}
}
}
@@ -13788,7 +13790,7 @@
else {
accelerate_ship(aip, 1.0f + dot_to_goal);
if (dot_to_goal > 0.2f) {
- if (!(objp->phys_info.flags & PF_AFTERBURNER_ON )) {
+ if (ai_maybe_fire_afterburner(Pl_objp, aip) && !(objp->phys_info.flags & PF_AFTERBURNER_ON )) {
afterburners_start(objp);
aip->afterburner_stop_time = Missiontime + 2*F1_0;
}
Index: code/ship/afterburner.cpp
===================================================================
--- code/ship/afterburner.cpp (revision 5643)
+++ code/ship/afterburner.cpp (working copy)
@@ -87,6 +87,13 @@
Assert( shipp->ship_info_index >= 0 && shipp->ship_info_index < Num_ship_classes );
sip = &Ship_info[shipp->ship_info_index];
+ // When the ship has no afterburner, leave without doing anything.
+ // Caller should have tested ship equipment before deciding to start afterburners.
+ if ( !(sip->flags & SIF_AFTERBURNER) ) {
+ nprintf(("Warning","Ship type %s does not have afterburner capability\n", sip->name));
+ return;
+ }
+
// bail if afterburners are locked
if (shipp->flags2 & SF2_AFTERBURNER_LOCKED) {
return;
@@ -113,11 +120,7 @@
//boosters take precedence
if (objp->phys_info.flags & PF_BOOSTER_ON)
return;
-
- if ( !(sip->flags & SIF_AFTERBURNER) ) {
- return;
- }
-
+
// Check if there is enough afterburner fuel
if ( (shipp->afterburner_fuel < MIN_AFTERBURNER_FUEL_TO_ENGAGE) && !MULTIPLAYER_CLIENT ) {
if ( objp == Player_obj ) {
|
|
|
Patch uploaded. Waiting for approval. |
|
|
Looks fine to me. Committed. |
|
|
Aw, CRAP. Sushi drops the ball again. This patch has gameplay-changing effects. It makes it so that places where afterburner was once fired for ALL ships now depend on the AI class (ai_maybe_fire_afterburner returns different values for different AI classes). This means that after this patch, AI ships will sometimes NOT use afterburner where they always did before, specificially when evading collisions, and avoiding shockwaves. (Glide strafe is affected too, but since it isn't an official feature yet, it's not as big of a deal.) Totally mea culpa for not looking closer at this sooner. I'm sorry, Pedro. You posted your patch well in advance and I had plenty of time to make sure it wouldn't break anything, and I utterly failed to do so. :( P.S. I still think that this warning is basically useless and would be better off removed completely. |
|
|
No problem. I understand well that changing IA behavior would cause unpredictable bug in the game. I suppose the patch should be rollbacked. Should I provide a "rollback" patch plus a warning removal ? |
|
|
Please do them independently of each other if you do, first the rollback and then warning removal if it's agreed upon. |
|
|
Warning removal doesn't really solve the problem though. You have AI ships that believe they have fired their afterburners when they haven't. So you may have situations like following: Missile fired at ship Ship decides to use afterburners to evade Ship doesn't have afterburners but there is no check for that Timer is set no evasion occurs Ship takes direct hit by missile Now if the ship is checked for afterburners before the call the ship may then skip that routine and decide to use side thrust instead. Of course all this is theoretical and I really don't know how that would or play out. If it is possible you are ending up with extremely dumb AI just because they don't have afterburners. |
|
|
True, but the warnings don't prevent those situations: they just announce the problem. Presumably to inform the coder that they are starting up afterburners without first verifying that the ship has afterburners. But what if, in the code, you want the ship to do what it's doing anyway, and just add afterburner if they're available? This is the case in most places, at least, and I don't see the point of adding explicit checks for the existence of afterburners on the ship when the calling code doesn't really care whether they have them or not. If the calling code needs to do something different for ships without afterburners, it needs to make that check and branch itself (which is already the case both before and after this patch). Whether or not we keep the warnings, though, calling ai_maybe_fire_afterburner to make sure they're invoked is the wrong approach, since ai_maybe_fire_afterburner has other effects as well. I suppose you could have a new function that is in charge of checking for the existence of afterburners before firing them (and administering a firm warning if they aren't there), but I still don't really see the point. :) |
|
|
Well if you're certain that situations like FUBAR described can't actually occur, then there is no point, otherwise you're not listening to him :) |
|
|
Patch reverted in trunk rev 6083, since it has side effects that change AI behavior. The underlying issue should probably still be resolved. |
|
|
Is the issue in fact resolved? If it is, we should resolve the ticket. |
|
|
I don't have a working test case but when looking at the code the issue should still occur. I'll see if I can create a patch that keeps the warning but prevents calling afterburners_stop when there is no afterburner (simple check for SIF_AFTERBURNER or PF_AFTERBURNER_ON flag). PF_AFTERBURNER_ON can only get true when SIF_AFTERBURNER is true so both flags are equal for this check. I would keep the warning cause it hints that a specific case may have been overlooked in AI code that somehow leads to an afterburner_stop where it shouldn't. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2009-08-23 00:08 | FUBAR-BDHR | New Issue | |
| 2009-08-26 05:52 | karajorma | Note Added: 0011160 | |
| 2009-08-26 07:05 | FUBAR-BDHR | Note Added: 0011162 | |
| 2009-08-28 00:00 | karajorma | Status | new => assigned |
| 2009-08-28 00:00 | karajorma | Assigned To | => karajorma |
| 2009-08-28 00:05 | karajorma | Note Added: 0011166 | |
| 2009-09-28 22:30 | Sushi_CW | Note Added: 0011199 | |
| 2009-09-28 23:39 | FUBAR-BDHR | Note Added: 0011200 | |
| 2009-11-09 13:04 | pedro | Note Added: 0011228 | |
| 2009-11-09 13:04 | pedro | Note Edited: 0011228 | |
| 2009-11-09 13:06 | pedro | Note Edited: 0011228 | |
| 2009-11-10 22:57 | pedro | File Added: 0001982 .patch | |
| 2009-11-10 22:57 | pedro | Note Added: 0011241 | |
| 2009-11-14 09:51 | karajorma | Note Added: 0011277 | |
| 2009-11-14 09:51 | karajorma | Assigned To | karajorma => |
| 2009-11-14 09:52 | karajorma | Status | assigned => resolved |
| 2009-11-14 09:52 | karajorma | Fixed in Version | => 3.6.11 |
| 2009-11-14 09:52 | karajorma | Resolution | open => fixed |
| 2009-11-14 09:52 | karajorma | Assigned To | => karajorma |
| 2009-11-14 15:22 | Sushi_CW | Note Added: 0011282 | |
| 2009-11-14 15:22 | Sushi_CW | Status | resolved => feedback |
| 2009-11-14 15:22 | Sushi_CW | Resolution | fixed => reopened |
| 2009-11-16 09:42 | pedro | Note Added: 0011288 | |
| 2009-11-16 15:56 | chief1983 | Note Added: 0011289 | |
| 2009-11-16 22:05 | FUBAR-BDHR | Note Added: 0011290 | |
| 2009-11-17 23:10 | Sushi_CW | Note Added: 0011296 | |
| 2009-11-18 02:00 | chief1983 | Note Added: 0011298 | |
| 2010-04-20 12:39 | karajorma | Status | feedback => assigned |
| 2010-04-20 12:39 | karajorma | Assigned To | karajorma => Sushi_CW |
| 2010-05-01 20:38 | Sushi_CW | Note Added: 0011920 | |
| 2011-06-24 04:50 | Zacam | Category | --------- => gameplay |
| 2014-06-30 03:21 | Goober5000 | Note Added: 0015953 | |
| 2014-06-30 10:03 | Admiral MS | Note Added: 0015962 | |
| 2014-06-30 15:47 | Admiral MS | Note Edited: 0015962 |
