Fix minetest.get_node_boxes for connected node boxes

[grorp]

modlib.minetest.get_node_boxes sometimes returns wrong results for connected node boxes because …

1. … the connect_sides node definition field is a list, but get_node_boxes assumes it is a dictionary. If connect_sides is specified in the node definition, the function only returns the fixed part of the node box.

2. … the sides the node can connect to (local variable connect_sides) are stored in a dictionary. Dictionaries in Lua don't seem to be ordered, so the sides don't stay in the right order and box_id sometimes points to the wrong box if you have more than one side connected.

In this PR, the local connect_sides variable is made a list and the connect_sides node definition field is treated as a list, so you get correct results for connected node boxes. In a separate commit, I replaced pairs with ipairs where possible.

I hope my English can be understood :-)

[appgurueu]

I hope my English can be understood :-)

Yes, the issues are clear:

• connect_sides is {"x", "y", "z"}, not {x = true, y = true, z = true}
• ordering is required for box_id to be valid

I think I can keep the dictionary if I also keep a separate table with the keys for the order. The advantage is that I can save myself some linear searches. I'll have to flip the for loops around however (e.g. iterate over the values of the connects to field and only keep those entries). I'll look into this when I have time (most likely this weekend).

[grorp]

OK, that's nice. Another thing you could do:

diff --git a/minetest/boxes.lua b/minetest/boxes.lua
index fd4a059..ff30897 100644
--- a/minetest/boxes.lua
+++ b/minetest/boxes.lua
@@ -15,7 +15,11 @@ end
 local function get_node_boxes(pos, type)
        local node = minetest.get_node(pos)
        local node_def = minetest.registered_nodes[node.name]
-       if (not node_def) or node_def.walkable == false then
+       if (
+               not node_def or
+               (type == "selection_box" and not node_def.pointable) or
+               (type == "collision_box" and not node_def.walkable)
+       ) then
                return {}
        end
        local boxes = {{-0.5, -0.5, -0.5, 0.5, 0.5, 0.5}}

[appgurueu]

OK, that's nice. Another thing you could do:

diff --git a/minetest/boxes.lua b/minetest/boxes.lua
index fd4a059..ff30897 100644
--- a/minetest/boxes.lua
+++ b/minetest/boxes.lua
@@ -15,7 +15,11 @@ end
 local function get_node_boxes(pos, type)
        local node = minetest.get_node(pos)
        local node_def = minetest.registered_nodes[node.name]
-       if (not node_def) or node_def.walkable == false then
+       if (
+               not node_def or
+               (type == "selection_box" and not node_def.pointable) or
+               (type == "collision_box" and not node_def.walkable)
+       ) then
                return {}
        end
        local boxes = {{-0.5, -0.5, -0.5, 0.5, 0.5, 0.5}}

oof, looks like I missed that when generalizing from collision boxes to various boxes - will fix

edit: fixed in e6c5860

note: I'm explicitly not using not walkable because I want an absent (nil) field to be treated as true because that's the default

[appgurueu]

This raises another question: When modders specify connect_sides in a different order (e.g. right, back, left), which order will Minetest use for the box IDs?

Side note: When I initially wrote this, I didn't need proper box IDs; all I needed was a list of boxes in any order. Testing with fences didn't reveal the wrong assumption because fences connect to all sides.

[appgurueu]

Alright, after digging a bit this luckily seems to match the docs:

		BOXESPUSHBACK(nodebox.fixed);

		if (neighbors & 1) {
			BOXESPUSHBACK(c.connect_top);
		} else {
			BOXESPUSHBACK(c.disconnected_top);
		}

		if (neighbors & 2) {
			BOXESPUSHBACK(c.connect_bottom);
		} else {
			BOXESPUSHBACK(c.disconnected_bottom);
		}

		if (neighbors & 4) {
			BOXESPUSHBACK(c.connect_front);
		} else {
			BOXESPUSHBACK(c.disconnected_front);
		}

		if (neighbors & 8) {
			BOXESPUSHBACK(c.connect_left);
		} else {
			BOXESPUSHBACK(c.disconnected_left);
		}

		if (neighbors & 16) {
			BOXESPUSHBACK(c.connect_back);
		} else {
			BOXESPUSHBACK(c.disconnected_back);
		}

		if (neighbors & 32) {
			BOXESPUSHBACK(c.connect_right);
		} else {
			BOXESPUSHBACK(c.disconnected_right);
		}

		if (neighbors == 0) {
			BOXESPUSHBACK(c.disconnected);
		}

		if (neighbors < 4) {
			BOXESPUSHBACK(c.disconnected_sides);
		}

This is the relevant code. It pushes connected/disconnected alternatingly, and in top, bottom, front, left, back, right order (which happens to be the order they are documented in). Ugh.

[appgurueu]

Fixed in 26d9b13, 2eaaca5 and e6c5860 hopefully.

Thank you very much for your bug report & proposed fixes.

Now I'm curious: Which project lead you here? Where are you using modlib.minetest.boxes?

[grorp]

Thank you very much too, it seems to work now :-) I'm currently writing a graffiti mod for Minetest. It works by placing invisible canvas entities on the surfaces of node selection boxes. This allows you to spray basically any node. At first I started to write all the code for getting the selection boxes myself, but then I found modlib.minetest.get_node_selectionboxes.

[grorp]

Hello again,
Could you possibly create a new release of Modlib on ContentDB? The latest version available there seems not to contain modlib.minetest.get_node_selectionboxes yet.

[appgurueu]

Hello again, Could you possibly create a new release of Modlib on ContentDB? The latest version available there seems not to contain modlib.minetest.get_node_selectionboxes yet.

Argh, I really wanted to delay rolling-99, the last rolling release before a planned switch to semver, esp. as the "dev" version currently breaks adv_chat and cellestial because I haven't switched them to the new configuration system yet…

Anyways, release created: https://content.minetest.net/packages/LMD/modlib/releases/14965/

[grorp]

Thank you :-)