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

Fix default item callbacks to work with nil users #5819

Merged
merged 2 commits into from Oct 28, 2017

Conversation

Projects
None yet
9 participants
@raymoo
Copy link
Contributor

commented May 25, 2017

Fixes the issues mentioned by @HybridDog in #4668.

Potential issues (please comment if you have a problem with them):

  • core.item_drop already handled nil and non-players, but it did not return a modified itemstack. I modified it to do so, to make the behavior more similar to player behavior.
@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

By the way the mod code I used to test was this:


local function wrap_thing(name)
        local old = core[name]
        local function new(itemstack, _, ...)
                print(name)
                return old(itemstack, nil, ...)
        end
        core[name] = new
end

local function wrap_node(name)
        local old = core[name]
        local function new(pos, node, _, ...)
                print(name)
                return old(pos, node, nil, ...)
        end
        core[name] = new
end

for _, name in ipairs({ "item_place", "item_secondary_use", "item_drop", "on_use" }) do
        wrap_thing(name)
end

wrap_node("node_punch")
wrap_node("node_dig")

I only tested for nil users, not non-player ObjectRef.

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

Just tested with non-player ObjectRef too with


minetest.register_entity("param_test:obj", {})

local function wrap_thing(name)
        local old = core[name]
        local function new(itemstack, _, ...)
		local obj = minetest.add_entity({ x = 0, y = 0, z = 0 }, "param_test:obj")
                print(name)
                return old(itemstack, obj, ...)
        end
        core[name] = new
end

local function wrap_node(name)
        local old = core[name]
        local function new(pos, node, _, ...)
                local obj = minetest.add_entity({ x = 0, y = 0, z = 0 }, "param_test:obj")
		print(name)
		return old(pos, node, obj, ...)
        end
        core[name] = new
end

for _, name in ipairs({ "item_place", "item_secondary_use", "item_drop", "on_use" }) do
        wrap_thing(name)
end

wrap_node("node_punch")
wrap_node("node_dig")

This is how I tested (both versions of the test mod):

  • Dig a node (includes punching the node)
  • Place the node
  • Use some item (bronze axe) and right click it into the sky
  • Drop one of the dug nodes
  • Don't need to test on_use since there is no default behavior to override
