sdktools: Entity Output Hooks changes by Wend4r · Pull Request #1411 · alliedmodders/sourcemod

Wend4r · 2020-12-26T20:18:47Z

Add to missing output hooks names having only Activator as a output name. Learn more at sourcepawn branch.
Fix return values type of natives to bool instead of void.

Bundle with Pull Request in SourcePawn branch.

Add to missing output hooks names having only Attacker as a output name. Learn more at sourcepawn branch. Fix return values type of natives to `bool` instead of `void`.

peace-maker · 2021-01-06T08:13:11Z

Can you elaborate what issue you are trying to fix with this change, please? What's the use case?

Wend4r · 2021-01-06T17:32:29Z

Can you elaborate what issue you are trying to fix with this change, please? What's the use case?

Does not hooks the entity Outputs related to the bomb because the name is stored in Activator (as "player" in this example).

https://github.com/alliedmodders/sourcepawn/compare/e8d7b67b0665baf1faf6d4985c1e92ac1163e43f...c3777de58ce6577607c2ceae51c180b202ab25a0#diff-bb24c32f09684b1540bfccc5f5208f9bdb05e408fb56a797123f98ac177a3067

Wend4r · 2021-01-08T19:44:46Z

I have to give my plugin clients this Pull Request for the plugin to work tied to these bomb outputs.

KyleSanderson · 2021-03-08T01:35:57Z

@psychonic I'm really not qualified to review this, it looks okay from a cursory level but it's fundamentally changing the behaviour of EntityOutputManager::FindOutputName. Can you please take a look?

KyleSanderson · 2021-05-20T23:03:24Z

@psychonic WDYT?

asherkin

It would be good if you could make the changes to the testsuite plugin here, it make it a lot clearer what the problem is.

I don't totally feel comfortable that this is the correct fix here though, it seems rather heavyweight - it is possible this is the best fix, or the API needs correcting, but I think a bunch of reverse engineering investigation is required to nail down why these events are special.

Given the pain around output hooks already, I'd like to see a nice in-depth investigation.

asherkin · 2021-06-21T11:55:19Z

plugins/include/sdktools_entoutput.inc

 * @error               Entity Outputs disabled.
 */
-native void HookEntityOutput(const char[] classname, const char[] output, EntityOutput callback);
+native bool HookEntityOutput(const char[] classname, const char[] output, EntityOutput callback);


Do you need this information?

No, but I'm not sure if it's correct to return a value of 1 when void

It is fine, all natives internally return an int, void just tells plugins they don't need to care about what the return value is.

Wend4r · 2021-06-21T12:53:14Z

@asherkin,
I do not know why this happens in CS:GO, but my debugging shows that Activator can contain the Output name, but not Caller.

output.zip

asherkin · 2021-06-21T13:17:38Z

Looking at the outputinfo testsuite plugin and the cstrike15_src code and comparing OnBombBeginDefuse to OnLightOn, are you sure

HookEntityOutput("player", "OnBombBeginDefuse", OutputHook);
HookEntityOutput("player", "OnBombDefused", OutputHook);
HookEntityOutput("player", "OnBombDefuseAborted", OutputHook);

shouldn't be

HookEntityOutput("planted_c4", "OnBombBeginDefuse", OutputHook);
HookEntityOutput("planted_c4", "OnBombDefused", OutputHook);
HookEntityOutput("planted_c4", "OnBombDefuseAborted", OutputHook);

?

Wend4r · 2021-06-21T13:30:48Z

I also looked at cstrike15_src code 2017 year, so I need a hook in the code of my plugin in the likeness OnBombBeginDefuse which is called after setting the values of all CPlantedC4 (planted_c4) defuse fields. I first tried using

HookEntityOutput("planted_c4", "OnBombBeginDefuse", OutputHook);

but it didn't called. I started to find out and came across the problem of this Output name is in the pActivator

asherkin · 2021-06-21T13:50:14Z

Could you test it again on 1.11 where #1074 is fixed? The classname "player" being passed in instead of "planted_c4" would certainly explain why these code changes were required for it to be called - but it looks like the existing logic should all work with "planted_c4" (which would be the caller, but maybe there is some other hackery going on that the debug logging might reveal why it isn't matching).

Wend4r · 2021-06-21T13:56:30Z

#1411 (comment)
I tested it on

sm version
 SourceMod Version Information:
    SourceMod Version: 1.11.0.6692
    SourcePawn Engine: 1.11.0.6692, jit-x86 (build 1.11.0.6692)
    SourcePawn API: v1 = 5, v2 = 13
    Compiled on: Jun  7 2021 16:34:45
    Built from: https://github.com/alliedmodders/sourcemod/commit/d01c72f
    Build ID: 6692:d01c72f
    http://www.sourcemod.net/

With this PR and output.cpp in output.zip

Wend4r · 2021-06-21T14:04:07Z

I added more debugging with classnames and realized that the activator is "planted_c4"

Start EntityOutputManager::FindOutputName(): pOutput = 0xe4463c0, pActivator = 0xe445eb0, pCaller = 0xe2b3480
pCaller classname = "player"
pActivator classname = "planted_c4"
Found name by pActivator = "OnBombBeginDefuse"
L 06/21/2021 - 17:01:52: [outputtest.smx] [ENTOUTPUT] OnBombBeginDefuse

output.zip

asherkin · 2021-06-21T14:41:41Z

Heh, I looked at the code again and you've found a bug in the game, they've accidentally flipped the params to FireOutput in the CPlantedC4 implementation, that definitely explains the behaviour here.

asherkin · 2021-06-21T14:58:25Z

