139 - Smarter Barbarians #396
Conversation
…e based from now on. The behavior is still the same (aside from grouping barbarian actions by tribe), but this sets the groundwork for creating tribe-specific behavior.
… given the opportunity.
…her to attack nearby units, and will only do so if they calculate the odds to be at least 1/3 in their favor. Their odds calculation is not yet the most sophisticated in the world (well, I guess it is in the C7 world at this time, but you know what I mean), but this does mean they won't take on hopelessly long odds. They will still occasionally make hopeless attacks on units when they randomly choose to do so from exploring...
…e destroyed, so barbarians don't keep spawning from them. Also increase barbarian spawn rate a bit to make it more fun.
C7Engine/MapUnitExtensions.cs
Outdated
| foreach (BarbarianTribe tribe in ((BarbarianPlayer)(player)).getTribes()) { | ||
| if (tribe.GetUnits().Contains(unit)) { | ||
| tribe.RemoveUnit(unit); | ||
| goto endDisband; |
There was a problem hiding this comment.
Yikes, I didn't know C# even supported goto. Can't this just be a return?
There was a problem hiding this comment.
You know, it actually can in this case.
I wound up with goto as an alternative to Java's ability to break out of an outer for loop, e.g.:
outer: foreach (someVar in outerCollection): {
//find corresponding innerCollection
foreach (innerVar in innerCollection): {
if (innerVar.shouldBreakLoop()) {
break outer; //Only supported in Java, not C#
}
}
}
//do some additional tasks
This is the idiom that I don't know how to C#-ize, and which leads to my uses of goto. In this case there are no additional tasks, so return works, but in most of the other goto uses I've added, there are additional tasks once the outer loop is broken.
What is the C# way of doing this? I suppose I could add an additional variable to set and check at the end of the outer loop, and if it's true, break out of it? But I'm not sure that's actually any cleaner than goto.
There was a problem hiding this comment.
This has now been refactored and is part of BarbarianAI and is a break.
C7Engine/MapUnitExtensions.cs
Outdated
| { | ||
| if (player.units.Contains(unit)) { | ||
| player.units.Remove(unit); | ||
| if (!player.isBarbarians) { |
There was a problem hiding this comment.
More broadly though, a MapUnit should know which player it belongs to if it doesn't already, and Player should be able to handle disbanding its own unit (with BarbarianPlayer providing an override) rather than having to do this search here.
There was a problem hiding this comment.
I agree. I will see if I can find a better way to implement this.
(It's also cool to see comments on my pull request!)
There was a problem hiding this comment.
This has now been refactored as suggested. Surprisingly we only had 7 virtual functions previously.
| BarbarianPlayer barbarians = (BarbarianPlayer)EngineStorage.gameData.players.Find(player => player.isBarbarians); | ||
| foreach (BarbarianTribe tribe in barbarians.getTribes()) { | ||
| if (tribe.GetCamps().Contains(tile)) { | ||
| tribe.RemoveCamp(tile); |
There was a problem hiding this comment.
eventually, when the components are in place... this kind of thing should be event based
| double attackerStrength = attacker.unitType.attack * StrengthBonus.ListToMultiplier(attackBonuses), | ||
| defenderStrength = defender.unitType.defense * StrengthBonus.ListToMultiplier(defenseBonuses); | ||
|
|
||
| return attackerStrength / (attackerStrength + defenderStrength); |
There was a problem hiding this comment.
This seems like per-hit odds, isn't it?
| foreach (Tile tile in validTiles) { | ||
| if (tile.unitsOnTile.Exists(mapUnit => UndefendedUnit(mapUnit))) { | ||
| bool alive = unit.move(unit.location.directionTo(tile)); | ||
| // TODO: Restructure so we can avoid gotos. |
There was a problem hiding this comment.
Right, these are making me nervous. To me this suggests breaking up the nested loops into functions that return a boolean or nullable object and check the return value to decide the next step
| } | ||
|
|
||
| Tile newLocation = validTiles[GameData.rng.Next(validTiles.Count)]; | ||
| //Because it chooses a semi-cardinal direction at random, not accounting for map, it could get none |
There was a problem hiding this comment.
Do the neighbor functions not account for map edges?
| foreach (Tile tile in tribe.GetCamps()) { | ||
| //7% chance of a new barbarian. Probably should scale based on barbarian activity. | ||
| int result = GameData.rng.Next(100); | ||
| log.Verbose("Random barb result = " + result); |
There was a problem hiding this comment.
If this is happening at each camp maybe include the location
| MapUnit newUnit = new MapUnit(); | ||
| newUnit.location = tile; | ||
| newUnit.owner = barbPlayer; | ||
| newUnit.unitType = gameData.barbarianInfo.basicBarbarian; | ||
| newUnit.experienceLevelKey = gameData.defaultExperienceLevelKey; | ||
| newUnit.experienceLevel = gameData.defaultExperienceLevel; | ||
| newUnit.hitPointsRemaining = 3; | ||
| newUnit.isFortified = true; //todo: hack for unit selection | ||
|
|
||
| tile.unitsOnTile.Add(newUnit); | ||
| gameData.mapUnits.Add(newUnit); | ||
| tribe.AddUnit(newUnit); | ||
| log.Debug("New barbarian added at " + tile); | ||
| } else if (tile.NeighborsWater() && result < 7) { | ||
| MapUnit newUnit = new MapUnit(); | ||
| newUnit.location = tile; | ||
| newUnit.owner = barbPlayer; | ||
| newUnit.unitType = gameData.barbarianInfo.barbarianSeaUnit; | ||
| newUnit.experienceLevelKey = gameData.defaultExperienceLevelKey; | ||
| newUnit.experienceLevel = gameData.defaultExperienceLevel; | ||
| newUnit.hitPointsRemaining = 3; | ||
| newUnit.isFortified = true; //todo: hack for unit selection | ||
|
|
||
| tile.unitsOnTile.Add(newUnit); | ||
| gameData.mapUnits.Add(newUnit); | ||
| tribe.AddUnit(newUnit); | ||
| log.Debug("New barbarian galley added at " + tile); |
There was a problem hiding this comment.
sounds like a call for factory methods!
Closes #139
This is a set of changes to make the barbarians more dangerous. More changes can be added to make them more dangerous, and may be as part of this PR, or later.
It also organizes them into tribes, with the idea of various tribes having different priorities.
So far the changes center around making them more opportunistic:
Corollary, the barbarians aren't the best at figuring out if they think they have a chance of winning yet. I added a very simple risk calculator that takes into account defensive bonuses, but not the round-based nature of combat, meaning the barbarians will be more optimistic than they should be.
Long-term, I envision adding a more accurate combat odds calculator, and perhaps a multi-unit combat calculator (do my five units have a chance of taking that city?). Which combat odds calculator the barbarians (and AI) use could vary based on difficulty level.