The Combat Score Bug

So, this was an interesting one which I’m going to write about, it was something insanely convoluted that ended up actually being a very simple mistake in the calculation logic…

So, what was the reported problem? Combat scores were completely out of whack, people were reporting multi-million score increases, even if they were in a situation where combat didn’t occur for them that turn.

@Lowlife reported that his CS went from 700k(ish) to 22k, while not having combat, @Muffinman reported similar, 2.1m CS being randomly added to his score, despite no combat occurring…

There were reports that combat between other people were seemingly being added to their scores, implying a potential ID mismatch when storing the results, so this was my frame of reference when attempting to debug this issue…

Reading through the code, everything looked ok. As part of battle reports, the score a single unit achieves is actually logged, so a simple BR internally looks something like this:

<CombatReport turn="24">
   <Alliance id="0" name="The Galactic Police" tag="TGP"/>
   <Player id="0" name="FrostyCoolSlug">
      <Alliance id="0"/>
   </Player>
   <Player id="1" name="Zedd"/>
   <Location id="0" name="Uninhabited Planet" coordinates="1, 1, 1, 1"/>
   <CombatMobile id="1" name="FleetFrosty" moveTurns="0" score="0">
      <Player id="0"/>
      <Destination id="0"/>
      <Origin id="0"/>
      <ScoredUnit id="95" name="Fighter" amount="30" amountAfter="20" score="1080000"/>
      <ScoredUnit id="96" name="Bomber" amount="9" amountAfter="0" score="0"/>
   </CombatMobile>
   <CombatMobile id="2" name="FleetZedd" moveTurns="0" score="0">
      <Player id="1"/>
      <Destination id="0"/>
      <Origin id="0"/>
      <ScoredUnit id="95" name="Fighter" amount="12" amountAfter="0" score="672000"/>
      <ScoredUnit id="96" name="Bomber" amount="11" amountAfter="0" score="216000"/>
   </CombatMobile>
   <Target id="0"/>
</CombatReport>

From this you can determine that ‘FleetFrosty’ achieved 1080000 CS (achieved by the fighters), and ‘FleetZedd’ achieved 888000 CS from the fight.

While reviewing the code, the calcs on these scores was always correct, inside all BRs the score values were accurate, so combat was calculating score correctly… The code which actually performs the logging of the score seemed pretty straight forward as well…

class Combat {
	public boolean performCombat(int locationId, List<ICombatMobile> mobileList) {
		// SNIP
		// - Group mobiles into their alliances (per other post)
		// - Perform Combat
	  
		// Add Score
		allianceMobileGroupList.forEach(this::applyScores);
		persistScores();
	}

	public void applyScores(DGAllianceMobileGroup group) {
	  // Iterate the Mobiles in this Group..
	  for (DGCombatMobile combatMobile : group.getMobiles()) {
		if (!scoreMap.containsKey(combatMobile.getPlayerId())) {
		  scoreMap.put(combatMobile.getPlayerId(), 0L);
		}

		// Iterate the Units, add them to the player score..
		for (DGCombatUnit combatUnit : combatMobile) {
		  scoreMap.put(combatMobile.getPlayerId(), scoreMap.get(combatMobile.getPlayerId()) + combatUnit.getCombatScore());
		}
	  }
	}

	public void persistScores() {
	  IGroup group = getDependency(IGroupCache.class).getByName(scoreGroup);
	  int turn = getDependency(ITurnUpdater.class).getTurnNumber();

	  List<IPlayerDynamicScoreRow> scoreRows = new ArrayList<>();

	  for (int playerId : scoreMap.keySet()) {
		IPlayerDynamicScoreRow scoreRow = new PlayerDynamicScoreRow();
		scoreRow.setTurn(turn);
		scoreRow.setGroupId(group.getId());
		scoreRow.setPlayerId(playerId);
		scoreRow.setScore(scoreMap.get(playerId));
		scoreRows.add(scoreRow);
	  }

	  IPlayerDynamicScoreRowPersistor persistor = getDependency(IPlayerDynamicScoreRowPersistor.class);
	  persistor.addAll(scoreRows);
	}
}

And this code makes complete sense, the score calculation here was pulled directly form the battle reports for the users, and each battle score persisted separately to the database (to be picked up by the ScoreUpdaterComponent later), it doesn’t look like any Ids are being mis-set… So, the next step was to look into how the scores were being updated.

Now, combat score is a somewhat unique type of score, it’s a ‘Dynamic’ score, as well as being an ‘Additive’ score, so let me explain.

Dynamic scores are scores which are set by plugins which don’t directly interact with the scoring system, and are persisted to the database as-is for later handling, in the code above the persistor specified the turn, the score group, the player Id and the score, which gets logged to a row for every battle that’s occurred… Some users may have multiple rows if they’re engaged in multiple battles, but that’s fine. The Score Updater will read these rows and collate them into a single ‘total’ score for that score group, then cleanup the data (leaving the values un-inspectable post-calculation).

An ‘Additive’ score is simply a score which should be added to the value of the previous turn.

