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

Add support for all hook libraries that exist not just ulib/ulx #3244

Merged
merged 1 commit into from
Jan 21, 2024

Conversation

Srlion
Copy link
Contributor

@Srlion Srlion commented Jan 15, 2024

This will make it work with any hook library not just ulx/ulib's library, like my hook library https://github.com/Srlion/Hook-Library

@FPtje
Copy link
Owner

FPtje commented Jan 17, 2024

Thanks for the PR! It looks like you've found some of DarkRP's ugliest, but also some of its most revised code. Hooks are a huge pain point in Gmod. The default system is lacking, and alternative systems very often break some niche edge case that some addon somewhere ends up depending on. Not to mention the amount of times some addon incorrectly just always returns a value in some hook, silently breaking everything else. Ugh

I see your addon is a bit more explicit about returning values, and that your code in this PR is a lot shorter and more elegant than what is currently there. I think that's really nice.

One huge worry of mine, though, is these edge cases. Between this PlayerSay thing, and the bad hooks I've disabled in the workarounds files, there's a big history of fixing things w.r.t. other addons. How do we know that this change would not regress these fixes? This is an open question, and a hard one. Maybe the answer (partially) lies in how you tested this change.

Thanks again!

@Srlion
Copy link
Contributor Author

Srlion commented Jan 17, 2024

I checked workarounds and found 2 for PlayerSay

  1. CAC is dead
  2. ULXMeCheck and tested if it gets removed or not and it indeed gets removed:
FAdminChatCommands      =       function: 0xf0bbbd72
FAdmin_Chatmute =       function: 0xf0d8ef2a
FSpectate       =       function: 0xf0a8aba2
ULXGimpCheck    =       function: 0xf0ce99fa
ULXLogSay       =       function: 0xf0c93ae2
ULXMeCheck      =       function: 0xf0d90c6a
ULib_saycmd     =       function: 0xf0db4ff2
ulxPlayerSay    =       function: 0xf0ceade2
------------------
FAdminChatCommands      =       function: 0xf0bbbd72
FAdmin_Chatmute =       function: 0xf0d8ef2a
FSpectate       =       function: 0xf0a8aba2
ULXGimpCheck    =       function: 0xf0ce99fa
ULXLogSay       =       function: 0xf0c93ae2
ULib_saycmd     =       function: 0xf0db4ff2
ulxPlayerSay    =       function: 0xf0ceade2

@FPtje
Copy link
Owner

FPtje commented Jan 21, 2024

Thanks for this PR! I see no objection to merging it. Let's merge it and see if people start complaining. If they don't for long enough, I can upload it to workshop 👍

@FPtje FPtje merged commit 1b77898 into FPtje:master Jan 21, 2024
4 checks passed
@Srlion
Copy link
Contributor Author

Srlion commented Jan 22, 2024

Thank you <333

@FPtje FPtje mentioned this pull request Jan 24, 2024
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

Successfully merging this pull request may close these issues.

2 participants