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

Allow optional actor ObjectRef value in node interaction calls #14505

Merged
merged 10 commits into from Apr 5, 2024

Conversation

Emojigit
Copy link
Contributor

@Emojigit Emojigit commented Mar 30, 2024

This PR adds an optional operator (ObjectRef) parameter to node interaction calls in l_env.cpp.

Previously, mimicking all the behaviors when a player digs a node was complex when specifying an ObjectRef. With this PR, the operator can be passed onto the callbacks.

This will be the true solution to programmed node interactions needing interaction checks, e.g. minetest-mods/areas#54.

Fixes #13563
Related to #11336
Related to #14004 (comment)
Related to minetest-mods/areas#54

To do

This PR is Ready for Review.

  • minetest.place_node
  • minetest.dig_node
  • minetest.punch_node

Potential to-dos and issues

  • minetest.punch_node custom pointed_thing
  • Check code quality (I am new to C++)

How to test

I use //luatransform in WorldEdit to test my modifications. For example, to test dig_node:

# Specify the node position first

# These two pass no operator field and should work as how they would
//luatransform minetest.dig_node(pos)
//luatransform minetest.dig_node(pos, nil)

# This one passes the operator field and should:
# 1. Logged as the player's operation
# 2. Check for area protection
# 3. Add items into the player's inventory, if the game is coded so
//luatransform minetest.dig_node(pos, minetest.get_player_by_name("YOUR_USERNAME"))

# This should raise an error complaining about passing in non-ObjectRef
//luatransform minetest.dig_node(pos, "")

This code may help visualize the calls:

minetest.register_on_placenode(function(pos, newnode, placer, oldnode, itemstack, pointed_thing)
        minetest.log("action", "place; " .. minetest.pos_to_string(pos) .. "; " .. dump(newnode) .. "; " .. placer:get_player_name() .. "; " .. dump(oldnode) .. "; " .. itemstack:to_string() .. "; " .. dump(pointed_thing))
end)

minetest.register_on_dignode(function(pos, oldnode, digger)
        minetest.log("action", "dig; " .. minetest.pos_to_string(pos) .. "; " .. dump(oldnode) .. "; " .. digger:get_player_name())
end)

minetest.register_on_punchnode(function(pos, node, puncher, pointed_thing)
        minetest.log("action", "punch; " .. minetest.pos_to_string(pos) .. "; " .. dump(node) .. "; " .. puncher:get_player_name() .. "; " .. dump(pointed_thing))
end)

Example

https://github.com/C-C-Minetest-Server/twi_mods/blob/2e658d0df4e404dccb43d48934b631762ac80055/func_areas_limitations/init.lua#L27-L37

@Emojigit Emojigit marked this pull request as ready for review March 30, 2024 04:40
@Emojigit Emojigit changed the title Allow optional operator ObjectRef value in node interaction calls Allow optional actor ObjectRef value in node interaction calls Mar 30, 2024
Emojigit added a commit to C-C-Minetest-Server/twi_mods that referenced this pull request Mar 30, 2024
@grorp grorp added @ Script API Feature ✨ PRs that add or enhance a feature Concept approved Approved by a core dev: PRs welcomed! labels Mar 30, 2024
@grorp grorp added Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR and removed Concept approved Approved by a core dev: PRs welcomed! labels Mar 30, 2024
@grorp grorp self-assigned this Mar 30, 2024
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The three changed functions can be changed to use a pattern like this instead of duplicating code for the nil case and the ObjectRef case:

ServerActiveObject *placer = nullptr;
if (!lua_isnoneornil(L, 3)) {
    ObjectRef *ref = checkObject<ObjectRef>(L, 3);
    placer = ObjectRef::getobject(ref);
}

bool success = scriptIfaceItem->item_OnPlace(item, placer, pointed);
lua_pushboolean(L, success);

I'm wondering about something: Suppose you want to use minetest.dig_node in an automation mod. With this PR, you can specify a player so that the automation machine can dig in areas protected by the player. But if you specify a player, the items dug by the automation machine will end up directly in the player's inventory, instead of as dropped items or in the automation machine's inventory. How can this be solved / how is this solved in automation mods?

I think it could be useful to allow customizing the other parameters as well. For example, being able to set a custom pointed_thing for minetest.place_node might allow mods to fix #11335. (I saw you already have this on the to-do list.)

@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 30, 2024
@Emojigit
Copy link
Contributor Author

Emojigit commented Mar 30, 2024

The three changed functions can be changed to use a pattern like this ...

Fixed in cafeb62

... the items dug by the automation machine will end up directly in the player's inventory ...

This PR will not cover this entirely as that would require creating a "virtual player" object type. However, this PR still helps in area protection checks (e.g. minetest-mods/areas#54) and player operation extension (e.g. optimizing the code of Technic Mining Drills).

I think it could be useful to allow customizing the other parameters as well.

The main problem is the lack of an existing function to convert a pointed_thing Lua table into a PointedThing C++ object. I am new to Minetest's C++ API (and C++ as a whole), so I am afraid I can't do that.

@Emojigit Emojigit requested a review from grorp March 30, 2024 11:12
@grorp grorp removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 30, 2024
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior difference between place_node (nullptr = nil ObjectRef) and dig_node/punch_node (nullptr = invalid ObjectRef) is weird, but it already existed before this PR, so 🤷

src/script/lua_api/l_env.cpp Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
src/script/lua_api/l_env.cpp Outdated Show resolved Hide resolved
src/script/lua_api/l_env.cpp Outdated Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Mar 30, 2024
@Emojigit Emojigit requested a review from grorp March 30, 2024 22:33
doc/lua_api.md Outdated Show resolved Hide resolved
@grorp grorp added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Mar 30, 2024
@sfan5 sfan5 merged commit 2d8e4df into minetest:master Apr 5, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature ✨ PRs that add or enhance a feature One approval ✅ ◻️ @ Script API Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minetest.dig_node in protected areas will not work (even if you own the area)
3 participants