I did a search of SDK2013 (and others) with rg -tcpp --pcre2 'FireOutput\(\s*this,\s*(?!this)\S+\s*[,)]' and there are a few with incorrect calls like this. I think this change is sane given that - it makes sense to let these outputs be hooked, but it needs an explicit callout in the source that it is only a fallback implementation for broken callers (and name this specific broken caller). I think we also probably need to fixup the classname match to match the one that we found the name from (as that is the entity with the output in its datatable). What do you think?

game/server/point_playermoveconstraint.cpp
151:                            m_OnConstraintBroken.FireOutput( this, pPlayer );

game/shared/func_ladder.cpp
395:    m_OnPlayerGotOnLadder.FireOutput(this, pPlayer);
407:    m_OnPlayerGotOffLadder.FireOutput(this, pPlayer);

game/shared/cstrike15/weapon_c4.cpp
489:                            m_OnBombDefuseAborted.FireOutput( this, m_pBombDefuser );
784:            m_OnBombDefused.FireOutput( this, pDefuser );
1095:                   m_OnBombBeginDefuse.FireOutput(this, player);

game/server/cstrike15/hostage/cs_simple_hostage.cpp
655:                    m_OnRescued.FireOutput(this, player);
1567:                           m_OnHostageBeginGrab.FireOutput(this, pPlayer);
1628:           m_OnFirstPickedUp.FireOutput(this, pPlayer);
1707:                   m_OnDroppedNotRescued.FireOutput(this, player);

Wend4r · 2021-06-21T15:27:49Z

I think can return the classname where was the Output found from EntityOutputManager::FindOutputName() and already work with it as DataTable Output classname.

This reverts commit 3c79b77.

asherkin · 2021-06-21T15:55:01Z

It's probably worth getting some more input here, I'm not sure what makes sense - do you know of any game code or maps using any of these questionable events? After thinking about it a good while I'm probably leaning towards defining the SM-side stuff (classname, entity index for single hooks) as explicitly being the caller (whatever that is, normally the entity with the output, the player in this broken case) and leaving it up to plugins to deal with. It is slightly logically weird compared to the datatable, but it seems to make sense consistency wise.

EDIT: That change looks sane if we want to go the route of trying to correct for it (but then the caller doesn't match what's expected?) - if going the other way we should also handle the caller entity being invalid, there is at least one where it is called with NULL.

Wend4r · 2021-06-21T16:11:30Z

EDIT: That change looks sane if we want to go the route of trying to correct for it (but then the caller doesn't match what's expected?) - if going the other way we should also handle the caller entity being invalid, there is at least one where it is called with NULL.

a0b05dc
Since this commit, pCaller has not been checked for NULL . I'm sure it's always there if SDKTools works for such a long time with this.

asherkin

Thanks for your patience here. I've had a really deep dive through the engine's output system and SM's output hooks and it looks like there are quite a few holes in how this has been implemented in SM - it is very heavily relying on convention of the common entities and I can see quite a few cases where it will be falling apart.

As this PR seems to move things in the right direction I'm gonna merge it into 1.11 (what this makes work won't get any more broken if we start handling outputs properly) - I've added a commit with a couple of little style tweaks, some notes about the design issues inline, and a slightly expanded test plugin.

sdktools: Entity Output Hooks changes

3c79b77

Add to missing output hooks names having only Attacker as a output name. Learn more at sourcepawn branch. Fix return values type of natives to `bool` instead of `void`.

Wend4r force-pushed the master branch 2 times, most recently from 3a9dad3 to 3c79b77 Compare January 6, 2021 02:02

KyleSanderson requested a review from psychonic March 8, 2021 01:34

asherkin reviewed Jun 21, 2021

View reviewed changes

Wend4r added 2 commits June 21, 2021 18:49

Revert "sdktools: Entity Output Hooks changes"

f7dec9b

This reverts commit 3c79b77.

sdktools: Entity Output Hooks changes 2

ce3f7e9

Style and test suite changes, added some notes

1a09724

asherkin approved these changes Jul 17, 2021

View reviewed changes

asherkin merged commit 54364d2 into alliedmodders:master Jul 17, 2021

Uh oh!

Conversation

Wend4r commented Dec 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peace-maker commented Jan 6, 2021

Uh oh!

Wend4r commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wend4r commented Jan 8, 2021

Uh oh!

KyleSanderson commented Mar 8, 2021

Uh oh!

KyleSanderson commented May 20, 2021

Uh oh!

asherkin left a comment

Choose a reason for hiding this comment

Uh oh!

asherkin Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

Wend4r Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asherkin Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

Wend4r commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asherkin commented Jun 21, 2021

Uh oh!

Wend4r commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asherkin commented Jun 21, 2021

Uh oh!

Wend4r commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wend4r commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asherkin commented Jun 21, 2021

Uh oh!

asherkin commented Jun 21, 2021

Uh oh!

Wend4r commented Jun 21, 2021

Uh oh!

asherkin commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wend4r commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asherkin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Wend4r commented Dec 26, 2020 •

edited

Loading

Wend4r commented Jan 6, 2021 •

edited

Loading

Wend4r Jun 21, 2021 •

edited

Loading

Wend4r commented Jun 21, 2021 •

edited

Loading

Wend4r commented Jun 21, 2021 •

edited

Loading

Wend4r commented Jun 21, 2021 •

edited

Loading

Wend4r commented Jun 21, 2021 •

edited

Loading

asherkin commented Jun 21, 2021 •

edited

Loading

Wend4r commented Jun 21, 2021 •

edited

Loading