alliedmodders / sourcemod

SourceMod - Source Engine Scripting and Administration
http://www.sourcemod.net/
987 stars 423 forks source link

Post UserMessageHook sent param bugged #959

Open Kenzzer opened 5 years ago

Kenzzer commented 5 years ago

Help us help you

Environment

Description

It seems the sent param in the post UserMessageHook to be always true. And I believe this is the reason but I'd rather not open a PR if there's an explanation for it. https://github.com/alliedmodders/sourcemod/blob/aae71612731eaa6771067a31f3a7ad34bdb0df28/core/UserMessages.cpp#L810-L812 https://github.com/alliedmodders/sourcemod/blob/aae71612731eaa6771067a31f3a7ad34bdb0df28/core/UserMessages.cpp#L543-L545 https://github.com/alliedmodders/sourcemod/blob/aae71612731eaa6771067a31f3a7ad34bdb0df28/core/UserMessages.cpp#L644

Problematic Code (or Steps to Reproduce)

public void OnPluginStart()
{
    HookUserMessage(GetUserMessageId("SayText2"), Hook_SayText2, true, Hook_SayText2Post);
}
public void Hook_SayText2Post(UserMsg msg_id, bool sent)
{
    PrintToChatAll("sent %i", sent);
}
public Action Hook_SayText2(UserMsg msg_id, BfRead message, const int[] players, int playersNum, bool reliable, bool init)
{
    return Plugin_Handled;
}

Logs

KyleSanderson commented 5 years ago

You're right, we only run set m_BlockEndPost when Plugin_Stop is set. However, the documentation we have is...

MsgHook
Syntax:
functag public Action:MsgHook(UserMsg:msg_id, Handle:msg, const players[], playersNum, bool:reliable, bool:init);

Usage:
 msg_id     Message index.
 msg            Handle to the input bit buffer or protobuf.
 players        Array containing player indexes.
 playersNum Number of players in the array.
 reliable       True if message is reliable, false otherwise.
 init           True if message is an initmsg, false otherwise.
Notes:
Called when a message is hooked

Return:
Ignored for normal hooks. For intercept hooks, Plugin_Handled blocks the message from being sent, and Plugin_Continue resumes normal functionality.

Version Added:
1.0.0.1946

Unfortunately I think it's too late to fix this now. UserMessages have always had bizarre semantics, I think the best we can do here is fix-up the docs.

Kenzzer commented 5 years ago

Wouldn't it be fine to have the sent param set to false for Plugin_Handled as well?

I know I'm going from an assumption and it's bad to think that way. But since the param never worked as intended (except for Plugin_Stop) I doubt any plugins made use of it?

If the documentation is fixed to: Ignored for normal hooks. For intercept hooks, **Plugin_Stop** blocks the message from being sent, and Plugin_Continue resumes normal functionality.

We would also have to mention Plugin_Handled has the ""same"" behaviour as Plugin_Stop except it doesn't set the sent param to false? Or just clearly state not to use but that would be weird no?

Alienmario commented 1 year ago

I've just come across this myself. One of the solutions above should absolutely get implemented.

Having the correct value in "sent" is important for modifying (=recreating) user messages because you get an error when starting a new message and the original was not blocked (sent=true).