Implemented ping to tag non-player characters to give/remove items & equipment.

[SChinchi]

Intercept a ping to check if the targetted entity is a CharacterBody. This can then be used in the commands

• give_item
• give_equip
• random_items
• remove_item
• remove_equip

as the designated target by using the special "pinged" name for the player argument, e.g., "give_item hoof 5 pinged". Obviously, a dedicated server will only try to match the name to players. Otherwise, the name "pinged" will check for the existence of a tagged body. This does present the unlikely problem where a player is called "pinged" and is the intended explicit target, but I find that to be both unlikely, and a potential issue regardless of what special name were to be used.

I adapted the remove_equip command to also work for dedicated servers (along with the ping functionality), as I see no reason why it shouldn't.

This functionality could be implemented in more commands, e.g., change_team and true_kill, but I decided against it at this time because those are implied to only apply to players due to in which project files they have been defined.

The remove_item command presented a dilemma. In the original it's implied that the count and player arguments are always in the second and third positions, if they exist. But if someone wants to remove every item from inventory, it would make sense to be able to do "remove_item all \<target>", instead of "remove_item all \<irrelevantCount> \<target>". This is similar to how give_item can skip the count argument. However, in this case this allows for an edge case to have an unexpected behaviour, where "remove_item all all" is implied to mean to remove all items from the player partially matching the name "all". However, "all" is also used for the count, thereby concluding that no target has been specified, which will default to the command caller. How should we proceed here?

[xiaoxiao921]

Really nice work, for the remove_item edge case, I personally think that if all is used as the first argument then the count arg should be skipped, but then it doesn't allow to remove each item a specified amount of time, if we want to handle all cases we could make a new command called remove_all_items count target

[SChinchi]

Oh, I see. Removing a certain number of stacks from all items didn't occur to me. To be honest, the original code doesn't even behave like that. If all is passed for the item name, it overrides everything and just clears the inventory.

If we remove the all option from the item name, we can make the functionality the exact opposite of give_item, which would be quite intuitive. remove_all_items count target seems too niche of a usage to me, as item stacks tend to be independent from one another. But there is merit for remove_all_items target.

[xiaoxiao921]

I agree with both of your suggestions, I don't want to put too much work on you so I'll let you decide if you want to make these changes on this pr or not

[SChinchi]

No, please, it isn't a burden. The heavy work has been done already.

Having thought about it a bit more, the issue stems from having the preceeding argument to the target accept a string. So even if we split the commands, that would mean the count for remove_item would still not be able to accept all without a risk. Limiting it to just an integer pretty much makes this command an alias for calling give_item with a negative count.

There is merit to having a command for removing all stacks of an item, or all items at once, e.g., remove_all_item_stacks ({item_name}|"all"}) [target], but then this would run into the same problem. It boils down to the following options:

1. Keep all functionality consolidated in remove_item, but make a note for the end user that if they use all for the name, the count arg must be skipped and if they intend to use all for the target, a (garbage) count arg must be provided.
2. Allow all only for the item name and make a note for the end user in this scenario the count arg must not be provided. Then have a separate command to remove all stacks from an explicit item.
3. Have separate commands for all of the following:
   • remove_item {item_name} [count] [target]
   • remove_all_item_stacks {item_name} [target]
   • remove_all_items [target]

If it were up to me, I would probably go with the third one just to be over with it.

[xiaoxiao921]

I find the third option definitely the most elegant one

[SChinchi]

What just occured to me is that while a few commands have optional arguments, or some can be run from a dedicated server, give_item and remove_item are unique in that the usage is effectively:

• give_item item [count] [target] for non-ds
• give_item item [count] target for ds

It is noted in the command description extra arguments are required from a DS, but I don't know whether it may make more sense to be explicit and say from a DS the usage is item target [count]. Even the code checks for the presence of two arguments for a DS caller, but it will still attempt to parse the second one as a count. It will fail correctly if it doesn't find a target, but the original failsafe only gives feedback if args.Count < 2. It also makes sense to have all necessary arguments before any optional ones It would also avoid any issues with all for remove_item, in which case all relevant commands could be fused back into one. The counter to all of this is that it's fairly clear what is expected from the user and it would break backwards compatibility if it were to change. It is also just two commands for now, but if we were to add more things, such as add_buff name [stacks] [target], we would run into the same problem.

[xiaoxiao921]

I don't mind any breaking change for dedicated server with multiple reasons:

• very little usage of DS overall in the community
• this tool was and is first of all a tool for helping devs debugging stuff with their mods, so game client has a much bigger priority than DS, that said mod creators could use a DS to debug their mods in multiplayer, but the likelihood of them having specific scripts / commands that use specific Debugtoolkit DS behaviors sounds incredibly low

And personally the fact that a middle parameter was skippable in the first place sounds like an horrible bug, especially with how the command system is and how we don't have an easy of specifiying when we want to skip a parameter

[SChinchi]

Unless we adopt a parser approach with named flags for optional arguments (which feels an overkill for this project), the best I can think of is ensuring any optional parameters up to the last inputted one are required. This is how spawn_ai really does it, which is one of the few commands with multiple optional arguments. So in essence it would be command necessary1 necessary2 [optional1 [optional2 [optional3 [etc]]]]. We could make it easier for the user by allowing _ for the default value, e.g., command necessary1 necessary2 _ optional2.

[xiaoxiao921]

...ensuring any optional parameters up to the last inputted one are required.

Yep I agree

We could make it easier for the user by allowing _ for the default value, e.g., command necessary1 necessary2 _ optional2.

That could work but a small issue I see with it is that an universal character for default value would not always work on every situation ( do we then use a different character if an optional parameter happens to be a player name, which could contain the default value character? ), I think in insight we could have used the double quote system for delimiting parameters ( used with a regular escaping mecanism with \ ) unless RoR2 already does it, I'm not sure at this point since it been a long time since I've done that stuff, I know that the original Ror2 programmer had heavy inspiration from the concommands to the convar stuff which originated from the source games, which used double quotes for delimiting

[SChinchi]

I meant we would explicitly check for _ at the beginning for resorting to the default value, before partially matching it to any other string.

From a quick test, only _:. work on their own without being surrounded in double quotes, everything else is skipped or used as a delimeter themselves. And double quotes apparenly work with just the opening one. For example, command 5 < _ "%$^" assets/ror2 "assets/ror2 parses 5, _, %$^, assets, ror2, assets/ror2. Unquoted forward slash+space behaves in a strange way, but suffice to say, one should use quotes to avoid any surprises.

[xiaoxiao921]

I meant we would explicitly check for _ at the beginning for resorting to the default value, before partially matching it to any other string.

Not sure I follow

Also, if quotes are supported wouldn't that make more sense then to use "" for default value? Or the game doesn't allow it?

[SChinchi]

if (args[i] == "_") {
    value = defaultValue; // or raise an error if the parameter is required for DS
}
else
{
    // whatever parsing logic we currently have
}

I assume "" would just be an empty string, so it would also work just fine. I only suggested the underscore because it's a single keystroke and can be associated with a dummy variable.

[xiaoxiao921]

The problem with the underscore unless I'm mistaken is that there could be rare cases where it could be a valid value ( not saying that's it's a good one ) where an empty string ( double quote) it's much less likely

[SChinchi]

I see, fair. Shall I update the code to treat middle optional arguments as positional then with "" for their default values, seeing how it's only give_item and remove_item that suffer from this.

[xiaoxiao921]

Yes please if you don't mind

[xiaoxiao921]

Thanks a lot for the contribution!