Skip to content
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

Clear Inventory Chatcommand (/clearinv) #4994

Merged
merged 1 commit into from May 20, 2017

Conversation

Projects
None yet
@octacian
Copy link
Contributor

octacian commented Jan 5, 2017

This PR introduces a chatcommand which allows players to clear either their own inventory or another player's inventory. /clearinv with not parameters clears the inventory of the player who executed the command, while /clearinv <playername> attempts to clear the inventory of the player specified.

Clearing your own inventory requires no additional privileges, while clearing another player's inventory requires the pre-existing server privilege.

I've tested both in singleplayer and multiplayer.

@@ -90,3 +90,7 @@ core.register_privilege("debug", {
description = "Allows enabling various debug options that may affect gameplay",
give_to_singleplayer = false,
})
core.register_privilege("clearinv", {
description = "Can use clear inventory command",

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jan 5, 2017

Member

for other players

Also, maybe rename to make it even more obvious it's not for your own inv

This comment has been minimized.

Copy link
@octacian

octacian Jan 5, 2017

Author Contributor

@rubenwardy will definitely change desc. Ideas for better priv name? clear_others_inv?

This comment has been minimized.

Copy link
@octacian

octacian Jan 5, 2017

Author Contributor

Maybe just require server priv... IDK. For now, I'll change desc and leave priv name until better suggestion.

return false, "Player must be online to clear inventory!"
end
else
local player = core.get_player_by_name(name)

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jan 5, 2017

Member

code share possible here:

local player
if param and param ~= "" then
    -- check priv, set player to player
else
   -- set player to caller
end

if player then
    -- clear inv of player
else
    -- error
end
else
local player = core.get_player_by_name(name)
if player then
player:get_inventory():set_list("main", {})

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jan 5, 2017

Member

craft? craft result?

@rubenwardy

This comment has been minimized.

Copy link
Member

rubenwardy commented Jan 5, 2017

The commit message should be:

Add /clearinv chat command

We can change this on merge, however

Notice how this is imperative (Add rather than added)

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Jan 5, 2017

@rubenwardy will amend when I change a few things.

@octacian octacian force-pushed the octacian:clearinv branch from c0aaa17 to 33ffc09 Jan 5, 2017

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Jan 5, 2017

As @rubenwardy requested:

  • updated priv desc
  • simplified code for chat command
  • made command clear craft and craftpreview inventories

I do agree that the privilege needs a new name altogether, but I'm really not sure what to call it. I was thinking clear_others_inv as I stated in an above comment to rubenwardy's review, but I'd like some extra input on this.

@nerzhul nerzhul self-requested a review Jan 5, 2017

@nerzhul

nerzhul approved these changes Jan 5, 2017

Copy link
Member

nerzhul left a comment

okay for me

@nerzhul nerzhul added the One approval label Jan 5, 2017

@SmallJoker

This comment has been minimized.

Copy link
Member

SmallJoker commented Jan 5, 2017

Apart from creative mode - does this command have any use case?

@nerzhul

This comment has been minimized.

Copy link
Member

nerzhul commented Jan 5, 2017

@SmallJoker it's useful for modders or developers to clear their inventory sometimes

player:get_inventory():set_list("main", {})
player:get_inventory():set_list("craft", {})
player:get_inventory():set_list("craftpreview", {})
if pmsg then core.chat_send_player(param, pmsg) end

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jan 5, 2017

Member

Don't use one line if statements

This comment has been minimized.

Copy link
@octacian

octacian Jan 5, 2017

Author Contributor

@rubenwardy fixed. I'll keep that in mind in the future :D

@octacian octacian force-pushed the octacian:clearinv branch from 33ffc09 to fb6b9b4 Jan 5, 2017

func = function(name, param)
local player, msg, pmsg
if param and param ~= "" then
if not core.check_player_privs(name, {clearinv=true}) and name ~= param then

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jan 5, 2017

Member

maybe move name ~= param to the if statement on line 948? Make it param ~= name
Stops the cleared your inventory being printed it it's yourself

params = "[name]",
description = "Clear the inventory of yourself or another player",
func = function(name, param)
local player, msg, pmsg

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jan 5, 2017

Member

msg is unused

@octacian octacian force-pushed the octacian:clearinv branch from fb6b9b4 to a43e947 Jan 5, 2017

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Jan 5, 2017

Made changes requested by rubenwardy.

BTW, the msg variable had been used to return a different value depended on whether the player's own inventory or that of another player was being cleared. But, it is true that I'm not using it anymore as I decided it wasn't really needed, so removed.

.. " to run this command (missing privilege: clearinv)"
end
player = core.get_player_by_name(param)
pmsg = name.." cleared your inventory."

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Jan 6, 2017

Member

Maybe send the message here? Removes need for pmsg

This comment has been minimized.

Copy link
@octacian

octacian Jan 10, 2017

Author Contributor

@rubenwardy done. Not sure why I didn't do that in the first place.

@bigfoot547

This comment has been minimized.

Copy link
Contributor

bigfoot547 commented Jan 6, 2017

Hello @octacian , I have some suggestions 😄!

  • [maxcount] argument to remove only this many items. (Not itemstacks, ItemStack:get_count())
  • [item] argument to only remove a certain itemstring.
  • [listname] only remove from this invref list.
  • This is somewhat in the noise, but after execution, tell the player how many ItemStacks and items were removed.
  • Do not use inv:set_list, iterate through the list like in my morecomands mod.

Thanks bigfoot547.
Also 👍.

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Jan 8, 2017

@bigfoot547 interesting suggestions. First thing, of most importance, why should I not use inv:set_list? Iterating through each itemstack seems incredibly inefficient. About the other things, I think that they should be a part of a different command, maybe /clearstack not /clearinv. IDK, just a thought. Adding all that to this command just seems a little overly complicated considering how it would generally be used. I really understand why being able to clear only specific things would be useful, just don't think it fits in this PR.

@bigfoot547

This comment has been minimized.

Copy link
Contributor

bigfoot547 commented Jan 9, 2017

Okay. Thanks for the consideration 😄

@sofar

This comment has been minimized.

Copy link
Member

sofar commented Jan 10, 2017

What exactly requires this to be in builtin ?

In other words, please explain why this can't be put inside minetest_game or minimal_game instead, and why this command is needed for every (every) subgame that exists.

Once we add this, it will be in every subgame. This will make it more difficult for subgames that rely on items not being destroyable, and may be abused by players. Imagine for instance a CTF game where someone grabs the opponents flag and does /clearinv making it impossible for his team to win.

Solutions:

  • put this in minetest_game instead?
  • require a priv to empty your own inventory? (not my preference)

Note: the fact that a trashcan exists in some of the inventory mods is not relevant: mods/subgames can remove the trashcan as well as the possibility to drop items or prevent them from being destroyed in e.g. lava.

@octacian octacian force-pushed the octacian:clearinv branch from a43e947 to ef40e61 Jan 10, 2017

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Jan 10, 2017

This doesn't necessarily need to be in builtin. Other devs said I should put it in builtin, that's the only reason why it's here. If it were to be put in MTG, I really think that rather than creating a new mod like killme, we should just add a mod specifically for trivial chatcommands like this. (I think that killme should be moved to something like that either way.)

If /clearinv stays in builtin, I agree that a priv for players to clear their own inventory isn't the way to go. I think we'd be better off to simply introduce a setting (maybe disable_clearinv) that if false would prevent the command and privilege from being registered. Not sure if that's a good way to do it at all, just throwing around ideas.

I can see, though, that putting it in MTG could still be preferable because subgames that didn't need the functionality wouldn't need to make any changes. I'm more than willing to open a PR on MTG instead. The question for that though, is where to put it. Personally I would learn toward something like chatcommands.lua in default as I mentioned above.

@sofar

This comment has been minimized.

Copy link
Member

sofar commented Jan 10, 2017

A setting is client-side, so it would have no effect.

Note: I'm perfectly OK with having this command widely available, so I'm puzzled how we can nicely solve this issue.

Maybe we should just assume the server priv is required for clearing out other players privs, and add a new (default) priv allowing players to empty their own inventory? Then servers can revoke that priv for all players.

@kahrl

This comment has been minimized.

Copy link
Contributor

kahrl commented Jan 11, 2017

Players can already delete items with the /pulverize command, which is also in builtin and requires no privs.

Maybe an API could be provided that allows mods to delete any registered chat command?

-- in builtin/game/chatcommands.lua
function core.unregister_chatcommand(cmd)
    core.chatcommands[cmd] = nil
end
@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Jan 19, 2017

@sofar just went on a CTF as I thought the flag wasn't stored as an inventory item. It isn't. So, /pulverize or /clearinv shouldn't create any gameplay issues for CTF.

Either way, I'd support a feature to either unregister or override a command. Might be nice to have both. I'd be happy to submit a PR for unregistering and/or overriding.

@octacian octacian force-pushed the octacian:clearinv branch 2 times, most recently from 2d0b1e4 to 86177d8 Jan 19, 2017

@bigfoot547

This comment has been minimized.

Copy link
Contributor

bigfoot547 commented Jan 20, 2017

What happens when a mod creates its own inv list for the player?
Will those lists get cleared?

unified_inventory, 3d_armor comes to mind.

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Jan 20, 2017

Since #5076 has been merged, I'd say this is all ready to go. With the ability to override and unregister chatcommands, the issue with mods/games that don't want this should be dealt with.

@bigfoot547 I think that mods like unified_inv or 3d_armor should either provide their own command or override this one. I guess I could iterate through the lists, but then what if there was a mod that didn't want all the inventories cleared. (e.g. clearing all would cause the player hand to be reset to default.)

@bigfoot547
Copy link
Contributor

bigfoot547 left a comment

I examined it, looks good!

@Zeno- Zeno- added the WIP label Jan 26, 2017

@Zeno-

This comment has been minimized.

Copy link
Contributor

Zeno- commented Jan 26, 2017

@twoelk

This comment has been minimized.

Copy link

twoelk commented Jan 26, 2017

I think the option "Can clear another player's inventory" would come in extremely handy in an educational environment. In such a use case this would not only be usefull for the admin but rather any staff or moderator that might be assigned the job only temporarly.

Think of a class that visits an existing world to take up a new task. the teacher could assign one pupil as moderator who may need to clear the inventories of all other players now and then to get the task done as planned out to achieve some educational goal. Just as he might need the ability to provide the pupils in bulk or predefined groups with certain amounts of resources.

I think minetest indeed lacks some more tools a teacher or an admin might find useful to monitor players in groups. As interest seems to grow for minetest as educational tool I do hope development would listen more to the needs of educational personall as I could imagine minetest would benefit greatly from a gain in popularity among such people and institutions.

Actually it might indeed be extremely usefull if minetest had a built in function to assign players to more groups that may even overlap and commands could be targeted at certain groups of players be it based on privs or maybe proximity to some coordinates, special nodes or even elements within the player name.

Of course anything as powerfull as this should be completely configurable, so a server admin or a game designer may decide to use the feature, hide it or completely dissable it. Hoping #5076 catches all use cases.

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Jan 26, 2017

With twoelk's comment above, I realize that in some cases that he mentioned, one might want the craft grid to be cleared as well. Otherwise, a student could stash something there that they were not supposed to have. And, if a student in each group were to be assigned to be a "moderator," you wouldn't want to give them the server priv.

I won't move to the server priv right away, I'd like some more feedback. And, with the craftgrid, I think the best way to go is to either introduce a command like clearcraft or an option like clearinv_clears_craftgrid. I think the best way would be a command specifically for it, will need some feedback though.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Jan 26, 2017

How about keeping it simple, a single command to clear inventory, crafting grid and output?

Why the ability to clear other's inventories? Let's only add things that are actually needed, to keep it simple, the classroom example given above is too narrow and specialist to justify.
Then we wouldn't need the 'clearinv' priv either.

I feel this is getting needlessly complex.

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Feb 11, 2017

Personally, I am against merging without the ability to clear other's inventories. Yes, the classroom example is narrow, but it is important and isn't the only place features like that could be needed. Take an adventure map, for example, the creator might want to clear the player's inventory.

Yes, the extra privilege is not necessarily needed, though it could, be useful in the classroom or other situations. However, considering that there is an API to override chat commands it shouldn't really be an issue as if used in a school, I would assume that they would already have a mod in use to make MT work better for their school. However, I'm still neutral on this. On one hand, it's nice for things to just work, rather than having to make modifications. Really, though, the extra privilege is like 3 or 4 lines... IDK

About clearing both the inventory and crafting grid. This too could be overridden, so might not be an issue. The original reason for not clearing the crafting grid was because one might want to have a place to put items that they didn't want to lose (e.g. screwdriver). But now that I think about it, that's what chests are supposed to be for.

@rubenwardy
Copy link
Member

rubenwardy left a comment

apart from that, 👍

params = "[name]",
description = "Clear the inventory of yourself or another player",
func = function(name, param)
local player, pmsg

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy Feb 11, 2017

Member

pmsg is unused

This comment has been minimized.

Copy link
@octacian

octacian Feb 12, 2017

Author Contributor

Fixed. Missed it in last update somehow.

@octacian octacian force-pushed the octacian:clearinv branch from 86177d8 to cb89cc1 Feb 12, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Feb 13, 2017

The priv has the same name as the basic command, to avoid confusion perhaps it should be clearotherinv, clearinvother or something that makes clear it is for clearing other player's inventories?
I still disagree with adding a priv and ability to clear other player's inventories, it will only be very rarely used or needed.

@octacian octacian force-pushed the octacian:clearinv branch from cb89cc1 to 7c4befc Feb 13, 2017

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Feb 14, 2017

Concerning where to put this, in MTGame or builtin, i'm neutral.

@Wuzzy2

This comment has been minimized.

Copy link
Contributor

Wuzzy2 commented Feb 24, 2017

I approve. :-)

Hmm, after 10 seconds of thinking I have the ultimate undisputable use case for clearing other people's inventories: Command blocks (from Mesecons)! And placing them in adventure maps.

But in either case, I approve no matter if this command can only clear your own inventory or the inventory of any player.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented Feb 24, 2017

Ok, more people are saying this is useful, and this now uses the server priv, so i'm neutral now.

@paramat paramat removed the WIP label Feb 24, 2017

@sorcerykid

This comment has been minimized.

Copy link

sorcerykid commented Mar 4, 2017

I seem to recall another discussion in the issue tracker how privileges are "supposed to describe abilities not status" and that "one priv for many different things is not nice" and that "server owners want to grant only specific abilities." Clearing player inventories has nothing to do with server operations. Hence there should be a specific privilege for clearing player inventories that can be granted selectively to any user.

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented Mar 14, 2017

@sorcerykid that's true, the server privilege is kinda an exception, though I personally still prefer to introduce new privileges for stuff like this it's really up to the devs.

Anybody got further thoughts?

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented May 16, 2017

From my perspective, this is ready to merge. All of the requested changes have been made, and to my knowledge, I've satisfied those who have commented.

Anyone else willing to review? I'd really like to see this merged before 0.4.16.

@paramat

This comment has been minimized.

Copy link
Member

paramat commented May 16, 2017

First post and commit message needs updating about required priv.

Add /clearinv chat command
Allow players to clear their own inventory or that of another player with /clearinv command. server privilege is required to clear another player's inventory, no privileges are required to clear your own inventory.'

@octacian octacian force-pushed the octacian:clearinv branch from 7c4befc to 1d0068f May 16, 2017

@octacian

This comment has been minimized.

Copy link
Contributor Author

octacian commented May 16, 2017

@paramat Done.

@nerzhul nerzhul merged commit dada983 into minetest:master May 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@octacian octacian deleted the octacian:clearinv branch May 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.