AmbiguousMenu: Automagically create menu when multiple targets match

[BotoX]

Added hack to make plugins open a menu with all possible targets on ReplyToTargetError COMMAND_TARGET_AMBIGUOUS.

Explanation:
There are two clients in the server, one named gene, the other one "Ene special characters".
An admin issues "sm_slay Ene" and gets following error message: More than one client matched the given pattern.
What this hack will do is: Use GetCmdArg(0, ...); to get the command name "sm_slay".
Use GetCmdArgString(...); to get the arguments supplied to the command.
Use GetLastProcessTargetString(...); (which was implemented in this commit) to retrieve the arguments that were passed to the last ProcessTargetString call.
It will then pass this data to the DynamicTargeting plugin through its AmbiguousMenu native.
The plugin will open up a menu on the client and list all targets which match the pattern that was supplied to ProcessTargetString.
If the client selects a menu entry, FakeClientCommand will be used to re-execute the command with the correct target.

sourcemod.inc is kinda ugly

[KyleSanderson]

Politics aside this is a pretty cool feature. It looks like this may crash a lot. Can you address the licensing issues so we can proceed forward?

[KyleSanderson]

I believe these are messed up 😿 you'll crash or use an old buffer from pawn if not invoked immediately.

[BotoX]

Absolutely, if I call GetLastProcessTargetString when the SourcePawn locals of the last ProcessTargetString call are already gone then something weird will definitely happen.
Could copy the data I need to a different struct instead to be safe.

[KyleSanderson]

can you unmove this?

[BotoX]

These definitions need to exist before #include <commandfilters> I think.
So either move them to the top or move the commandfilters include to the bottom.
Actually why not move all of the includes to the bottom so they can use these sourcemod natives.

I use these natives in commandfilters here:
BotoX@6f38356#diff-04ed686e7e10bc10466c487fadc9ea8eR151

[asherkin]

The feature is awesome but as discussed previously I'd be super reluctant to take this implementation in-tree - the coupling between core and the plugin is very tight, it's requiring a couple of odd escape hatches to the internal implementation and I don't see an alternative implementation of the plugin ever really being viable. As it changes a stock it also requires plugins to be recompiled to pick it up.

A better option might be for this to live entirely in C++ land, triggered internally from ProcessTargetString, with a core.cfg option to enable it and possibly a targeting flag to skip it if required. Or even just a separate native plugins could use as a starting position (which could be added to the FindTarget simple wrapper.)

In an ideal world we'd be able to have async behaviour inline here, but anything re-calling the command is always going to need to be super cooperative, and there is a lot of odd command callback implementations out there.

[BotoX]

The implementation is indeed hacky, it also requires a plugin recompile to make use of this feature.
It also only works if plugins follow the following scheme:

if((target_count = ProcessTargetString(...)) <= 0) {
    ReplyToTargetError(client, target_count);
    return Plugin_Handled;
}

I haven't encountered a single plugin which does this different.
Actually I believe it even was my intention to straight up only support plugins which do targeting the "correct way".
Aka. if there is an error in ProcessTargetString then call ReplyToTargetError and exit.
The error which is caught in this case is COMMAND_TARGET_AMBIGUOUS which would print More than one client matched the given pattern.
So if this is the case (and the plugin author hasn't specifically passed dynamic=false to ReplyToTargetError) it'll check if SM has the needed capabilities: GetLastProcessTargetString and if the DynamicTargeting plugin is even loaded.
https://github.com/alliedmodders/sourcemod/pull/1069/files#diff-04ed686e7e10bc10466c487fadc9ea8eR147
From there on everything should be smooth sailing and never cause any issues.

Then again I also can't guarantee that the plugin will return after calling ReplyToTargetError.
If it calls ReplyToTargetError multiple times then this would create multiple ambiguous menus.
If it calls ProcessTargetString and ReplyToTargetError again (maybe manipulating the args itself trying to be smart?) then this would also create multiple ambiguous menus.
All in all mostly the plugins fault imo. and nothing really bad will happen because of it anyways.

How would you go about adding this in ProcessTargetString anyways?
What if the plugin doesn't call ReplyToTargetError and exit but instead does some other crazy stuff instead.

I don't see the recompiling thing as a dealbreaker, at least that way if this would break a certain plugin it'd only do so if the author/user compiles it with the current SM codebase. It'd be easy for them to fix it and simply recompile.
But I'm sure there's plenty of cases where people just download an .smx from somewhere and drop it into their server.
Where they have no way of recompiling it if something is wrong.
And if they want this feature in one of those old .smx then yeah they'll have to go the extra mile and recompile it.
But we won't break their old .smx at least by merging this.

Looking over this again I still really like the way I've done this, minimal changes to SM and zero changes needed to plugin code.
Almost the entire functionality is implemented in plugin code, only some glue has been added to SM core.
Then again some people might want to have stuff like this as C++ code in the core, others don't mind or prefer that it's mostly plugin code.

[KyleSanderson]

@asherkin what are your current thoughts for this one? I'm apathetic between either method as they're both improvements, but I'm optimistic that this will hopefully land and improve the targeting situation that's beneficial for all end-users.