Initial cleanup of melee code

[mugling]

The melee code is amongst the oldest in the code base. It is poorly documented and contains many inconsistencies.

Key changes:

• Start separate piercing (STAB, SPEAR) damage from cutting. In the long-term I'd like to start further differentiating these types.
• Unarmed skill is also heading towards being a 'first-class' weapon skill
• Items benefit from weapon skills when they do more than MELEE_STAT (5) damage of the relevant type. Below this threshold only melee skill can provide bonuses.
• Crits and to hit chance are now dependent on the highest damage type. Previously it was possible for high bashing skill to cause crits for a cutting 30, bashing 5 sword etc
• Starts defining some constants, especially BIO_CQB_LEVEL
• Add encapsulation eg. damage_melee( DT_BASH ) as opposed to type->melee_dam so that flags such as REDUCED_BASHING and DIAMOND are always correctly handled

[codemime]

I'd also suggest renaming is_weap() and weap_skill() to is_melee() and melee_skill() respectively.
Renaming is_gun() and gun_skill() to is_ranged() and ranged_skill() would be also nice. The strongest argument, apart from consistency, is that bows, slingshots, throwable spears e.t.c aren't guns.

[codemime]

Forgotten break?

[mugling]

Fixed

[mugling]

I'd also suggest renaming is_weap() and weap_skill() to is_melee() and melee_skill() respectively.

Yes, but separate PR (keep line count down for now)

[mugling]

Ready

[Noctifer-de-Mortem]

@mugling Maybe not the place but since changes to CQB Bionic are included maybe #18470 could be considered on another melee code re-balance PR?

[Coolthulhu]

That name is really ambiguous.
The old option - having is_cutting_weapon etc. was better.
Could be combined into is_weapon( damage_type ) - this would be both clear and centralized.

[mugling]

Or is_melee( damage_type ) to combine it with @codemime's suggestion

[mugling]

I've updated that retaining the constant only so that item_factory.cpp assigns the weapon category consistently with the check in is_melee()

[Coolthulhu]

This makes guns with bayonets lose any scaling.

[Coolthulhu]

Oh and it also makes pure unarmed combat have no weap_skill.

[mugling]

This makes guns with bayonets lose any scaling.

I'm not sure what you mean about scaling.

Oh and it also makes pure unarmed combat have no weap_skill

The null item shouldn't have a skill but I've added checks for unarmed combat where necessary in 3fa13d4:

• player::get_hit_weapon
• player::crit_chance

[Coolthulhu]

I'm not sure what you mean about scaling.

If you're using a gun with a bayonet as a spear, it should train spears (and vice versa).

The null item shouldn't have a skill

It should - it is treated as the fist in pretty much all cases.

[mugling]

If you're using a gun with a bayonet as a spear, it should train spears (and vice versa).

That would mean both is_gun() and is_melee() could be true at the same time. That isn't necessarily a problem but we would need that so code continues to perform the checks in that order (currently the case for all existing code)

The null item shouldn't have a skill

It should - it is treated as the fist in pretty much all cases.

I'm less sure about this. The only cases where it matters were the two functions above

[Coolthulhu]

Could just be an array. damage_type is an enum small enough that map will both slow things down and make it occupy more memory.

[mugling]

Agreed, done

[Coolthulhu]

You declared a constant for this purpose and aren't using it here.

[mugling]

Fixed

[Coolthulhu]

This block will conflict with #18351, which waited much longer and changes much more.
This PR should probably wait for it.

[mugling]

Yes, #18351 should take precedence

[mugling]

What is the plan with #18351 as I don't want to hold either PR up given I've reviewed #18351 and it's otherwise ready to merge and we are also a long way along with this.

If you have an update ready for test tolerances and standard survivors then I'll wait on that, otherwise one of us can do that in a followup PR.

[Coolthulhu]

This significantly changes the behavior of training - now you only train the highest (or first of the highest) skill.
Meaning that using a wood axe will only teach you bashing. Same for fire axe which is 20/20.

[mugling]

Eventually I want to move to a system where the player makes either a bashing, cutting or stabbing attack (dependent on what would be most effective). For now we could split 150% of the current XP between skills proportionally?

[Coolthulhu]

Splitting into different attacks is good, but making them all "typed" is not. We had this earlier in the development for knives and it lead to them always training only one of the types due to it scaling better at low levels.

Splitting training sounds good.
But it shouldn't be 150% - more two practices which sum up to 150%, but each is at most 100%.
This is because low amount practices are inherently worse when repeated a lot. Each practice drains focus regardless of amount.
Alternatively, if training one type only, it could be trained twice instead of for twice the amount.

[mugling]

Can you look at what I wrote (d1438d8) which defacto results in either 1 or 2 weapon skills being trained plus melee.

[Coolthulhu]

