Support for nodemeta forms #12

bell07 · 2016-10-24T07:23:15Z

+some small enhancements and corrections

Please review all changes before merging, because I am a newbie ;)

+some small enhancements and corrections

rubenwardy

All looks mostly good.

In the docs you should mention that a node meta form is seen the same for all viewers, even after element changes (?) so to show different content, you'll need to open a different form from within the node form.

Also, feel free to clean up the code around your changes. The code style is pretty awful

rubenwardy · 2016-10-24T09:22:07Z

api.md

 * state.param - The parameters supplied by form:show.
 * state.def - The form definition.
 * state.is_inv - Boolean which is true if this form is being shown as an inventory.
+* state.nodepos - Node position if attached to node meta


It may be better to depreciate state.player (but keep present for backwards compatibility) and instead use state.location which is:

{ type = "singleplayer", name = "player name" }

or

{ type = "node", pos = { x = 0, y = 1, z = 2 }, players = { "hey", "yo" } }

EDIT: state.location, as that's more consistent with the MT api

Agree partily. In case of type "node" there is no player relation. Therefore

{ type = "node", pos = { x = 0, y = 1, z = 2 }, }

The viewers should be handled at other place (not static)
Because of less time the merging of "state.player" + "state.nodemeta" to "state.location" is low-prio for me.

Any idea for is_inv handling at this place?

{ type = "part_of_players_inventory", --?? or just "inventory" ? name = "player name" }

PS: I don't like the type = "singleplayer" because of misleadingly. type="player" is better.

both inventory and player are good

rubenwardy · 2016-10-24T09:24:57Z

smartfs.lua

@@ -99,13 +100,15 @@ function smartfs.add_to_inventory(form,icon,title)
 	end
 end

-function smartfs._makeState_(form,player,params,is_inv)
+function smartfs._makeState_(form,player,params,is_inv, nodepos)


please fix commas here

(form, player, params, is_inv, nodepos) or (form,player,params,is_inv,nodepos) ???

rubenwardy · 2016-10-24T09:26:12Z

smartfs.lua

@@ -251,7 +260,7 @@ end
 -- Show a formspec to a user
 function smartfs._show_(form, player, params, is_inv)
 	local state = smartfs._makeState_(form, player, params, is_inv)
-	state.show = state._show_
+	state.show = state._show_ --?? needed? is already defined in smartfs._fdef[]


no idea. Haven't read this code in like a year

rubenwardy · 2016-10-24T09:28:12Z

smartfs.lua

 -- Receive fields from formspec
 local function _sfs_recieve_(state,name,fields)
 	if (fields.quit == "true") then
-		if not state.is_inv then
+		if state.nodepos then
+			state:close() --mark as closed for this user (will be reopened if other user is on the node)


If multiple users are viewing with the same state, it will prematurely set closed to true

closed = true A opens. closed = false B opens closed = false A closes closed = true <-------------- B closes closed = true

doing reference counting would be better

viewers = 0 A opens. viewers = 1 B opens viewers = 2 A closes viewers = 1 B closes viewers = 0

It is a hack, in current implementation the "closed" is reset in smartfs.nodemeta_on_receive_fields after the call if other users are connected (line 368) state.closed = nil --not closed

You are right, the viewers count will be better. Or just reading of next(state.multiplayer)
Should I rename state.multiplayer to something other? My proposal to change the state.player from currently "playername" to players table and replace the multiplayer table

rubenwardy · 2016-10-24T09:30:54Z

smartfs.lua

