Time Nick Message 10:33 MTDiscord merging #14330 in 5m 10:33 ShadowBot https://github.com/minetest/minetest/issues/14330 -- Support absent scene node names by appgurueu 12:10 sfan5 great, sqlite fails to build with recent android toolchains 13:48 Krock meeting in roughly 1 hour 15:00 Krock Meeting starts. https://dev.minetest.net/Meetings#2024-02-18 15:00 Krock ping @luatic sfan5 Desour 15:01 sfan5 hi 15:01 MTDiscord hi 15:01 Krock hello! :) 15:01 Desour o/ 15:01 [MTMatrix] heyo 15:02 Krock looking promising so far. so let's get started. 15:02 Krock > Consider converting unfinished / abandoned PRs to draft rather than closing them. 15:03 Krock @luatic brought this up. is there a particular PR that you had in mind? 15:03 Krock "adoption needed" is generally added for PRs that are complete but neglected due to absence of reviews 15:04 [MTMatrix] big 👎️ for me. We're already drowning in PRs, there's no need to stalling even more. Also, closing a PR is sometimes the fastest way for OP to start working on their PR again 15:04 [MTMatrix] *stall 15:04 MTDiscord Yes. The (rather popular) "dual wielding" PR series was closed due to inactivity. 15:04 sfan5 i dont see a reason for this either 15:04 MTDiscord As said, I think converting to draft would probably signal "unfinished" better. When looking for PRs to review, I usually just ignore draft PRs, and I don't mind an increased PR counter. 15:05 Krock there's also the "Action/Change needed" flags which I think are the better way to communicate. 15:05 [MTMatrix] Lizzy has been tagged multiple times. If there is no answer, why should we keep it open? 15:05 Desour if I'd see a draft PR as contributor, I'd think the PR author would eventually complete the PR, and not be inclined to open it another time 15:05 MTDiscord Good point 15:05 MTDiscord Maybe closing + adoption needed is the better approach here 15:05 [MTMatrix] which is what we already do 15:06 Krock so keep as-is 15:06 MTDiscord alright fine 15:06 Krock > Do we want approvals to be revoked when new commits are pushed? 15:07 Krock there is no general answer 15:07 sfan5 "it depends" 15:07 Krock changes can be minor or major 15:07 Desour (if we want more adoption, maybe draw new contributors' attention to them, e.g. by linking to adoption needed PRs like we're linking to good first issues) 15:07 proller__ maybe itstime to discuss how to merge https://github.com/minetest/minetest/pull/12142 ? 15:07 [MTMatrix] So we don't want a machine decide for us 15:07 MTDiscord sfan5, Krock: well yes, but what is the better default? 15:07 sfan5 if unsure we should ask the approver if they still approve of it 15:07 MTDiscord seems fair 15:07 Krock I'd opt for the decision of the 2nd reviewer 15:08 Krock revoke the first review if they think that the PR has changed too much 15:08 sfan5 or alternatively: if unsure revoke the approval and wait for the (former) approver to say something agian 15:08 sfan5 again* 15:08 MTDiscord dismissing approvals risks incurring additional round trips just for the 2nd review to confirm that they re-approve minor changes 15:08 MTDiscord okay, good 15:09 Krock Desour: such PRs tend to be complicated, thus unsuited for new contributors. nonetheless it would be nice to have them somehow linked 15:09 [MTMatrix] or: just ask the former approver if they're still fine, without revoking anything. No answer = go for it 15:09 Desour if we handle this like trivial PRs, the PR *should* not be merged by the 2nd reviewer, as they are the one who decided that the change after PR was trivial enough to not require a re-approval of first approver 15:09 MTDiscord "no answer = go for it" seems a bit risky in the case of major changes 15:10 Krock how often does this happen? 1-2 times per year? 15:10 [MTMatrix] probably 15:10 Krock I think we're trying to cover edge-dases here 15:10 Krock *cases 15:10 sfan5 +1 15:11 [MTMatrix] "hope the other approver has got a brain"? 15:11 MTDiscord yes-ish. but there's the risk that requested "minor" changes introduce a regression which goes unnoticed (like breaking backwards compat, or a segfault, or a memleak etc.), where it might be good to have a second pair of eyes on it. 15:11 MTDiscord a third pair of eyes, strictly speaking 15:12 Desour or at least please wait a little before merging after your 2nd approval if some things changed 15:12 MTDiscord +1 15:13 MTDiscord Lars: do you have an example where that happened? 15:13 Desour for example, in #13992 there were some changes with how the pointabilites are stored. PR was merged in <1hour after 2nd approval. I could've maybe spotted that uaf issue earlier if there was some time 15:14 ShadowBot https://github.com/minetest/minetest/issues/13992 -- Tool specific pointing and blocking pointable type by cx384 15:14 MTDiscord yep, that's what i had in mind 15:14 Krock yes. it's generally best to keep PRs resting a bit 15:15 Krock although chances are there that you wouldn't have noticed it until 1 day later 15:15 Krock so it's always a trade-off. we cannot be perfect, so I think the current strategy suffices for 95% of the cases, which is good enough. 15:16 MTDiscord how long should I let PRs rest? 15:17 MTDiscord a PR I probably merged too early after the second approval is #14071, but no issues were found with that one yet 🥲 15:17 ShadowBot https://github.com/minetest/minetest/issues/14071 -- Move hard coded minimap to builtin by cx384 15:17 Krock or introduce a reminder for the 2nd reviewer to invalidate other reviews on major changes 15:17 Krock or both 15:18 Krock suggestions? 15:19 Krock I'd personally prefer a short notice in the wiki that the 2nd reviewer should take care of the validity of the other review 15:19 sfan5 sounds good 15:19 Desour probably not worth adding a strict rule. the general issue has been discussed and reviewers are now wary of this 15:19 Desour +1 for Krock suggestion 15:19 MTDiscord okay 15:20 cx384 grorp: this PR has an issue since it uses the deprecated hud_elem_type field 15:20 MTDiscord so if there have been major changes, we'll require a re-review from the first approver? 15:20 MTDiscord cx384: well, that's not nearly as bad as a UAF ;) 15:20 MTDiscord cx384: ah right, but that's already been present when the first approval was given, right? 15:20 Krock @grorp: yes. it's then up to the 1st reviewer to decide how much time they want to spend on the re-review 15:21 cx384 yes 15:21 Krock (or any other reviewer for that matter) 15:22 Krock editing the wiki 15:22 Krock I'll send the link to the changes later on 15:22 Krock > hen do we want 6.0 to happen? After 5.9, after 5.10, even later? (appgurueu) 15:23 sfan5 i dont think it makes much sense to decide on a time now 15:23 Krock aside the open internal discussion, there don't seem to be any urgent points if I remember corrrectly 15:23 sfan5 unless major decisions happen we'll proceed with 5.9 5.10 5.11 5.12 15:24 Krock we'll do it when there's enough open issues to justify a breakage 15:24 sfan5 also the opportunity to break compatibility should be well used and as many points as possible adressed 15:24 sfan5 (doc/breakages.md) 15:24 Desour btw. what do we break in 6.0? 15:24 Desour api? network? 15:25 proller__ maybe its time to switch network to enet ? 15:25 Krock optimally as much as possible, as already said 15:25 MTDiscord so we want 6.0 to be considered far away for now? 15:25 proller__ and make world 32bit ? 15:25 MTDiscord i agree that we should collect many planned breakages to make it worth the major release 15:25 sfan5 Desour: API definitely, network is usually done too 15:26 [MTMatrix] reminder: there's a breakage.md file where we note breakages 15:26 Krock proller__: I gave enet a try and I'd much prefer to wait for a proper QUIC implementation for proper encryption support while still getting decent performance 15:26 Desour I see 15:26 sfan5 we could also switch to an user-space sctp impl 15:27 Krock that's also an option yes. I think we have an issue somewhere about that too 15:27 proller__ usrsctp is good but have some... problems 15:27 proller__ one fork have test implementation of usrsctp 15:28 Desour oh, enet is what sauerbraten uses? 15:28 sfan5 IIRC yes 15:28 proller__ also it have multiprotocol mode, server can serve mt + enet + usrsctp on separate ports 15:30 proller__ so protocols can be added without disabling old clients 15:32 Krock ([previous topic] added invalidate reminder to https://dev.minetest.net/Git_Guidelines#Upstream_commit_rules 15:32 Krock ) 15:33 Krock moving on to the PR reviews 15:34 Krock #14154 ( @Zughy ) 15:34 ShadowBot https://github.com/minetest/minetest/issues/14154 -- Simplify bug report form by rubenwardy 15:34 Krock already closed. further questions? 15:34 MTDiscord ruben closed the PR, though i'm not sure why. our current form is still overly complex for many issues, imo. 15:35 rubenwardy I'm against a hard break in network 15:35 Krock https://github.com/minetest/minetest/commit/8c3a6a81 happened in the meantime 15:35 Desour I'd still prefer a simple text template 15:35 MTDiscord Agreed 15:36 proller__ rubenwardy, but current networking is still very bad, nobody fixed it in last 10 years 15:36 [MTMatrix] +1 Desour/luatic 15:36 Krock Desour: what would be the benefits? 15:37 rubenwardy I'm against a "simple" text template and prefer the form 15:38 Desour Krock: more freedom (e.g. you can add more sections), it doesn't look different after you press edit, general simplicity 15:38 rubenwardy but most of the information should just come from a copy from the Minetest client, no one will know what an irrlicht device is 15:39 Krock Desour: you can still do that with the current template. It's a template, not a form (where you have to adhere to the format) 15:39 sfan5 I also prefer the text template 15:39 MTDiscord I see five fields that are often useless ("irrlicht device" to "active renderer") 15:39 Desour (anyway, I haven't opened too many bug reports recently. could live with a good form *shrug*) 15:39 Krock simple text might get chaotic. this way we'll at least have the information that we need 15:40 Krock nicely grouped by headers 15:41 MTDiscord oh lol i just realized that i'm allowed to open a blank issue 15:41 MTDiscord i'll probably just use that in the future (and tag it myself) then 15:42 MTDiscord the design is a bit too subtle for my taste tbh but it works 15:42 MTDiscord https://cdn.discordapp.com/attachments/747163566800633906/1208800655515582545/Screenshot_from_2024-02-18_16-41-46.png?ex=65e49a56&is=65d22556&hm=2a0207cb29af25e714797175acecf2e3d54016e8bb1e739aea720c5118f4a993& 15:42 sfan5 i think it's intentionally subtle so most people don't click on it 15:43 [MTMatrix] ^ 15:44 Krock perhaps the issue template could be further tweaked (again) but I'd prefer to keep it similar so that newcomers don't forget to provide necessary information 15:44 Desour also, if you report a bug, you'll assume that you have to use the form, or else the maintainers will hate you 15:45 MTDiscord can non-maintainers even open blank issues? 15:46 Krock should be possible I think 15:46 Krock yes they can. tested it on another repo where I'm not a maintainer 15:47 Krock if there's specific suggestions, we'll await PRs, I think? 15:48 Desour we could also have both, simple text and form. and let the submitter choose 15:49 MTDiscord plugging #13605 into the issue template discussion as usual (looks like I need to rebase it) 15:49 ShadowBot https://github.com/minetest/minetest/issues/13605 -- Allow exporting config and debug.txt with one click (on all platforms) by grorp 15:49 MTDiscord we have simple text, it's just not prominent, so i'm good :) 15:49 Desour we don't have simple text with template though 15:51 Krock @grorp this is helpful for errors and crashes but not so much for any other issue. Platform information on the other hand is somewhat welcome 15:51 MTDiscord so you want something like a "Bug (simple)" template? 15:51 Desour yes 15:52 Desour (but as said, I can live with a good form) 15:52 Krock from what I understood there's currently 2 vs 2 whether or not to use a text template 15:52 MTDiscord Krock: yes, then you could remove the detailed fields (Irrlicht device etc.) and just have people paste all the platform information at once 15:53 Krock @grorp Yes, that makes sense. I'd welcome to update the template after this PR is done 15:53 Krock s/done/merged/ 15:54 Krock alternatively such information could also be shown in a textarea formspec 15:54 Krock and let the user copy & paste (manually or with a button). so they don't have to open a file that's located somewhere 15:54 MTDiscord huh? my PR adds a button that copies this stuff to the clipboard 15:55 MTDiscord sorry, I gtg 15:56 Krock oh. sorry. By exporting I somewhat understood that it's dumped to a file. Indeed. Clipboard is great. 15:57 Krock added myself so that I don't forget to review it 15:57 Krock #14225 15:57 ShadowBot https://github.com/minetest/minetest/issues/14225 -- Add bulk_get_node function by sfence 15:58 Krock this is another one for discussion (not related to the issue template) 15:58 Krock > So are we doing this or not? (sfan5) 15:59 Krock according to @luatic's review the test itself is rather flawed, so take my testing results with a big grain of salt 16:00 MTDiscord As said, I think we can do this for the API, but we shouldn't advertise it as very relevant for performance 16:00 MTDiscord (yet) 16:00 Desour I'll soon open a PR for optimized get_node, FYI 16:00 MTDiscord especially as future work may optimize get_node further 16:00 MTDiscord Desour: +1 16:01 MTDiscord I'm also not sure that the bulk_get_node api design is optimal for performance, in case we want to optimize it in the future as well 16:02 MTDiscord specifically, taking position tables and producing node tables might incur some unnecessary overhead vs. a more voxelmanip-like API 16:02 Desour another issue with bulk_get_node is that users might overestimate its performance benefit, and get much more nodes than they'd need to 16:02 Krock the promising aspect of the bulk function is that it reduces the Lua -> C++ calls, but only that. The exact positions still have to be calculated manually and tables created from that 16:04 MTDiscord I see grorp just commented 15 min ago (https://github.com/minetest/minetest/pull/14225#issuecomment-1951366947): 16:04 MTDiscord "If we add this, I think we should go a step further: We should explicitly state that there is no performance benefit to avoid modders doing premature optimisation." 16:05 Krock the underlying issue #14110 that it promises to solve did not receive any particular player support so far 16:05 ShadowBot https://github.com/minetest/minetest/issues/14110 -- lua api: Function to get adjacent nodes. 16:05 Krock (player or modder) 16:05 Krock performance aspects only appeared later on 16:05 Krock > Please may you include benchmarks, as that is the justification for this feature 16:06 Krock that makes me assume that without a performance uplift there's no support from rubenwardy ? 16:07 rubenwardy that's correct, I don't see the point of this unless it improves performance. It's not easier to use 16:07 Krock so that kinda of defies the request in the original issue 16:08 sfan5 maybe someone should re-benchmark bulk_set_node too 16:10 Krock I'll try to remember. If there's other volunteers, feel free to provide the results in the PR. 16:11 Krock Until then I'd suggest to hold the PR and see what Desour comes up? 16:11 cx384 Without performance benefits this feature does not seam to be very useful and could lead to even more unnecessary functions like minetest.bulk_remove_node or minetest.bulk_get_node_or_nil. 16:11 MTDiscord well, bulk_set_node has some things going for it: 16:11 MTDiscord (1) we can't remove it because modders have started using it; 16:11 MTDiscord (2) it is actually somewhat convenient if you happen to have the positions where you want to place nodes laying around; 16:12 MTDiscord (3) because it only sets nodes of the same type, it might have a slight performance advantage over set_node, which has to re-convert the node table each time 16:12 cx384 The API should hot get bloated imo. 16:12 cx384 *not 16:12 MTDiscord I agree 16:14 Krock hmm. the most recent benchmarks do still promise a 28% uplift ( https://github.com/minetest/minetest/pull/14225#issuecomment-1899728126 ) which is a bit strange based on @luatic's theory of what's executed first 16:15 sfan5 someone should look at #14383 since I just pushed some irrlicht commits that require these changes 16:15 ShadowBot https://github.com/minetest/minetest/issues/14383 -- Irrlicht support changes by sfan5 16:15 sfan5 afk now 16:16 MTDiscord Krock: Let me try to re-benchmark this. Though I'm not sure the current benchmarks are fine, since they warm up bulk_get_node (which may or may not warm up get_node as much). 16:17 Krock @luatic Thank you for taking care of this. 16:18 Krock your theory of warm-up does somewhat make sense. perhaps we could get this more reliable by pinning the CPU frequency to maximum (reduce OS noise) 16:20 Krock are there any other PRs to discuss? 16:21 MTDiscord go through one approval prs? 16:23 Krock > https://github.com/minetest/minetest/labels/One%20approval%20%E2%9C%85%20%E2%97%BB%EF%B8%8F 16:23 Desour opened #14384 16:23 ShadowBot https://github.com/minetest/minetest/issues/14384 -- Implement get_node with a get_node_raw by Desour 16:23 Krock the meeting is now again running for almost 1.5 hours so it's rather running dry now (in terms of motivation) 16:24 MTDiscord Krock: Okay seeing Desour's performance claims, I think benchmarking bulk_get_node has become obsolete, unless it receives the same optimization 16:24 Krock for me it's fine as-is. I'll bring up specific PRs in the next meeting if needed. 16:24 MTDiscord "Rough benchmark results: master: 300-800 ms; PR: around 155 ms (it's much more persistent, idk why)" <- Desour's PR 16:27 Krock Desour: that looks promising. thank you. 16:27 Krock saved the meeting notes. moved to past meetings. thank you for participating 16:28 MTDiscord Thanks Desour for the PR from me too :) 16:29 MTDiscord Ah btw I will probably be rather inactive for the next 1.5-ish weeks due to exams 16:34 Krock good luck! 16:41 MTDiscord thanks :) 17:19 sfan5 huh apparently a PR / issue can have multiple assignees 18:13 MTDiscord always has? 18:19 sfan5 for some reason I thought this wasn't possible 18:38 proller__ if i make zero version of 32bit patch with ONLY massive type renames (v3s16 -> v3pos + v3bpos + v3opos) and no-op conversion functions - what chance to be reviewed and merged in next 1-3 years? 21:09 proller__ ... looks like 0%