Wrench support extend

[nixnoxus]

Adds wrench support for bones, connected_chests and some nodes from pipeworks and xdecor.

For more information see: https://github.com/mt-mods/wrench/tree/wrench_support_extend#supported-mods-and-nodes

Related to https://github.com/mt-mods/technic/pull/251

[OgelGames]

I made some changes and added support for more_chests, I hope you don't mind, I thought it would be easier to work on the same branch so we can both work on support for different mods :)

[nixnoxus]

I'm curious about why you changed this, get_timeout() always returns a value, with 0 being no timer running.

you are right, i fixed it I removed the 'if timeout> = 0' so that technic.injector may remain disabled after placement

Is it different in other minetest versions? (I use 5.5.0-dev)

i think no. i use 5.4.1

[OgelGames]

Ah ok 👍 I guess I didn't test it enough to notice that :P

[nixnoxus]

drop can also be a table: lua_api.txt#L7861-L7914.

oops. but I would like the possibility to take the drop name from the mod. how about this:

diff --git a/functions.lua b/functions.lua
index a5424e3..453fb05 100644
--- a/functions.lua
+++ b/functions.lua
@@ -47,10 +47,14 @@ function wrench.pickup_node(pos, player)
                        return false, errors.owned:format(owner)
                end
        end
-       local drop = def.drop and minetest.registered_nodes[node.name].drop
-       if drop then
+       if type(def.drop) == "string" then
+               node.name = def.drop
+       elseif type(def.drop) == "boolean" then
                -- drop node name like digging
