Change Command: remove_item
DestroyedClone opened this issue · 7 comments
Which Command
remove_item
What should it do?
Just as give_item NAME with no count parameter gives one item, remove_item NAME should remove one of that item with no count parameter.
As it is right now, performing remove_item NAME will throw, requiring you to put remove_item NAME 1 instead.
Issue (truncated):
Error: Unity Log] ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
Stack trace:
System.ThrowHelper.ThrowArgumentOutOfRangeException (System.ExceptionArgument argument, System.ExceptionResource resource) (at <44afb4564e9347cf99a1865351ea8f4a>:0)
System.ThrowHelper.ThrowArgumentOutOfRangeException () (at <44afb4564e9347cf99a1865351ea8f4a>:0)
DebugToolkit.Commands.Items.CCRemoveItem (RoR2.ConCommandArgs args) (at <c00c5baed02c476aa2ebf2461599349c>:0)
remove_item is setup differently than give_item
give_item has the optional parameters be item count (1) and player (self)
remove_item on the other hand, has only one optional parameter: player (self)
This got tagged as bug but it seems kind of intentional when it was added due to the Lang formatted differently, though I can't imagine why.
GIVEITEM_ARGS = "Requires 1 (2 if from server) argument: give_item {localised_object_name} {Count:1} ({PlayerID:self}|{Playername})",
REMOVEITEM_ARGS = "Requires 1 argument: remove_item {localised_object_name} ({Count}|\"all\") ({PlayerID:self}|{Playername})",Working on a PR to setup remove_item nearly the same as give_item
This doesn't seem to be an issue anymore. Close?
This makes me wonder though that since give_item and remove_item work exactly the same now, should we consider only keeping give_item, similar as to how give_lunar removes coins for a negative count?
No point on removing it, what we can do however is sharing the method implementation for code reuse
How about stacking both ConCommand attributes on CCGiveItem and then using args.commandName to figure out what to do after all the parsing?
I guess that works too, unless I'm missing something the only diff would be putting a - before the count right ?
That's pretty much what it boils down to, yes.
Additionally, if we want to be more accurate with the logged result, give_item with a negative count should probably say "Removed X items" and vice versa. Also, having 1 hoof and removing 5 will only remove one technically, since it never takes you to negative stacks. But these inaccuracies are present in the current state of the commands anyway.
Closed by #142