[TF2] Fix extended melee range while charging not working

[Kajek777]

This pull request attempts to fix the broken intended mechanic of extending the range of melee attacks while charging with one of Demoman's shields not applying to attacks that begun in the middle of the charge's duration.

Within the GetSwingRange() function, there is a check whether the player is charging or not that extends the range of the melee attack to 128 hammer units if they are. However, before this function is called, the start of the attack ends any ongoing charge, so this check almost always fails unless the attack was started before the charge was.
I tried adding an additional condition to that check that activates when a charge was ended 250ms ago or sooner. I understand the implementation might not be ideal, so any criticism and/or improvements are welcome.

[FlaminSarge]

Rather than this kind of implementation, I think there are two options to take for this kind of fix:

1. In CTFWearableDemoShield::EndSpecialAction, CalcChargeCrit is setting some value on the player so that their next hit is a crit, and you can co-opt that value to also use for the extended melee range (so MELEE_CRIT, MELEE_MINICRIT, or MELEE_RANGE, where all 3 grant the bonus range in GetSwingRange). Arguably, based on the (seemingly unused?) variable m_bCurrentAttackIsDuringDemoCharge in CTFWeaponBaseMelee::PrimaryAttack, you probably don't even have to add MELEE_RANGE and just make it extend range if it's able to minicrit or crit. If you want, I can make a commit (or a PR) for this implementation.
2. In CTFWeaponBaseMelee::PrimaryAttack you can save the charging status before it calls EndClassSpecialSkill, then add a parameter to Swing() that can increase the swing size. That seems a bit more involved than the other option, though.

In either case, a cvar to enable or disable this for balance purposes would probably be useful, since it does affect balance.

[Kajek777]

