FAForever
Fix campaign objectives being completed by AI player
Description of the proposed changes
Currently it's possible to have an AI teammate (e.g. using a custom AI mod such as M28AI). However, on some campaign missions they can be rendered impossible to complete if the AI completes them, where the objective is testing only against human players.
This updates the function used for such tests so it includes player slots 2-4 as human players in the campaign gamemode even if they are AI players.
For example, the opening supcom campaign missions have an objective to build 3 mexes. With this change, the objective registers as compelte if the AI player builds the mexes.
Testing done on the proposed changes
Tested on a replay with and without the change (to confirm with the change the replay desynced and the objective was completed).
Additional context
Thanks to Grandpa Sawyer for referencing the function (which saved me time in having to trace things back).
per the discord discussion: https://discord.com/channels/944679492293636116/944697747376853003/1266076712605585438 I've not made changes to GetPlayerArmy since checking its usage although it is used for intel triggers it also appears to be used for placing markets and VFX, and I'm unsure if there could be issues from applying this to an AI (probably not, but I also figured intel based conditions wouldnt matter as there's shared intel and if the player dies currenlty the co-op mission ends such that it shouldn't matter too much).
@maudlin27 could you quickly add an example of the contents of strArmy into the additional context just so the reader knows what they contain as an example.
I do not fully understand the context of the change. Wouldn't it be better in general to simplify the check to just verifying that it is a valid player slot?
@speed2CZ do you understand the context of this function?
The code should work. Coop missions use "HumanPlayer" as param to objectives, which gets translated to armies. So far it was checking if the army has Human flag, which isnt true if you play with AI. Coop player armies are always named "PlayerX" so this would correctly recognize the slot even if its filled with AI
@maudlin27 can you format the snippet by starting it with - (#6369)? We'll soon document that properly 😃 , after that you have my approval
Formatting the snippet is a minor change I think we can do ourselves as repository maintainers @Garanas.
I do not agree. You're right that this is a trivial change that does not take much time to apply for a maintainer. But that argument can also be applied in the other direction: it also does not take the contributor much time to process this.
In my point of view as a maintainer one should try to just review and not write code in the pull requests of contributors. Sure, you can do that. But it is important that the contributor learns the conventions. Not that the maintainer applies them over and over again.
There is a benefit with contributors doing such minor things so that they can learn the conventions better, but at a point a PR can become quite old for the amount of effort required to finish it. I think giving contributors time until the next milestone is a reasonable amount of time before maintainers should step in.
Thanks, fwiw I was unaware there was an issue with this as I don't receive alerts for github to a place that I check regularly and I'd long since forgotten about the fix (only just noticed it now after the activity last week)