Added CharacterClassOverride Table

[tegstewart]

Adds CharacterClassOverride table, which can be used to override the following class properties: Spec multiplier Base HP Base weaponskill, both melee and ranged Max pulsing spells Auto trainable skills Eligible race/class combos

Everything but ClassID is optional, so only properties you wish to override need to be entered.

DB entries are stored in a static dictionary, which is pulled from after inherited constructors have set hardcoded defaults.

A lot of classes had redundant hardcoded properties, which were already set to the same value by classes they inherit from. I removed most of those to allow base classes to be modified via the DB rather than using multiple rows to alter the inherited classes. As an example, Classid 0 can be used to set max pulsing spells to 2 for all classes, though the handful of classes that already have it set to 2 will still take precedence, or the Mystic ClassID could be used to change spec multiplier for all Midgard cloth casters.

A few other eccentricities had to be addressed: Max pulsing spells was only being checked > 1, which hard capped max pulsing spells at 2. That has been resolved, so classes set to run more than 2 pulsing spells at once are able to do so. Adding static methods required removing CharacterClassBase being abstract. This made DefaultCharacterClass redundant.

[tegstewart]

I'm going to modify the test DB to include the new table.

[tegstewart]

I'm utterly confused by this. These tests pass when I run them locally. The only tests failing are GetArmorAbsorb_LevelZeroNPCHasOneConstitution_Circa5Percent() and ApplyEffectOnTarget_50ConBuffOnL50NPC_51Constitution(), which also fail when I run them on the current master branch locally.

[tegstewart]

I'm going to need somebody else to go over this, because I cannot reproduce these errors locally, and am thus having to guess at what is causing the problem.

[NetDwarf]

I am looking into it. Right off the bat there seems to be a problem with initializing FakePlayer. But haven't reproduced yet. Have you tried removing build and Debug/Release folder before recompiling/testing? Because depending on the problem you might use artifacts from previous builds that let the tests pass.

[NetDwarf]

Two columns removed from the table since they are not really used and it's difficult to take it back afterwards. If we really need it we can still add it later, however I feel like MaxPulsingSpells might be a better fit for a ServerProperty and changes around Songs/PulsingSpells in contrast to an additional column. BaseWeaponSkillRanged is not CharacterClass dependent as far as I can tell. For the old mechanics it was dependent on the bow and it does not apply for new archery at all.

[tegstewart]

How would you implement changing MaxPulsingSpells for specific classes via server property?

[NetDwarf]

Not sure, but I would not add it as part of the official database since it's not how DAoC works. Over the time they only changed the number of simultaneous pulsing spells and songs. They started with one pulsing spell and one song maximum iirc. Currently it's 8 pulsing spells max and infinite songs as far as I can tell.

Can you override it in a subclass? i.e.

[CharacterClass((int)eCharacterClass.Warden, "Warden", "Naturalist")]
public class ClassWardenReloaded : ClassWarden
{
  m_maxPulsingSpells = 8;
}

[tegstewart]

Overloading is possible, but I'm trying to put as much control as possible into the DB. That allows us to support all versions of live out of the box, and I've already got a few DB customizations ready to go once this PR is finalized which bring a few of the things in CharacterClass closer to live.

I'm also working on a DB only PvE solo server, and having to maintain a code base I have to regularly merge DOL changes back into is a PITA.

[tegstewart]

It looks like we need a check in LoadClassOverrideDictionary() to keep it from trying to load the dictionary more than once and throwing an ArgumentException.

It's also only calling LoadClassOverride for the specific class being loaded i.e. only loading ClassID 10 when I load a Friar, when it should be calling LoadClassOverride for ClassID 0, ClassID 16, and lastly ClassID 10, so that multiple classes can be altered with a single row rather copy/pasting for every ClassID you want to update.

[NetDwarf]

Overloading is possible, but I'm trying to put as much control as possible into the DB. That allows us to support all versions of live out of the box, and I've already got a few DB customizations ready to go once this PR is finalized which bring a few of the things in CharacterClass closer to live.

You could use a serverproperty for changing pulsing spell maximum for all classes. A class-specific maximum is not really part of Live as far as I can tell.

It looks like we need a check in LoadClassOverrideDictionary() to keep it from trying to load the dictionary more than once and throwing an ArgumentException.

I blame Autopilot™ :facepalm: The not so clever cousin of Copilot.

It's also only calling LoadClassOverride for the specific class being loaded i.e. only loading ClassID 10 when I load a Friar, when it should be calling LoadClassOverride for ClassID 0, ClassID 16, and lastly ClassID 10, so that multiple classes can be altered with a single row rather copy/pasting for every ClassID you want to update.

The approach to accidental same data reuse is not really recommended. It makes the code more bloated and harder to maintain as if you'd just copy the data out and not reuse it at all. I am inclined to do just that actually. That's also how we ended up with this weird inheritance tree.

[NetDwarf]

Moved the call to LoadClassOverrideDictionary() up to SkillBase init.

edit: That being said: More than likely this is going to be replaced anyway, but it should be okay in the meantime. So in case of fire it's mergeable :slightly_smiling_face:

[tegstewart]

I'm using class specific max pulsing spells, mostly for balance purposes. From a solo PvE perspective, a skald is a paladin with worse armour and fewer spec points, so being able to run all their chants at once helps balance them out, but paladins being able to run all their chants at once maintains the imbalance.

[tegstewart]

The approach to accidental same data reuse is not really recommended.

That's a solid point. I know how the inheritance works, so I can use it effectively, but somebody who isn't familiar with the code could easily get it wrong, and have no idea how to fix it.

I hate the idea of having to put in dozens of nearly identical DB rows when just one could potentially do it, but it's better to have that complexity in the DB rather than in code.

[NetDwarf]

Is there anything missing, @tegstewart ? Otherwise I merge it :slightly_smiling_face:

I hate the idea of having to put in dozens of nearly identical DB rows when just one could potentially do it, but it's better to have that complexity in the DB rather than in code.

But for custom behavior something like that is needed, however there is also a breaking change planned, which should get rid of a lot of classes. Alternatively you could adjust the spells themselves or do adjustments how the spells work like on Live (i.e. instead of combat heal chant, instant heal every 30s).

[tegstewart]

Go for it. Looking forward to seeing the new change, and I can revisit it then.

Thinking of putting pretty much everything class related in the DB?

[NetDwarf]

Thinking of putting pretty much everything class related in the DB?

Thinking about it yes, not yet done tho. Currently I use a hardcoded dictionary of all (default) CharacterClass properties much like a DB and replacing all subclasses of CharacterClassBase. The database is untouched just yet.

See: https://github.com/NetDwarf/DOLSharp/tree/CharacterClassRefactor

CharacterClassBase is the starting point here. CharacterClass and DefaultClassBehavior was extracted from it.

This way a lot of redundancy was removed and especially the absurd amount of subclasses. Going forward Behavior is going to be inlined, but it's probably not part of this PR, because doing it mechanically would be worse I think (i.e. adding code to the god class GamePlayer).