~gpcf/advtrains-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
14 3

[PATCH] Wagon formspec: advtrains.wagons[wagon.id] nil checks, and minor optimizations

Details
Message ID
<20240904144444.100370-1-yiufamily.hh@gmail.com>
DKIM signature
pass
Download raw message
Patch: +40 -31
From: 1F616EMO <root@1f616emo.xyz>

This patch adds nil checks before using advtrains.wagons[wagon.id] in wagon formspec codes. Additionally, I made some workflow optimizations.
---
 advtrains/wagons.lua | 71 +++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 31 deletions(-)

diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index 536c8d4..b7a1154 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1157,21 +1157,24 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local data = advtrains.wagons[wagon.id]
					if fields.inv then
						if wagon.has_inventory and wagon.get_inventory_formspec then
							minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
						end
					elseif fields.seat then
						local val=minetest.explode_textlist_event(fields.seat)
						if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
						--get on
							wagon:get_on(player, val.index)
							--will work with the new close_formspec functionality. close exactly this formspec.
							minetest.show_formspec(player:get_player_name(), formname, "")
					if data then
						if fields.inv then
							if wagon.has_inventory and wagon.get_inventory_formspec then
								minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
							end
						elseif fields.seat then
							local val=minetest.explode_textlist_event(fields.seat)
							if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
							--get on
								wagon:get_on(player, val.index)
								--will work with the new close_formspec functionality. close exactly this formspec.
								minetest.show_formspec(player:get_player_name(), formname, "")
							end
						end
					end
				end
			end
			return
		end
		uid=string.match(formname, "^advtrains_seating_(.+)$")
		if uid then
@@ -1186,33 +1189,37 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
					end
				end
			end
			return
		end
		uid=string.match(formname, "^advtrains_prop_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
				return true
			end
			if fields.save or not fields.quit then
				if fields.whitelist then
					data.whitelist = fields.whitelist
				end
				if fields.roadnumber then
					data.roadnumber = fields.roadnumber
				end
				if fields.fc then
					wagon.set_fc(data, fields.fc)
				end
				if fields.fcp then
					wagon.prev_fc(data)
					wagon.show_wagon_properties({id=uid}, pname)
			if data then
				if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
					return true
				end
				if fields.fcn then
					advtrains.step_fc(data)
					wagon.show_wagon_properties({id=uid}, pname)
				if fields.save or not fields.quit then
					if fields.whitelist then
						data.whitelist = fields.whitelist
					end
					if fields.roadnumber then
						data.roadnumber = fields.roadnumber
					end
					if fields.fc then
						wagon.set_fc(data, fields.fc)
					end
					if fields.fcp then
						wagon.prev_fc(data)
						wagon.show_wagon_properties({id=uid}, pname)
					end
					if fields.fcn then
						advtrains.step_fc(data)
						wagon.show_wagon_properties({id=uid}, pname)
					end
				end
			end
			return
		end
		uid=string.match(formname, "^advtrains_bordcom_(.+)$")
		if uid then
@@ -1221,12 +1228,13 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
					wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			end
			return
		end
		uid=string.match(formname, "^advtrains_inv_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if fields.prop and data.owner==pname then
			if fields.prop and data and data.owner==pname then
				for _,wagon in pairs(minetest.luaentities) do
					if wagon.is_wagon and wagon.initialized and wagon.id==uid and data.owner==pname then
						wagon:show_wagon_properties(pname)
@@ -1234,6 +1242,7 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
					end
				end
			end
			return
		end
end)
function wagon:seating_from_key_helper(pname, fields, no)
-- 
2.46.0
Details
Message ID
<b9d5890f-859a-47b1-938b-53d58f686e18@protonmail.com>
In-Reply-To
<20240904144444.100370-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
I would suggest additionally refactoring this a bit so that we can have 
less nesting. In particular, it would be nice to have (1) a function 
that returns an iterator for wagon entities and (2) a function that 
returns the wagon entity with the given ID.

On 04/09/2024 16:44, 1F616EMO wrote:
> @@ -1157,21 +1157,24 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
>   			for _,wagon in pairs(minetest.luaentities) do
>   				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
>   					local data = advtrains.wagons[wagon.id]
> -					if fields.inv then
> -						if wagon.has_inventory and wagon.get_inventory_formspec then
> -							minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
> -						end
> -					elseif fields.seat then
> -						local val=minetest.explode_textlist_event(fields.seat)
> -						if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
> -						--get on
> -							wagon:get_on(player, val.index)
> -							--will work with the new close_formspec functionality. close exactly this formspec.
> -							minetest.show_formspec(player:get_player_name(), formname, "")
> +					if data then
> +						if fields.inv then
> +							if wagon.has_inventory and wagon.get_inventory_formspec then
> +								minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
> +							end
> +						elseif fields.seat then
> +							local val=minetest.explode_textlist_event(fields.seat)
> +							if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
> +							--get on
> +								wagon:get_on(player, val.index)
> +								--will work with the new close_formspec functionality. close exactly this formspec.
> +								minetest.show_formspec(player:get_player_name(), formname, "")
> +							end
>   						end
>   					end
>   				end
>   			end
> +			return
>   		end

"return true" would IMO be better here as further callbacks are no 
longer relevant.

Also: wouldn't it make sense to also return at the end of the "if data" 
block? We don't (or: shouldn't) have multiple trains with the same ID, 
so the rest of the minetest.luaentities table would not really be relevant.

> @@ -1186,33 +1189,37 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
>   					end
>   				end
>   			end
> +			return

Same as above: "return true" would IMO be better here, and an early 
return at the end of the "if wagon.is_wagon" block would be nice to stop 
break out of the loop.

>   		uid=string.match(formname, "^advtrains_prop_(.+)$")
>   		if uid then
>   			local pname=player:get_player_name()
>   			local data = advtrains.wagons[uid]
> -			if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
> -				return true
> -			end
> -			if fields.save or not fields.quit then
> -				if fields.whitelist then
> -					data.whitelist = fields.whitelist
> -				end
> -				if fields.roadnumber then
> -					data.roadnumber = fields.roadnumber
> -				end
> -				if fields.fc then
> -					wagon.set_fc(data, fields.fc)
> -				end
> -				if fields.fcp then
> -					wagon.prev_fc(data)
> -					wagon.show_wagon_properties({id=uid}, pname)
> +			if data then
> +				if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
> +					return true
>   				end
> -				if fields.fcn then
> -					advtrains.step_fc(data)
> -					wagon.show_wagon_properties({id=uid}, pname)
> +				if fields.save or not fields.quit then
> +					if fields.whitelist then
> +						data.whitelist = fields.whitelist
> +					end
> +					if fields.roadnumber then
> +						data.roadnumber = fields.roadnumber
> +					end
> +					if fields.fc then
> +						wagon.set_fc(data, fields.fc)
> +					end
> +					if fields.fcp then
> +						wagon.prev_fc(data)
> +						wagon.show_wagon_properties({id=uid}, pname)
> +					end
> +					if fields.fcn then
> +						advtrains.step_fc(data)
> +						wagon.show_wagon_properties({id=uid}, pname)
> +					end
>   				end
>   			end
> +			return

