X2CommunityCore / X2WOTCCommunityHighlander

https://steamcommunity.com/workshop/filedetails/?id=1134256495
MIT License
60 stars 68 forks source link

Add New AbilityTagExpandHandler To Allow Reflection #419

Open Onidotmoe opened 5 years ago

Onidotmoe commented 5 years ago

Currently AbilityTagExpandHandler in X2DownloadableContentInfo doesn't allow much dynamic handling and trying to expand the existing AbilityTag class without overriding it or getting deadlocked by its need for native(core) or how doing :

XComEngine(class'GameEngine'.static.GetEngine()).LocalizeContext.LocalizeTags.AddItem(new class'X2AbilityTag_Custom');

Does nothing.

I propose changing(in X2AbilityTag.uc) :

//  allow DLC info to handle the tag
    DLCInfos = `ONLINEEVENTMGR.GetDLCInfos(false);
    foreach DLCInfos(DLCInfo)
    {
        if (DLCInfo.AbilityTagExpandHandler(InString, OutString))
            return;
    }

`RedScreenOnce("Unhandled localization tag: '"$Tag$":"$InString$"'");
OutString = "<Ability:"$InString$"/>";

to

//  allow DLC info to handle the tag
    DLCInfos = `ONLINEEVENTMGR.GetDLCInfos(false);
    foreach DLCInfos(DLCInfo)
    {
        if (DLCInfo.AbilityTagExpandHandler(InString, OutString))
        {
            return;
        }

        if (DLCInfo.AbilityTagExpandHandler_CH(InString, OutString, ParseObj, StrategyParseObj, GameState))
        {
            return;
        }
    }

`RedScreenOnce("Unhandled localization tag: '"$Tag$":"$InString$"'");
OutString = "<Ability:"$InString$"/>";

and adding another static function to X2DownloadableContentInfo

static function bool AbilityTagExpandHandler_CH(string InString, out string OutString, Object ParseObj, Object StrategyParseOb, XComGameState GameState)

Will preserve compatibility with existing mods and will allow new mods to use it.

Allowing this level of reflection will help make neat code that doesn't use a switch monster of the likes in vanilla X2AbilityTag.uc

Would also mean that you could dynamically change the display text of abilities in the tactical game.

Ofcourse would do it myself but currently too deep in on my current mod to try to fix the highlander, can't run the game without it just crashing when changing anything.

robojumper commented 5 years ago

Have you considered adapting a solution from XModBase? XModBase wraps the existing tag with its own tag, and delegates any requests it doesn't handle to the wrapped tag. Code pointers:

https://github.com/RossM/XModBase/blob/80478952694f730fc9f7bf7fef4f6ce835cfb1ff/XModBase/Src/XModBase_Core_2_0_1/Classes/XMBDownloadableContentInfo_XModBase.uc#L43-L81

https://github.com/RossM/XModBase/blob/80478952694f730fc9f7bf7fef4f6ce835cfb1ff/XModBase/Src/XModBase_Core_2_0_1/Classes/XMBAbilityTag.uc#L197-L206

I'm not super opposed to adding a hook like this to the HL, but there are non-HL options that work equally well.

Onidotmoe commented 5 years ago

Wouldn't this cause compatibility issues with the Highlander? From what i can tell there is no cleanup that would ensure an old Tag inside the Engine not being used if the user loads a game where they have disabled the host mod. This would cause a crash.

Also my solution was much neater, not touching the XComEngine class and because it would be inside that loop, it will never call a function in a mod that isn't active.

robojumper commented 5 years ago

There are no compatibility issues with this method, and there is no cleanup required -- nothing of this persists in the save. The localization tag structures (in fact, the whole Engine class and associated systems) are created when launching the game, and discarded when the game closes, just like templates.

Onidotmoe commented 5 years ago

Dunno seems a bit too dirty, is there any reason why we don't implement things like this into the Highlander so people can mainly rely on the Highlander for these things? Also i was thinking of maybe adding a collection of Helper functions to the Highlander to make it easier for modders to update staffslots, get certain slots or change abilities without needing to do all the legwork themselves each time.

robojumper commented 5 years ago

I'm not opposed to adding an enhanced hook here, but the reason we don't have it already is that it wasn't needed yet -- the "wrap the tag and replace it" solution worked well, so nobody added a Highlander solution. And because there wasn't a Highlander solution, people kept using the "wrap the tag and replace it" method.

That said, feel free to submit a PR here.

However, I'm not 100% keen on including any change aimed at reducing boilerplate code required -- almost all Highlander changes actually benefit on a technical level from being implemented in a Highlander, and third-party library mods like XModBase have worked well so far. Any changes increase the maintenance required in the future, and the Highlander is such a central part of the modding ecosystem that it should be somewhat conservative in scope. HOWEVER, there definitely is a case to be made for having a library mod ("X2WotCLib"?) under this GitHub organization; feel free to tell us if you're interested in getting something like this running (Highlander chat channel: see #399).

Onidotmoe commented 5 years ago

Ey a pre-build library that works with the Highlander would be very useful, it could also help to cut down on clutter and maybe also keep the quality of code up.

I'll join the discord.

Onidotmoe commented 5 years ago

Done : https://github.com/X2CommunityCore/X2WOTCCommunityHighlander/pull/423

Onidotmoe commented 5 years ago

Done Again : https://github.com/X2CommunityCore/X2WOTCCommunityHighlander/pull/429