alliedmodders / metamod-source

Metamod:Source - C++ Plugin Environment and Detour Library for the Source Engine
http://www.metamodsource.net/
Other
383 stars 87 forks source link

Templated version of sourcehook.h #134

Open mixern6 opened 12 months ago

mixern6 commented 12 months ago

Hello, I'm using sourcehook in my extensions and would like to propose that it should be changed. I see several drawbacks in the current implementation:

One templated class could replace all the SHDECL* macros and solve these problems.

template<typename R, typename... A> // return type and argument types
class VirtualHookHandler
{
public:
    void Reconfigure(...);
    void Enable(...);
    void Disable(...);
    void AddListener(...); 
    void RemoveListener(...);
    void CallOriginal(...);
    ...
};
dvander commented 12 months ago

If you'd like to submit PR with a working prototype I'd be happy to review it.

The macros cannot be completely obsoleted but I think there's a strong chance the easy cases could be more modern.

mixern6 commented 11 months ago

The implementation is half-ready. I had to get rid of all the static stuff and now faced a problem that the HookManagerPubFunc is a member function, but RemoveHookManager, AddHook and RemoveHook only accept pointers to static functions (HookManagerPubFunc). Have no idea how it can be bypassed as HookManagerPubFunc can't be turned back into a static function because I can't generate random names for different hooks like macros do.

dvander commented 11 months ago

Very cool, thanks for looking into this. Feel free to share patches and I can take a look.

Note that we have to stay ABI and API compatible but new additions should be no problem, the interfacs are versioned.

One solution to HookManagerPubFunc would be to add a new interface method to take in a virtual object, with a DeleteThis method if SH will own the pointer. Unfortunately due to linkage restrictions we can't malloc/free or share stl structures across library boundaries so std::function won't work.

mixern6 commented 11 months ago

I have some progress on the task: https://github.com/mixern6/metamod-source/commit/dc8d8701433e3167027f78c0311d5990a813b04f One moment is confusing to me. I almost got rid of the MFHCls structure, moved out all the functions. Now I have reinterpret_cast<void*>(this) across the code which point to the CVirtualHookHandler instance, should it be the same in the last argument of SetInfo (line 5267)? Like: reinterpret_cast<void*>(reinterpret_cast<char>(this) + mfi.vtbloffs)[mfi.vtblindex]

The structure is initially empty, no data members. Is this a sort of hack or am I missing something?

mixern6 commented 10 months ago

@dvander Could you please have a look at the current version. It compiles without errors, but the metamod doesn't start and shows this error: "You must change the map to activate Metamod:Source." https://github.com/alliedmodders/metamod-source/compare/master...mixern6:metamod-source:templated_vhooks The hook wrapper code is not used, I believe the problem is related to the function wrapper "HookManagerPubFunc"

psychonic commented 10 months ago

@dvander Could you please have a look at the current version. It compiles without errors, but the metamod doesn't start and shows this error: "You must change the map to activate Metamod:Source." master...mixern6:metamod-source:templated_vhooks The hook wrapper code is not used, I believe the problem is related to the function wrapper "HookManagerPubFunc"

Is that error when using meta before or after issuing map <mapname>? It would be normal for that message to be printed beforehand.

mixern6 commented 10 months ago

It shows when I type "meta" and even after changing the level. If I revert my changes, metamod loads and works without errors.

UPD: I've just checked the wrapper "HookManagerPubFunc" If I change it back to function pointer, metamod works. Don't see what is wrong with the wrapper though