AI add buying zero move units #5842
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProPurchaseUtils.java
Outdated
Show resolved
Hide resolved
game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProPurchaseUtils.java
Outdated
Show resolved
Hide resolved
| <= (purchaseOption.isConstruction() ? remainingConstructions : remainingUnitProduction); | ||
| } | ||
|
|
||
| private static boolean hasReachedMaxUnitBuiltPerPlayer( |
There was a problem hiding this comment.
Method hasReachedMaxUnitBuiltPerPlayer has a Cognitive Complexity of 11 (exceeds 5 allowed). Consider refactoring.
| return false; | ||
| } | ||
|
|
||
| private static boolean hasReachedConstructionLimits( |
There was a problem hiding this comment.
Method hasReachedConstructionLimits has a Cognitive Complexity of 26 (exceeds 5 allowed). Consider refactoring.
| return false; | ||
| } | ||
|
|
||
| private static boolean hasReachedConstructionLimits( |
There was a problem hiding this comment.
Method hasReachedConstructionLimits has 41 lines of code (exceeds 25 allowed). Consider refactoring.
Codecov Report
@@ Coverage Diff @@
## master #5842 +/- ##
============================================
+ Coverage 22.95% 23.02% +0.06%
- Complexity 6471 6550 +79
============================================
Files 1094 1099 +5
Lines 77468 78091 +623
Branches 11394 11480 +86
============================================
+ Hits 17782 17979 +197
- Misses 57497 57917 +420
- Partials 2189 2195 +6
Continue to review full report at Codecov.
|
|
Looks like there are a few codeclimate issues around the length/complexity of some of the helper methods for validating unit production. These are already refactored from the original method. I'm not sure whether its worth further breaking these up or if its easier to keep the logic together. Open to feedback here. |
|
Extract class, replace parameter with method and/or introduce parameter object refactorings all probably could help with the codeclimate issues. To keep logic together, it does not always have to be broken up across methods, but perhaps introducing new classes. |
|
I suggest Domination 1914 NML (trenches) as test map, as it is also interesting for having immobile units that provide hitpoints only (thus highly complementary to land units, differently from the NWO bunkers, that are substantially self-sufficient). Maybe also, then, lowering their cost to 2 PUs only, see if the AI will be down to buy significantly more of them. The only issue is that trenches, there, are made worth less by the fact that there is gas, having an anti-trenches behaviour, and the AI cannot see this. So, not seeing this, the AI should probably buy more trenches than a good player would. Moving forward, the other map I suggest is Rome Total War, where you have fairly efficient immobile infrastructures that provide, instead, defensive powers (and are capturable). Again a case of high complementarity with mobile units (and specifically with archers, through support), but substantially opposite to D1914NML. |
|
@Cernelius Yep, I tested NML as well and the AI does buy trenches. Its still probably a bit more conservative than that average human player but buys them when it really needs to defend things like factories. But I'd rather keep it conservative to start as overbuying static defenses can have really bad results. Once this is merged, I hope that a few folks will help test other maps as well and then I can always make some tweaks. |
| return false; | ||
| } | ||
|
|
||
| private static boolean hasReachedConstructionLimits( |
There was a problem hiding this comment.
Method hasReachedConstructionLimits has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
| return false; | ||
| } | ||
|
|
||
| private static int findNumberOfConstructionTypeToPlace( |
There was a problem hiding this comment.
Method findNumberOfConstructionTypeToPlace has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
| return numConstructionTypeToPlace; | ||
| } | ||
|
|
||
| private static int findMaxConstructionTypeAllowed( |
There was a problem hiding this comment.
Method findMaxConstructionTypeAllowed has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
| } | ||
|
|
||
| public static void removeInvalidPurchaseOptions( | ||
| final GamePlayer player, |
There was a problem hiding this comment.
Method removeInvalidPurchaseOptions has 7 arguments (exceeds 4 allowed). Consider refactoring.
|
|
||
| /** Removes any invalid purchase options from {@code purchaseOptions}. */ | ||
| public static void removeInvalidPurchaseOptions( | ||
| final GamePlayer player, |
There was a problem hiding this comment.
Method removeInvalidPurchaseOptions has 9 arguments (exceeds 4 allowed). Consider refactoring.
| } | ||
|
|
||
| private static boolean hasReachedMaxUnitBuiltPerPlayer( | ||
| final ProPurchaseOption purchaseOption, |
There was a problem hiding this comment.
Method hasReachedMaxUnitBuiltPerPlayer has 5 arguments (exceeds 4 allowed). Consider refactoring.
| } | ||
|
|
||
| public static boolean canUnitsBePlaced( | ||
| final ProData proData, |
There was a problem hiding this comment.
Method canUnitsBePlaced has 5 arguments (exceeds 4 allowed). Consider refactoring.
|
|
||
| /** Check if units can be placed in given territory by specified factory. */ | ||
| public static boolean canUnitsBePlaced( | ||
| final ProData proData, |
There was a problem hiding this comment.
Method canUnitsBePlaced has 6 arguments (exceeds 4 allowed). Consider refactoring.
| @@ -0,0 +1,279 @@ | |||
| package games.strategy.triplea.ai.pro.util; | |||
There was a problem hiding this comment.
File ProPurchaseValidationUtils.java has 251 lines of code (exceeds 250 allowed). Consider refactoring.
| <= (purchaseOption.isConstruction() ? remainingConstructions : remainingUnitProduction); | ||
| } | ||
|
|
||
| private static boolean hasReachedMaxUnitBuiltPerPlayer( |
There was a problem hiding this comment.
Method hasReachedMaxUnitBuiltPerPlayer has a Cognitive Complexity of 11 (exceeds 5 allowed). Consider refactoring.
| return false; | ||
| } | ||
|
|
||
| private static boolean hasReachedConstructionLimits( |
There was a problem hiding this comment.
Method hasReachedConstructionLimits has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
| return false; | ||
| } | ||
|
|
||
| private static int findNumberOfConstructionTypeToPlace( |
There was a problem hiding this comment.
Method findNumberOfConstructionTypeToPlace has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
| return numConstructionTypeToPlace; | ||
| } | ||
|
|
||
| private static int findMaxConstructionTypeAllowed( |
There was a problem hiding this comment.
Method findMaxConstructionTypeAllowed has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.
|
Alright. I took a pass on refactoring. I think there are some improvements though now that I've split a class in half, CodeClimate thinks its all new code (doesn't consider moved methods) so is now I went up to 17 issues! I think my patience to battle with CodeClimate further is probably at about its end for this PR. |
|
I think this is a major step for the AI, as I believe purchasable immobile combat units can really add a lot to games, on multiple levels (starting from realism: Maginot Line and Atlantic Wall anyone?). Just to be sure, does this mean that the AI will also consider buying immobile infrastructures (like the forts or Rome Total War)?
The fort of Rome Total War is an example of both point 3 and point 4 above. Just to know what I would be going to expect, once passing to the testing phase (I've actually a game under development in which I have "castle" units, that are immobile defensive infrastructures, though they are also built in two steps, because you just don't dump concrete off the sky, so I guess nevermind until that is supported too). |
|
The other major item would be distinguishing the case of truly immobile units from the case of movement 0 units that are actually supposed to be sea/land/air transported, instead, though, currently, it is not simply possible to sea transport movement 0 unit, as loading counts as moving for the loading unit itself. |
There was a problem hiding this comment.
Took a look over, can't say I fully grok'ed why this update accomplishes the stated goal. Left some comments/questions around tactical clarity issues.
Re: codeclimate, fair enough to only do so much; when fixed correctly the violations result in cleaner code, when fixed to placate codeclimate only then we're probably more likely to fall inject inappropriate abstractions. It's a balance for reviewability and understandability, I think gets back to our classic problem that we need to do many small iterations to clean code up before we can reasonably update it with sane and clear PRs.
game-core/src/main/java/games/strategy/triplea/ai/pro/ProPurchaseAi.java
Outdated
Show resolved
Hide resolved
game-core/src/main/java/games/strategy/triplea/ai/pro/ProPurchaseAi.java
Show resolved
Hide resolved
game-core/src/main/java/games/strategy/triplea/ai/pro/data/ProPurchaseOption.java
Show resolved
Hide resolved
game-core/src/main/java/games/strategy/triplea/ai/pro/data/ProPurchaseOption.java
Outdated
Show resolved
Hide resolved
game-core/src/main/java/games/strategy/triplea/ai/pro/data/ProPurchaseTerritory.java
Outdated
Show resolved
Hide resolved
game-core/src/main/java/games/strategy/triplea/ai/pro/util/ProPurchaseValidationUtils.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,283 @@ | |||
| package games.strategy.triplea.ai.pro.util; | |||
There was a problem hiding this comment.
File ProPurchaseValidationUtils.java has 252 lines of code (exceeds 250 allowed). Consider refactoring.
|
@DanVanAtta Made some updates based on your review. @Cernelius Responses:
For example, if I have a regular AA Gun, except that has movement 0 and target some other units than just air (possibly all units in the game), will the AI considering purchasing it?
For example, if I have an infrastructure unit that has movent 0 but positive defence value, will the AI considering purchasing it?
For example, if I have an infrastructure unit that has movent 0 but defensive support abilities (usually either bonus to allied or malus to enemy units or both), will the AI considering purchasing it?
|
Now I wonder if this is actually about movement 0 units or units that are both movement 0 and construction? I mean, will movement 0 units be considered only if they are also constructions (like in NWO and D1914NML)? Does that mean that this supports both 0 movement units and construction units, or that it supports units that are both? Like, is this a thing also for the Mortella_Tower of Napoleonic Empires and the fort of 270BC (both normal placement, albeit with mobile factories in the game)? |
| @@ -0,0 +1,283 @@ | |||
| package games.strategy.triplea.ai.pro.util; | |||
There was a problem hiding this comment.
File ProPurchaseValidationUtils.java has 252 lines of code (exceeds 250 allowed). Consider refactoring.
|
@Cernelius It will consider 0 move and/or construction units (they are just often used together). This change was primarily focused on 0 move units as there was a hard check preventing the AI from buying them. But there needed to be some enhancements to how the AI deals with construction units as well and it needed to handle the various construction limit parameters. It should consider buying Mortella_Tower of Napoleonic Empires and the fort of 270BC but generally values 0 move units low and only to defend high strategic territories like factories. |
I don't think it is necessary being that extreme. Just take as example the Mortella_Tower and the Fortress of Napoleonic Empires. The mortella tower is defence 2 and cost 3 and the fortress is defence 4 and cost 5. Chasseurs are att/def 1/3, cost 5 and movement 1. So, a movement 1 unit that is att/def 0/2 may be balanced costing 3.5, that is only 0.5 PUs more than the cost of the Mortella Tower. You can just treat immobile units has having attack 0 in all cases, then value them at something from 0.5 to 0.8 the same movement 1 unit, not as much as 0.1 only! |
|
The Mortella_Tower, despite being only slightly better than what you can get at movement 1, is fairly consistently bought in Napoleonic Empires, despite being clearly a bad unit. The Fortress is even worse than the Mortella Tower, and basically just a last-minute unit for desperate situations. |
|
@Cernelius It might be better to move this conversation to the forum as its gonna kind of get lost here once this PR is merged. I realize the an order of magnitude is a bit extreme but the point is I don't want the AI to overbuy 0 move units as that could cause very bad behavior. I essentially only want it to buy them when it absolutely needs them for defense. I may eventually adjust this some but would rather start overly conservative. I actually rarely buy mortella towers and fortresses in NE and only in situations where I really need to squeeze every PU out or need to defend a factory territory from constant naval invasion threat. IMO those units are pretty minor parts of NE and are more often traps for inexperienced players that waste PUs building too many of them. |
|
Sure, but what I was saying is that Mortella_Tower, let's say it is attack/defence 0/2, is a unit that, if it would move 1, would have a balanced cost of 3 or 3.5 PUs, no more. Instead, at movement 0, costs 3 PUs. This means that unit is priced at something like 90% than its balanced cost if it would be able to move 1 and attack (at attack power 0). So, if you just set immobile at 0.5 the value of movement 1, provided that immobile is seen as always having attack value 0, then you are already valuing it far less than what most TripleA games do. So that's already very conservative, and also 0.8 I'm pretty sure would make the AI buy Mortella_Tower only very rarely. The way you are going, I'm pretty sure Mortella_Tower will be never bought, since it is not even a unit you want to drop at the last minute, to maximize defence (that is Fortress). |
|
@Cernelius 0.5 would probably be enough but the point is I really only want the AI to buy 0 move units when it has no other options for now. So it will still buy Mortella_Tower but only to defend a factory when it has 3 PUs left over, it will buy bunkers and trenches because its the only construction defense option. Something like 0.8 would probably cause issues as the AI could then potentially build them kind of randomly in places that don't really even need them as there is a good amount of randomness added into purchasing. I'm open to adjusting this in the future and there are additional improvements that can be made around this. |
|
You are also at risk that mapmakers will start going nuts with all kind of hacks to make the AI buy those immobile at the 0.1 valuation, like triggering in and out some super stats, during purchase. |
|
@Cernelius Well, anything is possible. I tested on NML and NWO/WaW which are by far the most popular maps that have 0 move units that are frequently bought. I think my point is that 0 move units are a somewhat unique type and are more situational (shouldn't just be compared to all other 1+ move units when purchasing in any factory). There are certain situations when they should be purchased and modeling that in the AI is the goal. |
|
I don't care much this discussion getting lost, as all I'm saying, as far as units balance, are things that if someone cannot agree, there's probably no point talking with anyways. Ok, well, yes anyways probably Napoleonice Empires is the best example of, instead, movement 0 units that have nothing special, so that can be taken as a good example of balancing such units, and we can agree Napoleonic Empires does it in a very cautious way, making movement 0 units barely worth buying at all, and very marginal. The other example is the Fortress, that is defence 4 and cost 5. We can consider a unit that is att/def 1/4 and movement 1. All considered, the balanced cost for such a unit would be 6.5 PUs, rounding it to 0.5 PUs. So, the balanced cost for an att/def 0/4 and movement 1 unit (you can still use it as fodder, in attack) may be 6 PUs. Hence, the Fortress at 5 PUs is priced at something like 83% of what it may be priced if it would also be a movement 1 and attack 0 unit. So, I think that for your aim of assuring an extreme level of marginalism on normal purchase choices, rather than fixing it at 10%, one could go with the rule of thumb of fixing it at 50% of what appers to be the usual map balancing, that is 50% of 80%, thus 40%. So I would suggest you at least upping that value from the current 0.1 to a still very conservative 0.4, if you really want to be extremely cautious. Then, the ability of using units to place in excess of placement limits, like the bunker of NWO or the trenches of D1914NML, thus having a premium price for the ability of placing more than my production limits, is probably something that should be considered beside the fact of being movement 0 at all, as nothing mandates the two being related in any ways (one could make a map with mercenaries that are constructions and cost more than normal units at same value, to represent raising such troops once you cannot get any more of your men). |
| @@ -0,0 +1,283 @@ | |||
| package games.strategy.triplea.ai.pro.util; | |||
There was a problem hiding this comment.
File ProPurchaseValidationUtils.java has 252 lines of code (exceeds 250 allowed). Consider refactoring.
|
Code Climate has analyzed commit 98031c7 and detected 17 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Address forum requests: https://forums.triplea-game.org/topic/1707/simple-trigger-help/20
Add AI support to purchase 0 move and/or construction units which are generally used as static defenses. These are fairly important in some maps.
Testing
[x] Manual testing done
Ran several all AI games on NWO, NML, and revised to ensure there are no issues and the AI appropriately considers buying bunkers in NWO and trenches in NML.