2021-03-04 12:22 EST


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0001982FSSCPgameplaypublic2014-06-30 11:47
ReporterFUBAR-BDHR 
Assigned ToSushi_CW 
PrioritynormalSeverityfeatureReproducibilityalways
StatusassignedResolutionreopened 
Product Version3.6.11 
Target VersionFixed in Version3.6.11 
Summary0001982: Ships with no afterburners call afterburner_start and afterburner_stop
DescriptionFound 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 Information3.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".
TagsNo tags attached.
Attached Files
  • patch file icon 0001982 .patch (3,489 bytes) 2009-11-10 17:57 -
    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 file icon 0001982 .patch (3,489 bytes) 2009-11-10 17:57 +

-Relationships
+Relationships

-Notes

~0011160

karajorma (administrator)

Wouldn't it be smarter to just have Afterburner_start do a quick exit if the ship doesn't actually have an afterburner? :D

~0011162

FUBAR-BDHR (developer)

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.

~0011166

karajorma (administrator)

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

~0011199

Sushi_CW (developer)

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.

~0011200

FUBAR-BDHR (developer)

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?

~0011228

pedro (reporter)

Last edited: 2009-11-09 08:06

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.

~0011241

pedro (reporter)

Patch uploaded. Waiting for approval.

~0011277

karajorma (administrator)

Looks fine to me. Committed.

~0011282

Sushi_CW (developer)

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.

~0011288

pedro (reporter)

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 ?

~0011289

chief1983 (administrator)

Please do them independently of each other if you do, first the rollback and then warning removal if it's agreed upon.

~0011290

FUBAR-BDHR (developer)

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.

~0011296

Sushi_CW (developer)

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

~0011298

chief1983 (administrator)

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

~0011920

Sushi_CW (developer)

Patch reverted in trunk rev 6083, since it has side effects that change AI behavior. The underlying issue should probably still be resolved.

~0015953

Goober5000 (administrator)

Is the issue in fact resolved? If it is, we should resolve the ticket.

~0015962

Admiral MS (developer)

Last edited: 2014-06-30 11:47

View 2 revisions

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.

+Notes

-Issue History
Date Modified Username Field Change
2009-08-22 20:08 FUBAR-BDHR New Issue
2009-08-26 01:52 karajorma Note Added: 0011160
2009-08-26 03:05 FUBAR-BDHR Note Added: 0011162
2009-08-27 20:00 karajorma Status new => assigned
2009-08-27 20:00 karajorma Assigned To => karajorma
2009-08-27 20:05 karajorma Note Added: 0011166
2009-09-28 18:30 Sushi_CW Note Added: 0011199
2009-09-28 19:39 FUBAR-BDHR Note Added: 0011200
2009-11-09 08:04 pedro Note Added: 0011228
2009-11-09 08:04 pedro Note Edited: 0011228
2009-11-09 08:06 pedro Note Edited: 0011228
2009-11-10 17:57 pedro File Added: 0001982 .patch
2009-11-10 17:57 pedro Note Added: 0011241
2009-11-14 04:51 karajorma Note Added: 0011277
2009-11-14 04:51 karajorma Assigned To karajorma =>
2009-11-14 04:52 karajorma Status assigned => resolved
2009-11-14 04:52 karajorma Fixed in Version => 3.6.11
2009-11-14 04:52 karajorma Resolution open => fixed
2009-11-14 04:52 karajorma Assigned To => karajorma
2009-11-14 10:22 Sushi_CW Note Added: 0011282
2009-11-14 10:22 Sushi_CW Status resolved => feedback
2009-11-14 10:22 Sushi_CW Resolution fixed => reopened
2009-11-16 04:42 pedro Note Added: 0011288
2009-11-16 10:56 chief1983 Note Added: 0011289
2009-11-16 17:05 FUBAR-BDHR Note Added: 0011290
2009-11-17 18:10 Sushi_CW Note Added: 0011296
2009-11-17 21:00 chief1983 Note Added: 0011298
2010-04-20 08:39 karajorma Status feedback => assigned
2010-04-20 08:39 karajorma Assigned To karajorma => Sushi_CW
2010-05-01 16:38 Sushi_CW Note Added: 0011920
2011-06-24 00:50 Zacam Category --------- => gameplay
2014-06-29 23:21 Goober5000 Note Added: 0015953
2014-06-30 06:03 Admiral MS Note Added: 0015962
2014-06-30 11:47 Admiral MS Note Edited: 0015962 View Revisions
+Issue History