★ wanayoo — archive 1999 https://github.com/alliedmodders/sourcemod/pull/981Nouvelle recherche | Portail wanayoo
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

Incorporate DHooks into base SourceMod #981

Open
wants to merge 11 commits into
base: master
from
Open

Incorporate DHooks into base SourceMod #981

wants to merge 11 commits into from

Conversation

@Headline
Copy link
Member

@Headline Headline commented Apr 21, 2019

Through discussion in #sourcemod we are planning on bringing DHooks in-tree, so I guess I volunteered for the dirty work.

There's still concerns about this and I also have yet to scour DHooks source for possible improvements so this is currently WIP.

@NiagaraDryGuy
Copy link
Contributor

@NiagaraDryGuy NiagaraDryGuy commented Apr 21, 2019

Next let's add in VoiDeD's transitional helpers and why not a color thing or two while we're at it 😄

Edit: Incorporate 👨‍🏫

@KyleSanderson
Copy link
Member

@KyleSanderson KyleSanderson commented Apr 21, 2019

Yikes, style doesn't match core/exts at all :(. I was under the impression this was going to be made more SDKTools like to match the existing API we have before a merge was proposed.

@Headline
Copy link
Member Author

@Headline Headline commented Apr 21, 2019

@KyleSanderson this is WIP; not a proposal. I wanted to see linux builds

Also wanted to see clear history of changes

@XutaxKamay
Copy link

@XutaxKamay XutaxKamay commented Apr 22, 2019

Hello would it be possible to implement the fix for the native DHookSetReturnVector before completing this pull request?
I've modified it recently for my sourcemod plugin.
https://github.com/XutaxKamay/dhooks/blob/master/vhook.cpp#L744

Previous code was **(SDKVector**) when it should be *(SDKVector*) I assume

@qubka

This comment was marked as off-topic.

@Drifter321

This comment was marked as off-topic.

@Fyren Fyren changed the title Incorperate DHooks into base SourceMod Incorporate DHooks into base SourceMod May 1, 2019
@Headline Headline force-pushed the dhooks branch from 889ae51 to 831a9f1 May 6, 2019
vptr += sizeof(void *);
paramInfo = (SourceMod::PassInfo *)malloc(sizeof(SourceMod::PassInfo) * dg->params.size());

for(int i = 0; i < (int)dg->params.size(); i++)

This comment has been minimized.

@KyleSanderson

KyleSanderson May 10, 2019
Member

Isn't the real fix here to make this unsigned?

This comment has been minimized.

@Headline

Headline May 13, 2019
Author Member

yes, I intentionally sided on this change as it was the cast that was used in other places throughout DHooks.

My plan going forward is to create another - separate - pull request that will merge changes into this branch that way we can separately review improvements to DHooks as a whole. It'll help with keeping history nice and tidy, unless you suggest that I keep it all in here. I'm not exactly sure what route is the best to keep history nice, but I imagine another PR based off this branch will help keep things clear

{
vptr += sizeof(void *);
paramInfo = (SourceMod::PassInfo *)malloc(sizeof(SourceMod::PassInfo) * dg->params.size());
for(int i = 0; i < (int)dg->params.size(); i++)

This comment has been minimized.

@KyleSanderson KyleSanderson requested a review from Drifter321 May 10, 2019
@KyleSanderson
Copy link
Member

@KyleSanderson KyleSanderson commented May 10, 2019

@Drifter321 are you okay with re-licensing this project to AM?

@Headline
Copy link
Member Author

@Headline Headline commented May 13, 2019

@Drifter321 are you okay with re-licensing this project to AM?

We got approval on Discord from him, but It doesn't hurt to make sure :)

@Drifter321
Copy link
Member

@Drifter321 Drifter321 commented May 13, 2019

Copy link
Member

@Drifter321 Drifter321 left a comment

I made these changes to my version after you created this pr so just a heads up on those. Other changes would be to somehow keep bcompat and make it work more like sdktools.

extensions/dhooks/vhook.cpp Outdated Show resolved Hide resolved
extensions/dhooks/vhook.cpp Outdated Show resolved Hide resolved
extensions/dhooks/vhook.cpp Outdated Show resolved Hide resolved
@Headline Headline force-pushed the dhooks branch from 7d07c77 to 6c7aafd May 30, 2019
Headline added 2 commits May 30, 2019
@KyleSanderson
Copy link
Member

@KyleSanderson KyleSanderson commented Aug 7, 2020

@Headline can you do a final sync? We can get Ruben to unblock after that.

@peace-maker
Copy link
Member

@peace-maker peace-maker commented Aug 7, 2020

Due to missing SM 1.10 builds of DHooks and a tendency to detour everything instead of using virtual hooks by plugin authors nowadays, I think a lot of servers are running the DHooks version with detouring support. Adding the one without detour natives in tree might cause some conflicts now 🙁

@KyleSanderson
Copy link
Member

@KyleSanderson KyleSanderson commented Aug 17, 2020

@peace-maker I think the best course of action here based on what's transpired over time is if you open a PR for your fork (as long as you're okay with the relicense to AM for your additions). @Drifter321 has already given his permission for this to be relicensed to AM (thanks man), and we close this one out not as INCOMPLETE but SUPERSEDED.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.