Melee attacks from ranged weapons now identifiable in soldier stats

[karadoc]

Kill stats record the weapon name and the ammo name used for each kill.
But until now, there was no way of telling if a gun's melee attack was used.
This led to incorrect damage types when awarding commendations.

To fix this, we now list 'BA_HIT' as the ammo type when the secondary melee
damage is used. Melee weapons and ammoless guns still put the weapon itself
as the ammo field so that their main damage data is used.

When checking commendation conditions, 'BA_HIT' indicates that the ranged
weapon's melee damage data should be used rather than the default data.

[karadoc]

As far as I can tell, the only place the ammo name for stats is used in when checking commendations. (The weapon name is used in a few different places, but the ammo name is not.) So using the ammo name in this way should be ok; and I imagine that one day we might want to use BA_PSI or something like that for similar reasons.

The changes I've made to the commendations check have one minor side effect: they will now count matched commendation conditions when the weapon (or ammo) is not properly set. Previously, if the weapon name didn't match an item, then the kill could not count towards any commendations at all; whereas now the kill can still count - as long as the weapon type is not one of the conditions being matched. (As a result, getting things like a double-kill from blowing up an environmental object can now award a commendation.)

The new code is also significantly faster... but that isn't very important, since the checks don't run often anyway.

[MeridianOXC]

Has this been requested by someone?
Has this been discussed with the modders?
Have all side effects been explained to the modders?
I'm not changing anything until at least a couple of key mods/modders confirm they are 100% fine with all of this.

[Yankes]

After fast glance at this code I do not think this is correct solution.
In theory battle action, weapon and ammo should be enough to determine what damage type was.
I could say this soldiers diary logic was not updated to match my damage logic in other places in code.

[karadoc]

Nar, I've barely discussed it with anyone. You're right, I should have asked around a bit more before posting it here. I guess I was just in a 'lets get it fixed right now' kind of mood. Sorry for rushing it.

The bug that this change fixes has been around for ages, and is well known. i.e. kills with the melee attack of ranged weapons give commendations from the wrong damage type. My goal is to get the correct damage type, with minimal changes to how the system works so as not to break save backward compatibility.

After fast glance at this code I do not think this is correct solution. In theory battle action, weapon and ammo should be enough to determine what damage type was. I could say this soldiers diary logic was not updated to match my damage logic in other places in code.

I agree that battle action along with weapon and ammo is enough - but currently battle action is not included. My reasoning is 'shooting' actions are the only things that use ammo, and therefore the ammo data is only needed if the kill was achieved by shooting. So then, I simply set the battle action instead of the ammo when I don't want to use the ammo data. I thought that was a pretty elegant way to do it.

I'm not sure what you mean by 'this soldiers diary logic was not updated to match my damage logic'. I guess you must be referring to some changes that I'm not aware of; perhaps made between when I wrote this code and when I posted the pull request (because I spent a couple of weeks just play-testing).

In any case, if either of you two dislike the change, then I guess it's probably best to just close the pull request without merging. And I apologise again if I've gone about this request the wrong way.

[Yankes]

I was referring to my refactor of multiple ammo slots, is was done couple of years ago but I do not recall to updating Diaries too.

[karadoc]

I believe the diary records the correct ammo already. It gets name of the damage_item passed to it by the function that handles the damage being done. So if the correct ammo is used for damage, then it will also be recorded in the diary. The only special case, where the damage_item's power and damage type are not used, is BA_HIT on non-melee items. In that special case the melee data is used rather than the normal data. This special case happens near the start of ExplosionBState::init.

(And the info is passed to the diary via ExplosionBState::explode calling checkForCasualties)

[MeridianOXC]

Okay, so this is primarily a bugfix, less a new feature.
(I didn't have such a bug noted anywhere, it slipped my attention)

So my feedback to the code:
1/ For future-compatibility (if we ever decided to save also battle actions), I would not use the "BA_HIT" string literal, because it would falsely match (commendations ruleset is unfortunately completely typeless). I'd use something like "XXX" or "N/A" or "GUNBUTT" (last one being my preference).

2/ If I understand correctly the new "BA_HIT" and a non-existing ammo (e.g. STR_AMMO_REMOVED_BY_THE_MODDER_IN_THE_LAST_VERSION) are currently indistinguishable and would both try to go for a weapon's melee damage type. This would result in a mismatch. The new "BA_HIT" needs to be checked explicitly to prevent this. And the non-existing ammo should be skipped without trying to match any damage type.

3/ You say the code is significantly faster. Why exactly is that? Is it because of the (re)moved battletype and damagetype pre-calculations or is it also something else?

[karadoc]

1/ I originally was using "STR_NONE" or something like that, but I decided that it would be better to explicitly say what it was; partially just because it makes it understand, partially because we might later in the future want to do a similar thing with PSI; but also partially because maybe modders might want to deliberately match that string. i.e. make a commendation for using gun-butts regardless of damage type. I'm not sure why you prefer "GUNBUTT" over "BA_HIT", but either of those sound good to me.

2/ I didn't bother checking for "BA_HIT" in the commendation code because I figured this was the only legitimate case in which the ammo wouldn't be an item (and my original idea was to just use any non-item name). So that was a bit of coding laziness I guess. You're right that real ammo items which are later removed from the mod would cause incorrect behaviour. I didn't think of that. So I agree, the code should to be changed to explicitly check for the exact string.

3/ It's faster just because of how the string matching is done. Previously, each condition was compared to every possible damage type until a match was found, and then checks if that damage type matches the item. Since most conditions are not damage types, that means almost every condition is compared to every damage type string. My new version just does the other way around. i.e. it gets the damage type of the item, then compares the condition to that damage string only. So each condition is only compared to 1 damage string instead of 21. Similarly for battle types. It's not directly related to the bug fix. It was just a tweak to the code I made while I was working on it.

So, I guess that's two things:

• Possibly change the magic string from "BA_HIT" to "GUNBUTT" (your choice)
• Explicitly check for the magic string in SoldierDiary::manageCommendations, rather than assuming it for non-items.

By the way, the reason I choose "BA_HIT" is because that's the internal name used for that action in the code. 'Battle action hit'.
Would you like me to make those changes? Or will you just set it up the way you like it yourself?

[Yankes]

Side comment, I would not use directly GUNBUTT, as some mod can use this as name for some item.
We need make clear that is impossible that this is mod item. better maybe use names like at least __GUNBUTT?
And even ban user for naming items that start with __.

(this is copy-pase of rule that C++ use to resolve possible conflicts between user code and standard library code)

[karadoc]

One other (minor) point about the "GUNBUTT" thing... I just did a quick search through my save file where I've been using this patch to see where / how often BA_HIT comes up. Many of the examples are in fact gun butt attacks... but there are also a lot of weapons like STR_QUARTERSTAFF and STR_KUNG_FU, which make use of the ranged-weapon melee game mechanics but aren't really guns.

That said, I'm not fussed about what string is used. I'd be happy with the current "BA_HIT", or "__GUNBUTT" or anything else really.

[MeridianOXC]

@karadoc Yes, please make the two changes:
1/ BA_HIT to __GUNBUTT
2/ add explicit check for __GUNBUTT, and ignore non-items
I'll test and merge afterwards.

[karadoc]

Ok. It is done. But I haven't tested this latest change at all, other than to see that it compiles.