Trinitycore: Weird script base implementation

[Romain-P]

Description:

We can see CreatureScript and PlayerScript classes are both derived of UnitScript.

• https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Scripting/ScriptMgr.cpp#L2187
• https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Scripting/ScriptMgr.cpp#L2280

When the parent class is called in the constructor, the second parameter is set to false. That means the UnitScript won't be added.
Why ?

Current behaviour:

I guess it was to avoid doubloons of the same instance in 2 differents registries (UnitScripts and PlayerScripts).
But now assuming I'm creating a custom CreatureScript (which also is a derived class of UnitScript), the UnitScript hooks can be overrided but will never be triggered. This is true for every derived class of UnitScript calling the constructor with false that actually means Do Not Add To UnitScripts Registry.

PlayerScript is indeed an UnitScript also called with false in the constructor, but a hack patch has been released long time ago, I guess, see:

• https://github.com/TrinityCore/TrinityCore/blob/3.3.5/src/server/game/Scripting/ScriptMgr.cpp#L2084

We can notice that for each UnitScript hook, a new line that shouldn't be here is added to fix PlayerScripts overriding UnitScript hooks.

void ScriptMgr::OnHeal(Unit* healer, Unit* reciever, uint32& gain)
{
    FOREACH_SCRIPT(UnitScript)->OnHeal(healer, reciever, gain);
    FOREACH_SCRIPT(PlayerScript)->OnHeal(healer, reciever, gain);
}

This is actually a hack.

Expected behaviour:

2 possibilities:

• Add new hacks for all UnitScript derived class
• Put true to the constructor so the script instance will be in both registries

Or maybe a third I don't see right now.

Steps to reproduce the problem:

N/A

Branch(es):

3.3.5

TC rev. hash/commit: 58c5dfac1d8d314b9243fde52466b3c933fb7d7f

LATEST

Operating system: ALL

[TrinityCoreBot]

This issue is not considered valid because of the following reasons:

• No proper commit hash (revision)

Please read http://bit.ly/tc-issuetracker-and-you and add the missing information to this issue. Thanks.

[Shauren]

It was done to prevent double free when scripts are unloaded (shutdown/hotswap)
It would also be strange to write a PlayerScript with OnHeal hook and get it called with creature argument (as Unit*)... which is also what the current hack also causes

[Romain-P]

I dont get you, who cares about the argument. OnHeal hook is working good for player scripts due of the hack.
I mean, with the current code, OnHeal won't be triggered e.g unto a CreatureScript

[Romain-P]

The current Implementation is not well-designed, why would a CreatureScript be a derived class of UnitScript if it doesn't have any benefit. And I mean by benefits, having the possibility to hook UnitScript events.

[jackpoz]

could you describe in detail what's your case ? what are you trying to code with a PlayerScript in the OnHeal hook ?

[Warpten]

Some SFINAE magic?

template <typename T>
struct ScriptRegistry {
   template <typename U = T>
   void AddScript(U* o) { 
       scripts.push_back(o);
       if constexpr (std::is_base_of<T, U>::value) // c++17
          owned_scripts.push_back(o);
   }

   void free() {
       for (auto&& itr : owned_scripts) delete itr;
   }

   std::vector<T*> scripts;
   std::vector<T*> owned_scripts;
}

Not pretty but i think this can do an acceptable job?

[Romain-P]

https://github.com/TrinityCore/TrinityCore/blob/master/src/server/game/Scripting/ScriptMgr.h#L416

class TC_GAME_API CreatureScript : public UnitScript, public UpdatableScript<Creature>

A CreatureScript should be able to override UnitScript hooks.
So the following code should work, but it doesn't!

class MyCustomCreatureScript : public CreatureScript {
public:
    MyCustomCreatureScript() : CreatureScript("MyCustomCreatureScript")

    void OnHeal(Unit *a, Unit *b, uint32 &c) override {
        //this scope will never be triggered
        sWorld->SendWorldMessage("This message willl never be sent, cuz the hook is not registered")
    }
}

[Romain-P]

Not pretty but i think this can do an acceptable job?

By the way, a smart pointer could be enough e.g std::shared_ptr. But a decent implementation would be even better, cba using shared pointers for this lol..

[Warpten]

I despise shared_ptr because it has all sorts of quirks (enable_shared_from_this can't work within ctors, for one) and because generally speaking there are more efficient ways to track object lifetime than refcounting. Not the point though

[jackpoz]

Taking a look at the history of the implementation, it's just the result of 2 PRs, https://github.com/TrinityCore/TrinityCore/pull/7867 and https://github.com/TrinityCore/TrinityCore/pull/10981 .
Currently these hooks are never used in TC scripts so we have no clue how our users would want to use them. You shouldn't consider these hooks to be the primary way to create custom TC scripts.

What would make more sense would be to have 1 single type called GlobalHooks with no inheritance and with all the hooks exposed. Or remove the inheritance as that's how the original code from which the PR was created looked like.

As asked above, please give us some infos about what you are trying to do with these hooks so we can better understand the real issue and think about a solution. I don't think you want to call sWorld->SendWorldMessage(..) every time any Unit is healed.

[Romain-P]

I've added more hooks on my personnal trinitycore, that's why you might be confused about my usage of UnitScripts.
My hooks are useful for both players and monsters so, to avoid copy/past or even more code, I've just put the new hook in the UnitScript class.

I don't need help, I'm just notifying that it seems generic, but it's not. Weird codes should be reviewed that's all.

[jackpoz]

This strange design started with https://github.com/TrinityCore/TrinityCore/pull/7867#issuecomment-8854396 , I opened a PR to restore the original design.

A better naming like PlayerGlobalHook/UnitGlobalHook would also clarify better what these "scripts" are about.

[jackpoz]

https://github.com/TrinityCore/TrinityCore/commit/5b7ead65bb116195f4336934bcff830e1b2af5bb