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&is=65d22556&hm=2a0207cb29af25e714797175acecf2e3d54016e8bb1e739aea720c5118f4a993& |
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 |