Update TF enums, refactor TFCond to more closely match tf.fgd and internal cond names

[FlaminSarge]

No description provided.

[FlaminSarge]

Below this are CritOnDamage and UberchargedOnTakeDamage, which in the FGD and internally are "TF_COND_CRITBOOSTED_CARD_EFFECT" and "TF_COND_INVULNERABLE_CARD_EFFECT". I'd say these ought to be changed as well, but CARD_EFFECT means nothing to users in this case, so I was hesitant. Thoughts?

[Kenzzer]

I think old names should be kept otherwise all plugins using those conds will fail to compile.
Perhaps

TFCond_UberchargedHidden,
TFCond_UberchargedCanteen,
TFCond_InvulnerableHideUnlessDamage = 51,
TFCond_InvulnerableUserBuff,
So the enum still fine.

Edit: Nvm I saw the second part of your enum.

CritOnDamage is used by MvM robots for alwayscrit attrib so TFCond_AlwaysCrit?

[FlaminSarge]

Is it? I thought AlwaysCrit used CritUserBuff/CritCanteen.
The condition itself seems to be used when the attribute "crit_on_damage" is on a weapon, but if it's used in another situation as well, then CritOnDamage is certainly not the right name for it. I'll change it to CritCardEffect for now until we can figure out where all it gets used.

[psychonic]

I still don't understand why we would want to add all this clutter for near-identical or identical-meaning names in many cases.

TeleportedGlow -> Teleported
CloakFlicker -> StealthedBlink
Charging -> ShieldCharge
OnFire -> Burning
Overhealed -> HealthOverhealed
DefenseBuffed -> DefenseBuff
Milked -> MadMilk

And my favourite, one that actually goes further from the game's own naming of "TF_COND_CRITBOOSTED_HYPE"
CritHype -> SodaPopperHype

I could go on.

New ones are fine to be named whatever (and adding them is appreciated), but going back and changing mostly or fully accurate names, matching or not, just seems like change for the sake of change.

Edit: for any existing ones that do need further clarification, a comment can be added on them, but it doesn't seem like that would be many.

[FlaminSarge]

You're correct. A lot of these are almost-similar or already unambiguous. I can change those back. A few are still ambiguous, like you point out (e.g. charging could be misconstrued as Charging up the Cow Mangler, rather than a Shield Charge), and we could add comments for those, but I still think it's better to disambiguate here.

What are your opinions on attempting to match the internal names/FGD? I feel like it'd be better to establish the convention now rather than continuing to allow new conditions to be named somewhat arbitrarily (e.g. SpawnOutline over TeamGlows, where the former details the circumstances under which the condition occurs, while the latter matches the internal name and details the behavior of the condition more accurately).

I changed SodaPopperHype in anticipation of Valve updating the FGD to match the internal name for it, which they changed to TF_COND_SODAPOPPER_HYPE at some point. If they end up not doing it, then that'll be a mistake, so I can revert that as well until Valve makes the change.

For reference:
https://gist.github.com/FlaminSarge/0a6f3cc96cf2245dd9272a95d11f32ac

[KyleSanderson]

Is this still outstanding?

[FlaminSarge]

The discussion around how to handle these never happened.
I still believe that the condition names should match the internal names and the old names should be deprecated, especially given that Valve doesn't update the game nearly as often anymore and as a result the internal names are not likely to change anymore.

I also think that, if internal names are no-go, the correct convention should at least be naming the condition based on what it does/is rather than the circumstances under which it occurs (e.g. SpawnOutline should be TeamGlows).

[KyleSanderson]

Totally cool with that, is there any rot from age or new conds we can add? I'm seeing a merge conflict so something's changed :-).

[FlaminSarge]

I'll rebase and fix the conflicts, and doublecheck to make sure the names are correct/accurate/all present.
At some point was TFCond_NoTaunting renamed to TFCond_NoTaunting_DEPRECATED? Is there special handling for that so that it's not backwards-incompatible, or was the decision made at the time to just ditch it?

[Kenzzer]

Any update on this?

[FlaminSarge]

I'll have to go through and doublecheck the list, then I'll update the PR.

[KyleSanderson]

