Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make all formspecs use minetest.show_formspec rather than legacy formspec meta mess #17

Open
wsor4035 opened this issue Mar 6, 2022 · 16 comments
Labels
controversial Strongly divided opinions

Comments

@wsor4035
Copy link
Contributor

wsor4035 commented Mar 6, 2022

what the title says, for obvious reasons, like upgradability, etc

@OgelGames
Copy link
Contributor

Since when are metadata formspecs legacy? IMO they are better for simple formspecs.

@wsor4035
Copy link
Contributor Author

wsor4035 commented Mar 6, 2022

no, they really are not better, as it typically requires a lbm to update them as they are hardcoded into meta.

EDIT: there only saving grace is that they are sent to the client, so you get instant loading when the server is lagging, however if you have meta inventory rules, buttons, etc, that all has to go to the server so you lose that advantage anyways

@wsor4035
Copy link
Contributor Author

wsor4035 commented Mar 6, 2022

for reference mt-mods/travelnet#39

@S-S-X
Copy link
Member

S-S-X commented Mar 6, 2022

There's reasons to drop metadata semi-static formspecs but if there's no real reason then dropping metadata formspecs is downgrade instead of upgrade.
Fully dynamic formspecs are a lot worse for performance while metadata formspecs are cached client side immediately during mapblock transfer.

@OgelGames
Copy link
Contributor

for reference mt-mods/travelnet#39

I completely agree with the use there, travelnet formspecs are big and, more significantly, are different depending on who uses them. Most of the pipeworks formspecs are small and rarely need updating.

@sirrobzeroone
Copy link

sirrobzeroone commented Aug 10, 2022

Oddly I'm here because I'm working on wine mod which has a pipe interface but stumbled on this issue. I've handled this in the wine mod updates I'm doing, I wouldn't really class formspec stored in the meta of a node as hard coded. This can be checked and updated on every right-click if you need to. Sure it's a little more work but you can set a version flag if you need to so you don't update the meta every time. It does mean you need code in the on rightclick event, most node formspecs are normally stored somewhere as a template in the mod code. Not sure if pipeworks uses a timer but if it does that's already calling every X seconds and can also be used as a hook for formspec refreshes - the furnace from default for example updates its node formspec every second when it's active :)

Here is some quasi-code as an example, sorry I'm adapting this on the fly so it may not be 100% correct:

inside nodes on_rightclick - note when a meta value is not set result returned is 0

		local meta = minetest.get_meta(pos);
		if meta:get_int("version") <= 1 then
			mod_name.valid_formspec(meta) 		
		end

Inside mod_name.valid_formspec(meta)

		-- Could be as simple as
		meta:set_string("formspec", mod_name.formspec(pos))

		-- or you can get complex and go
		local new_meta = meta:set_int("new_meta", 1)

		local node_inv = meta:get_inventory()
		node_inv:set_size("new_inv", 1)

		if not node_inv:is_empty("old_inv") then
			local src_old_stack = node_inv:get_stack("old_inv", 1)
			node_inv:set_stack("new_inv", 1, src_old_stack)
		end

		-- If you go complex you then set your formspec here

		-- Update our version
		meta:set_int("version",2) 

Anyways back to looking for the info I'm after :)

@wsor4035
Copy link
Contributor Author

see https://content.minetest.net/threads/3425/ actually need to switch over. currently as the mod is set up, you either have to install i3 before loading pipeworks, or not switch back to sfinv once on i3 unless you want to update the meta for all the nodes. relating to the thread, now that the inventory settings are per player, need to dynamically make the formspec based on the players settings. in addition, this will resolve the above issue where a player switches back and forth between different inventory mods

@fluxionary
Copy link
Member

static (i.e. metadata) formspecs are preferable in some contexts - they load much faster for the client. until we've got functional SSCSM, i think they have important use-case? they can't be used everywhere, but this isn't a situation where i think reducing complexity is the only meaningful consideration.

unless it's being done thousands of times per second, there's no significant cost to updating a node's metadata.

@fluxionary
Copy link
Member

see https://content.minetest.net/threads/3425/ actually need to switch over. currently as the mod is set up, you either have to install i3 before loading pipeworks, or not switch back to sfinv once on i3 unless you want to update the meta for all the nodes. relating to the thread, now that the inventory settings are per player, need to dynamically make the formspec based on the players settings. in addition, this will resolve the above issue where a player switches back and forth between different inventory mods