@@ -653,7 +653,7 @@ core.register_chatcommand("pulverize", {
core.rollback_punch_callbacks = {}

core.register_on_punchnode(function(pos, node, puncher)
local name = puncher:get_player_name()
local name = puncher and puncher:get_player_name() or "!nonplayer"

This comment has been minimized.

Copy link
@rubenwardy

rubenwardy May 26, 2017

Member

no need to log rollback for this

@raymoo raymoo force-pushed the raymoo:fix_item_params branch from 8c538db to 788670d May 26, 2017

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

Addressed @rubenwardy's comment

@raymoo raymoo force-pushed the raymoo:fix_item_params branch from 788670d to e21039a May 26, 2017

@nerzhul nerzhul added the Bugfix label May 26, 2017

@nerzhul nerzhul added this to the 0.4.16 milestone May 26, 2017

@nerzhul nerzhul added this to To Do in Minetest 0.4.16 May 26, 2017


local function is_protected(pos, name, user)
return core.is_protected(pos, name) and
not (name ~= non_player_name and minetest.check_player_privs(user, "protection_bypass"))

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker May 26, 2017

Member

-> core.check_player_privs
This function also accepts a player name as argument, so no need for user in this function.

This comment has been minimized.

Copy link
@ShadowNinja

ShadowNinja May 27, 2017

Member

Also, this compound conditional is very hard to read. It should be broken up.

@@ -205,6 +207,16 @@ function core.get_node_drops(nodename, toolname)
return got_items
end

local non_player_name = "!nonplayer"

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker May 26, 2017

Member

Ugly workaround. Why not simply use nil, which directly indicates that there's no player name?

This comment has been minimized.

Copy link
@raymoo

raymoo May 26, 2017

Author Contributor

Was mainly for protection stuff, so I didn't need to choose whether nil / non players should be always protected or never protected. Or should minetest.is_protected be required to accept nil also?

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker May 26, 2017

Member

:get_player_name() returns an empty string for non-player ObjectRef, so that would be another possibility. Pipeworks already uses :pipeworks as dummy player but there's nowhere defined what dummy players are allowed to do. Thus, I'm not sure what the best solution here might be.

local p = {x=pos.x, y=pos.y+1.2, z=pos.z}
local cs = itemstack:get_count()
local dropper_is_player = dropper and dropper:is_player()
local p = pos

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker May 26, 2017

Member

How about

pos = table.copy(pos) -- Deep copy, drop reference

if dropper_is_player then
	pos.y = pos.y + 1.2

This comment has been minimized.

Copy link
@raymoo

raymoo May 26, 2017

Author Contributor

No real point since we never mutate p

EDIT: Actually nevermind, I see what you meant.

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented May 27, 2017

Instead of !nonplayer this should use nil or the empty string.

@raymoo raymoo force-pushed the raymoo:fix_item_params branch from e21039a to ee9ca93 May 27, 2017

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2017

Ok, addressed @SmallJoker and @ShadowNinja's comments. I made user_name output empty string for nonplayers, and made it so that logging is only done for actual players.

EDIT: Forgot to mention, I tested it the same way as before.

@nerzhul

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

@SmallJoker @ShadowNinja can you finish the review we need this PR for release

@@ -205,6 +207,25 @@ function core.get_node_drops(nodename, toolname)
return got_items
end

local non_player_name = ""
local function user_name(user)
return user and user:is_player() and user:get_player_name() or ""

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Jun 2, 2017

Member
  • get_player_name(): returns "" if is not a player
    From lua_api.txt

This expression can be simplified: return user and user:get_player_name() or ""


-- Returns a logging function. For nil names, does not log.
local function make_log(name)
if name ~= non_player_name then

This comment has been minimized.

Copy link
@SmallJoker

SmallJoker Jun 2, 2017

Member

Comment: * For empty names

Do we need non_empty_player? As we already have this empty string documented as non-player player name it would be fine to simply do if name ~= "" then.

This comment has been minimized.

Copy link
@raymoo

raymoo Jun 2, 2017

Author Contributor

Right, I don't think it's necessary

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

Otherwise looks good.

@raymoo raymoo force-pushed the raymoo:fix_item_params branch from ee9ca93 to 104527c Jun 2, 2017

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

@SmallJoker Ok, fixed those points

@nerzhul nerzhul moved this from To Do to Delayed from release in Minetest 0.4.16 Jun 3, 2017

@nerzhul nerzhul removed this from the 0.4.16 milestone Jun 3, 2017

@nerzhul nerzhul removed this from Delayed from release in Minetest 0.4.16 Jun 3, 2017

@nerzhul nerzhul added this to Ready to merge in Minetest 5.0.0 blockers Jun 3, 2017

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented Jun 10, 2017

According to the docs item_drop must return the leftover itemstack (because it's the default for on_drop, which requires those semantics). Was it not already doing that?

@raymoo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2017

@ShadowNinja It was doing so for real players, but there was a case for non-players that did not.

end
if good_tool then
break
if toolname then

This comment has been minimized.

Copy link
@nerzhul

nerzhul Jun 22, 2017

Member

ident level is crazy can you move that do a function please ?

not minetest.check_player_privs(name, "protection_bypass")
end

-- Returns a logging function. For nil names, does not log.

This comment has been minimized.

Copy link
@sfan5

sfan5 Jun 23, 2017

Member

it doesn't do so for empty names, not nil names

@SmallJoker
Copy link
Member

left a comment

LGTM

@sfan5

sfan5 approved these changes Oct 27, 2017

Copy link
Member

left a comment

lgtm

@sfan5 sfan5 added >= Two approvals and removed One approval labels Oct 27, 2017

@nerzhul nerzhul merged commit a5d5728 into minetest:master Oct 28, 2017

1 check passed

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

@paramat paramat referenced this pull request Nov 11, 2017

Closed

Add nil checks for placer #1907

@paramat

This comment has been minimized.

Copy link
Member

commented Nov 11, 2017

sfan5 added a commit that referenced this pull request Nov 19, 2017

Fix default item callbacks to work with nil users (#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

@SmallJoker SmallJoker moved this from Ready to merge to Done in Minetest 5.0.0 blockers Dec 2, 2017

@sfan5 sfan5 referenced this pull request Dec 5, 2017

Closed

Backport 0.4 #6746

sfan5 added a commit that referenced this pull request Dec 5, 2017

Fix default item callbacks to work with nil users (#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

rubenwardy added a commit to rubenwardy/minetest that referenced this pull request Dec 7, 2017

Fix nodes dropping the wrong item
Regression from minetest#5819 which caused handle_node_drops
to ignore drops in a node's definition.

rubenwardy added a commit to rubenwardy/minetest that referenced this pull request Dec 7, 2017

Fix nodes dropping the wrong item
Regression from minetest#5819 which caused handle_node_drops
to ignore drops in a node's definition.

sfan5 added a commit that referenced this pull request Dec 7, 2017

Fix nodes dropping the wrong item
Regression from #5819 which caused handle_node_drops
to ignore drops in a node's definition.

t0ny2 pushed a commit to t0ny2/minetest that referenced this pull request Jan 23, 2018

Fix default item callbacks to work with nil users (minetest#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

sfan5 added a commit that referenced this pull request Feb 2, 2018

Fix default item callbacks to work with nil users (#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

sfan5 added a commit that referenced this pull request Feb 3, 2018

Fix default item callbacks to work with nil users (#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

sfan5 added a commit that referenced this pull request Apr 21, 2018

Fix default item callbacks to work with nil users (#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

sfan5 added a commit that referenced this pull request May 13, 2018

Fix default item callbacks to work with nil users (#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

SmallJoker added a commit that referenced this pull request Jun 3, 2018

Fix default item callbacks to work with nil users (#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

osjc added a commit to osjc/minetest that referenced this pull request Jan 11, 2019

Fix default item callbacks to work with nil users (minetest#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

osjc added a commit to osjc/minetest that referenced this pull request Jan 23, 2019

Fix default item callbacks to work with nil users (minetest#5819)
* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`
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.