Security Check around spelladding

[kjk001]

Hi,

when adding spells you directly call: spellList.SpellsByLevel[2].m_Spells.Add(FuryOftheSunAbility.ToReference<BlueprintAbilityReference>()); with no security check, this method makes no check if the spell is already in the list and will actually create duplicates. This is a problem for spontaneous casters because while the spell is still added, it breaks the ability to select it on levelup if it is in the list multiple times.

If you check the following condition first and only add the spell if it is not fullfilled:

private static Boolean listContainsSpell(BlueprintSpellList list, BlueprintAbility spell) {
            foreach (var level in list.SpellsByLevel) {
                foreach (var comparespell in level.Spells) {
                    if (spell.AssetGuid.m_Guid.ToString("N").Equals(comparespell.AssetGuid.m_Guid.ToString("N"))) {
                        return true;
                    }
                }
            }
            return false;
        }

You will prevent such duplicates.

I noticed this because I wrote my own little addon to also add the TomeOfTheFirebird spells to the class and decided to be clever and just merge every major spellbook as a rebuild after all the other mods are loaded in(since this class has them all anyway) only to have the ExpandedContent spells dissapear on me.

PS. if someone else did the work for you, would you be open to switching the project over to using TabletopTweaks-Core for its utility classes rather than your own and automatically publishing to the wrath folder?

[kjk001]

ps. don't trust the check list.SpellsByLevel[level].Spells.Contains(spell) in the spelltools of the tabletoptweaks-core, that one will still break if one spell was referenced from the cleric and the other from the wizard list even if both are the same spell and have the same GUI Id

[JohN100x1]

It was the most simple and direct way to add the spells I had thought of at the time. I hadn't really thought about adding a check. But I think this is a good thing to add to help with any future people who want to work on this mod.

edit: let me know what you think of the code. My main programming language isn't c# so it might look unconventional

[kjk001]

speaking of other people working on this mod, what about my first ps question?

It is not my main language either but overall I would say it looks no better or worse than the code of any other mod I have seen. A missed security check can happen to anyone and at least yours was not the kind that breaks the game if an update changes something the slightest bit.

[JohN100x1]

I don't mind, most of the modding framework was based on TabletopTweaks-Core anyway and refactoring should be a minor issue. I can add you as a contributor if you're interested.

[kjk001]

Then I would like to offer to do it for you.

[kjk001]

Ok, change created as a new branch TTTCmerge and ready to be commited and reviewed once I am added.

Now, for my next projects I would volunter to add my already existing spelllist merge logic to the Postfix method, that should ensure we grab any spell as long as it is added to at least one base class spellbook and not initialized only after all the normal patching is already done...

After that I would take a stab at some Archetypes like Bloodline Hero, Isekaid Oracle, and Isekaid Tactician. Assuming there are not already versions of them being worked on.

Edit: I had overlooked that I already had an invte, so have fun reviewing.

[JohN100x1]

go for it! I'm a bit pre-occupied with other issues at the moment so I won't be on this repo as frequently