Master Level Overhaul

[Graveen]

i'd check the .Add() personnally. Can you monitor the lists ?

[Leodagan]

Well I was using SharpDevelop Profiler around this but it mostly reports "Method Times" not really Heap or Stack Size...

I check the content of the collection returned by this method and the content is always short (about a dozen of elements), I now that the result must be short and most "re-ordering" method are expensive, the bad behavior comes from some kind of reference that doesn't get garbage collected during this !

[Graveen]

do you have same behaviour when compiled as release build ?

[Leodagan]

That was a good hint but yes it's still there in "release" build, even if the code is 3 time faster then

This doesn't happen with list spells that query the same collections but use a lot less "grouping" so I think I can narrow it for now

(Spamming a Trainer will rebuild Max Level skill list for display each time, this is perfectly harmless even for Hybrid as the spec code rely on list caster display to show ALL hybrid spells !)

I'll try to comment some part and run test again, or use different code even if it gives a broken spell list, just to target the line where there is this leak

Edit : the real weird thing is that memory don't get collected even when player is logged out... I can manage to make a 400MB overflow and it never gets collected !

[Leodagan]

OK I'm a jerk...

I was testing List Casting Spell on an Hybrid Class, without even looking at the returned count "0"...

When testing on a list caster, the spell list also trigger a memory leak, so that explains a lot of weird things I saw in profiler

So the definitive leaker is my new method "SkillBase.GetSpellList(string line)", which is trying to clone each Spell object before sending it (preventing from modifying SkillBase objects !!), and this leads me to the new "Tooltip" code within Spell, which is probably triggering something as it is called by the Spell constructor on Clone()'ing...

I think this whole tooltip thing handled in spell object is pretty bad, for all other skills I rely on a database field and it already have some flows :

GameEffect, like Berserk triggered by an Ability, doesn't have a TooltipID when it should...
Abilities should trigger SpellEffect everytime ! I can't use an Ability ID to target an Ability Effect, client expect a spell ID for buff/debuff display icons.
We shouldn't be handling ANYTHING else than GameSpellEffect (to display in player UI...)

So I'm not gonna rely on weird code to support tooltip just for spell when there is nothing around fixing berserk or such effects, we don't have a portal working with 1.115+ client anyway

Shards Admin / DOL User will need to update database records to have a flawless 1.115+ experience...

[Leodagan]

I removed all "Auto Generating" Tooltip code...

I think the only part that could need some specific behavior is around scripted spells, DB Spell should use a unique ID and that's all, I don't check if there is duplicate in DB, it could be needed for some reason (subspell displaying parent spell tooltip ?) and I don't register Tooltip from scripted spell that are already used for a DB spell (for easy debug, a scripted spell displaying a DB spell description will mean it needs an other ID)

I'll provide some easy query to fill up Spell Table with unique values, and this still doesn't use the primary key SpellID so it won't change any other behavior than display in 1.110+

This removed all the Memory leaks I had and made the "skills" computing a bit faster !

Actually the CPU time of rebuilding the skill list is under 1 ms (400ms/1000 iteration for hybrids, 150ms/1000 iteration for list caster)
And latency went down from 35~40ms to ~20ms (first time building skills on connect)

[Graveen]

Totally acceptable :p

[Leodagan]

I'm starting to work on migrating champion spell to my new methods.

Here is how I'm going to build it :

I'm making a new Specialization type "MiniLineSpec"

These specs can hold mini skill list without too much constraint (one skill by level is expected or it will become a mess, but it doesn't check it allowing for any use)

All spec methods return empty skills list to work as a "dead weight", then only the "Champion" Career will be able to gather these MiniLines together to offer Styles and Spells as Hybrid types

MiniLines are still relying on common spec mechanism and should have entries in SpecXability, Spelllines or Styles

I will gather MiniLines by "Base Class Trainer" using a common subclass (which doesn't override anything), these subclass will be hardcoded for now, CL are pretty much specific and I don't see any other "Custom Behavior" that would need such specific code to handle skill trees

The purpose is to keep most of code "common" and still allow to easily update skill tree by editing database...

If any new skill tree is needed it's going to need a small subclass similar to any character class implementation...

If anyone want to completely customize how MiniLines or Subclass lines are granted to player they can still use the same database logic but a different "ChampionCareer" class to handle how the skill are gathered and/or granted.