Minetest logo

IRC log for #minetest-dev, 2024-02-18

| Channels | #minetest-dev index | Today | | Google Search | Plaintext

All times shown according to UTC.

Time Nick Message
01:42 fluxionary joined #minetest-dev
01:43 SFENCE joined #minetest-dev
02:31 SFENCE joined #minetest-dev
03:21 SFENCE joined #minetest-dev
04:17 behalebabo joined #minetest-dev
04:40 Lupercus joined #minetest-dev
05:00 MTDiscord joined #minetest-dev
05:21 calcul0n joined #minetest-dev
06:34 SFENCE joined #minetest-dev
06:45 YuGiOhJCJ joined #minetest-dev
06:54 YuGiOhJCJ joined #minetest-dev
07:20 SFENCE joined #minetest-dev
08:20 hlqkj joined #minetest-dev
09:22 Warr1024 joined #minetest-dev
09:46 Warr1024 joined #minetest-dev
10:33 MTDiscord <luatic> merging #14330 in 5m
10:33 ShadowBot https://github.com/minetest/minetest/issues/14330 -- Support absent scene node names by appgurueu
10:39 SFENCE joined #minetest-dev
11:23 appguru joined #minetest-dev
12:10 sfan5 great, sqlite fails to build with recent android toolchains
13:29 cx384 joined #minetest-dev
13:48 Krock meeting in roughly 1 hour
14:40 Desour joined #minetest-dev
14:58 proller joined #minetest-dev
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 <luatic> hi
15:01 Krock hello! :)
15:01 Desour o/
15:01 [MTMatrix] <Zughy> 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] <Zughy> 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] <Zughy> *stall
15:04 MTDiscord <luatic> 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 <luatic> 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] <Zughy> 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 <luatic> Good point
15:05 MTDiscord <luatic> Maybe closing + adoption needed is the better approach here
15:05 [MTMatrix] <Zughy> which is what we already do
15:06 Krock so keep as-is
15:06 proller__ joined #minetest-dev
15:06 MTDiscord <luatic> 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] <Zughy> So we don't want a machine decide for us
15:07 MTDiscord <luatic> 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 <luatic> 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 <luatic> dismissing approvals risks incurring additional round trips just for the 2nd review to confirm that they re-approve minor changes
15:08 MTDiscord <luatic> 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] <Zughy> 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 <luatic> "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] <Zughy> 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] <Zughy> "hope the other approver has got a brain"?
15:11 MTDiscord <luatic> 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 <luatic> 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 <grorp> +1
15:13 MTDiscord <grorp> Lars: do you have an example where that happened?
15:13 proller joined #minetest-dev
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 <luatic> 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 <luatic> how long should I let PRs rest?
15:17 MTDiscord <grorp> 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 <luatic> okay
15:20 cx384 grorp: this PR has an issue since it uses the deprecated hud_elem_type field
15:20 MTDiscord <grorp> so if there have been major changes, we'll require a re-review from the first approver?
15:20 MTDiscord <luatic> cx384: well, that's not nearly as bad as a UAF ;)
15:20 MTDiscord <grorp> 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 <luatic> so we want 6.0 to be considered far away for now?
15:25 proller__ and make world 32bit ?
15:25 MTDiscord <luatic> 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] <Zughy> 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 <luatic> 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 <luatic> Agreed
15:36 proller__ rubenwardy, but current networking is still very bad, nobody fixed it in last 10 years
15:36 [MTMatrix] <Zughy> +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 <luatic> 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 <luatic> oh lol i just realized that i'm allowed to open a blank issue
15:41 MTDiscord <luatic> i'll probably just use that in the future (and tag it myself) then
15:42 MTDiscord <luatic> the design is a bit too subtle for my taste tbh but it works
15:42 MTDiscord <luatic> https://cdn.discordapp.com/attachments/747163566800633906/1208800655515582545/Screenshot_from_2024-02-18_16-41-46.png?ex=65e49a56&amp;is=65d22556&amp;hm=2a0207cb29af25e714797175acecf2e3d54016e8bb1e739aea720c5118f4a993&amp;
15:42 sfan5 i think it's intentionally subtle so most people don't click on it
15:43 [MTMatrix] <Zughy> ^
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 <luatic> 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 <grorp> 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 <luatic> 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 <luatic> 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 <grorp> 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 <grorp> huh? my PR adds a button that copies this stuff to the clipboard
15:55 MTDiscord <grorp> 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 <luatic> 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 <luatic> (yet)
16:00 Desour I'll soon open a PR for optimized get_node, FYI
16:00 MTDiscord <luatic> especially as future work may optimize get_node further
16:00 MTDiscord <luatic> Desour: +1
16:01 MTDiscord <luatic> 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:01 Lupercus joined #minetest-dev
16:02 MTDiscord <luatic> 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 <luatic> I see grorp just commented 15 min ago (https://github.com/minetest/minetest/pull/14225#issuecomment-1951366947):
16:04 MTDiscord <luatic> "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 <luatic> well, bulk_set_node has some things going for it:
16:11 MTDiscord <luatic> (1) we can't remove it because modders have started using it;
16:11 MTDiscord <luatic> (2) it is actually somewhat convenient if you happen to have the positions where you want to place nodes laying around;
16:12 MTDiscord <luatic> (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 <luatic> 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 <luatic> 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 <wsor4035> 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 <luatic> 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 <luatic> "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 <luatic> Thanks Desour for the PR from me too :)
16:29 MTDiscord <luatic> Ah btw I will probably be rather inactive for the next 1.5-ish weeks due to exams
16:29 fluxionary joined #minetest-dev
16:34 Krock good luck!
16:37 cx384 joined #minetest-dev
16:41 MTDiscord <luatic> thanks :)
16:45 SFENCE joined #minetest-dev
17:13 imi joined #minetest-dev
17:19 sfan5 huh apparently a PR / issue can have multiple assignees
18:13 proller__ joined #minetest-dev
18:13 MTDiscord <wsor4035> 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?
19:29 lhofhansl joined #minetest-dev
20:06 [MTMatrix] joined #minetest-dev
20:09 [MTMatrix] joined #minetest-dev
20:18 SFENCE joined #minetest-dev
20:39 [MTMatrix] joined #minetest-dev
21:03 SFENCE joined #minetest-dev
21:09 proller__ ... looks like 0%
22:51 appguru joined #minetest-dev
23:32 panwolfram joined #minetest-dev
23:32 YuGiOhJCJ joined #minetest-dev

| Channels | #minetest-dev index | Today | | Google Search | Plaintext