Add More Substantial Override Paths For Physical Combat, 9/11/2024

[magicono43]

The idea behind these changes is to allow for much more of the vanilla behavior for the physical combat systems to be circumvented if a modder chooses to, the override methods in FormulaHelper being there so they can also do whatever actions they want to instead. In this instance hopefully with as little impact on other parts of the code and execution as possible, 9/11/2024.

My current desire for these is that just overriding the CalculateDamage method does not all suppression of vanilla stuff that happens regardless of what is changed in that damage calculation method, things such as sounds being played being the most notable one, and others as well.

Note: I don't know why in Github Desktop said CRLF was used instead of LF, my IDE said everything was in LF when I was doing these changes. Hopefully that is not an issue and can be corrected if necessary.

[magicono43]

Also just a FYI, I have not tested these changes, I mostly wanted to get this PR out there for opinions and critique, etc. If it seems practical and low-impact then continue from there hopefully.

[petchema]

If I understand the logic correctly, what this does is allow a mod to disable some "roll the dice" damage computations, most likely because they're replaced by another mechanism?
If so, I suggest giving more accurate names than "Alter..." (say "Bypass..." or "Skip..."?), given the effect is to actually disable something (replacing them with something else is what the mod does, not those functions).
Adding some comments to explain the purpose of the new functions is also always a good thing.

As a second thought, I wonder if it wouldn't be simpler (hence easier to understand), and yet more flexible, to allow to override the whole calling function (ApplyDamageToPlayer, etc.), so your mod could disable them by overriding them with a NoOp, but also other mods could also replace them with another computation.
As usual they're also drawbacks (what happens if we fix bugs in core, what happens if more than one mod wants to modify those functions,...) and I'm not very familiar with modding in general, so this is at best just an idea. You may even have dismissed it for good reasons that I failed to consider.

[magicono43]

If I understand the logic correctly, what this does is allow a mod to disable some "roll the dice" damage computations, most likely because they're replaced by another mechanism? If so, I suggest giving more accurate names than "Alter..." (say "Bypass..." or "Skip..."?), given the effect is to actually disable something (replacing them with something else is what the mod does, not those functions). Adding some comments to explain the purpose of the new functions is also always a good thing.

As a second thought, I wonder if it wouldn't be simpler (hence easier to understand), and yet more flexible, to allow to override the whole calling function (ApplyDamageToPlayer, etc.), so your mod could disable them by overriding them with a NoOp, but also other mods could also replace them with another computation. As usual they're also drawbacks (what happens if we fix bugs in core, what happens if more than one mod wants to modify those functions,...) and I'm not very familiar with modding in general, so this is at best just an idea. You may even have dismissed it for good reasons that I failed to consider.

I really just put it where I did because it seemed least likely to be effected by future updates to that particular area of code, while also being able to skip over the rest of the code in that method, if the modder wanted to. I'm open for suggestions obviously, besides changing the name as you mentioned, I can't really think of a better way in this case to achieve what I'd like here. In my case I'd like to be able to change as much of the original combat function as I'm allowed, especially things like the knockback and when sound are played, etc.

[KABoissonneault]

While I think it makes sense to move hardcoded behavior to overridable events, I am not a fan of the approach.

1. moddedPhysicalAttackBehaviorEnabled is not necessary. This is an extra step for modders, the bool is in a weird spot (enables 3 events while only 1 event is related to the bool), and removing it does not change affect the rest of the change (the default 'Alter' functions all return false)
2. This is not very consistent with the rest of FormulaHelpers in how it handles the original logic. Usually, all the code is found in FormulaHelper, while this PR instead leaves all the logic in the original classes.
3. This is not consistent with the rest of FormulaHelpers in what it does. FormulaHelpers are normally for simple "if Player throws spell Fireball, how much damage would this Rat take?". These overrides are meant to handle skilling, enchantment payloads, crime, sound, etc...

I think this should avoid FormulaHelper entirely, and just use normal C# delegates. An example would be the ones we added in EnemySenses. The delegate returns a bool for whether the mod wants to prevent the original code, same idea as yours. Since this goes in EnemyAttack mostly, it should be fine like that

[magicono43]

Alright, thanks for the input, next time I get the chance I'll try to revise the PR (however that is done.) I'll also have to take a look at how you used delegates in EnemySenses and see if I can replicate something similar for the combat related classes and such.

[magicono43]

Sorry, not sure if "request review" pinged you or something, obviously still not too familiar with the whole pull request thing. Probably should have made this PR a draft first, whatever difference that really makes.

Either way, I changed quite a bit after doing a little reading on Dunny's example in EnemyMotor, as well as just general reading on Delegates and such. Not sure if my revision here would do exactly as I was intending, but hopefully those reading this could give some wisdom on how this might be getting colder or warmer. I think it is looking better atleast, but not certain on a few things, especially the fact WeaponDamage was public, so I'm not sure if that could potentially effect mods that called that method and such.