View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0000804 | FSSCP | gameplay | public | 2006-02-13 05:28 | 2020-12-06 15:48 |
Reporter | Goober5000 | Assigned To | Goober5000 | ||
Priority | high | Severity | major | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Summary | 0000804: Need to investigate AI in aiturret.cpp | ||||
Description | When 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. | ||||
Tags | No tags attached. | ||||
|
* 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. |
|
It's basically a stale reminder. If you wouldn't mind doing that, it would be amazingly helpful. :) |
|
K, I'll start going through it later tonight. |
|
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. |
|
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. |
|
"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(). |
|
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. |
|
* 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. |
|
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. |
|
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. |
|
Can this be closed? |
|
I still want to look at it. Though it's probably fine at this point. |
|
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. |
|
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. |
|
Turns out there is at least one issue: https://github.com/scp-fs2open/fs2open.github.com/pull/2367 There may be more, unfortunately. |
|
Additional PR posted with other issues. https://github.com/scp-fs2open/fs2open.github.com/pull/2959 |
|
With the merging of the PR, this can finally be marked resolved. |
Date Modified | Username | Field | Change |
---|---|---|---|
2006-02-13 05:28 | Goober5000 | New Issue | |
2006-02-13 05:28 | Goober5000 | Status | new => assigned |
2006-02-13 05:28 | Goober5000 | Assigned To | => Goober5000 |
2006-09-09 07:04 | taylor | Note Added: 0006570 | |
2006-09-09 17:00 | Goober5000 | Note Added: 0006586 | |
2006-09-09 18:04 | taylor | Note Added: 0006588 | |
2006-09-11 02:33 | taylor | Note Added: 0006593 | |
2006-09-11 19:06 | taylor | Note Added: 0006595 | |
2006-09-11 22:28 | Goober5000 | Note Added: 0006596 | |
2006-09-13 02:09 | taylor | Note Added: 0006615 | |
2006-10-21 04:59 | taylor | Note Added: 0006957 | |
2006-10-21 10:50 | Goober5000 | Note Added: 0006961 | |
2008-10-08 22:58 | chief1983 | Note Added: 0009840 | |
2008-10-08 22:58 | chief1983 | Priority | normal => high |
2010-10-22 13:25 | The_E | Note Added: 0012427 | |
2010-10-24 05:31 | Goober5000 | Note Added: 0012435 | |
2012-02-24 09:24 | Echelon9 | Note Added: 0013356 | |
2012-02-24 09:24 | Echelon9 | Status | assigned => closed |
2012-02-24 09:24 | Echelon9 | Resolution | open => fixed |
2012-02-25 02:56 | Goober5000 | Note Added: 0013359 | |
2012-02-25 02:56 | Goober5000 | Status | closed => assigned |
2012-02-25 02:56 | Goober5000 | Resolution | fixed => open |
2020-05-05 05:42 | Goober5000 | Note Added: 0016991 | |
2020-12-06 15:48 | Goober5000 | Note Added: 0017043 | |
2020-12-06 15:48 | Goober5000 | Status | assigned => resolved |
2020-12-06 15:48 | Goober5000 | Resolution | open => fixed |
2020-12-06 15:48 | Goober5000 | Note Added: 0017044 |