Closed SChinchi closed 1 year ago
Can you refactor this with
iCount = Math.abs(iCount);
if (args.commandName == "give_item")
{
if (Run.instance.IsItemExpansionLocked(item))
{
Log.MessageNetworked("Additional content enabled is required to grant this item.", args, LogLevel.MessageClientOnly);
return;
}
inventory.GiveItem(item, iCount);
Log.MessageNetworked($"Gave {iCount} {item} to {targetName}", args);
}
else
{
int stacks = Math.Min(iCount, inventory.GetItemCount(item));
inventory.RemoveItem(item, iCount);
Log.MessageNetworked($"Removed {stacks} {item} from {targetName}", args);
}
That would make give_item hoof 2
behave exactly as give_item hoof -2
, in which case we may as well prohibit negative values (some other commands already do that).
However, Inventory.GiveItem
and Inventory.RemoveItem
accept negative counts by passing the responsibility to the opposite method. This is also how the commands here have always worked and I assume a lot of people have relied on that feature so far. Unless I'm just speaking for myself, I think give_item
with a negative count is more convenient for removing an item.
Ok yeah sounds good, I wrote it because I really didnt like the string action and if you check your commit you do abs in the give_item branch thats why I wrote that in the refactor
The same abs is in the other branch as well. I think it's a necessarily evil because both commands can end up doing the opposite. It could be written more compactly as
var amount = (args.commandName == "give_item" ? 1 : -1) * iCount;
iCount = Math.Abs(iCount);
if (amount >= 0)
{
// give logic
}
else
{
// remove logic
}
Where amount
serves the same conceptual role as action
. It has the slightly unintended consequence of remove_item item 0
saying "Gave 0 X" instead of "Removed 0 X", but this isn't harmful.
I intend to make a small commit to make it explicit in the docs that give_item
with a negative count is an alias for remove_item
and vice versa. Let me know if you would also prefer the code rewritten with the "amount" approach.
The same abs is in the other branch as well.
yes, this is exactly why I wrote the code snippet above, i'm not sure what you were going for, just in case you didnt catch it yet my code snippet had identical behaviour to this https://github.com/harbingerofme/DebugToolkit/pull/142/files#diff-d86681cfab0c329f153dbc06709fc7fd6c92b1956056aca68ab1f779ebd025fdR148-R165
Ah, I see. Unless I'm missing something your snippet doesn't quite work because choosing the give/remove branch requires the combined information of the command name and the original iCount sign. This is what I addressed in my snippet which was inspired by yours.
Agree with the amount approach and telling in the doc that negative count is alias for the opposite command
Also it probably would be better to log an error to the user and not continue the command if the item count is zero as it doesn't make sense
All done. I opted doing if (amount > 0) { //give } else if (amount < 0) { //remove } else { //nothing }
to keep it similar to give_lunar
.
Thanks!
As discussed in #122 both commands now use the same method to parse the arguments. Additional changes:
give_item
with negative count reports removing items andremove_item
with negative count reports adding them.RoR2.Inventory.GiveItem
also does this, so we may as well report properly that an item can't be granted instead of saying "Gave X" when nothing happened.