Minetest_game: Please make chest definition functions/variables global

[VanessaE]

Too many things about chests are local-ized within minetest_game, keeping them out of reach of...well..everything. This completely breaks Pipeworks' handling of chests, for example, requiring a large rewrite of its chest-handling code.

Adding node definition fields (such as the tube{} table to define connections and item-insertion behavior), is doable with override_item(), but Pipeworks also needs to be able to modify chests' formspecs, which is basically impossible since they're stored in a confusing way (partly in metadata, partly live). In addition, Pipeworks needs to add things to their on-receive-fields, on-place, after-dig, and on-construct callbacks, among others. Those are theoretically doable with override_item() but the result would be hard to read and even harder to make it work perfectly.

Currently (as shown by the above link), Pipeworks just overrides the entire chest definition, but since the advent of the new "open" chests, that doesn't work anymore.

Any pending point release really needs to include this so that I can fix Pipeworks properly.

I could theoretically make a pull request to fix this, but I simply do not know how much would need to be changed for it to be "proper" in the eyes of Minetest developers. My first instinct is to remove local from all functions and top-level variables, and add default. or minetest. in front of their names so that they don't pollute the global namespace, but that's probably wrong

[paramat]

@sofar or anyone? I would not know what needs to be global.
There is already good reason for a point release #1768 , this issue is another.
I would be fine with making some functions global.

[stujones11]

I do know that @sofar is a fan of keeping things local and there are many good reasons for doing this, not least Lua performance.

@VanessaE form what you describe, you were basically overriding the chests before, just not using the proper method, as you pointed out yourself, override_item() is the solution here, as I see it.

[VanessaE]

override_item() won't work here because I need to modify the formspecs also, and there's that bit of code that tries to track which chests a player currently has open.

[stujones11]

That should not be a problem so long as you override _all_ of the fields and supply your own versions.

[VanessaE]

...and that's the problem. The formspecs all use show_formspec(), and the content is generated from a function that's local within minetest_game.

Plus, see above about tracking open chests. That global on_receive_fields also gets in the way of toggling the "split stacks" switch that Pipeworks wants to add to the formspec.

[stujones11]

As I see it the on_player_receive_fields would become redundant as there would be no names is the open_chests table if you override the code that inserts them. You could then write your own receive handler if you want to retain support for keys, just override the new chests with the code of the old ones and job done.

[VanessaE]

My intent is to modify the chests as little as possible. It would be far simpler if I could just wedge some code into the existing functions and use override_item() to modify the various items.

Any way you slice it, it's messy and hacky and just plain wrong to have to copy and modify big chunks of code like this.

[stujones11]

For some things, I agree, if MTG is to be a base for mods then it should expose more of its internals through the default namespace or whatever. However, in this case you basically are re-writing the chest code anyway, you may as well make a proper job of it :^)

[VanessaE]

The problem is there are some things I can't rewrite, because they're local and there's really no way to properly override them.

For example, if another mod uses the register chest function, my code won't be able to add Pipeworks support to it.

[stujones11]

You can override default.register_chest as well :P Though I guess that won't work unless the mod depends on yours, so fair point.

[VanessaE]

Right, I could, but the thing is, I need to override get_chest_formspec, to add the split-stacks switch. I need to override chest_lid_obstructed, so that I can make tubes disconnect/reconnect from the top of the chest when it's opened or closed. I need to wedge into on_player_receive_fields, so I can toggle the state of the switch.

And then, there's the whole problem of using show_formspec, it conflicts with metadata-supplied formspecs. I don't remember the details here, but it's something I ran into before.

[stujones11]

By overriding the chests completely, none of that matters any more. The only legitimate argument is the registration function, however, if a mod does wish to add pipeworks support to a chest, then the likelihood is that it depends on pipeworks already.

I will leave it here as I do not understand your requirements enough to know what would need to be changed, if anything. I am pretty sure there are ways around this, you just don't like them.

[VanessaE]

I don't understand why it's such a problem to make the functions/variables non-local.

[stujones11]

If it is simply a matter of make a local function global then I am sure it won't be a problem, however, you do not say which functions/variables you require.

[VanessaE]

Actually, I just did :smile:

[stujones11]

Okay, I missed that, sorry. If that is the case then I see no reason why those two functions could not be included in the default namespace, I am not sure what can be done about the form handler though, you would likely still have to override parts of the nodedef.

[stujones11]

Suggestion, make a PR with only the two namespace additions, that is likely to be approved. The 'wedging' of the form handler is another issue.

[VanessaE]

That would certainly be a trivial PR but I'm waiting for opinions from other mt_game devs, in case someone has a better way.

[stujones11]

Nothing urgent then? :laughing:

[VanessaE]

...

[JurajVajda]

would nice to have default.register_chest in core scope i.e. core.register_chest or minetest.register_chest and all dependent functions with it, so mods can use the new opening chests also..

[SmallJoker]

@JurajVajda This is a Lua subgame. All minetest. functions are and must be defined in the minetest engine (minetest/minetest), otherwise this would mess up the entire purpose of "namespaces". The register function will stay this way.

[VanessaE]

Seems not to be wanted, and pipeworks has a workaround now.

[paramat]

1835 merged.