Thank you for this explanation, feel free to make any changes that are better than the abomination i tried to do (i'm not too familiar with the source engine (or github for that matter))

[wgetJane]

this is a pretty strange implementation
- this creates a new thread every time a demoknight swings their melee while charging
- this measures real-world time, not the engine curtime
- this completely bypasses prediction
- the server will probably crash if the player disconnects during that 250ms interval

the implementations suggested by @FlaminSarge are a lot better

if you want to implement a timer on the player or a weapon, you should just create a (probably predicted or networked) field that gets checked in a think/update function (like ItemPostFrame)

see how that pattern is used for the melee swing-to-smack time for example:

source-sdk-2013/src/game/shared/tf/tf_weaponbase_melee.cpp

Line 322 in fa288c4

m_flSmackTime = GetSmackTime( m_iWeaponMode );

source-sdk-2013/src/game/shared/tf/tf_weaponbase_melee.cpp

Lines 792 to 795 in fa288c4

	float CTFWeaponBaseMelee::GetSmackTime( int iWeaponMode )
	{
	return gpGlobals->curtime + m_pWeaponInfo->GetWeaponData( iWeaponMode ).m_flSmackDelay;
	}

source-sdk-2013/src/game/shared/tf/tf_weaponbase_melee.cpp

Lines 374 to 380 in fa288c4

	void CTFWeaponBaseMelee::ItemPostFrame()
	{
	// Check for smack.
	if ( m_flSmackTime > 0.0f && gpGlobals->curtime > m_flSmackTime )
	{
	m_flSmackTime = -1.0f;
	Smack();

though i personally wouldn't use a timer for this, the bug happens because swinging ends the charge immediately, so i would probably set a predicted bool in Swing() that tracks if the swing happened while charging or not, then just check that in GetSwingRange()

and also yes, this seems like a pretty significant balance change for the demoknight

[Kajek777]

I redid the implementation according to @FlaminSarge's suggestion, using the function IsCurrentAttackDuringDemoCharge(), as well as added a convar tf_shield_charge_range_extend that disables the added check when set to 0.

[FlaminSarge]

You should be good to just call the cvar inside the condition; that way it doesn't get evaluated at all if the previous conditions aren't met.

Name suggestion for cvar: tf_shield_charge_melee_range (could even remove shield if you want to try to match tf_max_charge_speed but that's less clear).

If you want to go above and beyond, you could make the cvar actually control the range, and default it to the original non-128 range. Not sure if that should be set up as a bonus (like, +cvar value or *cvar value) though, especially since it sets 128 for both swords and non-swords. Could just be fine to set the range itself via the cvar, using its value directly.
For that matter, maybe the sword range should be its own cvar value.

[Kajek777]

Should i flag the cvar as sv_cheats protected as well?

[FlaminSarge]

Looking at the implementation as it currently is, something's off about the way we're using the cvar. As in, unintuitive, sort of.

Also in order to maintain stock behavior you may need to set the default cvar value to 0.

I don't think it'll need the cheats flag, though, but still, something about the way it's set up right now doesn't seem right (even though I'm the one that suggested it). Maybe it's because it gets the cvar value twice? But you can't get around that if you want to preserve stock behavior when the value is 0... Trying to think of a better way to arrange it. Maybe just the toggle on/off cvar was better in terms of how intuitive the cvar is, vs this impl which has cvar value for range + 0 for disable the logic. I could see some use case for someone wanting to set melee range to actual 0 during a charge for a mod or something.
Sorry, not able to put it into words better.

[Kajek777]

Maybe a value of -1 could mean disabled? Or we could just go back to a 0/1 toggle.

[FlaminSarge]

I redid the implementation according to @FlaminSarge's suggestion, using the function IsCurrentAttackDuringDemoCharge(), as well as added a convar tf_shield_charge_range_extend that disables the added check when set to 0.

Nice work. And ha, didn't realize they already had a helper method for the bool, nice find.

[FlaminSarge]

You will have to move this line before Swing() is called for the bonus range to take effect (unless CTFWeaponBaseMelee::CalcIsAttackCriticalHelperNoCrits is somehow already doing that? Which it might be?)

[Kajek777]

Better to move it than not, I suppose.

[wgetJane]

this is MASSIVELY improved since last i saw this pr

only things i can think of:

• maybe m_bCurrentAttackIsDuringDemoCharge should be a predicted networked var? (very minor, not really necessary)
• this seems to apply the fix by default, which again is a significant balance change imo

i'm personally not opposed to this change, but it's possible that many may not like it and start complaining about demoknight's melee range buff

[Marxvee]

The current iteration of the PR would theoretically make melee hits impossible to connect by default if one were to swing before charging (the if statement still passing if the player simply has condition TF_COND_SHIELD_CHARGE) due to the range now being 0 by default. This would particularly break melee weapons for MvM bots equipped with the attribute class attack_not_cancel_charge, as the attribute will allow demoknights to keep charging while swinging; most commonly used by Samurai Demoknights.

[Kajek777]

Would something like this be acceptable?

if ( pOwner && ( pOwner->m_Shared.InCond( TF_COND_SHIELD_CHARGE ) || ( IsCurrentAttackDuringDemoCharge() && tf_shield_charge_melee_range.GetInt() ) ) )
{
    if (tf_shield_charge_melee_range.GetInt())
    {
	    return tf_shield_charge_melee_range.GetInt();
    }
    else
    {
	    return 128;
    }

[Marxvee]

That seems to work out!

[FlaminSarge]

Honestly, at this point I'd split it into two conditions for clarity:

if ( pOwner && pOwner->m_Shared.InCond( TF_COND_SHIELD_CHARGE ) )
{
	// mvm bots that can attack while charging
	return 128;
} else if ( IsCurrentAttackDuringDemoCharge() && tf_shield_charge_melee_range.GetInt() )
{
	// maybe don't repeat the cvar call, somehow, but not that important
	return tf_shield_charge_melee_range.GetInt();
} else {
...
}

As for the '0' case, I'd say people can just use value '1' instead, so not that big a deal to have '0' as 'off'. They do it for m_flMaxSpeed like that anyways.

[FlaminSarge]

( Potentially keep the codestyle the same as the original code, with all the extra spaces in the parentheses )

Also... does this break if you set the cvar to a negative value? Since mods can sometimes break cvar bounds, might be worth sanity checking, but if something funny and not-crashy happens when you set it negative, you could also leave it in as a 'feature'.

[Kajek777]

Setting it to -1 made all attacks made during the charge impossible to hit.

I could change it from if ( IsCurrentAttackDuringDemoCharge() && tf_shield_charge_melee_range.GetInt() ) to if ( IsCurrentAttackDuringDemoCharge() && tf_shield_charge_melee_range.GetInt() > 0 ) to count negative values as disabling it as well.

[Kajek777]

While doing some extra testing, I've noticed that the extra range is only added when I attack after shield bashing something, and not when attacking while charging, and I can't figure out why.

[FlaminSarge]

Setting it to -1 made all attacks made during the charge impossible to hit.

What about -128 (or -900 or something silly)?

And yeah that way of doing a negative check is probably sane.

For the bash behavior, it's possible you have to edit the other location where m_bCurrentAttackIsDuringDemoCharge is getting set, too.

[SolarLightTF2]

Hi,

I'm not a coder, but I am concerned about gameplay ramifications.

1. Sniping enemy players with crits could be really annoying for the victim, especially if the Demoknight player has high ping values
2. If this fixes the "extender" bug where swings made before charging provides the range boost, this would remove a niche and balanced alternative to medium-range minicrit charges (was viewed as fine - give up crit-boost for a faster hit)
3. Even if it doesn't fix "extender", constant access to range-boost irrespective of crit status would mostly diminish the strategy anyway, making Demoknight simpler to play and removing nuance

Could be a potential Thermal Thruster stomp controversy repeat if implemented, though mostly from non-demoknight players dying to criticals

[Kajek777]

I was trying to do this in a way that keeps the old extender (after all, mvm samurai bots make use of it) and that can be enabled and disabled at the server owner's will with the tf_shield_charge_melee_range console variable, so if it gets implemented, Valve can choose to keep it disabled in casual servers.

[Kajek777]

Seems to be working fine now; nothing changes when cvar set to a negative value or 0 (the default), range extends properly when set to positive number (tested with 128, 1000 and 10000), original behaviour when swinging before charging is preserved with both settings