Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Templated version of sourcehook.h #134

Open
mixern6 opened this issue Oct 12, 2023 · 7 comments
Open

Templated version of sourcehook.h #134

mixern6 opened this issue Oct 12, 2023 · 7 comments

Comments

@mixern6
Copy link

mixern6 commented Oct 12, 2023

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:

  • The code in sourcehook.h is hard to read.
  • Debugging the code generated by SH_DECL_* macros is difficult because they contain many nested macros with generated variable names.
  • The macros cannot be wrapped into classes and they always have to be defined at the top of the CPP file.
  • All the hooks have to be managed "manually" as there is no RAII to handle them.
  • There is a separate macro for different number of arguments.
  • The macros are not well handled by Visual Studio and Qt Creator.

One templated class could replace all the SH_DECL_* 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
Copy link
Member

dvander commented Oct 12, 2023

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
Copy link
Author

mixern6 commented Oct 19, 2023

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
Copy link
Member

dvander commented Oct 19, 2023

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
Copy link
Author

mixern6 commented Oct 29, 2023

I have some progress on the task:
mixern6@dc8d870
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
Copy link
Author

mixern6 commented Nov 19, 2023

@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"

@psychonic
Copy link
Member

@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
Copy link
Author

mixern6 commented Nov 19, 2023

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

zer0k-z added a commit to zer0k-z/metamod-source that referenced this issue Jun 7, 2024
Add CConcreteEntityList, CEntityComponent, CScriptComponent, CGameEntitySystem, rewrite IHandleEntity to use CEntityHandle instead of CBaseHandle, update NUM_SERIAL_NUM_BITS, comment out old CBaseEntity, obsolete basehandle.h
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants