Implemented new method of defining valid faces through the secondary node registration

[oversword]

Fixes #4 again, but better this time

• Moved dir_to_side and side_to_dir into tubelib base module
• Allow definition of valid sides through the add_secondary_node_names function
• Fill in defaults for each side, allowing simpler definition of invalid faces
• Update the logic in get_secondary_node to use the new system
• De-coupled valid side determination from get_secondary_node
• Add new is_valid_dir and is_valid_side functions for easy & fast valid side checking

This should be tested thoroughly, but should probably not be merged until we're sure it provides a good method for disallowing connections

This should be tested along side joe7575/techpack#74

[oversword]

Might also want to change is_secondary_node so it always returns boolean, don't think it's breaking things but it will return the valid sides list after this change. Knee deep in the other issue ATM, just leaving this here to remind myself

[oversword]

@joe7575 I'm pretty confident in this one now, most of the code I originally added has actually been removed, this is looking a lot more like a new feature should.

[joe7575]

👍 I will look into this later today, I am still busy...

[joe7575]

I think I found the bug #6. Line https://github.com/joe7575/tubelib2/blob/master/tube_api.lua#L372 is wrong. It has to be:

self:after_dig_node(tele_pos, {peer_meta:get_int("tube_dir")})

This will remove all cached data and terminate the connection between both ends.
I would like to switch back to v1.9, add the bugfix, and then discuss again whether we need any more changes.

[oversword]

Okay let me pull master and test it out, I still think "valid_faces" would be a nice new feature, but yeah let's talk about it

[oversword]

@joe7575 I'm not seeing spooky connections fixed at all on master... What's your testing scenario?

[oversword]

@joe7575 Actually in combination with the valid faces fix that is doing the trick, this is updating the cache as expected, can still do nasty things without valid faces though

[oversword]

Not sure where this change came from, leaving a note so when I undo it (to keep this PR clean) it doesn't vanish into the aether

[oversword]

A similar function was modified in tube_api in the same way

[oversword]

@joe7575 after a little testing I'm pretty sure all of these changes, and the ones in this PR joe7575/techpack#74 are still required. I can still cause the spooky connection bug without these changes.
I have tested them and they seem to be working, tried it on a chest so I could test push & pull and it's working well
One last thing I want to fix: placing a node with invalid faces next to a tube will cause the tube to connect to an invalid face.
Once this is done I think this feature is complete

[oversword]

As I was testing things previously I came up with a few more scenarios that produce the bug, basically you need to try every combination you can think of tubes, wrong sides and destination or not:

[ source > teleporter facing sideways (air gap) teleporter > chest ]
Removing the source teleporter will not update the sides, connection remains

[ source > teleporter (air gap) teleporter > air ]
Removing the destination teleporter does not update the cache

(need to test these in your testing scenario though)

[joe7575]

Please keep in mind:
tubelib2 is not only used for TechPack. Techage and Hyperloop also rely on tubelib2. Especially techage is using 6 instances of tubelib2 for different kinds of tubes, pipes, and cables. The cables in addition can be hidden in walls. Therefore, I had to overload several tubelib2 functions like get_secondary_node to be able to implement 'cables' as metadata in any kind of node. Any change in the core or behaviour of tubelib2 and techage will blow up...
But I have to teleport nodes, I know why :)

[oversword]

I'm pretty sure the changes here will not change behavior if valid_sides is never set

[joe7575]

When I use internal2.lua and tube_api.lua from this PR, the server immediately crashes:

2021-01-23 13:49:34: ERROR[Main]: ServerError: AsyncErr: ServerThread::run Lua: Runtime error from mod 'tubelib2' in callback node_on_timer(): /home/joachim/minetest5/bin/../mods/tubelib2/tube_api.lua:235: attempt to index local 'valid_sides' (a boolean value)
2021-01-23 13:49:34: ERROR[Main]: stack traceback:
2021-01-23 13:49:34: ERROR[Main]: 	/home/joachim/minetest5/bin/../mods/tubelib2/tube_api.lua:235: in function 'is_valid_dir'
2021-01-23 13:49:34: ERROR[Main]: 	/home/joachim/minetest5/bin/../mods/tubelib2/internal2.lua:391: in function 'get_next_tube'
2021-01-23 13:49:34: ERROR[Main]: 	/home/joachim/minetest5/bin/../mods/tubelib2/internal2.lua:420: in function 'walk_tube_line'
2021-01-23 13:49:34: ERROR[Main]: 	/home/joachim/minetest5/bin/../mods/tubelib2/tube_api.lua:332: in function 'get_connected_node_pos'
2021-01-23 13:49:34: ERROR[Main]: 	/home/joachim/minetest5/bin/../mods/tubelib2/tube_test.lua:199: in function </home/joachim/minetest5/bin/../mods/tubelib2/tube_test.lua:197>

[joe7575]

