2020-06-04 14:39 EDT


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0000804FSSCPgameplaypublic2020-05-05 01:42
ReporterGoober5000 
Assigned ToGoober5000 
PriorityhighSeveritymajorReproducibilityalways
StatusassignedResolutionopen 
Product Version 
Target VersionFixed in Version 
Summary0000804: Need to investigate AI in aiturret.cpp
DescriptionWhen I went through the AI code, I got every file except for WMC's turret code. I still need to do that (though if WMC chipped in, I wouldn't complain ;)). The AI should behave identically to retail, and any changes should be tied to a flag in ai_profiles.
TagsNo tags attached.
Attached Files

-Relationships
+Relationships

-Notes

~0006570

taylor (administrator)

* BUMP *

Do you still need help with this? I know that the turret code already has some ai_profile checks in it, so I wasn't sure if this is just a stale reminder or what. I don't mind doing an aiturret.cpp comparison against retail code this weekend if you still need it done. I may not have a chance to get through it 100%, but I'll get through most of it.

~0006586

Goober5000 (administrator)

It's basically a stale reminder. If you wouldn't mind doing that, it would be amazingly helpful. :)

~0006588

taylor (administrator)

K, I'll start going through it later tonight.

~0006593

taylor (administrator)

I didn't find anything really bad, with only one questionable spot. Another ??? or so, but not something that should actually affect AI handling much, if at all.

The one "spot":
  turret_should_pick_new_target() has uncommented the check that will pick a new target if there currently is no target. Because of how the code works, it could decide to not have a target, then try to pick a new one, possibly giving it a greater chance at firing at something. This was done in the move from aicode.cpp to aiturret.cpp, since there it was never a change in the old aicode.cpp and was in the inital commit of aiturret.cpp. This needs to be fixed to retain retail target selection behavior, regardless of how often this would actually be an issue.

The main ??? spot (the other probably isn't even worth a mention):
  MAX_AIFFT_TURRETS was bumped to 200, from 60. I don't think that this would affects AI behavior really, but I do question the change. Despite it's name it's the max size of an array of target subsystems to possibly target. Unless we actually support 200 ship subsystems this is just wasting some memory. It also just picks out a random fraction of the list to try and validate as a target. Where it might affect behavior is that it's able to try against slightly more subsystems, with slightly more turrets, per target now. Although I can see the initial draw of this, it will eventually get to all of the subsystems so it's not like anything really gets skipped or avoided in the end.

The only other possible issue is just the differences in ai_fire_from_turret(). Nothing really looks wrong, but it's different enough from the retail function that it's a little more difficult to determine any behavior change unless the function was totally dissected into small blocks of functionality first. It may need another pair of eyes on that one function just in case, but like I said, nothing really jumped out at me when I was going over it.

~0006595

taylor (administrator)

Ok, the one "spot" is fixed in CVS, and I fixed an out-of-bounds issue too.

I'll leave the MAX_AIFFT_TURRETS thing as is until I hear from you, though I have half a mind to go ahead and change it back to the retail value.

~0006596

Goober5000 (administrator)

"This was done in the move from aicode.cpp to aiturret.cpp"
Crud. I hope this was the only thing that got changed. File renaming should happen independently of code changing. :(

As for reverting MAX_AIFFT_TURRETS: go for it. Even if there was a valid reason for the change (which from your description there doesn't appear to be) I'd favor changing it and tying the new behavior to aip* somehow.

I'll need to find some time to go through ai_fire_from_turret().

~0006615

taylor (administrator)

Ok, the MAX_AIFFT_TURRETS thing is in CVS now. I added a comment about it to in order to try and disuade future change attempts. I guess about the only thing left is ai_fire_from_turret().


I will go ahead and mention the other "???" thing I had though, and that's the flags situation. It will check that every weapon on a turret has a particular flag before considering it a beam, that it attacks captital ships, or whatever. This shouldn't affect retail behavior in any way (that I can think of), but it could allow for some misuse or strange bugs for some mods. I think that the code is needed (the retail stuff was just too basic to be of much use) but in the future we may want to add some better error checking to it all to help guard against unforseen problems.

~0006957

taylor (administrator)

* BUMP *

Any progress here?

I've got numerous changes to aiturret which need to go in soon but I don't want to do that until this review is done with, since there it's a little counterproductive adding new/changed code to the mix at this point. The new/changed code is already "behavior safe", but I don't want to cause a set-back with the review process already most of the way done.

~0006961

Goober5000 (administrator)

Oof, sorry. I'll need to go through and check, but I don't think I'll have time this weekend or next. :( I'll see what I can do.

~0009840

chief1983 (administrator)

Not sure what the difficulty on this is, but I'm marking it high in the hopes that it can be completed (or closed if it has already been done) for 3.6.10.

~0012427

The_E (administrator)

Can this be closed?

~0012435

Goober5000 (administrator)

I still want to look at it. Though it's probably fine at this point.

~0013356

Echelon9 (developer)

Seeing as it wasn't an issue before the 3.6.10 release, it likely won't be for 3.6.14 as well.

Closing.

~0013359

Goober5000 (administrator)

It's not an "issue", but I still want to review it, since any bugs that were introduced back then would likely still affect things now. There are still balance bugs we haven't detected -- witness the change in play of "A Game of TAG" between retail and FSO.

~0016991

Goober5000 (administrator)

Turns out there is at least one issue:
https://github.com/scp-fs2open/fs2open.github.com/pull/2367

There may be more, unfortunately.
+Notes

-Issue History
Date Modified Username Field Change
2006-02-13 00:28 Goober5000 New Issue
2006-02-13 00:28 Goober5000 Status new => assigned
2006-02-13 00:28 Goober5000 Assigned To => Goober5000
2006-09-09 03:04 taylor Note Added: 0006570
2006-09-09 13:00 Goober5000 Note Added: 0006586
2006-09-09 14:04 taylor Note Added: 0006588
2006-09-10 22:33 taylor Note Added: 0006593
2006-09-11 15:06 taylor Note Added: 0006595
2006-09-11 18:28 Goober5000 Note Added: 0006596
2006-09-12 22:09 taylor Note Added: 0006615
2006-10-21 00:59 taylor Note Added: 0006957
2006-10-21 06:50 Goober5000 Note Added: 0006961
2008-10-08 18:58 chief1983 Note Added: 0009840
2008-10-08 18:58 chief1983 Priority normal => high
2010-10-22 09:25 The_E Note Added: 0012427
2010-10-24 01:31 Goober5000 Note Added: 0012435
2012-02-24 04:24 Echelon9 Note Added: 0013356
2012-02-24 04:24 Echelon9 Status assigned => closed
2012-02-24 04:24 Echelon9 Resolution open => fixed
2012-02-24 21:56 Goober5000 Note Added: 0013359
2012-02-24 21:56 Goober5000 Status closed => assigned
2012-02-24 21:56 Goober5000 Resolution fixed => open
2020-05-05 01:42 Goober5000 Note Added: 0016991
+Issue History