The function name here is ambiguous in a very important aspect: is this damage base or modified?
Currently all weapon bashes are done on base power.

[mugling]

Intentional change to use modified values. For example REDUCED_BASHING should reduce the effectiveness

[Coolthulhu]

That's a nerf to proper unarmed (ie. no punch daggers), which is already underpowered.

[mugling]

What should be done here?

[Coolthulhu]

Old code allowed max( weap.type->m_to_hit, unarmed / 2 ).
It could be a good idea to allow pure unarmed (!is_armed() as opposed to unarmed_strike(), preferably with a comment explaining it) to benefit from the max, but still require regular unarmed to only use weapon type to hit.

[mugling]

I see. Given I fixed player::unarmed_attack() to actually match its function docs (c9678a3) we could restore that initial check using that?

[mugling]

Done

[mugling]

Everything fixed except waiting for #18351 and the last comment about nerf to unarmed which may not still apply?

[Coolthulhu]

Should probably train for 50%-75% of primary weapon skill. Otherwise it will always be higher than any weapon skill.

[mugling]

It's a copy-pasta from the original but sure, fixed

[Coolthulhu]

This check should not be caught like that. If anything calls for a damage_type outside range, it should either debugmsg, crash or bug out hard to show that something is going wrong.

[mugling]

Fixed with an assert

[Coolthulhu]

This whole check is needlessly restricting.
Consider the cases:

• Someone grabs an alien leg (is_food() == true) and bashes stuff with it
• Gun is used to bash/stab something
• Someone mods in gasoline as edible, chainsaw suddenly stops being a weapon when loaded
• Bible-thumping with a very large bible

is_null would make sense (but be redundant due to low damage) here if you didn't add the unarmed check below - if unarmed weapons are weapons, then pure unarmed should count too.

[mugling]

Fair enough, have pushed fix

[Coolthulhu]

Is it zeroed properly somewhere?
I created at least 2 bugs by not explicitly zeroing arrays.

[mugling]

No - I had presumed it would be default initialized - is this not the case?

[Coolthulhu]

Pretty sure it isn't - otherwise I wouldn't have made those 2 bugs.
It can be compiler dependent.

[Coolthulhu]

Redundant

[mugling]

Why? If the player has no weapon they are by default making an unarmed attack?

[Coolthulhu]

"none" has the flag:

{
        "type":"GENERIC",
        "id": "null",
        "symbol": "$",
        "color": "red",
        "name": "none",
        "name_plural": "none",
        "description": "seeing this is a bug",
        "price": 0,
        "volume": 0,
        "flags": ["PSEUDO", "UNARMED_WEAPON", "TRADER_AVOID"], "//" : "Fist is internally represented as 'wielding a none as a weapon'"
    }

[Coolthulhu]

Side note: sum of damage is a better measure of "weaponiness" than maximum.

[mugling]

Can overhaul that whole function in a further PR

[Coolthulhu]

Would be significantly more readable as

if( cut > 0 || stab > 0 ) {
    proj.impact.add_damage( cut > stab ? DT_CUT : DT_STAB, std::max( cut, stab ) );
}

Or even just by allowing stuff to deal both types separately.

[mugling]

Agreed, fixed

[mugling]

Fixed everything.

What is the plan with #18351 as I don't want to hold either PR up given I've reviewed #18351 and it's otherwise ready to merge and we are also a long way along with this.

If you have an update ready for test tolerances and standard survivors then I'll wait on that, otherwise one of us can do that in a followup PR.

[Coolthulhu]

Is this special case needed?
It probably doesn't hurt, but looks totally redundant, considering that the check below now covers all sorts of unarmed.

[mugling]

Agreed, fixed

[mugling]

Includes all fixes from review and merged against master to resolve conflict from #18351

Ready

[Coolthulhu]

src/melee.cpp: In member function ‘virtual void player::melee_attack(Creature&, bool, const matec_id&)’:
src/melee.cpp:367:18: error: variable ‘bashing’ set but not used [-Werror=unused-but-set-variable]
             bool bashing = (d.type_damage(DT_BASH) >= 10 && !unarmed_attack());
                  ^
src/melee.cpp:368:18: error: variable ‘cutting’ set but not used [-Werror=unused-but-set-variable]
             bool cutting = (d.type_damage(DT_CUT) >= 10);
                  ^
src/melee.cpp:369:18: error: variable ‘stabbing’ set but not used [-Werror=unused-but-set-variable]
             bool stabbing = (d.type_damage(DT_STAB) >= 10);

[mugling]

@Coolthulhu fixed - that entire block had become redundant

[Coolthulhu]

Hint for the future: CQB bionic skill adjustment could easily be handled wholly in get_skill_level by adding a flag to affected skills and checking said flag there.

[mugling]

Good idea

[Zireael07]

This PR broke the buildbot?
See #18711