search

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

Add templated handler for manual hooks #150

Open mixern6 opened 10 months ago

mixern6 commented 10 months ago

Add templated wrapper for manual hooks. Simple hooks are not supported yet and will be added later. The handler supports any return type and arbitrary number of arguments. A hook can be defined like this: // void - return type // const char* - first argument SourceHook::ManualHookHandler<void, const char*> setEntityModelHook; Then it can be reconfigured: setEntityModelHook.Reconfigure(offset); And this way it can be connected to a callback method: const int hookID = setEntityModelHook.Add(baseEntityPtr, this, &MyClass::OnSetEntityModel, true); Remove the hook: g_SHPtr->RemoveHookByID(hookID);

the Recall method can be used instead of the RETURN_META* macros.

mixern6 commented 10 months ago

@psychonic what would you suggest to fix the tests?

dvander commented 10 months ago

At a glance, this looks ABI-safe and source compatible which is my main concern. I'll take a more in-depth look soon.

dvander commented 10 months ago

Looks like there are some build failures in the tests that need to be resolved, too.

mixern6 commented 10 months ago

@dvander can't figure out the right way for sorting out the compilation errors. It looks like I can't simply rename the variables or place them into separate name spaces and yet I have to keep the extern ones in the header. What would you suggest?

Do you think it is important to have the void Reconfigure() method? It seems to me the method and the related variables are not actually required.

I'd like to make the constructor constexpr, but this playing with the memory addresses in msProto_ makes me fill uncomfortable. Would be nice to have something like ProtoInfo<Params..> : public IProtoInfo instead

mixern6 commented 10 months ago

@dvander all the errors are fixed. Could you have a look please?

dvander commented 10 months ago

This is a huge improvement to the API, thanks for working on this! I'm up against some tight deadlines this week so I was only able to get halfway through the review. I'll try to do the rest as soon as I can.

Mostly, I'm looking for compatibility issues that would break existing binaries, and for any maintenance/readability issues in the new code. There's not much: it's a great change, but it's also a big one and I want to do it justice.

Please do include tests for the new API. test2.cpp and testmanual.cpp could probably be copy & pasted and refactored into the new world order. (That will also help me validate with valgrind/asan).