As I was testing things previously I came up with a few more scenarios that produce the bug, basically you need to try every combination you can think of tubes, wrong sides and destination or not:

[ source > teleporter facing sideways (air gap) teleporter > chest ]
Removing the source teleporter will not update the sides, connection remains

[ source > teleporter (air gap) teleporter > air ]
Removing the destination teleporter does not update the cache

(need to test these in your testing scenario though)

Right, it does not work as expected. I will try something else...

[oversword]

When I use internal2.lua and tube_api.lua from this PR, the server immediately crashes:

2021-01-23 13:49:34: ERROR[Main]: ServerError: AsyncErr: ServerThread::run Lua: Runtime error from mod 'tubelib2' in callback node_on_timer(): /home/joachim/minetest5/bin/../mods/tubelib2/tube_api.lua:235: attempt to index local 'valid_sides' (a boolean value)

This should not be possible unless secondary_node_names['some_node'] is set to true, which was the old way of doing things, but should not be possible in the new way due to the changes in add_secondary_node_names... can you check the function on your machine matches the function in this PR? There's a chance the merge went wrong due to the reverting of master...

Actually I'm looking at the tube_test.lua file now, you seem to be setting secondary_node_names in there, and in a way I'm surprised ever worked... I will enable tube_test.lua and make sure that's working with the new changes

[joe7575]

Actually I'm looking at the tube_test.lua file now, you seem to be setting secondary_node_names in there, and in a way I'm surprised ever worked... I will enable tube_test.lua and make sure that's working with the new changes

Perhaps it was no good idea to store the valid_sides info in self.secondary_node_names.
A second table like self.valid_node_contact_sides or something like that would be better and self.secondary_node_names can keep untouched.

[oversword]

Ahh wish I'd seen that before I made the change! I don;t think that's a bad idea, take a look at how I've fixed it and let me know if you still want me to separate them.

What I've done is make sure we pass that table through add_secondary_node_names and also made that function a bit more complex so that you can pass in "names" like { "tubelib:node_name1", ["tubelib:node_name2"]={U=falase,L=false} }

Where "tubelib:node_name1" will get all set to the default values, and "tubelib:node_name2" will get them set to custom values

[joe7575]

I would prefer if we could separate the new features as much as possible from the existing code. A third new function like:

Tube:set_valid_sides({ ["tubelib:node_name2"]={U=true, L=true} })

would be the best.
I contrast to your function, I would provide the valid sides, not the invalid ones. But the other way is also ok.

[oversword]

Okay, I'm working on that now. Would you prefer Tube:set_valid_sides ? I was going to go with Tube:set_valid_node_contact_sides based on your last comment.

I contrast to your function, I would provide the valid sides, not the invalid ones. But the other way is also ok.

I really feel like true should be the default. Though it makes the definitions shorter for the ones we care about here (teleporter, pusher), I think there are as many cases where defining the disallowed faces is shorter (see joe7575/techpack#76), and I suspect the developers of any new nodes would prefer to turn certain sides off, rather than turning certain sides on.

I could always do both? Perhaps a Tube:set_valid_node_contact_sides and Tube:set_invalid_node_contact_sides where the invalid function is the inverse

[joe7575]

I implemented a teleporter node completely inside tube_test.lua. It is a real secondary node (like the TechPack detector) and does not use tubelib2 pairing functionality:

It has a callback function:

	on_push_item = function(pos, dir, item)
		local tube_dir = M(pos):get_int("tube_dir")
		if dir == tubelib2.Turn180Deg[tube_dir] then
			local s = M(pos):get_string("peer_pos")
			if s and s ~= "" then
				push_item(S2P(s))
				return true
			end
		end
	end,

Objectives:

• Clear separation between tubing and pairing ("source -> teleporter air teleporter -> chest" are two separate tubes)
• The node checks the side of incoming items which would perfectly fit to the new functions
• The node checks whether the pairing is still valid and otherwise discards the items

[joe7575]

Okay, I'm working on that now. Would you prefer Tube:set_valid_sides ? I was going to go with Tube:set_valid_node_contact_sides based on your last comment.

I contrast to your function, I would provide the valid sides, not the invalid ones. But the other way is also ok.

I really feel like true should be the default. Though it makes the definitions shorter for the ones we care about here (teleporter, pusher), I think there are as many cases where defining the disallowed faces is shorter (see joe7575/techpack#76), and I suspect the developers of any new nodes would prefer to turn certain sides off, rather than turning certain sides on.

I could always do both? Perhaps a Tube:set_valid_node_contact_sides and Tube:set_invalid_node_contact_sides where the invalid function is the inverse

Its up to you, both is ok. Two function for valid/invalid would be perfect :)

[oversword]

Shorter is better, I will go with set_valid_sides

I will write both a set_valid_sides and set_invalid_sides function, I picture that set_invalid_sides({ ['node_name']={U=true,L=true} }) would actually turn U and L off but keep the rest on, is that what you had in mind too? Or would it make more sense if they had to be false?

I implemented a teleporter node completely inside tube_test.lua. It is a real secondary node (like the TechPack detector) and does not use tubelib2 pairing functionality:

If you think that's a better way to fix teleporters that's great, but I think this valid_sides feature has grown a little beyond that.

We could start to implement that in this PR if you think it's needed joe7575/techpack#74

The only piece of code that new teleporter method would invalidate is the tube_walk changes - now in get_teleporter - even then I think we would have to put in some condition to stop people making another secondary node that's like a tube, or we would need that code again.

[joe7575]

Shorter is better, I will go with set_valid_sides

I will write both a set_valid_sides and set_invalid_sides function, I picture that set_invalid_sides({ ['node_name']={U=true,L=true} }) would actually turn U and L off but keep the rest on, is that what you had in mind too? Or would it make more sense if they had to be false?

In the case of the teleporter, which has one valid side (lets say left), it see the following possibilities:

set_valid_sides({ ['node_name'] = {L=true} })
set_valid_sides({ ['node_name'] = {"L"} })
set_valid_sides('node_name', {"L"})

set_invalid_sides({ ['node_name'] = {B=true, R=true, F=true, D=true, U=true} })
set_invalid_sides({ ['node_name'] = {"B", "R", "F", "D", "U"})
set_invalid_sides('node_name', {"B", "R", "F", "D", "U"})

I would prefer: set_valid_sides('node_name', {"L"})

I implemented a teleporter node completely inside tube_test.lua. It is a real secondary node (like the TechPack detector) and does not use tubelib2 pairing functionality:

If you think that's a better way to fix teleporters that's great, but I think this valid_sides feature has grown a little beyond that.

The valid_sides feature is great for all types of secondary nodes, also for the teleporters. Removing the teleporter code from tubelib2 would simplify and cleanup the code, unfortunately this can't be done. But it can be ignored for future implementations.

We could start to implement that in this PR if you think it's needed joe7575/techpack#74

The only piece of code that new teleporter method would invalidate is the tube_walk changes - now in get_teleporter - even then I think we would have to put in some condition to stop people making another secondary node that's like a tube, or we would need that code again.

This can't be done without changing tubelib2. And even if somebody tries this. If you implement a node wrong, you can do anything...

Edit: Of course, it always has to be set_valid_sides, independent if we allow one or more nodes as paramater

[oversword]

I would prefer: set_valid_side('node_name', {"L"})

Ahh of course, consider it done. I already made it set_valid_sides('node_name', sides), with an additional set_valid_sides_multiple(table) that looks like the original way we were thinking about this.

This can't be done without changing tubelib2. And even if somebody tries this. If you implement a node wrong, you can do anything...

I'm unsure what the conclusion is here, do I need to make any further changes?

[joe7575]

I'm unsure what the conclusion is here, do I need to make any further changes?

I don't see the need, but I'm kind of overwhelmed with all of these changes right now

[joe7575]

One remark, but this is more a personal style:
You wrote:

if self:is_valid_dir_pos(pos, dir) == false then return end

I prefer:

if not self:is_valid_dir_pos(pos, dir) then return end

The second has the benefit, that is is always true, if it is not false, nil or not available at all.
That means, you don't have to initialize default tables with false values.

[oversword]

The second has the benefit, that is is always true, if it is not false, nil or not available at all.

Actually, this is intentional, the return values are:
true = valid side
false = invalid side
nil = undefined, no node, incorrect dir, or no side known

In all cases I've used it in, nil basically means we assume the side is valid, but I can imagine a scenario where you're asking "do we know if this side is valid or not", or "has this side been registered as valid/invalid" instead of "is this side valid or not". I prefer to keep nil around as the secret third boolean value, and also my checks against the exact values it will return.

[joe7575]

The second has the benefit, that is is always true, if it is not false, nil or not available at all.

Actually, this is intentional, the return values are:
true = valid side
false = invalid side
nil = undefined, no node, incorrect dir, or no side known

In all cases I've used it in, nil basically means we assume the side is valid, but I can imagine a scenario where you're asking "do we know if this side is valid or not", or "has this side been registered as valid/invalid" instead of "is this side valid or not". I prefer to keep nil around as the secret third boolean value, and also my checks against the exact values it will return.

Ah, I understand.

[oversword]

Alright I think this is the one, I've checked this over a few times and updated the related PRs too. I like how this is looking

Can't believe we made it past 40 comments xD

[joe7575]

Alright I think this is the one, I've checked this over a few times and updated the related PRs too. I like how this is looking

Can't believe we made it past 40 comments xD

Yep, I got (felt) 100 emails from you :)
But I believe, it is almost perfect, so lets call it a day.
I will check it out and test it against my other mods.