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
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
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 trueend)
+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
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
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.
>> + 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
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
> @@ -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
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
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
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
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
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.