Skip to content

Added GetArgs #663

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Added GetArgs #663

wants to merge 13 commits into from

Conversation

schwarper
Copy link
Contributor

[ConsoleCommand("css_testx")]
public void OnTest(CCSPlayerController player, CommandInfo info)
{
    var args = info.GetArgs();

    Console.WriteLine($"TESTX args count => {args.Count}");

    for (int i = 0; i < args.Count; i++)
    {
        Console.WriteLine($"{i}. {args[i]}");
    }
}

[ConsoleCommand("css_testy")]
public void OnTest2(CCSPlayerController player, CommandInfo info)
{
    var args = info.GetArgs(0, 3);

    Console.WriteLine($"TESTX args count => {args.Count}");

    for (int i = 0; i < args.Count; i++)
    {
        Console.WriteLine($"{i}. {args[i]}");
    }
}

image

@schwarper schwarper requested a review from roflmuffin as a code owner November 7, 2024 18:51
@schwarper
Copy link
Contributor Author

#504

I improved this a bit.

@KillStr3aK
Copy link
Contributor

I'd also like the enumerator version maybe? from #504

@schwarper
Copy link
Contributor Author

I'd also like the enumerator version maybe? from #504

I thought that getting arg once and split them is better than using GetArg multiple times. However, it might be better.

@WidovV
Copy link
Contributor

WidovV commented Nov 13, 2024

Perhaps a solution like this would be the way forward, as I can see calling getarg multiple times might not be the best way forward (my solution), but nor do I think creating a list and a regex expression is (your solution).
I made a minor test project to showcase what could become the end result. It kinda combines both yours and mine. The if statement or rather ternary don't need to be implemented, I was just too lazy to copy the original.

   // Emulation of argstring
   private static string ArgString = "my test string";
   // Emulation of ArgCount
   private static int ArgCount = ArgString.Split().Length;
   static void Main(string[] args)
   {
       Console.WriteLine(string.Join(" ", TestMethods(2))); // Output: string
       Console.WriteLine(string.Join(" ", TestMethods(1))); // output: test string
       Console.WriteLine(string.Join(" ", TestMethods(1, 2))); // Output: test
       Console.WriteLine(string.Join(" ", TestMethods(0, 1))); // Output: my
       Console.WriteLine(string.Join(" ", TestMethods())); // Output: test string
   }

   private static IEnumerable<string> TestMethods(int startIndex = 1, int endIndex = -1)
   {

       endIndex = (endIndex > ArgCount || endIndex < 0) ? ArgCount : endIndex;
       startIndex = (startIndex > ArgCount ||  startIndex < 0 || startIndex > endIndex) ? ArgCount : startIndex;
       Console.WriteLine($"Start: {startIndex} | end: {endIndex}");
       
       return ArgString.Split((char[]?)null, StringSplitOptions.RemoveEmptyEntries).AsEnumerable().Skip(startIndex).Take(endIndex - startIndex);
   }

Splitting with (char[]?)null is reffering to any whitespace as I understand.
The reason for starting startindex at one is, iirc, argstring comes with the command input as first argument, in case of not, my bad. Not sure that "all" of these method calls on enumerable is that performance heavy, but we're avoiding object initialization by using As, which is a thumbs up. Either Take or Skip could be removed by using "TakeWhile" and checking index also.

Glad to see that a PR I made back in June and forgot all about is something that other people (you at least) would have the benefit of 😅

@roflmuffin
Copy link
Owner

roflmuffin commented Mar 25, 2025

I think perhaps we are better off just having an IEnumerable<string> Args => that yields in a for loop without any index access, and just pay the performance cost of multiple calls to GetArg(i), that way the user can use regular methods to skip/take or do array accesses from the enumerable.

I'm open to also starting this index at 1 so that we skip the "command" arg.

i.e.

 public static IEnumerable<string> GetValue {
        get
        {
            for (int i = 0; i < ArgCount; i++)
            {
                yield return GetArg(i);
            }
        }
    }

@schwarper
Copy link
Contributor Author

schwarper commented Mar 25, 2025

I think perhaps we are better off just having an IEnumerable<string> Args => that yields in a for loop without any index access, and just pay the performance cost of multiple calls to GetArg(i), that way the user can use regular methods to skip/take or do array accesses from the enumerable.

I'm open to also starting this index at 1 so that we skip the "command" arg.

i.e.

 public static IEnumerable<string> GetValue {
        get
        {
            for (int i = 0; i < ArgCount; i++)
            {
                yield return GetArg(i);
            }
        }
    }

I don't think it's a good idea to use GetArg in loop. However, I'm (unfortunately) okay with both solutions

@WidovV
Copy link
Contributor

WidovV commented Mar 25, 2025

GetValue doesn't quite follow the naming convention since GetArg is a thing, hence GetArgs were the name for the method.
I do agree that calling GetArg multiple times instead of splitting ArgString seems like unnecessary performance waste, however I do not think that the first argument should be skipped. An example of usage could be a command listener for every chat message and only wanted to log messages starting with ! or /

@roflmuffin roflmuffin closed this Mar 25, 2025
@roflmuffin
Copy link
Owner

Have we actually determined that the performance of get arg would actually be worse than the string split? The con command on the native side would already know each arg so it's fairly cheap for it to make a copy of the arg and return it back to c#. I'd be interested in a benchmark comparing the two.

@roflmuffin roflmuffin reopened this Mar 25, 2025
@WidovV
Copy link
Contributor

WidovV commented Mar 26, 2025

I've contacted you over dm's on discord regarding how to setup a benchmark to test it 😄

@schwarper
Copy link
Contributor Author

public static IEnumerable<string> GetArgs(CommandInfo info, int startIndex = 0, int endIndex = -1)
{
    if (string.IsNullOrEmpty(info.ArgString))
        return [];

    string[] args = info.ArgString.Split(' ', StringSplitOptions.RemoveEmptyEntries);
    int lastIndex = args.Length - 1;

    startIndex = Math.Clamp(startIndex, 0, lastIndex);
    endIndex = Math.Clamp(endIndex < 0 ? lastIndex : endIndex, startIndex, lastIndex);

    string[] selectedArgs = startIndex == endIndex
        ? [args[startIndex]]
        : args[startIndex..(endIndex + 1)];

    return selectedArgs.Select(arg =>
        arg.Length >= 2 && arg[0] == '"' && arg[^1] == '"'
            ? arg[1..^1]
            : arg
    );
}

public static IEnumerable<string> GetArgsYield(CommandInfo info)
{
    if (info.ArgCount == 1)
        yield break;

    for (int i = 1; i < info.ArgCount; i++)
        yield return info.GetArg(i);
}

test1
test2

@schwarper
Copy link
Contributor Author

image

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.

4 participants