Same as above: "return true" would IMO be better here.

> @@ -1221,12 +1228,13 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
>   					wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
>   				end
>   			end
> +			return

Same as above: "return true" would IMO be better here, and an early 
return at the end of the "if wagon.is_wagon" block would be nice to stop 
break out of the loop.

>   		uid=string.match(formname, "^advtrains_inv_(.+)$")
>   		if uid then
>   			local pname=player:get_player_name()
>   			local data = advtrains.wagons[uid]
> -			if fields.prop and data.owner==pname then
> +			if fields.prop and data and data.owner==pname then
>   				for _,wagon in pairs(minetest.luaentities) do
>   					if wagon.is_wagon and wagon.initialized and wagon.id==uid and data.owner==pname then
>   						wagon:show_wagon_properties(pname)
> @@ -1234,6 +1242,7 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
>   					end
>   				end
>   			end
> +			return

Same as above: "return true" would IMO be better here, and an early 
return at the end of the "if wagon.is_wagon" block would be nice to stop 
break out of the loop.

[PATCH v2] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<20240906132708.197431-1-yiufamily.hh@gmail.com>
In-Reply-To
<b9d5890f-859a-47b1-938b-53d58f686e18@protonmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +76 -45
From: 1F616EMO <root@1f616emo.xyz>

---
 advtrains/trainlogic.lua | 22 ++++-----
 advtrains/wagons.lua     | 99 ++++++++++++++++++++++++++--------------
 2 files changed, 76 insertions(+), 45 deletions(-)