Now, both Dynamic and Additive scores were added to the engine at the same time with the intention to handle and implement Combat Score, all other scores are calculated via a simple summing up of assets done as part of score calculation and have been working fine for years, so this was my next port of call, because it was very possible I’d messed something up in this code.

Reading through the code of the Dynamic Score updater, I spotted that the code was a little hacky and it was potentially possible for combat score to be added to the asset score (whoops), but outside of this, after a little bit of cleaning and tidying the code, I didn’t really spot anywhere this code could go wrong, which lead to massive levels of debugging, query logging, code logging, behaviour logging and writing a test case to try and reproduce the problem.

First test case was simply create two fleets with extended debug, crash them together and see what the output results were… The results were fine, score was correctly distributed, all IDs correctly set and scores were appropriately set and distributed.

So the next test, with there being absolutely no evidence of badly set ids or scores being sent to the wrong player create a new test case… Turn 1, create two fleets crash them into 1.1.1.1, Turn 2, create two fleets and crash them into 1.1.1.2, turn 3 create two fleets and crash them into 1.1.1.3, then execute 24 turns to force the collision, it’s important that fleet composition for each ‘player’ was identical between the 3 separate tests (the BR above was one of these tests), because it provides a clean, consistent and easy to follow test… And this was where stuff got interesting…

Monitoring the SQL queries specifically between the three battles, I noticed the following happening:

[Executing] 'INSERT INTO turnengine_local.PlayerDynamicScore_0 VALUES(24,0,2,1080000),(24,1,2,888000)'
[Executing] 'INSERT INTO turnengine_local.PlayerDynamicScore_0 VALUES(25,0,2,2160000),(25,1,2,1776000)'
[Executing] 'INSERT INTO turnengine_local.PlayerDynamicScore_0 VALUES(26,0,2,3240000),(26,1,2,2664000)'

now, the format for the ‘Values’ are (TurnNumber, PlayerId, ScoreGroupId, Score), but despite the three fights being identical each turn with identical score outputs, the calculated score is growing, but only by the amount of 1 turn of combat, and this was happening inside the combat plugin.

So now to the ridiculously stupid part, I’m going to expand the above code a little bit, and hopefully the devs amongst you may be able to see the problem…

class Combat {
	private Map<Integer, Long> scoreMap;
	private static Combat INSTANCE = new Combat();

	private Combat() {
		scoreMap = new HashMap<>();
	}

	public static Combat getInstance() {
		return INSTANCE;
	}

	public boolean performCombat(int locationId, List<ICombatMobile> mobileList) {
		// Same as above..
	}

	public void applyScores(DGAllianceMobileGroup group) {
		// Same as above..
	}
}

So what we end up with here is a singleton model, when the engine starts up this object is created and is accessible directly. It’s never recreated, it exists as-is and gets recycled every turn.

So what was ultimately happening, is that the scoreMap defined was never getting cleared, if you scored 100 CS in turn 10, during turn 11 it thought you had also achieved 100CS regardless of whether you actually had combat occurring or not. When turns are happening every 60 seconds it can be really confusing because one minute you have 700k (ish) combat score, and 3 minutes later you’re up to 2.1million CS, because that 700k was being applied to your score every turn… The more CS you accumulated through actual combat, the higher the jump every turn became.

So why am I going into detail about this? Firstly, this ■■■■ is fun for me, while bugs are certainly not optimal, the research and debugging process is ultimately fun, and I enjoy sharing the general process (especially when it leads to a good result), but more importantly, it shows how complex bug reporting can be, especially during speed games. A lot of the suggested reasoning for the problem were admittedly creative and deep, but were actually mostly off-base with what was actually occurring! But it also demonstrates how simple oversights at a code level can lead to some really interesting results.

For now, combat score should be fixed for the next beta, which will be happening soon (just gotta tweak some combat stuff), but it’s been a blocker for a while, I’m glad I’ve had many hours to actually work on this and dig down through the code!

Until next time…
Stay Frosty

3 Likes

Nice, glad you got it sorted!

So this bug was actually a feature to reward early combat, that just got slightly out of hand😉

Not surprised it’s C#. That stuff has tripped me up before.
Hint: using object references as keys or values in a dictionary = bad idea.

I’m pretty sure it’s Java.

This was just a singleton re-use without resetting variables, the HashMap did only use primitive types.

1 Like

Correct… For a more technical reason, singletons in the engine aren’t as noticeable as they would be in standard Java, the objects involve get initialised during startup and stored in a map (by interface) where they get looked up later… so during engine start, there’s some code that says:

setDependency(IDarkGalaxyCombat.class, new DarkGalaxyCombat());

Then during the turn update, the following code runs:

getDependency(IDarkgalaxyCombat.class).execute()

So the object doesn’t have any of the standard ‘indicators’ that it’s a singleton in this context (getInstance methods and the likes) because it exists and is accessible where it needs to be… This is what lead to the mistake, an object that looks like a regular object, but is technically a single object :slight_smile: