Make detector tube use globalstep instead of core.after

[Watilin]

Detector tubes sometimes fail to turn off when the server is restarted (and possibly other circumstances). This PR changes them to use a node timerglobalstep instead of core.after, to make them more reliable.

[SwissalpS]

See #65

[S-S-X]

Following comment is based on just a quick glance, didn't test or read change set that carefully.

Considering actual game play, how this would affect behavior when node timer is paused? (when block deactivates)
I think this would be main difference between core.after and node timers.

Of course there would be other smaller details like corner case node timer / metadata bugs, some of these would be irrelevant here while some would change behavior a bit when(if) supporting things break. Personally for me it would however be fine to ignore anything where cause would be another bug.

[Watilin]

how this would affect behavior when node timer is paused? (when block deactivates)

Given that tubed_item entities load the area they're in (item_transport.lua#L376), i can't think of a situation where the detector would be traversed by an item in an unloaded block. I think that persistence is the only difference between a node timer and core.after in this context.

Of course there would be other smaller details like corner case node timer / metadata bugs

I took inspiration from default's furnace, using swap_node to preserve metadata and not trigger constructors/destructors needlessly. There aren't any metadata anymore anyway, since it only consisted in an item counter that i removed. I agree that if node timers have bugs in themselves, it shoud be out of scope of this PR.

[SwissalpS]

+1 for clean code layout
+1 for using swap_node
Have you considered clearing the item count node-meta field of older tubes? Or do you think this would be more overhead and not worth the trouble of saving some bytes?
Of course this kind of detail only needs looking at if proposed node-timer technic doesn't introduce more problems than currently exist.

[S-S-X]

how this would affect behavior when node timer is paused? (when block deactivates)

Given that tubed_item entities load the area they're in (item_transport.lua#L376), i can't think of a situation where the detector would be traversed by an item in an unloaded block. I think that persistence is the only difference between a node timer and core.after in this context.

Unloaded block is different from loaded but deactivated block. Basically there's 3 states for mapblocks:

• unloaded block.
• inactive loaded block.
• active loaded block.

Nodetimers work when block is both loaded and activated but not when it is just loaded but not yet activated, nodetimer will get paused when block gets deactivated and continues immediately when block gets activated again.

How blocks get activated and deactivated depends on player position, active_block_range and active_object_send_range_blocks. Former is simpler, just a static block range while latter depends on player orientation for activation/deactivation decision.
These are different from block loading and unloading mechanism.

Of course there would be other smaller details like corner case node timer / metadata bugs

I took inspiration from default's furnace, using swap_node to preserve metadata and not trigger constructors/destructors needlessly. There aren't any metadata anymore anyway, since it only consisted in an item counter that i removed. I agree that if node timers have bugs in themselves, it shoud be out of scope of this PR.

swap_node for sure is better and it also preserves state of nodetimers that live in node metadata.

[Watilin]

Unloaded block is different from loaded but deactivated block.

Thanks for the explanation. That's a limitation of node timers i was unaware of.

I've been able to test a situation where the detector tube is in a loaded but inactive block. One item traverses the tube and triggers its can_go function, but the node timer doesn't expire and the tube stays in its on state. Consistency is lost.

I'm gonna go back to core.after and look into mesecons code to figure out how they achieve consistency across restarts.

[Watilin]

So,
mesecons use register_on_shutdown to save pending actions into a file. I could copypaste this into pipeworks, but i feel like some reuse is possible.

Arguing that the node detector is a mesecon component, it could add its own action type in the action queue via mesecon.queue:add_function. On the other hand, it feels a bit like fiddling with unofficial API and, if other mods start tinkering with it as well, it could become a huge mess.

Another solution would be to create a separate mod with the purpose of providing persistency, on which both mesecons and pipeworks would depend. I don't know if the mesecons team would be happy with that.

What do you think?

[Watilin]

Since nobody replied to my previous comment, i went for the fastest route and copy-pasted mesecons code, with minor modification.

[SwissalpS]

I'd have used modstorage instead of file. Saves quite some LOC and saving on time is handled by core.

[Watilin]

I'd have used modstorage instead of file. Saves quite some LOC and saving on time is handled by core.

Ok so there is a preferred way to do this. I wonder why mesecons chose another way.

[SwissalpS]

I wonder why mesecons chose another way.

Most probably because [mesecons] predates modstorage ;)

[Watilin]

This is all done, tested and ready for review. Be aware though that this PR depends on #179 being merged beforehand.