diff --git a/advtrains/trainlogic.lua b/advtrains/trainlogic.lua
index f136577..81c4622 100644
--- a/advtrains/trainlogic.lua
+++ b/advtrains/trainlogic.lua
@@ -142,12 +142,11 @@ minetest.register_on_joinplayer(function(player)
		local pname = player:get_player_name()
		local id=advtrains.player_to_train_mapping[pname]
		if id then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id then
					local wdata = advtrains.wagons[wagon.id]
					if wdata and wdata.train_id == id then
						wagon:reattach_all()
					end
			local wagon = advtrains.get_wagon_by_id(id)
			if wagon and wagon.initialized then
				local wdata = advtrains.wagons[wagon.id]
				if wdata and wdata.train_id == id then
					wagon:reattach_all()
				end
			end
		end
@@ -160,12 +159,11 @@ minetest.register_on_dieplayer(function(player)
		if id then
			local train=advtrains.trains[id]
			if not train then advtrains.player_to_train_mapping[pname]=nil return end
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.train_id==id then
					--when player dies, detach him from the train
					--call get_off_plr on every wagon since we don't know which one he's on.
					wagon:get_off_plr(pname)
				end
			local wagon = advtrains.get_wagon_by_id(uid)
			if wagon and wagon.initialized and wagon.train_id == id then
				--when player dies, detach him from the train
				--call get_off_plr on every wagon since we don't know which one he's on.
				wagon:get_off_plr(pname)
			end
			-- just in case no wagon felt responsible for this player: clear train mapping
			advtrains.player_to_train_mapping[pname] = nil
diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index 536c8d4..9325a05 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1104,6 +1104,7 @@ function wagon:handle_bordcom_fields(pname, formname, fields)
		if fields["dcpl_"..i] then
			advtrains.safe_decouple_wagon(tpid, pname)
		elseif fields["wgprp"..i] then
			local wagon = advtrains.get_wagon_by_id(tpid)
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==tpid and data.owner==pname then
					wagon:show_wagon_properties(pname)
@@ -1154,44 +1155,48 @@ end
minetest.register_on_player_receive_fields(function(player, formname, fields)
		local uid=string.match(formname, "^advtrains_geton_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local data = advtrains.wagons[wagon.id]
					if fields.inv then
						if wagon.has_inventory and wagon.get_inventory_formspec then
							minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
						end
					elseif fields.seat then
						local val=minetest.explode_textlist_event(fields.seat)
						if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
						--get on
							wagon:get_on(player, val.index)
							--will work with the new close_formspec functionality. close exactly this formspec.
							minetest.show_formspec(player:get_player_name(), formname, "")
						end
			local wagon = advtrains.get_wagon_by_id(uid)
			if wagon and wagon.initialized then
				local data = advtrains.wagons[wagon.id]
				if fields.inv then
					if wagon.has_inventory and wagon.get_inventory_formspec then
						minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
					end
				elseif fields.seat then
					local val=minetest.explode_textlist_event(fields.seat)
					if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
					--get on
						wagon:get_on(player, val.index)
						--will work with the new close_formspec functionality. close exactly this formspec.
						minetest.show_formspec(player:get_player_name(), formname, "")
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_seating_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local pname=player:get_player_name()
					local no=wagon:get_seatno(pname)
					if no then
						if wagon.seat_groups then
							wagon:seating_from_key_helper(pname, fields, no)
						end
			local wagon = advtrains.get_wagon_by_id(uid)
			if wagon and wagon.initialized then
				local pname=player:get_player_name()
				local no=wagon:get_seatno(pname)
				if no then
					if wagon.seat_groups then
						wagon:seating_from_key_helper(pname, fields, no)
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_prop_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
			if not data then
				return true
			elseif pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
				return true
			end
			if fields.save or not fields.quit then
@@ -1213,29 +1218,33 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
					wagon.show_wagon_properties({id=uid}, pname)
				end
			end
			return true
		end
		uid=string.match(formname, "^advtrains_bordcom_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			local wagon = advtrains.get_wagon_by_id(uid)
			if wagon and wagon.initialized then
				wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
			end
			return true
		end

		uid=string.match(formname, "^advtrains_inv_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if fields.prop and data.owner==pname then
				for _,wagon in pairs(minetest.luaentities) do
					if wagon.is_wagon and wagon.initialized and wagon.id==uid and data.owner==pname then
						wagon:show_wagon_properties(pname)
						--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
					end
				local wagon = advtrains.get_wagon_by_id(uid)
				if wagon and wagon.initialized then
					wagon:show_wagon_properties(pname)
					--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			end
			return true
		end
		return true
end)

function wagon:seating_from_key_helper(pname, fields, no)
	local data = advtrains.wagons[self.id]
	local sgr=self.seats[no].group
@@ -1507,3 +1516,27 @@ function advtrains.get_wagon_at_index(train_id, w_index)
	-- nothing found, dist must be further back
	return nil
end

function advtrains.wagon_entity_pairs()
	local ent = minetest.luaentities
	local ent_i, wagon = next(ent, nil)

	return function()
		while ent_i do
			if wagon.is_wagon then
				local o_ent_i, o_wagon = ent_i, wagon
				ent_i, wagon = next(ent, ent_i)
				return o_ent_i, o_wagon
			end
			ent_i, wagon = next(ent, ent_i)
		end
	end
end

function advtrains.get_wagon_by_id(wagon_id)
	for _, wagon in advtrains.wagon_entity_pairs() do
		if wagon.id == wagon_id then
			return wagon
		end
	end
end
-- 
2.46.0

[PATCH] DO the optimization on bordcom

Details
Message ID
<20240906133403.200414-1-yiufamily.hh@gmail.com>
In-Reply-To
<20240906132708.197431-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +2 -7
From: 1F616EMO <root@1f616emo.xyz>

---
 advtrains/wagons.lua | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index 9325a05..54310fb 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1103,14 +1103,9 @@ function wagon:handle_bordcom_fields(pname, formname, fields)
	for i, tpid in ipairs(train.trainparts) do
		if fields["dcpl_"..i] then
			advtrains.safe_decouple_wagon(tpid, pname)
		elseif fields["wgprp"..i] then
		elseif fields["wgprp"..i] and data.owner==pname then
			local wagon = advtrains.get_wagon_by_id(tpid)
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==tpid and data.owner==pname then
					wagon:show_wagon_properties(pname)
					return
				end
			end
			wagon:show_wagon_properties(pname)
		end
	end
	--check cpl_eid_front and _back of train
-- 
2.46.0

[PATCH] nil check on wagon

Details
Message ID
<20240906133804.202472-1-yiufamily.hh@gmail.com>
In-Reply-To
<20240906133403.200414-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +3 -1
From: 1F616EMO <root@1f616emo.xyz>

---
 advtrains/wagons.lua | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index 54310fb..e28e773 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1105,7 +1105,9 @@ function wagon:handle_bordcom_fields(pname, formname, fields)
			advtrains.safe_decouple_wagon(tpid, pname)
		elseif fields["wgprp"..i] and data.owner==pname then
			local wagon = advtrains.get_wagon_by_id(tpid)
			wagon:show_wagon_properties(pname)
			if wagon then
				wagon:show_wagon_properties(pname)
			end
		end
	end
	--check cpl_eid_front and _back of train
-- 
2.46.0

Re: [PATCH v2] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<bc6541e3-1cb7-4716-9cde-f6335ed38c84@protonmail.com>
In-Reply-To
<20240906132708.197431-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Nice. I have some minor suggestions though:

On 06/09/2024 15:27, 1F616EMO wrote:
> diff --git a/advtrains/trainlogic.lua b/advtrains/trainlogic.lua
> index f136577..81c4622 100644
> --- a/advtrains/trainlogic.lua
> +++ b/advtrains/trainlogic.lua
> @@ -142,12 +142,11 @@ minetest.register_on_joinplayer(function(player)
>   		local pname = player:get_player_name()
>   		local id=advtrains.player_to_train_mapping[pname]
>   		if id then
> -			for _,wagon in pairs(minetest.luaentities) do
> -				if wagon.is_wagon and wagon.initialized and wagon.id then
> -					local wdata = advtrains.wagons[wagon.id]
> -					if wdata and wdata.train_id == id then
> -						wagon:reattach_all()
> -					end
> +			local wagon = advtrains.get_wagon_by_id(id)
> +			if wagon and wagon.initialized then
> +				local wdata = advtrains.wagons[wagon.id]
> +				if wdata and wdata.train_id == id then

I don't think we need the == id check here.

> @@ -160,12 +159,11 @@ minetest.register_on_dieplayer(function(player)
>   		if id then
>   			local train=advtrains.trains[id]
>   			if not train then advtrains.player_to_train_mapping[pname]=nil return end
> -			for _,wagon in pairs(minetest.luaentities) do
> -				if wagon.is_wagon and wagon.initialized and wagon.train_id==id then
> -					--when player dies, detach him from the train
> -					--call get_off_plr on every wagon since we don't know which one he's on.
> -					wagon:get_off_plr(pname)
> -				end
> +			local wagon = advtrains.get_wagon_by_id(uid)
> +			if wagon and wagon.initialized and wagon.train_id == id then

Same: IMO no need for the == id check here.

> diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
> index 536c8d4..9325a05 100644
> --- a/advtrains/wagons.lua
> +++ b/advtrains/wagons.lua
> @@ -1104,6 +1104,7 @@ function wagon:handle_bordcom_fields(pname, formname, fields)
>   		if fields["dcpl_"..i] then
>   			advtrains.safe_decouple_wagon(tpid, pname)
>   		elseif fields["wgprp"..i] then
> +			local wagon = advtrains.get_wagon_by_id(tpid)
>   			for _,wagon in pairs(minetest.luaentities) do

Wouldn't it make sense to remove this loop here?

> +function advtrains.wagon_entity_pairs()

IMO it would make sense to have a parameter for whether the wagon has to 
be initialized. The wagon.initialized check seems to be around in a lot 
of places.

> +	local ent = minetest.luaentities
> +	local ent_i, wagon = next(ent, nil)
> +
> +	return function()
> +		while ent_i do
> +			if wagon.is_wagon then
> +				local o_ent_i, o_wagon = ent_i, wagon
> +				ent_i, wagon = next(ent, ent_i)
> +				return o_ent_i, o_wagon
> +			end
> +			ent_i, wagon = next(ent, ent_i)
> +		end
> +	end

Alternatively this can be moved into a separate API function. Something 
like this (untested):

function advtrains.next_wagon_entity(ensure_init, k)
   local k2, ent = next(minetest.luaentities, k)
   if k2 == nil then
     return nil
   end
   if not ent.is_wagon or (ensure_init and not wagon.initialized) then
     return advtrains.next_wagon_entity(ensure_init, k2)
   end
   return k2, ent -- alternatively: return k2, ent.id, ent
end

(Note that this recursion is fine because of tail call optimization. I 
wrote this as a recursive function because it is the most intuitive for 
me, but an interative implementation is also fine.)

Then you can make advtrains.wagon_entity_pairs effectively a wrapper 
function around advtrains.next_wagon_entity.

[PATCH] Fix wrong return true

Details
Message ID
<20240906145059.233454-1-yiufamily.hh@gmail.com>
In-Reply-To
<20240906133804.202472-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +0 -1
From: 1F616EMO <root@1f616emo.xyz>

---
 advtrains/wagons.lua | 1 -
 1 file changed, 1 deletion(-)

diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index e28e773..bcb5351 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1239,7 +1239,6 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
			end
			return true
		end
		return true
end)

function wagon:seating_from_key_helper(pname, fields, no)
-- 
2.46.0

Re: [PATCH v2] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<TYCPR01MB10231A4E05DBCD1F446925DCFDB9F2@TYCPR01MB10231.jpnprd01.prod.outlook.com>
In-Reply-To
<bc6541e3-1cb7-4716-9cde-f6335ed38c84@protonmail.com> (view parent)
DKIM signature
pass
Download raw message
>> +	local ent = minetest.luaentities
>> +	local ent_i, wagon = next(ent, nil)
>> +
>> +	return function()
>> +		while ent_i do
>> +			if wagon.is_wagon then
>> +				local o_ent_i, o_wagon = ent_i, wagon
>> +				ent_i, wagon = next(ent, ent_i)
>> +				return o_ent_i, o_wagon
>> +			end
>> +			ent_i, wagon = next(ent, ent_i)
>> +		end
>> +	end
> Alternatively this can be moved into a separate API function. Something
> like this (untested):
I concur. Also, using closures is usually quite bad for performance I think,
as this returns a new closure object around the function for each call 
instead
of using a pre-existing function in the global scope.

[PATCH v3] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<20240907233537.33895-1-yiufamily.hh@gmail.com>
In-Reply-To
<20240906145059.233454-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +79 -47
From: 1F616EMO <root@1f616emo.xyz>

---
 advtrains/trainlogic.lua |  15 +++---
 advtrains/wagons.lua     | 111 +++++++++++++++++++++++++--------------
 2 files changed, 79 insertions(+), 47 deletions(-)

diff --git a/advtrains/trainlogic.lua b/advtrains/trainlogic.lua
index f136577..1941815 100644
--- a/advtrains/trainlogic.lua
+++ b/advtrains/trainlogic.lua
@@ -142,12 +142,11 @@ minetest.register_on_joinplayer(function(player)
		local pname = player:get_player_name()
		local id=advtrains.player_to_train_mapping[pname]
		if id then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id then
					local wdata = advtrains.wagons[wagon.id]
					if wdata and wdata.train_id == id then
						wagon:reattach_all()
					end
			local wagon = advtrains.get_wagon_by_id(id, true)
			if wagon then
				local wdata = advtrains.wagons[wagon.id]
				if wdata and wdata.train_id == id then
					wagon:reattach_all()
				end
			end
		end
@@ -160,8 +159,8 @@ minetest.register_on_dieplayer(function(player)
		if id then
			local train=advtrains.trains[id]
			if not train then advtrains.player_to_train_mapping[pname]=nil return end
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.train_id==id then
			for _, wagon in advtrains.wagon_entity_pairs(true) do
				if wagon and wagon.train_id == id then
					--when player dies, detach him from the train
					--call get_off_plr on every wagon since we don't know which one he's on.
					wagon:get_off_plr(pname)
diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index 536c8d4..3a57373 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1103,12 +1103,10 @@ function wagon:handle_bordcom_fields(pname, formname, fields)
	for i, tpid in ipairs(train.trainparts) do
		if fields["dcpl_"..i] then
			advtrains.safe_decouple_wagon(tpid, pname)
		elseif fields["wgprp"..i] then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==tpid and data.owner==pname then
					wagon:show_wagon_properties(pname)
					return
				end
		elseif fields["wgprp"..i] and data.owner==pname then
			local wagon = advtrains.get_wagon_by_id(tpid)
			if wagon then
				wagon:show_wagon_properties(pname)
			end
		end
	end
@@ -1154,44 +1152,48 @@ end
minetest.register_on_player_receive_fields(function(player, formname, fields)
		local uid=string.match(formname, "^advtrains_geton_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local data = advtrains.wagons[wagon.id]
					if fields.inv then
						if wagon.has_inventory and wagon.get_inventory_formspec then
							minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
						end
					elseif fields.seat then
						local val=minetest.explode_textlist_event(fields.seat)
						if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
						--get on
							wagon:get_on(player, val.index)
							--will work with the new close_formspec functionality. close exactly this formspec.
							minetest.show_formspec(player:get_player_name(), formname, "")
						end
			local wagon = advtrains.get_wagon_by_id(uid, true)
			if wagon  then
				local data = advtrains.wagons[wagon.id]
				if fields.inv then
					if wagon.has_inventory and wagon.get_inventory_formspec then
						minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
					end
				elseif fields.seat then
					local val=minetest.explode_textlist_event(fields.seat)
					if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
					--get on
						wagon:get_on(player, val.index)
						--will work with the new close_formspec functionality. close exactly this formspec.
						minetest.show_formspec(player:get_player_name(), formname, "")
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_seating_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local pname=player:get_player_name()
					local no=wagon:get_seatno(pname)
					if no then
						if wagon.seat_groups then
							wagon:seating_from_key_helper(pname, fields, no)
						end
			local wagon = advtrains.get_wagon_by_id(uid, true)
			if wagon then
				local pname=player:get_player_name()
				local no=wagon:get_seatno(pname)
				if no then
					if wagon.seat_groups then
						wagon:seating_from_key_helper(pname, fields, no)
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_prop_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
			if not data then
				return true
			elseif pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
				return true
			end
			if fields.save or not fields.quit then
@@ -1213,29 +1215,32 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
					wagon.show_wagon_properties({id=uid}, pname)
				end
			end
			return true
		end
		uid=string.match(formname, "^advtrains_bordcom_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			local wagon = advtrains.get_wagon_by_id(uid, true)
			if wagon then
				wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
			end
			return true
		end

		uid=string.match(formname, "^advtrains_inv_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if fields.prop and data.owner==pname then
				for _,wagon in pairs(minetest.luaentities) do
					if wagon.is_wagon and wagon.initialized and wagon.id==uid and data.owner==pname then
						wagon:show_wagon_properties(pname)
						--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
					end
				local wagon = advtrains.get_wagon_by_id(uid, true)
				if wagon then
					wagon:show_wagon_properties(pname)
					--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			end
			return true
		end
end)

function wagon:seating_from_key_helper(pname, fields, no)
	local data = advtrains.wagons[self.id]
	local sgr=self.seats[no].group
@@ -1507,3 +1512,31 @@ function advtrains.get_wagon_at_index(train_id, w_index)
	-- nothing found, dist must be further back
	return nil
end

function advtrains.next_wagon_entity(ensure_init, k)
	local k2, wagon = next(minetest.luaentities, k)
	if k2 == nil then
	  return nil
	end
	if not wagon.is_wagon or (ensure_init and not wagon.initialized) then
	  return advtrains.next_wagon_entity(ensure_init, k2)
	end
	return k2, wagon -- alternatively: return k2, ent.id, ent
 end

function advtrains.wagon_entity_pairs(ensure_init)
	local k, wagon

	return function()
		k, wagon = advtrains.next_wagon_entity(ensure_init, k)
		return k, wagon
	end
end

function advtrains.get_wagon_by_id(wagon_id, ensure_init)
	for _, wagon in advtrains.wagon_entity_pairs(ensure_init) do
		if wagon.id == wagon_id then
			return wagon
		end
	end
end
-- 
2.46.0

Re: [PATCH v3] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<a7cc6cfa-20e7-4f40-b590-dae6a1a26064@protonmail.com>
In-Reply-To
<20240907233537.33895-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
> @@ -142,12 +142,11 @@ minetest.register_on_joinplayer(function(player)
>   		local pname = player:get_player_name()
>   		local id=advtrains.player_to_train_mapping[pname]
>   		if id then
> -			for _,wagon in pairs(minetest.luaentities) do
> -				if wagon.is_wagon and wagon.initialized and wagon.id then
> -					local wdata = advtrains.wagons[wagon.id]
> -					if wdata and wdata.train_id == id then
> -						wagon:reattach_all()
> -					end
> +			local wagon = advtrains.get_wagon_by_id(id, true)

Is get_wagon_by_id(id, true) correct here? The id variables seems to be 
the train ID (hence the later wagon.train_id == id check).

(Oops, looks like my previous review on this part was incorrect ...)

> @@ -160,8 +159,8 @@ minetest.register_on_dieplayer(function(player)
>   		if id then
>   			local train=advtrains.trains[id]
>   			if not train then advtrains.player_to_train_mapping[pname]=nil return end
> -			for _,wagon in pairs(minetest.luaentities) do
> -				if wagon.is_wagon and wagon.initialized and wagon.train_id==id then
> +			for _, wagon in advtrains.wagon_entity_pairs(true) do
> +				if wagon and wagon.train_id == id then

Since this would also be used used above: How about an iterator function 
for getting the wagon entities of a train? A simple filter on 
next_wagon_entity should be enough.

Also: you don't need to check for the non-nilness of wagon if it is 
already (implicitly) checked by next_wagon_entity.

> @@ -1154,44 +1152,48 @@ end
>   minetest.register_on_player_receive_fields(function(player, formname, fields)
>   		local uid=string.match(formname, "^advtrains_geton_(.+)$")
>   		if uid then
> -			for _,wagon in pairs(minetest.luaentities) do
> -				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
> -					local data = advtrains.wagons[wagon.id]
> -					if fields.inv then
> -						if wagon.has_inventory and wagon.get_inventory_formspec then
> -							minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
> -						end
> -					elseif fields.seat then
> -						local val=minetest.explode_textlist_event(fields.seat)
> -						if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
> -						--get on
> -							wagon:get_on(player, val.index)
> -							--will work with the new close_formspec functionality. close exactly this formspec.
> -							minetest.show_formspec(player:get_player_name(), formname, "")
> -						end
> +			local wagon = advtrains.get_wagon_by_id(uid, true)
> +			if wagon  then

Duplicate space here.

> +function advtrains.next_wagon_entity(ensure_init, k)
> +	local k2, wagon = next(minetest.luaentities, k)
> +	if k2 == nil then
> +	  return nil
> +	end
> +	if not wagon.is_wagon or (ensure_init and not wagon.initialized) then
> +	  return advtrains.next_wagon_entity(ensure_init, k2)
> +	end
> +	return k2, wagon -- alternatively: return k2, ent.id, ent
> + end

You can remove this comment if you want. I put it there mainly as an 
implementation note because both return formats would IMO make sense 
given the context of iterating through wagon entities.

> +function advtrains.wagon_entity_pairs(ensure_init)
> +	local k, wagon
> +
> +	return function()
> +		k, wagon = advtrains.next_wagon_entity(ensure_init, k)
> +		return k, wagon
> +	end

Wouldn't "return advtrains.next_wagon_entity, ensure_init, nil" be enough?

It should be noted that this is also how pairs (and ipairs) is 
implemented (at least in PUC Lua):
* https://lua.org/source/5.1/lbaselib.c.html#luaB_pairs
* https://lua.org/source/5.1/lbaselib.c.html#base_open

> +function advtrains.get_wagon_by_id(wagon_id, ensure_init)

IMO advtrains.get_wagon_entity(id) would be better here. The wagon ID is 
more or less the only (unambiguous) index into wagons (so it does not 
have to be explicitly mentioned), and "wagon" can refer to two different 
things depending on the context: the wagon entity or a value in 
advtrains.wagons.

[PATCH v4] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<20240908140423.22712-1-yiufamily.hh@gmail.com>
In-Reply-To
<a7cc6cfa-20e7-4f40-b590-dae6a1a26064@protonmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +79 -47
From: 1F616EMO <root@1f616emo.xyz>

---
 advtrains/trainlogic.lua |  15 +++---
 advtrains/wagons.lua     | 111 +++++++++++++++++++++++++--------------
 2 files changed, 79 insertions(+), 47 deletions(-)

diff --git a/advtrains/trainlogic.lua b/advtrains/trainlogic.lua
index f136577..1941815 100644
--- a/advtrains/trainlogic.lua
+++ b/advtrains/trainlogic.lua
@@ -142,12 +142,11 @@ minetest.register_on_joinplayer(function(player)
		local pname = player:get_player_name()
		local id=advtrains.player_to_train_mapping[pname]
		if id then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id then
					local wdata = advtrains.wagons[wagon.id]
					if wdata and wdata.train_id == id then
						wagon:reattach_all()
					end
			local wagon = advtrains.get_wagon_by_id(id, true)
			if wagon then
				local wdata = advtrains.wagons[wagon.id]
				if wdata and wdata.train_id == id then
					wagon:reattach_all()
				end
			end
		end
@@ -160,8 +159,8 @@ minetest.register_on_dieplayer(function(player)
		if id then
			local train=advtrains.trains[id]
			if not train then advtrains.player_to_train_mapping[pname]=nil return end
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.train_id==id then
			for _, wagon in advtrains.wagon_entity_pairs(true) do
				if wagon and wagon.train_id == id then
					--when player dies, detach him from the train
					--call get_off_plr on every wagon since we don't know which one he's on.
					wagon:get_off_plr(pname)
diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index 536c8d4..3a57373 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1103,12 +1103,10 @@ function wagon:handle_bordcom_fields(pname, formname, fields)
	for i, tpid in ipairs(train.trainparts) do
		if fields["dcpl_"..i] then
			advtrains.safe_decouple_wagon(tpid, pname)
		elseif fields["wgprp"..i] then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==tpid and data.owner==pname then
					wagon:show_wagon_properties(pname)
					return
				end
		elseif fields["wgprp"..i] and data.owner==pname then
			local wagon = advtrains.get_wagon_by_id(tpid)
			if wagon then
				wagon:show_wagon_properties(pname)
			end
		end
	end
@@ -1154,44 +1152,48 @@ end
minetest.register_on_player_receive_fields(function(player, formname, fields)
		local uid=string.match(formname, "^advtrains_geton_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local data = advtrains.wagons[wagon.id]
					if fields.inv then
						if wagon.has_inventory and wagon.get_inventory_formspec then
							minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
						end
					elseif fields.seat then
						local val=minetest.explode_textlist_event(fields.seat)
						if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
						--get on
							wagon:get_on(player, val.index)
							--will work with the new close_formspec functionality. close exactly this formspec.
							minetest.show_formspec(player:get_player_name(), formname, "")
						end
			local wagon = advtrains.get_wagon_by_id(uid, true)
			if wagon  then
				local data = advtrains.wagons[wagon.id]
				if fields.inv then
					if wagon.has_inventory and wagon.get_inventory_formspec then
						minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
					end
				elseif fields.seat then
					local val=minetest.explode_textlist_event(fields.seat)
					if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
					--get on
						wagon:get_on(player, val.index)
						--will work with the new close_formspec functionality. close exactly this formspec.
						minetest.show_formspec(player:get_player_name(), formname, "")
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_seating_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local pname=player:get_player_name()
					local no=wagon:get_seatno(pname)
					if no then
						if wagon.seat_groups then
							wagon:seating_from_key_helper(pname, fields, no)
						end
			local wagon = advtrains.get_wagon_by_id(uid, true)
			if wagon then
				local pname=player:get_player_name()
				local no=wagon:get_seatno(pname)
				if no then
					if wagon.seat_groups then
						wagon:seating_from_key_helper(pname, fields, no)
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_prop_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
			if not data then
				return true
			elseif pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
				return true
			end
			if fields.save or not fields.quit then
@@ -1213,29 +1215,32 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
					wagon.show_wagon_properties({id=uid}, pname)
				end
			end
			return true
		end
		uid=string.match(formname, "^advtrains_bordcom_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			local wagon = advtrains.get_wagon_by_id(uid, true)
			if wagon then
				wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
			end
			return true
		end

		uid=string.match(formname, "^advtrains_inv_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if fields.prop and data.owner==pname then
				for _,wagon in pairs(minetest.luaentities) do
					if wagon.is_wagon and wagon.initialized and wagon.id==uid and data.owner==pname then
						wagon:show_wagon_properties(pname)
						--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
					end
				local wagon = advtrains.get_wagon_by_id(uid, true)
				if wagon then
					wagon:show_wagon_properties(pname)
					--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			end
			return true
		end
end)

function wagon:seating_from_key_helper(pname, fields, no)
	local data = advtrains.wagons[self.id]
	local sgr=self.seats[no].group
@@ -1507,3 +1512,31 @@ function advtrains.get_wagon_at_index(train_id, w_index)
	-- nothing found, dist must be further back
	return nil
end

function advtrains.next_wagon_entity(ensure_init, k)
	local k2, wagon = next(minetest.luaentities, k)
	if k2 == nil then
	  return nil
	end
	if not wagon.is_wagon or (ensure_init and not wagon.initialized) then
	  return advtrains.next_wagon_entity(ensure_init, k2)
	end
	return k2, wagon -- alternatively: return k2, ent.id, ent
 end

function advtrains.wagon_entity_pairs(ensure_init)
	local k, wagon

	return function()
		k, wagon = advtrains.next_wagon_entity(ensure_init, k)
		return k, wagon
	end
end

function advtrains.get_wagon_by_id(wagon_id, ensure_init)
	for _, wagon in advtrains.wagon_entity_pairs(ensure_init) do
		if wagon.id == wagon_id then
			return wagon
		end
	end
end
-- 
2.46.0

[PATCH v5] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<20240908140555.23318-1-yiufamily.hh@gmail.com>
In-Reply-To
<20240908140423.22712-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +88 -52
From: 1F616EMO <root@1f616emo.xyz>

---
Oops, forgot to add file changes... Ignore v4.

 advtrains/trainlogic.lua |  19 ++----
 advtrains/wagons.lua     | 121 ++++++++++++++++++++++++++-------------
 2 files changed, 88 insertions(+), 52 deletions(-)

diff --git a/advtrains/trainlogic.lua b/advtrains/trainlogic.lua
index f136577..2396d6d 100644
--- a/advtrains/trainlogic.lua
+++ b/advtrains/trainlogic.lua
@@ -142,13 +142,8 @@ minetest.register_on_joinplayer(function(player)
		local pname = player:get_player_name()
		local id=advtrains.player_to_train_mapping[pname]
		if id then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id then
					local wdata = advtrains.wagons[wagon.id]
					if wdata and wdata.train_id == id then
						wagon:reattach_all()
					end
				end
			for _, wagon in advtrains.train_wagons_pairs(id, true) do
				wagon:reattach_all()
			end
		end
end)
@@ -160,12 +155,10 @@ minetest.register_on_dieplayer(function(player)
		if id then
			local train=advtrains.trains[id]
			if not train then advtrains.player_to_train_mapping[pname]=nil return end
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.train_id==id then
					--when player dies, detach him from the train
					--call get_off_plr on every wagon since we don't know which one he's on.
					wagon:get_off_plr(pname)
				end
			for _, wagon in advtrains.train_wagons_pairs(id, true) do
				--when player dies, detach him from the train
				--call get_off_plr on every wagon since we don't know which one he's on.
				wagon:get_off_plr(pname)
			end
			-- just in case no wagon felt responsible for this player: clear train mapping
			advtrains.player_to_train_mapping[pname] = nil
diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index 536c8d4..d4caa8c 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1103,12 +1103,10 @@ function wagon:handle_bordcom_fields(pname, formname, fields)
	for i, tpid in ipairs(train.trainparts) do
		if fields["dcpl_"..i] then
			advtrains.safe_decouple_wagon(tpid, pname)
		elseif fields["wgprp"..i] then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==tpid and data.owner==pname then
					wagon:show_wagon_properties(pname)
					return
				end
		elseif fields["wgprp"..i] and data.owner==pname then
			local wagon = advtrains.get_wagon_entity(tpid)
			if wagon then
				wagon:show_wagon_properties(pname)
			end
		end
	end
@@ -1154,44 +1152,48 @@ end
minetest.register_on_player_receive_fields(function(player, formname, fields)
		local uid=string.match(formname, "^advtrains_geton_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local data = advtrains.wagons[wagon.id]
					if fields.inv then
						if wagon.has_inventory and wagon.get_inventory_formspec then
							minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
						end
					elseif fields.seat then
						local val=minetest.explode_textlist_event(fields.seat)
						if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
						--get on
							wagon:get_on(player, val.index)
							--will work with the new close_formspec functionality. close exactly this formspec.
							minetest.show_formspec(player:get_player_name(), formname, "")
						end
			local wagon = advtrains.get_wagon_entity(uid, true)
			if wagon then
				local data = advtrains.wagons[wagon.id]
				if fields.inv then
					if wagon.has_inventory and wagon.get_inventory_formspec then
						minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
					end
				elseif fields.seat then
					local val=minetest.explode_textlist_event(fields.seat)
					if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
					--get on
						wagon:get_on(player, val.index)
						--will work with the new close_formspec functionality. close exactly this formspec.
						minetest.show_formspec(player:get_player_name(), formname, "")
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_seating_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local pname=player:get_player_name()
					local no=wagon:get_seatno(pname)
					if no then
						if wagon.seat_groups then
							wagon:seating_from_key_helper(pname, fields, no)
						end
			local wagon = advtrains.get_wagon_entity(uid, true)
			if wagon then
				local pname=player:get_player_name()
				local no=wagon:get_seatno(pname)
				if no then
					if wagon.seat_groups then
						wagon:seating_from_key_helper(pname, fields, no)
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_prop_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
			if not data then
				return true
			elseif pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
				return true
			end
			if fields.save or not fields.quit then
@@ -1213,29 +1215,32 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
					wagon.show_wagon_properties({id=uid}, pname)
				end
			end
			return true
		end
		uid=string.match(formname, "^advtrains_bordcom_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			local wagon = advtrains.get_wagon_entity(uid, true)
			if wagon then
				wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
			end
			return true
		end

		uid=string.match(formname, "^advtrains_inv_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if fields.prop and data.owner==pname then
				for _,wagon in pairs(minetest.luaentities) do
					if wagon.is_wagon and wagon.initialized and wagon.id==uid and data.owner==pname then
						wagon:show_wagon_properties(pname)
						--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
					end
				local wagon = advtrains.get_wagon_entity(uid, true)
				if wagon then
					wagon:show_wagon_properties(pname)
					--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			end
			return true
		end
end)

function wagon:seating_from_key_helper(pname, fields, no)
	local data = advtrains.wagons[self.id]
	local sgr=self.seats[no].group
@@ -1507,3 +1512,41 @@ function advtrains.get_wagon_at_index(train_id, w_index)
	-- nothing found, dist must be further back
	return nil
end

function advtrains.next_wagon_entity(ensure_init, k)
	local k2, wagon = next(minetest.luaentities, k)
	if k2 == nil then
	  return nil
	end
	if not wagon.is_wagon or (ensure_init and not wagon.initialized) then
	  return advtrains.next_wagon_entity(ensure_init, k2)
	end
	return k2, wagon
 end

function advtrains.wagon_entity_pairs(ensure_init)
	return advtrains.next_wagon_entity, ensure_init, nil
end

function advtrains.get_wagon_entity(wagon_id, ensure_init)
	for _, wagon in advtrains.wagon_entity_pairs(ensure_init) do
		if wagon.id == wagon_id then
			return wagon
		end
	end
end

function advtrains.next_wagon_entity_in_train(var, k)
	local k2, wagon = advtrains.next_wagon_entity(var[1], k)
	if k2 == nil then
		return nil
	end
	if not (advtrains.wagons[wagon.id] and advtrains.wagons[wagon.id].train_id == var[2]) then
		return advtrains.next_wagon_entity_in_train(var, k2)
	end
	return k2, wagon
end

function advtrains.train_wagons_pairs(train_id, ensure_init)
	return advtrains.next_wagon_entity_in_train, { ensure_init, train_id }, nil
end
-- 
2.46.0

Re: [PATCH v5] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<8411a667-b634-455e-a87b-1890b144a20a@protonmail.com>
In-Reply-To
<20240908140555.23318-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Tested and (mostly) works:
* Showing and handling boardcom mostly works; exception: wagon 
properties form does not show up (see code review below).
* Showing and handling the wagon formspec (i.e. right-clicking the wagon 
from the inside) works.
* Showing and handling the wagon properties formspec works.
* Showing and handling the wagon inventory form works.
* Players are reattached to the wagon when joining.
* Players are removed from the wagon when dying.

(Also: this has turned into a small refactor for wagon code now.)

> -		elseif fields["wgprp"..i] then
> -			for _,wagon in pairs(minetest.luaentities) do
> -				if wagon.is_wagon and wagon.initialized and wagon.id==tpid and data.owner==pname then
> -					wagon:show_wagon_properties(pname)
> -					return
> -				end
> +		elseif fields["wgprp"..i] and data.owner==pname then
> +			local wagon = advtrains.get_wagon_entity(tpid)
> +			if wagon then
> +				wagon:show_wagon_properties(pname)

It seems like you need to return here to actually show the wagon 
properties formspec.

> +function advtrains.next_wagon_entity(ensure_init, k)
> +	local k2, wagon = next(minetest.luaentities, k)
> +	if k2 == nil then
> +	  return nil
> +	end
> +	if not wagon.is_wagon or (ensure_init and not wagon.initialized) then
> +	  return advtrains.next_wagon_entity(ensure_init, k2)
> +	end
> +	return k2, wagon
> + end
> +
> +function advtrains.wagon_entity_pairs(ensure_init)
> +	return advtrains.next_wagon_entity, ensure_init, nil
> +end
> +
> +function advtrains.get_wagon_entity(wagon_id, ensure_init)
> +	for _, wagon in advtrains.wagon_entity_pairs(ensure_init) do
> +		if wagon.id == wagon_id then
> +			return wagon
> +		end
> +	end
> +end

Big facepalm on my side: I just realized that there is the 
advtrains.wagon_objects table (indexed with the wagon ID) where the 
entities are also all initialized, so these three functions are not 
really needed. That said, we could keep advtrains.get_wagon_entity as a 
wrapper around advtrains.wagon_objects by checking the validity of the 
object and returning the luaentity with :get_luaentity().

Sorry for misleading you.

> +function advtrains.next_wagon_entity_in_train(var, k)
> +	local k2, wagon = advtrains.next_wagon_entity(var[1], k)
> +	if k2 == nil then
> +		return nil
> +	end
> +	if not (advtrains.wagons[wagon.id] and advtrains.wagons[wagon.id].train_id == var[2]) then
> +		return advtrains.next_wagon_entity_in_train(var, k2)
> +	end
> +	return k2, wagon
> +end

Given that advtrains.wagon_objects can be used it would IMO make sense 
to change this function to iterate through train.trainparts (note: I 
would discourage using next() since it _technically_ does not have a 
defined order of iteration).

> +function advtrains.train_wagons_pairs(train_id, ensure_init)

IMO wagon_entity_pairs_in_train would be better here for consistency.

[PATCH v6] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<20240914010404.113124-1-yiufamily.hh@gmail.com>
In-Reply-To
<8411a667-b634-455e-a87b-1890b144a20a@protonmail.com> (view parent)
DKIM signature
pass
Download raw message
Patch: +72 -52
From: 1F616EMO <root@1f616emo.xyz>

---
 advtrains/trainlogic.lua |  19 +++----
 advtrains/wagons.lua     | 105 ++++++++++++++++++++++++---------------
 2 files changed, 72 insertions(+), 52 deletions(-)

diff --git a/advtrains/trainlogic.lua b/advtrains/trainlogic.lua
index f136577..ce646da 100644
--- a/advtrains/trainlogic.lua
+++ b/advtrains/trainlogic.lua
@@ -142,13 +142,8 @@ minetest.register_on_joinplayer(function(player)
		local pname = player:get_player_name()
		local id=advtrains.player_to_train_mapping[pname]
		if id then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id then
					local wdata = advtrains.wagons[wagon.id]
					if wdata and wdata.train_id == id then
						wagon:reattach_all()
					end
				end
			for _, wagon in advtrains.wagon_entity_pairs_in_train(id) do
				wagon:reattach_all()
			end
		end
end)
@@ -160,12 +155,10 @@ minetest.register_on_dieplayer(function(player)
		if id then
			local train=advtrains.trains[id]
			if not train then advtrains.player_to_train_mapping[pname]=nil return end
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.train_id==id then
					--when player dies, detach him from the train
					--call get_off_plr on every wagon since we don't know which one he's on.
					wagon:get_off_plr(pname)
				end
			for _, wagon in advtrains.wagon_entity_pairs_in_train(id) do
				--when player dies, detach him from the train
				--call get_off_plr on every wagon since we don't know which one he's on.
				wagon:get_off_plr(pname)
			end
			-- just in case no wagon felt responsible for this player: clear train mapping
			advtrains.player_to_train_mapping[pname] = nil
diff --git a/advtrains/wagons.lua b/advtrains/wagons.lua
index 536c8d4..a7ebeea 100644
--- a/advtrains/wagons.lua
+++ b/advtrains/wagons.lua
@@ -1103,12 +1103,11 @@ function wagon:handle_bordcom_fields(pname, formname, fields)
	for i, tpid in ipairs(train.trainparts) do
		if fields["dcpl_"..i] then
			advtrains.safe_decouple_wagon(tpid, pname)
		elseif fields["wgprp"..i] then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==tpid and data.owner==pname then
					wagon:show_wagon_properties(pname)
					return
				end
		elseif fields["wgprp"..i] and data.owner==pname then
			local wagon = advtrains.get_wagon_entity(tpid)
			if wagon then
				wagon:show_wagon_properties(pname)
				return
			end
		end
	end
@@ -1154,44 +1153,48 @@ end
minetest.register_on_player_receive_fields(function(player, formname, fields)
		local uid=string.match(formname, "^advtrains_geton_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local data = advtrains.wagons[wagon.id]
					if fields.inv then
						if wagon.has_inventory and wagon.get_inventory_formspec then
							minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
						end
					elseif fields.seat then
						local val=minetest.explode_textlist_event(fields.seat)
						if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
						--get on
							wagon:get_on(player, val.index)
							--will work with the new close_formspec functionality. close exactly this formspec.
							minetest.show_formspec(player:get_player_name(), formname, "")
						end
			local wagon = advtrains.get_wagon_entity(uid)
			if wagon then
				local data = advtrains.wagons[wagon.id]
				if fields.inv then
					if wagon.has_inventory and wagon.get_inventory_formspec then
						minetest.show_formspec(player:get_player_name(), "advtrains_inv_"..uid, wagon:get_inventory_formspec(player:get_player_name(), make_inv_name(uid)))
					end
				elseif fields.seat then
					local val=minetest.explode_textlist_event(fields.seat)
					if val and val.type~="INV" and not data.seatp[player:get_player_name()] then
					--get on
						wagon:get_on(player, val.index)
						--will work with the new close_formspec functionality. close exactly this formspec.
						minetest.show_formspec(player:get_player_name(), formname, "")
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_seating_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					local pname=player:get_player_name()
					local no=wagon:get_seatno(pname)
					if no then
						if wagon.seat_groups then
							wagon:seating_from_key_helper(pname, fields, no)
						end
			local wagon = advtrains.get_wagon_entity(uid)
			if wagon then
				local pname=player:get_player_name()
				local no=wagon:get_seatno(pname)
				if no then
					if wagon.seat_groups then
						wagon:seating_from_key_helper(pname, fields, no)
					end
				end
			end
			return true
		end

		uid=string.match(formname, "^advtrains_prop_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
			if not data then
				return true
			elseif pname~=data.owner and not minetest.check_player_privs(pname, {train_admin = true}) then
				return true
			end
			if fields.save or not fields.quit then
@@ -1213,29 +1216,32 @@ minetest.register_on_player_receive_fields(function(player, formname, fields)
					wagon.show_wagon_properties({id=uid}, pname)
				end
			end
			return true
		end
		uid=string.match(formname, "^advtrains_bordcom_(.+)$")
		if uid then
			for _,wagon in pairs(minetest.luaentities) do
				if wagon.is_wagon and wagon.initialized and wagon.id==uid then
					wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			local wagon = advtrains.get_wagon_entity(uid)
			if wagon then
				wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
			end
			return true
		end

		uid=string.match(formname, "^advtrains_inv_(.+)$")
		if uid then
			local pname=player:get_player_name()
			local data = advtrains.wagons[uid]
			if fields.prop and data.owner==pname then
				for _,wagon in pairs(minetest.luaentities) do
					if wagon.is_wagon and wagon.initialized and wagon.id==uid and data.owner==pname then
						wagon:show_wagon_properties(pname)
						--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
					end
				local wagon = advtrains.get_wagon_entity(uid)
				if wagon then
					wagon:show_wagon_properties(pname)
					--wagon:handle_bordcom_fields(player:get_player_name(), formname, fields)
				end
			end
			return true
		end
end)

function wagon:seating_from_key_helper(pname, fields, no)
	local data = advtrains.wagons[self.id]
	local sgr=self.seats[no].group
@@ -1507,3 +1513,24 @@ function advtrains.get_wagon_at_index(train_id, w_index)
	-- nothing found, dist must be further back
	return nil
end

function advtrains.get_wagon_entity(wagon_id)
	if not advtrains.wagons[wagon_id] then return end
	local object = advtrains.wagon_objects[wagon_id]
	if object then
		return object:get_luaentity()
	end
end

function advtrains.next_wagon_entity_in_train(train, i)
	local wagon_id = train[i + 1]
	if wagon_id then
		return i + 1, advtrains.get_wagon_entity(wagon_id)
	end
end

function advtrains.wagon_entity_pairs_in_train(train_id)
	local train = advtrains.trains[train_id]
	if not train then return function() end end
	return advtrains.next_wagon_entity_in_train, train, 0
end
-- 
2.46.0

Re: [PATCH v6] Wagon iterator, lookup by id, and use them in code

Details
Message ID
<de1bd2d6-ff60-45a0-b77f-0c0fe5a5a6db@protonmail.com>
In-Reply-To
<20240914010404.113124-1-yiufamily.hh@gmail.com> (view parent)
DKIM signature
pass
Download raw message
Looks good beside some minor things.

> +function advtrains.next_wagon_entity_in_train(train, i)
> +	local wagon_id = train[i + 1]

Shouldn't this be train.trainparts?

> +	if wagon_id then
> +		return i + 1, advtrains.get_wagon_entity(wagon_id)

It would IMO be better to have a nil check here since other places use 
the entity directly without nil checks.
Reply to thread Export thread (mbox)