1F616EMO: 9 Wagon formspec: advtrains.wagons[wagon.id] nil checks, and minor optimizations Wagon iterator, lookup by id, and use them in code DO the optimization on bordcom nil check on wagon Fix wrong return true Wagon iterator, lookup by id, and use them in code Wagon iterator, lookup by id, and use them in code Wagon iterator, lookup by id, and use them in code Wagon iterator, lookup by id, and use them in code 14 files changed, 439 insertions(+), 283 deletions(-)
Nice. I have some minor suggestions though:
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.)
Looks good beside some minor things.
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~gpcf/advtrains-devel/patches/54867/mbox | git am -3Learn more about email & git
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
"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.
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
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.
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
Same as above: "return true" would IMO be better here.
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
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.
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
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.
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.
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
I don't think we need the == id check here.
+ 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
Same: IMO no need for the == id check here.
+ --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
Wouldn't it make sense to remove this loop here?
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()
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):Blockhead <jbis1337@hotmail.com>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.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.
+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
Nice. I have some minor suggestions though:
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
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
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
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)
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 ...)
+ 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
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.
--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
Duplicate space here.
+ 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
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
+end + +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.
+ for _, wagon in advtrains.wagon_entity_pairs(ensure_init) do + if wagon.id == wagon_id then + return wagon + end + end +end -- 2.46.0
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
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)
It seems like you need to return here to actually show the wagon properties formspec.
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
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.
+ return advtrains.next_wagon_entity_in_train, { ensure_init, train_id }, nil +end -- 2.46.0
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.)
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]
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.
+ 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
Looks good beside some minor things.