-
-
Notifications
You must be signed in to change notification settings - Fork 456
[Dhooks] Add the ability to work with the this parameter, which is an object #2219
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, especially given the situation that surround Address. The less we mess with memory directly in plugin, the better !
I just have a few request changes, but otherwise I think this is a good addition to dhooks.
|
I think I've finished this PR, I just need to check |
|
I tested it with this type address, everything works fine! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for seeing this through, it looks great! I'm going to let this sit a for little while longer, so others may review it as well. Otherwise I think this is ready to be taken in, thanks again!
|
Hey, following up on this PR — I just wanted to know if it’s still being considered. |
|
Still very much considered. I was going to merge this, but I'm currently rewriting dhooks from scratch (see Should the rewrite fail, I'll totally merge this. Should it succeed, I'll instead port the features of that PR over. |
|
I understand that you're focused on rewriting SourceHook, and merging it right now might be impractical, but I'm afraid it will take a long time, with source hook-related changes and testing. People won't see this change anytime soon, which is a shame. |
|
I refactored the code, moving similar blocks into a separate function in the old code, and added my own. As a result, the code I added looks like this. I've moved it here in a comment for easier understanding: |
I'm okay with merging this into 1.13 as I'll be rewriting it later anyways, however no backport for 1.12. (But feel free to convince the maintainers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still okay with merging with this, two needed changes.
The current implementation of SourceMod 1.12 with DHooks has limitations when working with the this pointer. Some plugins may crash or exhibit undefined behavior if used incorrectly. Additionally, certain types, such as EHANDLE, cannot be accessed correctly from within a plugin. This extension addresses these limitations, improving stability and usability while reducing memory-related issues. I often had to choose between writing a C++ extension or a plugin because the current API was limited, making simple fixes cumbersome. Until now, users were forced to write their own custom solutions for different types, such as the FollowTarget_Detour plugin for working with EHANDLE. I believe users should not need to rely on the unstable branch to access these improvements. The changes are minimal and do not break the existing plugin API. Furthermore, this PR has been open for a long time, and it would be beneficial for these improvements to be integrated and available. Since major changes are planned for the 1.13 branch, which will require extensive testing, it might make sense to make an exception for this PR and merge it into the 1.12 branch so that users can safely benefit from the improved functionality. |
Sometimes we have to work with this parameter which is a class but dhooks doesn't have this feature, this PR solves this problem. This code has been tested on both post and pre hooks. Below is an example code for working with the new functionality.