@FlaminSarge so there's a ridiculous amount of churn here. Can you rebase this (and I apologize for that)?
@psychonic I won't even begin to pretend I can merge this one reasonably as I'm lacking massive context (read-as: I haven't launched TF in years). Are we still missing these definitions? Is this worthwhile?

[KyleSanderson]

@asherkin / @voided does this still make sense to do with TF?

[voided]

@KyleSanderson does SM have a standard approach for renaming a truckload of constants?

I'd do it, but that's just my personal opinion.

[KyleSanderson]

@KyleSanderson does SM have a standard approach for renaming a truckload of constants?

"Don't break code" and "deprecate when necessary".

I'd do it, but that's just my personal opinion.

Yeah, this is brutal but keeping them in-sync is still useful I think.

[FlaminSarge]

deprecate when necessary

only when necessary? or...

(I need to get back to this...)

[Kenzzer]

Time to wake this from the deads. @FlaminSarge is this still desired ? @psychonic now that the tf2 sdk is public, does maintaining this seem saner ? Also with the advent of vscript, I think we do have a reason now to have the names match.

[FlaminSarge]

Time to wake this from the deads. @FlaminSarge is this still desired ? @psychonic now that the tf2 sdk is public, does maintaining this seem saner ? Also with the advent of vscript, I think we do have a reason now to have the names match.

...Would it be worth just copying the entire names wholesale from ETFCond instead of trying to reformat them into TFCond_KindOfCamelCase format?

[psychonic]

As long as we don't immediately remove support for using the existing naming that users already reference in their code, I'm fine with either route, adding aliases for existing naming that doesn't match and adding any missing condition values using their names, with either A) existing formatting or B) using the exact Valve naming.

I really don't want any existing plugin code to break from this, as it's largely just a quality-of-life change, and I don't think we can even mark individual enum members as deprecated to give a warning first, to then remove them in a later version. (But if that does work, please correct me)

[FlaminSarge]

I really don't want any existing plugin code to break from this

+1 all the existing names would exist in the enum regardless of what naming convention/etc we use moving forward.
Any idea if runtime compat breaks if typing changes? Or do the types get turned into ints on compile?

I couldn't find any way in sp to deprecate individual enum values nor to replace the entire TFCond enum with ETFCond and 'include' the old TFCond enum within ETFCond (or vice-versa), let me know if that's something that we can do. If it is, we can just take the entire ETFCond and include it in TFCond, and then stop updating TFCond in favor of ETFCond.

@Kenzzer I'll defer to your expertise on whether we should just match internal naming entirely vs reformatting the names to match existing TFCond names (underscore to camelcase will end up looking a little goofy if we convert the format over 1:1, but... eh). We will definitely leave the old enum values as backcompat.

[Kenzzer]

I think we should completely match the internal naming, we already do for other valve constants so these should be no different.
Also instead of deprecating anything (we shouldn't) or introducing a new enum, I vote that we just introduce 100+ constants.
#1618 (comment) like it was suggested here by @psychonic but for collision groups.

Blindly believing valve won't move those tfconds around or rename them again isn't a good plan. Maintaining this enum is going to be awful. So instead I think we should introduce a ton constant variables in tf2.inc

public const TFCond TF_COND_AIMING;
public const TFCond TF_COND_ZOOMED;
public const TFCond TF_COND_DISGUISING;
public const TFCond TF_COND_DISGUISED;
public const TFCond TF_COND_STEALTHED;
public const TFCond TF_COND_INVULNERABLE;
public const TFCond TF_COND_TELEPORTED;
...

Which we will fill with the correct values from TF2's sdk when the plugin gets first loaded. Aside the initial load of having to write that code, the maintenance would be little to none. As we would just have to update our hl2sdk mirror for changes to take effect, and if any cond are added/removed/renamed we would also more easily notice and opt to reflect those changes later. Benefits for plugins using those consts would be huge, as no recompilation would be required in the event a condition gets added, and if a condition is removed we can safely change the const cond value to 0 and prevent wonky plugin business logic.

[FlaminSarge]

Blindly believing valve won't move those tfconds around or rename them again isn't a good plan.

I don't think they'll move them around due to their own comment about demo compatibility, but yeah they could rename them (though I don't think that's happening this late in the game's lifecycle).
Dynamic runtime const makes sense though. Otherwise, just putting the entire ETFCond enum inside TFCond would also work, leaving the existing values intact.

[Kenzzer]

Closing in favor of #2386.
@FlaminSarge feel free to drop a review of your own on that PR.