+	if state.closed then -- current user leave the node
+		state.multiplayer[sender:get_player_name()] = nil
+		if next(state.multiplayer) == nil then -- multiplayer table empty
+			--update formspec on node to a initial one for the next usage (respecting param persistence


rubenwardy · 2016-10-24T09:31:36Z

smartfs.lua

+		state.multiplayer[sender:get_player_name()] = nil
+		if next(state.multiplayer) == nil then -- multiplayer table empty
+			--update formspec on node to a initial one for the next usage (respecting param persistence
+			state._ele = {} --reset the form


maybe add state:clear()? Would be nice as a public API, and there may be more things to clear in the future

I planned to implement clearing combined with form._reg(state) regeneration as refresh() method in next changes block (if I port the townchest to smartfs). The refresh() can be useful for semi-generic forms, if the smartfs.create( ,function()) build up different form specification each time depends on parameter content. In preparation the "state.param" is already persistant for nodes (saved/restored in node metadata).

rubenwardy · 2016-10-24T09:31:57Z

smartfs.lua

+			end
+			smartfs.opened[opened_id] = nil -- remove the old state
+		else
+			state.closed = nil --not closed


Shouldn't this be false?

The state.closed = nil is correct to revert the state:close() because before close there was not value (=nil). But this place will be reworked if I remove the hack above

bell07 · 2016-10-24T09:51:49Z

In the docs you should mention that a node meta form is seen the same for all viewers, even after element changes (?) so to show different content, you'll need to open a different form from within the node form.

A nodemeta formspec is the same for all viewers by Minetest design. The formspec is just written to the metadata without relation to user opens them. If 2 users have open the form at the same time and one of them update the form, the second user see instantly the change (maybe I write a chat-node as demo oO)

The code style is pretty awful

I did not understand, bad style or good style?

rubenwardy · 2016-10-24T10:09:05Z

You don't appear to modify show(), so any changes after the original create will be sent to users using minetest.send_formspec and not saved in node data

A nodemeta formspec is the same for all viewers by Minetest design. .....

Yeah, I understand that - I'd just like a note like "Changes are saved in node meta data and visible to all players, to show only a single player info please show them another form using form:show"

As for engine implementation details - If the formspec meta data changes whilst they are viewing a form, do they see the change? Or would you need to minetest.send_formspec to force them to see it?

I did not understand, bad style or good style?

Awful means really bad. This is quite old code so it's inconsistent and ugly in places

bell07 · 2016-10-24T10:23:03Z

I will add a small part "Formspec on nodes" to the README.md so the info about visibility will be at this place

The show() is not used for nodes. The next lines updates the form in nodemeta after data update and force the update on screens of other viewers

    if not state.closed then
        if state.nodepos then
            state:_attach_nodemeta_()
        else
            state:_show_()
        end
    else

If the last user leave the node, the smartfs.opened[opened_id] needs to be destroyed and the state is lost. Therefore there is a update the form with "initial" form because of no trigger if an user opens the form again.

            state._ele = {} --reset the form
            if form._reg(state) then --regen the form
                state:_attach_nodemeta_() --write form to node
            end

So the short answer to your question

f the formspec meta data changes whilst they are viewing a form, do they see the change?

yes, the other viewer see the change instantly, I tried it

bell07 · 2016-10-24T20:19:52Z

Awful means really bad. This is quite old code so it's inconsistent and ugly in places

You did not meant my changes as I understand first oO.
You are right, but it is better as if I would write them from scratch because of some development iterations in the past. I need to use it, not to develop it first oO) I compared the available forms implementation and smartfs is the most complete framework I found (ok, you called it me, I was irritated by name because "fs" is "file-system" for me so I ignored the mod at the first).

…erent mods

rubenwardy

Good progress. I'd prefer if you didn't bundle all these changes together. When merging, I'll split them out into separate commits. But next time you should only do one feature per PR - so one to add an image element, one to add new methods to each element, one to do a clean up and add nodemeta forms. You've made a lot of code style errors, but I'll fix those on merge as well.

If you could fix the other things that's not code style:

userdata vs. username
show() and AttachNodeMeta() should be the same function

I'm planning on doing some clean ups after this as well

rubenwardy · 2016-11-01T23:30:38Z

example.lua

+	on_receive_fields = smartfs.nodemeta_on_receive_fields
+})
+
+minetest.register_on_placenode(function(pos, newnode, placer, oldnode, itemstack, pointed_thing)


you can use the on_after_place (or whatever it's called) property of the node to do this

It is an example, I prefer the on_placenode for nodes placed by player and on_construct for generated nodes.

I wanted to change it but in documentation there is no "pos" parameter in the on_place function ??? (http://dev.minetest.net/minetest.register_node#on_place) I'l change it to on_construct

http://dev.minetest.net/minetest.register_node#after_place_node

rubenwardy · 2016-11-01T23:32:29Z

smartfs.lua

+local currentmod = minetest.get_current_modname() -- mod the file was loaded from
+local envroot = nil
+
+if not currentmod or currentmod == "" or --not minetest or something hacky


this should never return nil, so not currentmod is unnecessary

rubenwardy · 2016-11-01T23:34:29Z

smartfs.lua

+
+	-- obsolete api compatibility to previous versions
+	if type(player) == "string" then
+		player = minetest.get_player_by_name(player)


There's no need to store player user data here - a username is fine. The userdata is not used anywhere. It is also dangerous to store player objects in Lua memory across callbacks - you have to be very careful to delete them when the player leaves the game - it's just so much safer to just store usernames.

rubenwardy · 2016-11-01T23:35:49Z

smartfs.lua

+function smartfs._makeState_(form, newplayer, params, is_inv, nodepos)
+	-- Create object for monitoring of connected players. If no one connected the state can be free'd
+	local _make_players_ = function(form, newplayer)
+		local this = {}


the lua idiom is self

In lua documentation both is mentioned, an I liked "this" more :/ anyway to be uniform to the rest of the code I changed it already in the "views" commit.

rubenwardy · 2016-11-01T23:36:17Z

smartfs.lua

-function smartfs._makeState_(form,player,params,is_inv)
+function smartfs._makeState_(form, newplayer, params, is_inv, nodepos)
+	-- Create object for monitoring of connected players. If no one connected the state can be free'd
+	local _make_players_ = function(form, newplayer)


local function _make_players_(form, newplayer)

rubenwardy · 2016-11-01T23:58:28Z

smartfs.lua

+function smartfs.nodemeta_on_receive_fields(nodepos, formname, fields, sender)
+
+	-- get form info and check if it's a smartfs one
+	local meta = minetest.env:get_meta(nodepos)


minetest.env is deprecated. Just use minetest.get_meta

rubenwardy · 2016-11-01T23:59:03Z

smartfs.lua

+
+	-- get the currentsmartfs state
+	local opened_id = minetest.pos_to_string(nodepos)
+	local state = nil


= nil is redundant

rubenwardy · 2016-11-01T23:59:38Z

smartfs.lua

+	local state = nil
+	local form = smartfs:__call(nodeform)
+	if not smartfs.opened[opened_id] or      --if opened first time
+	       smartfs.opened[opened_id].def.name ~= nodeform then --or form is changed


Should be an extra indent here. Also, some spaces are used

A question about this indent. There should be a difference if the next line is the first command in the loop or if it is a part of the long condition. Or is there no visible difference in lua coding style?

if A and B and C and D and E then statements in if loop go here end

You mean 2x tab in case of long condition indent?

rubenwardy · 2016-11-02T00:01:12Z

smartfs.lua

+
+	--persist parameter for later usage
+	meta:set_string("smartfs_param", minetest.serialize(state.param))
+


The only comment needed is one above this if statement saying:

If there are no players using this formspec, reset it

rubenwardy · 2016-11-02T00:03:03Z

README.md

+
+## Formspec on nodes
+SmartFS is able to attach a formspec to a node metadata. The attached form can be opened by any player and all players see the same. If a player enter information and take updates on the form, all users see the updated form instantly. All changes are saved in node meta data and visible to all players, to show only a single player info please show them another form using form:show.
+There is a "reset" implemented if all players leave the node formspec, the initial state is restored. The parameters are implemented persistant so if you need to store sometging to node just use state:setparam() / state:getparam(). But do not save sensitive data since the nodemeta is sent to all player clients.


something

Also, you should wrap the text manually (the lines shouldn't be longer than 80 characters)

Other parts like "Inventory Support" are without manually wrapping too. Which viewer do you use? In github view there will be a narrow text strip in case of <80 characters. My default geany view display up to 200 characters in a line.

…lacenode in sample

… x = function :/

…nt on close from client (like trough button_exit[])

bell07 · 2016-11-02T20:04:25Z

I did the most requested changes. Please let me know about my "most misformatted things" so I can prepare the next pull request. I will try to split the next big change to multiple pull requests. But because of dependencies between enhancements this will consume time till all functionality is upstream.

… rest of smartfs concept

bell07 · 2016-11-04T21:15:39Z

I removed the nodemeta persistance stuff because I see it is not always usable. It was a quick-dirty hack without a concept. If you see some other things that should be removed from this "Enhancement-pack-pull-request" let me know.

Support for nodemeta forms

b0df04b

+some small enhancements and corrections

rubenwardy reviewed Oct 24, 2016

View reviewed changes

bell07 added 5 commits October 24, 2016 22:21

Additional update for nodemeta formspec support

b084e45

added namespace support to allow multiple versions to be used by diff…

c798eea

…erent mods

api backwards compatibility

c2a3e04

fixed bug with backwards compatibility

e684afa

the last fix for today :/

b840c80

rubenwardy reviewed Nov 2, 2016

View reviewed changes

bell07 added 10 commits November 2, 2016 19:19

Store player names instead of player objects internally

9ba07fa

merged state:_attach_nodemeta_ to state:_show_

b7eeccf

use node on_construct (or on_place) instead of minetest.register_on_p…

c613b42

…lacenode in sample

multi-instance framework optimization

9f5d96b

style things. self instead of this, local function x instead of local…

84c535c

… x = function :/

small runtime optimization, remove not needed check

e471d15

remove redundant quit handling. Reflect the fields.quit is allways se…

9cd0d57

…nt on close from client (like trough button_exit[])

minetest.env is deprecated. Just use minetest.get_meta

e52bfed

small style changes

7ccb3f6

nodemeta sample changed to after_place_node

18ac0b6

bell07 added 2 commits November 3, 2016 13:25

nodemeta sample updated in README

2221adf

removed persistance on nodemeta params since it does not match to the…

39d5423

… rest of smartfs concept

rubenwardy closed this Nov 13, 2016


		--persist parameter for later usage
		meta:set_string("smartfs_param", minetest.serialize(state.param))

Support for nodemeta forms #12

Support for nodemeta forms #12

Conversation

bell07 commented Oct 24, 2016

rubenwardy left a comment

Choose a reason for hiding this comment

rubenwardy Oct 24, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenwardy Oct 24, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bell07 commented Oct 24, 2016

rubenwardy commented Oct 24, 2016 • edited

bell07 commented Oct 24, 2016 • edited by rubenwardy

bell07 commented Oct 24, 2016

rubenwardy left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bell07 commented Nov 2, 2016

bell07 commented Nov 4, 2016

rubenwardy Oct 24, 2016 •

edited

rubenwardy Oct 24, 2016 •

edited

rubenwardy commented Oct 24, 2016 •

edited

bell07 commented Oct 24, 2016 •

edited by rubenwardy

rubenwardy left a comment •

edited