FAForever / fa

Lua code for FAF
226 stars 232 forks source link

Update campaign AI functionalities #6134

Closed Dhomie closed 4 months ago

Dhomie commented 5 months ago

Description of the proposed changes

The primary intent of this PR is to move from the thread-based method of processing units for the current campaign BaseManager to a callback-based one. The secondary intent was to do some cleanups, fixes (TMLs ignoring T2 units), code optimizations, and where I could, commenting, and annotations.

This affects some build condition files used by skirmish AIs, so @relent0r if you could review those, and tell me if anything conflicts, or the changes are wrong, let me know. If anyone else's reviewing this, and got feedback, I'm all ears.

Testing done on the proposed changes

I've done plenty of testing to ensure this works as intended, since all the changes can be found in a mod in the vault: Coop AI Adjustments You can test these changes either via the mod, or the dev enviroment.

GitHub link: https://github.com/Dhomie/FAF-Campaign-Experiments

Additional context

This is going to need some context, so bear with me please.

First of all, Engineer platoons, and even structures now use several unit callbacks, to assign unit names, upgrades, etc. instead of checking every X tick if a unit's state has changed. This issue was most noticable with the current upgrade thread. An example how to break it: Put 152475213675 Hives next to a T1 Land Factory that should upgrade all the way to T3, and it'll remain stuck at T2, because the structure upgrade finishes before the upgrading thread can register that a structure upgrade is being built. The same can happen to just Engineers building something, if it finishes before the currently assigned game ticks, the BaseManager won't register it.

The callbacks I've made use of are: OnStartBuild, OnStopBeingBuilt, OnFailedToBuild. In case an upgrade order somehow doesn't get processed, I've added a fallback thread for the BaseManager that will re-order the upgrade. Due to the reduction of overall checks thanks to the above, I've reduced game ticks needed to wait for certain tasks.

While doing all this, I've went over some campaign specific files, cleaned unnecessary stuff up, and added comments/explanations where possible.

Checklist

relent0r commented 5 months ago

At first glance I have no issues with the condition changes, looks like they are all just logic and performance improvements. We have debated some of this stuff before as it makes troubleshooting a little harder.

I'll need to take a longer look at the other changes as some of the stuff was quite interesting. I'd never considered doing adhoc unitbuilt callbacks for upgrading factories. A word of warning based on my own recent experiences with callbacks that I got caught out by. It may not be relevant for campaign AI but if an engineer gets a build command then for some reason it has issueclearcommands sent the OnFailedToBuild callback will fire. That particular one caused(causes) a bunch of problems with the default skirmish AI due to how it adds things to the engineerbuildqueue then later on clears the commands.

Dhomie commented 5 months ago

OnFailedToBuild does exactly what is expected of it in this case, lemme explain.

The main idea to move to build callbacks was to cache unit names on the Engineer (the BaseManager uses single-unit Engineer/ACU/sACU platoons for its bases). All the structures, and special units (ConditionalBuilds) are in the save.lua file with all the necessary data, like unit IDs, and most importantly their names, ie. 'UNIT_126' unless given a custom name, it's the naming method of the map editors.

So, when the Engineer finds something to build, AND the build order has been issued successfully, we cache that something's name on it, which is either a structure part of the base, or a ConditionalBuild. If the Engineer begins building, and this cached name is available on the Engineer, ONLY then we assign all the relevant data on the unit being built, which EngineerOnStartBuild() handles. There shouldn't be a case where both a BuildingUnitName and ConditionalBuildUnitName is cached on the Engineer, it's either one or the other.

This is where OnFailedToBuild comes in, because we ONLY issue 1 build related order at a time for Engineers in the campaign enviroment. What OnFailedToBuild does in this case, is that if for whatever reason (like the one you mentioned, the Engineer getting its orders cancelled) the build failed before it could even be started, all we do is set BOTH cached name types on the Engineer to nil, so it can either retry, or build something else.

relent0r commented 5 months ago

@Dhomie I'm not seeing any big issues here. The callbacks are the only major deviation, the rest looks like small improvements and some great documentation. There are some elements of consistency where it seems like you had one view on things when you started and a different one by the time you finished.(like converting WaitSeconds to WaitTicks or localizing variables vs table lookups).

Your callback explanation makes sense. My views on these are from the skirmish AI's perspective where callbacks can fire for multiple reasons and require different resolution logic depending on the engineers status and location. I've been going through an interesting process of creating a statemachine for AI engineers which is where some of these recent learnings have come from.

The skirmish AI uses a catchall function called processbuildcommand that most of these callbacks go to, its been modified quite a few times over the years and became a little detached from what the callbacks were trying to achieve so causes transient failures that are hard to track down.

Dhomie commented 5 months ago

I've reverted some of the unnecessary/wrong changes.

Engineer logic for the campaign enviroment is designed to be simple, and usually is linear in its task. It's usually 1 type of task at a time that is expected from them, and we don't do anything too fancy, we build special platoons if something fancy is needed (ie. if reclaiming is needed, a set of patrol orders will suffice, or just an attack-move).

Structure upgrading logic still resembles the one GPG intended, which means upgrading as soon as possible, no need to check for eco values and such. An AIs economy is mostly set up by the mission's maker, alternatively we simply give the right enviroment for their eco to start out (ie. give some starting Engineers to patrol wreck areas).

@speed2CZ I'd like your review on this PR as well, since it features overall campaign related refactoring, and I'd like your opinion on what you'd approve of, and not.

relent0r commented 4 months ago

@Dhomie I've approved this PR (I ran through some play testing on mind games and it seemed good). You can either wait for speed2 or I can merge in now.

speed2CZ commented 4 months ago

I dont have time for a proper review, but the stuff I've tried worked fine, so its probably good.

Dhomie commented 4 months ago

@relent0r It's all good from my end, you can merge whenever you're ready.