the fact that i3 thinks it owns the universe is i3's problem. we don't need to accommodate bad design decisions of other mods. modifying the size of storage nodes should not be in the bailiwick of an inventory manager.

@kilbith
Copy link

kilbith commented Sep 26, 2022

modifying the size of storage nodes should not be in the bailiwick of an inventory manager

Exactly and that's why I ask you to modify your own storage nodes. It should not be overriden remotely in a inventory mod.

we don't need to accommodate bad design decisions of other mods

Arguments?

the fact that i3 thinks it owns the universe is i3's problem.

Which is a personal attack. Also I remind you that i3 is the top mod.

@fluxionary
Copy link
Member

Which is a personal attack. Also I remind you that i3 is the top mod.

apologies for the personal attack. it may be better liked amongst users, but as a server admin and mod developer, it seems to mostly replace the myriad issues of unified inventory w/ equally troublesome issues of its own. i have to admit i've never used it extensively, though i've learned a bit from integrating it w/ stairsplus minetest-mods/moreblocks#191

@kilbith
Copy link

kilbith commented Sep 27, 2022

w/ equally troublesome issues of its own

I'm listening about what are these issues for you.

i have to admit i've never used it extensively, though i've learned a bit from integrating it w/ stairsplus minetest-mods/moreblocks#191

Talking about that, it still doesn't uses the i3's API properly. But that's OT.

@fluxionary
Copy link
Member

fluxionary commented Sep 27, 2022

Exactly and that's why I ask you to modify your own storage nodes. It should not be overriden remotely in a inventory mod.

oh, i see, because it's the size of the player's inventory that is different, not the storage node's. that is certainly a problem that needs some sort of solution.

@S-S-X
Copy link
Member

S-S-X commented Sep 27, 2022

I do think that i3 having different inventory sizes is not by itself valid reason for switching to active server side formspecs because it does very much sound like i3 has to implement generic solution for that anyway.

If there's no such issue on i3 bug tracker yet then maybe open one and possibly link to https://gitlab.com/luk3yx/minetest-fs51 as a one possible solution and also link here to this discussion.

Pipeworks itself does not currently need dynamic formspecs for anything and i3 needs generic solution, some of pipeworks formspecs would not really even need updating at all and could use direct metadata fields instead.

@S-S-X S-S-X added the controversial Strongly divided opinions label Sep 27, 2022
@fluxionary
Copy link
Member

oh, i see, because it's the size of the player's inventory that is a different size, not the storage node. that is certainly a problem that needs some sort of solution.

after thinking about this a bit, i feel like having non-standard inventory sizes is just not something minetest is set up to parameterize in its current state of development. it makes sense perhaps at the "game" level (e.g. mineclone), but most mods are built to default_game and are hard-coded to 32 slots. changing that number, and then expecting the entire rest of the universe to move to be compatible w/ your new system, is arrogant; particularly when there's no standard API to govern such a feature. additionally, parameterizing formspecs is currently terrible if you're building them out of strings, as most mods do. mods like formspec_flow are starting to get created, so perhaps someday we can have a mod that makes it so that individual mods don't have to care about what the size of the player inventory is when they're creating formspecs. but i think that still needs to be written before asking that other mods support it, instead of expecting them all to implement something piecemeal.

@fluxionary
Copy link
Member

fluxionary commented Sep 28, 2022

what the title says, for obvious reasons, like upgradability

i do admit that the usual way of dealing w/ this - writing LBMs to update formspecs - doesn't feel like the ideal solution. but as it stands, they're far from being un-upgradable. what are the other obvious reasons?

their only saving grace is that they are sent to the client, so you get instant loading when the server is lagging, however if you have meta inventory rules, buttons, etc, that all has to go to the server so you lose that advantage anyways.

that doesn't negate the advantage of immediately confirming that you've correctly interacted w/ them. nodes that have to wait for a packet round-trip to show a formspec cause some really obnoxious user experience, particularly if the server is lagging a lot (e.g. right-click, then walk away 20 nodes, then formspec shows up; right-click, then right-click on another formspec node. which formspec will be shown? you get packets for both, it might be in any order of "one then the other").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controversial Strongly divided opinions
Projects
None yet
Development

No branches or pull requests

6 participants