-               node.name = drop
+               local drop = def.drop and minetest.registered_nodes[node.name].drop
+               if type(def.drop) == "string" then
+                       node.name = drop
+               end
        end
        local data = {
                name = node.name,

we would then have both options

[OgelGames]

how about this we would then have both options

Good idea 👍

[nixnoxus]

Add proper tests Instead of adding debug code into mod. Optionally if debug code like that is needed locally file can be added to local .gitignore Local debug mod is also one option for this.

the debug code provides helpful information about objects that are not part of this mod (to support them). i played around with mineunit a bit, but haven't (yet) found a way to access objects from other mods.

we can simply remove the debug code before merge. This is why it is in debug.lua

[S-S-X]

played around with mineunit a bit, but haven't (yet) found a way to access objects from other mods

You either load other mod (not recommended) or add fixtures (recommended)

[S-S-X]

played around with mineunit a bit, but haven't (yet) found a way to access objects from other mods

You either load other mod (not recommended) or add fixtures (recommended)

I guess I could add simple Mineunit API tests which should give some idea about fixtures. Could as well try to improve Mineunit tool use a bit... was one of things I've been working on anyway, while usable it is bit too verbose and not exactly intuitive currently.

[nixnoxus]

sorry, for resolving. I clicked wrong:(

I also find it more logical to give the burned ones. that's what the wrench is for.

[nixnoxus]

pipeworks: broken_tube_1 should maybe too can be taken with the wrench. what do you all mean?

[OgelGames]

I think broken tubes should not be able to be picked up, as they are a placeholder for multiple different tubes.

[OgelGames]

I realized there's two problems with picking up burnt Luacontrollers; they don't have a description, and if wrench was later disabled on a server, players would be left with an "illegal" node in their inventory.

If there would be one rule to how the wrench should behave, it would be that it shouldn't put unobtainable items in a player's inventory.

The lack of description is the more obvious issue though:

EDIT: actually that would mean the colored technic chests would be not allowed, so that isn't a possible rule...

[nixnoxus]

If there would be one rule to how the wrench should behave, it would be that it shouldn't put unobtainable items in a player's inventory.

Maybe control this via privs? Then the server admin has control over it. Like:

diff --git a/nodes/mesecons.lua b/nodes/mesecons.lua
index e3f406e..23a39b8 100644
--- a/nodes/mesecons.lua
+++ b/nodes/mesecons.lua
@@ -92,4 +92,5 @@ end
 end

 luacontroller_defs.drop = nil
+luacontroller_defs.privs = "wrench_special_nodes"
 wrench.register_node("mesecons_luacontroller:luacontroller_burnt", luacontroller_defs)

Alternative wrench.special_nodes = true as setting.

This also affects bees:hive_wild.

[OgelGames]

I realized that's not a possible rule because of colored technic chests, those have always been able to be picked up, so that cannot change.

I fixed the Luacontroller and Lua tube to take their description from their unburnt nodes.

[nixnoxus]

@OgelGames: do you have a special mod for testing or do you also test manually?

[OgelGames]

I test manually. Even though some things are missed occasionally, I still find it better than any automated tests.

[nixnoxus]

the translations do not work for me with this code:

local S = rawget(_G, "intllib") and intllib.Getter() or function(s) return s end

I already noticed that with the technic mod. Does anyone against it if I rewrite it? Like:

local S = minetest.get_translator()

[OgelGames]

Go ahead, it was already planned to be done: https://github.com/mt-mods/technic/issues/104 👍

[nixnoxus]

FYI: I converted and updated the locale/ directory with https://github.com/nixnoxus/minetest_hacks/blob/main/bin/update-locale.sh

There is already a script for updating locale: https://github.com/minetest-tools/update_translations But it removes comments at the beginning (from the translators) and destroys the structure.

[nixnoxus]

i think it is time for review and merge. :rocket:

[OgelGames]

i think it is time for review and merge. 🚀

Not quite yet, still got a few more mods to add support for 😉

Also, it might be good to split the mesecons_* nodes into each mod, as they could be disabled (because mesecons is a modpack).

[nixnoxus]

Why an extra variable? How is that any different?

meta:to_table() is empty after minetest.remove_node(pos) That's why I save the table before in meta_table.

What is the purpose of this? The only places it's used doesn't seem necessary. (it can be replaced by nil)

This is the reason: https://github.com/mt-mods/wrench/pull/2/commits/3b8bf58b45e0a2be418f3a43de839042a8699459 For testing new nodes, I had installed this check. It prevents (if wrench.enable_debug = true) the pickup nodes with unknown metas or lists. This is to prevent metas or lists from being missed when adding new nodes. If the value is nil, the key no longer exists and cannot be checked.

[OgelGames]

meta:to_table() is empty after minetest.remove_node(pos) That's why I save the table before in meta_table.

Ah, of course 🤦

[BuckarooBanzay]

the commits are piling up on this one, is there a plan to merge this someday? (i'm just asking, no pressure)

[OgelGames]

Yeah... It really should be merged... :P

I think there are too many mods we could support to keep this open, so I think we can leave those for other PRs. I'll try to find some time to go over the "recent" additions and put a review in.

One question: Are we planning on keeping the debug stuff or removing it before merging? I don't really mind either way, keeping it just means more code to maintain. (also it's usefulness is limited IMO)

[nixnoxus]

I would like to keep the debug code, this helped me enormously with adding and testing new and old nodes.

Maybe we can also merge https://github.com/mt-mods/wrench/pull/4 into it. I would like to add tests for the API. But I could also do that in a separate PR.

In the next few days I will create a nice screenshot with all supported nodes.

I have one question about the authors in https://github.com/mt-mods/wrench/blob/wrench_support_extend/README.md#contributors-originally-technic-modpack Not all authors of technic have contributed to the wrench:

~/minetest.git/mods/.technic.mt-mods$ git remote -v
origin  https://github.com/mt-mods/technic (fetch)
origin  https://github.com/mt-mods/technic (push)
~/minetest.git/mods/.technic.mt-mods$ git log --pretty=format:"%an" | sort -u > a
~/minetest.git/mods/.technic.mt-mods$ git log --pretty=format:"%an" $(find wrench/) | sort -u > b
~/minetest.git/mods/.technic.mt-mods$ diff -u a b | grep -i -e kpoppel -e Nekogloop -e Ekdohibs -e  Nore -e ShadowNinja -e VanessaE -e BuckarooBanzay -e OgelGames -e int-ua -e groxxda -e SwissalpS
 BuckarooBanzay
-Ekdohibs
-kpoppel
-Luke aka SwissalpS
 OgelGames
 ShadowNinja
-VanessaE

Do we remove them (starting with '-') from the contributors list?

[S-S-X]

I would like to keep the debug code, this helped me enormously with adding and testing new and old nodes.

In my opinion debug code like that should not be included with mod but instead included through some other code snippet collection / debug repository / debug mod / just locally with git ignores added. Not 100% sure but it seems like that file could just be separate mod.

If it stays then at least make sure that there is separate commit for anything related to debug thing.

Maybe we can also merge #4 into it. I would like to add tests for the API. But I could also do that in a separate PR.

Not sure what you meant with this but I would recommend handling at least merges through separate PR possibly continuing on top of #4 or #5 instead of merging tests into this branch. Mostly because it is easier to handle cleaning up commits to get history that allows clean cherry picks, exclusion, filtering, merging, reverting and what not.

In the next few days I will create a nice screenshot with all supported nodes.

I would again recommend doing another branch and pull request for that instead of including those here, again makes reviews and cleaning up history a lot simpler.

I have one question about the authors

I'd say again would be better to make another PR for that and discuss it there. Also while commits do not always equal to contributors it might be true here because stuff is coming from technic mod, but that's just another reason to have separate PR for that.

...unless file was added here, then it makes sense to clean up before adding nonexistent contributors to master branch but just piling up unrelated stuff to PR commit list does not make sense and is not exactly helpful in long term development.

[nixnoxus]

I would like to keep the debug code, this helped me enormously with adding and testing new and old nodes.

In my opinion debug code like that should not be included with mod but instead included through some other code snippet collection / debug repository / debug mod / just locally with git ignores added. Not 100% sure but it seems like that file could just be separate mod.

In my eyes, besides tests, debug code also belongs in the respective project. Debug.lua could be swapped out, but the sense of wrench.META_TYPE_IGNORE and lists_ignore in nodes/*.lua would no longer be obvious. Both variables prevent (if wrench.enable_debug = true) pick up nodes with unknown meta.fields or inventorys. With this I had found among others these missing metas:

• https://github.com/mt-mods/wrench/pull/2/commits/1c7d0895a5613bc36bd8d474d520a38db8a9fad8#diff-adfdaef00d75544242b6d93abb39ef7e74f1f379a6878ea01feff1ff202038daR8
• https://github.com/mt-mods/wrench/pull/2/commits/989fccb9d8b3126ae87a1e0d4963ba95c1e7114f#diff-783d1f08873b6e6282a9c05b0e77438b90a74f96b66200b64f48daf19179a2d6R35
• https://github.com/mt-mods/wrench/pull/2/commits/03c6bfcf5ffdd383c228de2d6d31e8ded3e17f52#diff-ea7c22a9ce0a99c97f7f792921e79ee827da6a9966d44c6cde59fd19b2b492c0R33

On the other hand, we can also run this check in general and move it to wrench.pickup_node. This would prevent the possible loss of meta data after updating other mods. In this case I would do without debug.lua.

If it stays then at least make sure that there is separate commit for anything related to debug thing.

Maybe we can also merge #4 into it. I would like to add tests for the API. But I could also do that in a separate PR.

Not sure what you meant with this but I would recommend handling at least merges through separate PR possibly continuing on top of #4 or #5 instead of merging tests into this branch. Mostly because it is easier to handle cleaning up commits to get history that allows clean cherry picks, exclusion, filtering, merging, reverting and what not.

Okay.

In the next few days I will create a nice screenshot with all supported nodes.

I would again recommend doing another branch and pull request for that instead of including those here, again makes reviews and cleaning up history a lot simpler.

Okay.

I have one question about the authors

I'd say again would be better to make another PR for that and discuss it there. Also while commits do not always equal to contributors it might be true here because stuff is coming from technic mod, but that's just another reason to have separate PR for that.

...unless file was added here, then it makes sense to clean up before adding nonexistent contributors to master branch but just piling up unrelated stuff to PR commit list does not make sense and is not exactly helpful in long term development.

This is true. In this commit https://github.com/mt-mods/wrench/pull/2/commits/803d39166a233740d32dd565807020aa4e5db6b9 I had copied and pasted the authors and license from technic. That's why I would clean it up in this PR or generally leave it like this.

[S-S-X]

In my eyes, besides tests, debug code also belongs in the respective project.

Can it be moved to subdir debug for example and move all relevant stuff from main mod in a way that everything keeps working if that sub directory is removed and all advanced debug related stuff is contained within subdirectory? That's exactly how tests are working, there's no test related code in main mod at all, not single line or word and for some mods spec directory is simply stripped out from release package using git export rules.

[OgelGames]

I had a thought: is it possible to have mineunit perform the same checks as the debug code does? That would not only make the tests more useful, but also debug.lua would no longer be needed.

[S-S-X]

I had a thought: is it possible to have mineunit perform the same checks as the debug code does? That would not only make the tests more useful, but also debug.lua would no longer be needed.

If Minetest engine is able to do it then Mineunit should be able to do it. However not everything is implemented for Mineunit yet as I've added functionality when it is needed. Should take closer look at debug.lua but if it is some simple metadata checking then should be able to do that.

Biggest problem for fully automated integration testing is probably mod dependencies, Mineunit is able to load multiple mods (as you can see from example #5 which downloads drawers as dependency) but in my opinion that's not really good way to do regression testing.

Mostly because it is not that easy to decide which version of other mod should be used and if latest dev/git master then it would be testing and reporting possible errors in other mods. But also because mods needs to be downloaded for every test run, it should be possible but depending on what that requires from other mods it might not be good idea.

Fixtures can be used for proper API behavior testing against certain metadata and definitions, that's preferred way to do self contained regression testing. With this approach you basically create minimal "perfect" version of supported node by using minetest.register_node something like this: https://github.com/mt-mods/wrench/blob/b105f31d59bbc4176afb1bbbf22316ef2711041f/spec/fixtures/bitchange.lua#L6-L25

[OgelGames]

it is not that easy to decide which version of other mod should be used

Well AFAIK we are only supporting the versions listed in the readme, so wouldn't those versions be used for testing too?

https://github.com/mt-mods/wrench/tree/wrench_support_extend#supported-mods-and-nodes

Fixtures can be used for proper API behavior testing against certain metadata and definitions, that's preferred way to do self contained regression testing. With this approach you basically create minimal "perfect" version of supported node

The problem with that is if something changes in a mod (adding or changing metadata names), we won't know about it until someone reports it. :/

[nixnoxus]

The debug/debug.lua does not contain classic test code but:

• Lists after loading the supported nodes
• shows code to add new nodes when wrench is used with 'sneak'.
• may return a message if a node can't be pickup (not supported or node has no meta data)

[S-S-X]

Well AFAIK we are only supporting the versions listed in the readme, so wouldn't those versions be used for testing too?

The problem with that is if something changes in a mod (adding or changing metadata names), we won't know about it until someone reports it. :/

Your 2 statements contradicts with each other...

[nixnoxus]

I think that unit tests can only ensure a stable and consistent API.

The interaction with other mods we can only ensure with integration tests (with loading other mods).

[S-S-X]

Could possibly try separate integration tests with actual player interaction by actually loading mods and writing simple test loop where player picks up and places whitelisted nodes. Similar to node placement test for Technic mod https://github.com/mt-mods/technic/blob/7e466e9b41215306b220d83b932b754294f32e1d/technic/spec/nodes_spec.lua#L44-L46 And placement_test function: https://github.com/mt-mods/technic/blob/7e466e9b41215306b220d83b932b754294f32e1d/technic/spec/nodes_spec.lua#L15-L20

Probably need to implement some missing functionality for Mineunit and allow disabling test failures that happen because of deprecated Minetest Lua API use, many mods are using deprecated functions and currently Mineunit throws errors for some if deprecated version is used.

[nixnoxus]

@OgelGames i found some issues with your changes:

digistuff:detector https://github.com/mt-mods/wrench/pull/2/commits/29a48575363ba7189e8c44be937f71095f7aa57b#diff-0de17c9b8ddfd83bc350c98c87ecc8490af2647c2c963e15d08ec519fac173c9L84-R77 => config loss after replace

digistuff:eeprom https://github.com/mt-mods/wrench/pull/2/commits/29a48575363ba7189e8c44be937f71095f7aa57b#diff-0de17c9b8ddfd83bc350c98c87ecc8490af2647c2c963e15d08ec519fac173c9L107-R102 => right click dosn't work after replace

~~digistuff:piezo https://github.com/mt-mods/wrench/pull/2/commits/29a48575363ba7189e8c44be937f71095f7aa57b#diff-0de17c9b8ddfd83bc350c98c87ecc8490af2647c2c963e15d08ec519fac173c9L216 => can't pickup after digiline_send("beep", "longbeep")~~

[OgelGames]

digistuff:detector => config loss after replace

It works fine for me...

digistuff:eeprom => right click dosn't work after replace

Ah... that doesn't set it's formspec the same was as digistuff:ram does...

Fixed here: https://github.com/mt-mods/digistuff/commit/b60d9b3f09555c7064ef0b3dc690e842588ad8dc

[nixnoxus]

digistuff:detector => config loss after replace

It works fine for me...

My test setup: 1. place digistuff:detector 2. set channel = c_ob, radius = 1 3. pickup digistuff:detector 4. (re)place digistuff:detector ~~5. right click digistuff:detector ~~ ~~ => channel is now 0~~ same effect also with https://github.com/mt-mods/digistuff/commit/b60d9b3f09555c7064ef0b3dc690e842588ad8dc

Sorry, my mistake

[nixnoxus]

Looks good to me so far.

I think it would be good if we include support for digistuff:advtouchscreen again since it is still included in the upstream digistuff mod. :thinking: (support remove here https://github.com/mt-mods/wrench/pull/2/commits/29a48575363ba7189e8c44be937f71095f7aa57b#diff-0de17c9b8ddfd83bc350c98c87ecc8490af2647c2c963e15d08ec519fac173c9L276-L282)

[OgelGames]

I think it would be good if we include support for digistuff:advtouchscreen again since it is still included in the upstream digistuff mod.

IMO we should only support the versions stated in the readme, the API can be used for adding support for other versions/nodes. While it would be good to support more, I think in the long run it will be a lot easier to manage if we only support specific versions.

[nixnoxus]

@S-S-X please review.

[S-S-X]

Picking up bones: functions.lua:150: bad argument #1 to 'pairs' (table expected, got nil) Because placed bones do not have inventory, same situation can happen with any other node placed by other mods or direct commands, should be checked.

So no for blacklisting or whitelisting certain names but checking if that specific node has inventory and prevent pickup if it does not have, I would also recommend against digging said node because that can increase complexity and not exactly what wrench should be doing.

Simply do not do anything, maybe chat message telling that node cannot be picked up with wrench because it does not have inventory.

[nixnoxus]

Picking up bones: functions.lua:150: bad argument #1 to 'pairs' (table expected, got nil)

Fixed.

Simply do not do anything, maybe chat message telling that node cannot be picked up with wrench because it does not have inventory.

A chat message 'Cannot pickup node. Node has no inventory.' is written.

[nixnoxus]

if no objections come, I will merge this weekend and create a new branch / merge request for further customization.

[OgelGames]

It's been two months since I approved this, I think we should just merge this now.

[OgelGames]

I don't know why github deleted the branch automatically, but I'm gonna leave it to exist for a little bit... :)