Make infortext when machines Has no network customizable.

[sfence]

I am working on a mode that allows defining machines which can be powered from more types of power sources. And I come into a problem with automated update of infotext for machines not connected to the technic network.

Default state on infotext:

Wanted state of infotext, after change.

So, I have updated abm function for group:technic_machine to fix it.

[S-S-X]

Would it be somehow possible to make existing callback handle this: nodedef.technic_on_disable instead of introducing new callback for exact same functionality but just after setting infotext?

There's no need to add new callback method that is basically duplicate of technic_on_disable:

One backwards compatible solution could be just moving infotext setter before nodedef.technic_on_disable and then implementing custom infotext using nodedef.technic_on_disable.

Machines requiring that cannot register disabled state node if implemented as fully backwards compatible solution but instead would need to handle node swapping in nodedef.technic_on_disable. Another most probably compatible (but possible corner cases) and in my opinion better way would be to just always call nodedef.technic_on_disable if it is defined and do that as last action inside loop to allow it to override everything done.

[sfence]

@S-S-X

I have tried this variant:

                if nodedef then
                    local meta = minetest.get_meta(pos)
                    meta:set_string("infotext", S("%s Has No Network"):format(nodedef.description))
                end
                if nodedef and nodedef.technic_disabled_machine_name then
                    node.name = nodedef.technic_disabled_machine_name
                    minetest.swap_node(pos, node)
                elseif nodedef and nodedef.technic_on_disable then
                    nodedef.technic_on_disable(pos, node)
                end

But it cases blinking of info text. So for something which works smoothly and has full backward compatibility, I need to add something which disables infotext set.

So it is possible to do something like:

                local update_infotext = true
                if nodedef and nodedef.technic_disabled_machine_name then
                    node.name = nodedef.technic_disabled_machine_name
                    minetest.swap_node(pos, node)
                elseif nodedef and nodedef.technic_on_disable then
                    nodedef.technic_on_disable(pos, node)
                    update_infotext = false
                end
                if nodedef and update_infotext then
                    local meta = minetest.get_meta(pos)
                    meta:set_string("infotext", S("%s Has No Network"):format(nodedef.description))
                end

But, it causes, that all machines which now use technic_on_disable, will have to set their infotext in this callback.

The potential problem with the swap node in technic_on_disable can be solved by removing the parameter node from callback technic_no_network.

If you think that using techic_on_disable is better, I can do this change bigger and update the technic_on_disable callback too.

[SmallJoker]

Why? The current infotext provides much more and helpful information.

[S-S-X]

But it cases blinking of info text. So for something which works smoothly and has full backward compatibility, I need to add something which disables infotext set.

Does it cause blinking because engine sends metadata immediately or does it cause blinking because you had nodedef.technic_disabled_machine_name set for your active machine definition?

The potential problem with the swap node in technic_on_disable can be solved by removing the parameter node from callback technic_no_network.

If you think that using techic_on_disable is better, I can do this change bigger and update the technic_on_disable callback too.

I do like using techic_on_disable better because adding new callback method here is more just hack to work around problem caused by ABM enforcing infotext for machine when it gets disabled, original behavior is bad and now trying to think if there's some way to make it work without adding more callbacks without real purpose other than getting around old behavior.

Did you happen to try this one, if blinking happens because you had technic_disabled_machine_name set and not because engine sending meta immediately then this should fix blinking:

                if nodedef then
                    local meta = minetest.get_meta(pos)
                    meta:set_string("infotext", S("%s Has No Network"):format(nodedef.description))
                end
                if nodedef and nodedef.technic_disabled_machine_name then
                    node.name = nodedef.technic_disabled_machine_name
                    minetest.swap_node(pos, node)
                end
                if nodedef and nodedef.technic_on_disable then
                    nodedef.technic_on_disable(pos, node)
                end

Have to say that I would really like technic_on_disable called always if it is defined no matter what other properties are defined, current behavior is to call technic_on_disable only if it is defined and technic_disabled_machine_name is not defined.

I think there should not be machines that have both technic_disabled_machine_name and technic_on_disable because having both defined is useless for active machines. Is it this way to only allow calling that for disabled machines when active machine has technic_disabled_machine_name and both use common definition? I really hope it is not that but it can be...

[sfence]

@S-S-X I am not using nodedef.technic_disabled_machine_name, so it si probably caused by minetest engine. Ok, I will try to change it and use technic_on_disable, and set infotext in already defined on_disable callbacks.

[sfence]

@SmallJoker Because technic abm callback causes blinking of infotext, when a machine is powered by technic independent power source.

https://user-images.githubusercontent.com/17455197/132126116-e237d02f-e3f0-4bba-9eac-86a12e7d7cdc.mp4

[sfence]

@S-S-X I have accidentally used technic_no_disable instead of technic_on_disable. So I have moved infotext setting up and make technic_on_disable callback to be called always if is defined.

And it looks well.

[S-S-X]

I think current state of this PR is most correct approach. It does not only allow doing what originally asked but also makes technic_on_disable callback sane so I'd consider it generic improvement. Before this PR technic_on_disable required multiple rules to match, now it simply has to be defined to be called when machine gets disabled and that's how event callbacks should usually behave.

However I'm not one driving decisions here, just random internet person who hopes that above points will be considered when deciding how to proceed. Better to ask if @SmallJoker has time to review this one.

[SmallJoker]

I think the concept is okay. Would you please be so nice to document this new node field (Item Definition field) in api.md ? Otherwise it will stay a niche feature which is only used by the few people who have seen the commit or this PR.

[sfence]

I have updated documentation in api.md. So fields used in the changed ABM function are now documented.

[SmallJoker]

Did a few changes. Will merge this PR in a few days unless there are objections.