Time Nick Message 01:34 [MTMatrix] Desour: you might self assign some issues/PRs so other people will know they don't need to send their time there 01:34 [MTMatrix] *spend 12:01 Krock > -- Found ZLIB: optimized;D:/a/irrlicht/irrlicht/vcpkg/installed/x64-windows/lib/zlib.lib;debug;D:/a/irrlicht/irrlicht/vcpkg/installed/x64-windows/debug/lib/zlibd.lib (found version "1.3.0") 12:02 Krock it seems that MSVC has two library options in its path. I don't think that's intentional? 12:35 Krock solved: those are keywords evaluated in target_link_libraries depending on the build type 13:59 Krock friendly reminder: meeting in 1 hour 14:01 Krock meeting points: https://dev.minetest.net/Meetings#2024-01-21 <-- @ devs : in case you'd like to discuss something, feel free to add it 14:57 Desour @Zughy: right, I've self-assigned me now here and there. :) but most of the things further down on my todo list can also be done by others, and I'll only do them if nobody else does 14:58 Krock meeting in 2 minutes 14:59 sfan5 1 15:00 sfan5 wait no, zero 15:00 Krock pinging those who reacted. @Zughy sfan5 srifqi appguru / @appguru 15:00 [MTMatrix] hello 15:00 grorp hello 15:01 srifqi hello 15:01 Krock * @appgurueu 15:01 Desour or @luatic 15:01 Krock oh nice. this meeting will be quite well-visited for once 15:01 Krock meeting points: https://dev.minetest.net/Meetings#2024-01-21 15:02 Krock > PRs are on the rise and FOSDEM is coming 15:02 Krock there's always been a lot of PRs and I don't think there's much that can be done about it? 15:02 Krock what would you like to change or know @Zughy ? 15:02 sfan5 well I feel there have been much more recently 15:03 Krock sfan5: heh you're not innocent : :P (in a positive way) 15:03 sfan5 right, but I think the main target of this point was outside contributors 15:03 [MTMatrix] my proposal is in the second meeting point. It's complicated, because we're all volunteering, so for sure we can't force other people (but ourselves) to do something 15:04 sfan5 because I can definitely see the risk of the PRs being left lying around 15:04 Krock yes. assignments make sense to me, so that you feel obligated to take care of it 15:04 Krock (for PRs that are not on the roadmap) 15:05 srifqi Does "FOSDEM is coming" mean there might be more (probably new) contributors? 15:05 Krock @Zughy what's the link destination? it links to the meeting page. 15:06 Krock srifqi: it's a good public exposure which might raise attention and hence PRs and even more - tons of feature requests 15:06 [MTMatrix] I think we should also accept the fact that we'll see some PR not making it because our time is limited. I'm fine taking the feet stomping from contributors, which are warned when they open the PR. However, we should make the existence of the roadmap clearer than it is now. Maybe through the blog or I don't know 15:07 sfan5 regarding roadmap: I think many of recent PRs are on the roadmap, so that's not a reason we could use to reduce the numbers... 15:07 [MTMatrix] Krock: oh my bad, it was supposed to be a link to an IRC conversation, on it 15:08 [MTMatrix] here https://irc.minetest.net/minetest-dev/2024-01-15#i_6146007 15:09 Krock the github assignment feature could be used for that 15:10 Krock reading a wall of argument texts is quite tiresome - especially at high PR counts 15:10 sfan5 sounds useful 15:10 Krock looking for the appropriate wiki page .... 15:11 sfan5 unofficially it's supposed to work like this anyway and I try to review a PR until it's done if I review it once 15:12 Krock in the past this also worked but getting a second person involved was sometimes a bit complicated 15:13 Krock but for now we could write down that supporting core devs should try to assign themselves to PRs to clarify the situation. does that sound OK? I'd also add that to the wiki as-is. 15:13 sfan5 +1 15:13 grorp let's say I support a PR, self-assign it, review it and give it the first approval. now I'm effectively done with the PR and another coredev has to continue. do I remove my assignment at this point? or does it stay? 15:14 Krock grorp: I think should stay until it is merged because you'd then know best about the history of the PR 15:14 Krock in case there's questions 15:15 sfan5 usually there's multiple rounds of reviews, so until the end IMO 15:15 Krock because if you unassign yourself, it might end up in rebase hell because no-one takes care about getting it done 15:15 grorp hmm, okay 15:16 [MTMatrix] I also know for sure rubenwardy and v-rob aren't against this idea, so +2 15:16 srifqi i was checking whether GitHub allows multiple assignees 15:16 srifqi and it supports up to 10 people 15:16 srifqi +1 from me 15:16 Desour makes sense +1 15:16 grorp my "hmm, okay" is also a +1 15:17 Krock https://dev.minetest.net/Git_Guidelines#Issue_and_Pull-Request_Management this seems to be about the right place - or are there any files about that topic in the repo? 15:17 [MTMatrix] I'll assign core devs if I see some "supported by core dev" in case you forgot then. I'll avoid lhofhansl until they're aware of the situation I guess 15:17 Desour but right now I'm not sure when I should support an issue then. because reviewing the PR will then take up so much time, and I might want to prioritize other things first 15:18 [MTMatrix] Krock: it seems about right 15:19 rubenwardy Oh hey 15:19 grorp Krock: maybe CONTRIBUTING.md - but since this piece of information is more useful for coredevs than for contributors, the wiki page seems like a better place 15:19 Desour it would be bad for the contributor, if they make a PR for a supported issue, which was then only supported half-hearted. and then they'll wait and rebase for weeks or months 15:19 Krock question is whether we want to apply this only to PRs or issues as well 15:19 sfan5 only PRs imo 15:19 [MTMatrix] the issue is in issues with that label itself Desour 15:19 Krock PRs only +1 15:20 sfan5 and if I support an issue does that mean I am automatically assigned to a relevant PR? 15:20 rubenwardy You'd have to assign to issues, there's no such thing as reviewers for an issue 15:20 rubenwardy Could do it manually like sfan saus 15:21 srifqi wouldn't assigning an issue mean the assignee should work on that? 15:21 [MTMatrix] ^ 15:21 srifqi or at least that is what i see from other projects 15:21 Krock ^ good point 15:21 sfan5 well I'm posing that as a question because I don't think it should work like that 15:22 Krock the initial question was to solve the PR floods - ignoring issues for now. 15:22 srifqi +1 just for PRs 15:22 Krock we'd have to find a solution for the "Supported by core dev" issues sometime 15:22 [MTMatrix] I'll see what I can think of 15:23 Desour if just for PRs, then an issue supporting coredev should at least leave a comment explicitly saying *they* support it, when they add the label 15:24 srifqi by the way, what does that label means for issues? 15:24 srifqi from what i understand, that label on issues means, "It is a good idea. I would like to review PRs regarding the issue (not necessarily working on it)." 15:24 [MTMatrix] the issue is that you could actually support it but then nobody actually works on a PR for that issue, so a part of your brain is reserved for nothing 15:25 Krock srifqi: that's also how I interpret it 15:25 Desour about sfan5's question: if you are not automatically assigned to a relevant PR, aka if you are not ready to review respective PRs, why did you add the support label to the issue? 15:26 sfan5 I understood the label as "a coredev has looked at it and confirmed it makes sense within Minetest's design, constraints, etc." 15:26 [MTMatrix] because contributors might implement it if they see there is a will from development to actually have it 15:27 sfan5 so that people don't go and work on open issues that have no chance 15:27 Desour if it made no sense and had no chance, wouldn't the right thing be to close the issue? 15:27 [MTMatrix] but now, with the self assignment thing, is problematic 15:27 sfan5 I suppose "a coredev" is too passive, pretend I said "I" 15:27 Krock the concept is usually OK but the implementation is suboptimal 15:28 sfan5 in most cases this actually means that I would actually review a relevant PR 15:28 sfan5 but my time is limited and the potential problem I'm seeing here is the expectation that I will actually review any such PR 15:28 Desour maybe add a new label then. "I, as coredev, support this" 15:28 [MTMatrix] For instance: #13733 15:28 ShadowBot https://github.com/minetest/minetest/issues/13733 -- Optional delay when using items 15:28 srifqi so, there are two meanings: (1) it is good and a core dev would like to review it or (2) it is good, but that does not necessarily mean that a core dev would like to review it 15:29 [MTMatrix] ruben might have liked the idea but he might not have the slightest intention to review such PR. Now what does he do? 15:29 [MTMatrix] *to review a related PR 15:30 [MTMatrix] If someone opens a PR, we're supposed to automatically label it as "Supported by core dev" according to our rules. But with today's rule, it'd also mean that rubenwardy should be self-assigned, and I don't think it's fair 15:30 [MTMatrix] *be assigned 15:30 [MTMatrix] also, go to go, sorry :( I'll read later 15:31 Krock so it's like "Concept approval" (currently "Supported by core dev") vs "Approval + Willing to review" 15:31 Krock see you. 15:32 Desour maybe "the problem mentioned by the feature request should be fixed" vs. "the solution described here is conceptually fine, and worked out" vs. "a coredev is willing to review when a PR emerges" 15:33 Krock we can make this complicated but at the end of the day nobody is forced to review something 15:34 Krock let alone do anything. we'd like to not let PRs rot away so assigning to PRs you'd somewhen like to review would be a good start 15:35 Desour I don't want to assign myself to a PR that I'd like to review *eventually*, so after months or so 15:35 Krock optimally that's also such where you supported the issue by a "concept is fine"-like reply 15:36 Krock Desour: what approach would you suggest instead? 15:36 sfan5 my proposal: if a coredev reviews a PR for the first time he should stay with the PR for additional review rounds / rebases and to make that clear assign himself to the PR 15:37 Krock +1. at least that should be defined 15:37 srifqi how about "Supported by core dev" for PRs (i.e. a core dev would like to review it as explained by the assignee list) and "Concept approval" for issues (as explained by sfan5 before)? 15:37 sfan5 of course if for some reason the interest is lost (ideally this doesn't happen often) you can unassign yourself 15:37 Desour Krock: well, I'd review almost all of the old PRs out there. because bad ones are closed earlier 15:39 Desour +1 for what sfan5 described, as that's common practice as of now anyways. 15:40 Krock sfan5: with or without assignment? 15:40 sfan5 what do you mean? 15:41 Krock whether you'd like to assign yourself to a PR (respectively unassign) to show your interest and keep yourself - well - assigned to it 15:41 Krock mainly to see who's taking care. or is the "reviewer" overview of the PR good enough? 15:42 sfan5 yes to the question 15:42 sfan5 (not the second one) 15:45 srifqi i also agree with allowing unassign 15:45 srifqi with conditions as described by sfan5's proposal 15:45 Krock > Core devs who reviewed a PR once should stay with the PR for additional review rounds. Loss of interest (thus unsubscribing) should be signalled properly. 15:45 Krock >> This is best communicated by assigning yourself to the PR using the GitHub feature. 15:45 Krock example wording. suggestions? 15:47 sfan5 sounds fine 15:47 srifqi that seems fine 15:47 srifqi has the "assigning yourself to show interest" part been agreed? 15:48 Desour if one just leaves a small code comment, that is not counted as engaging in reviewing, right? 15:48 Krock Desour: right. it would just be a "sidekick". 15:49 Krock it is mainly to see who's taking care of the PR, eventually leaving the option to @ them to get things rolling 15:50 Desour +1 15:53 Krock added 15:54 Krock next up: #14103 15:54 ShadowBot https://github.com/minetest/minetest/issues/14103 -- Add drawtype sunken and covered by sfence 15:54 srifqi Krock: added to where? 15:54 Krock srifqi: https://dev.minetest.net/Git_Guidelines#Issue_and_Pull-Request_Management . I will put a link into the meeting notes. 15:55 srifqi thank you 15:56 Krock you're welcome 15:57 Krock about drawtype PR: it seems that no coredev is interested in it? 15:57 Krock the code isn't that readable from the first glance 15:58 Krock what are the use-cases? 15:59 Desour apparently they want to get liquid in other nodes 15:59 Desour but it's of course not a perfect solution 15:59 Krock there's already liquid levelled glasslike framed 15:59 srifqi is it like water-logged blocks in MC? 15:59 grorp Krock: also code duplication in the drawing functions 16:00 Desour Krock: liquid in glasslike framed is only useful for tanks and lava lamps, afaik 16:00 Krock grorp: code can always be improved. question is whether the concept is generic enough to justify adding 16:00 grorp of course 16:01 MTDiscord i think based on the comments on the pr itself, discussion in the community, existing hacks in the engine to already do this (plantlike rooted), it is wanted 16:01 MTDiscord *already do it to a very limited extent 16:02 MTDiscord arguably the concept is more generic than the existing plantlike rooted 16:02 Desour plantlike rooted works with arbitrary liquids though 16:03 MTDiscord but only works for one specific type (plants), requires you to have a specific type of node under it 16:03 Krock apparently "sunken" adapts to the liquid level around 16:04 MTDiscord you typically have only a fewe types of liquids, but many types of nodes you could want under it 16:04 srifqi sorry, i can not give much comments on this. i even haven't tried to use other drawtype (^_^ ") 16:05 Krock @wsor4035 right, thus I wonder whether this needs a specific drawtype or a new node def field to specify whether it can be submerged 16:06 Krock the "inner_node" field is kinda hacky 16:06 MTDiscord krock: seems the author is willing to make changes to get the concept through. maybe comment to that effect in the pr? 16:06 Krock I'd then link and summarize the points that we made here in the PR 16:07 Krock I hoped for some opinions 16:07 Krock your input is helpful in this regard. thanks. I'll just go ahead and summarize what we've got so far 16:08 Krock disclaimer: which is mostly my inputs 16:08 MTDiscord Not relevant for a comment to that PR, but I hope we can long-term come up with a better solution for "multiple nodes in the same place" than having adjacent nodes, but that's a breaking change and out of scope for this PR. 16:09 MTDiscord to avoid having to register new nodes for each combination, maybe a paramtype2 "waterloggable"? param2 would then contain the liquid type and the usual liquid information (which is too much information for param2 sigh) 16:09 Desour yeah, why does a node have only one drawtype in the first pace? 16:11 Desour place* 16:14 srifqi shall we proceed to the next item? 16:14 sfan5 sure 16:15 srifqi https://github.com/minetest/irrlicht/pull/248 16:15 grorp_ short off-topic: merging #14087 in 15 min 16:15 ShadowBot https://github.com/minetest/minetest/issues/14087 -- Touchscreen: Allow mods to swap the meaning of short and long taps by grorp 16:15 Desour irr#248 16:15 ShadowBot https://github.com/minetest/irrlicht/issues/248 -- Reformat the code by numberZero 16:15 sfan5 I would merge the irrlicht reformat PR just before importing it into MT 16:15 sfan5 to not hinder any of the existing dev work in the repo 16:16 Krock answered to the issue. that took a while to write down. 16:16 Desour they'll have to rebase on the style changes anyways though 16:16 srifqi grorp_: the last two commits are a good catch. i didn't check that 16:17 srifqi regarding reformatting code, will the clang format be removed (like the current minetest main code)? 16:17 grorp_ srifqi: thanks :) it was mostly a theoretical bug though, you'd need to tap with like 30 TPS to trigger it 16:17 Desour ... so idk, why we shouldn't merge the reformatting right away 16:18 Desour after the clang format is done, it's actually a great opportunity to add some clang tidy checks to CI, so we'll always have PRs with good code style 16:18 sfan5 Desour: once we move irrlicht from the repo to MT it's sort of a cut anyway and you have to manually open any wip PRs on the other repo 16:19 Krock I found Irrlicht-Mt pretty lightweight to build other apps, compared to stock Irrlicht, thus I'd personally would prefer to use submodules instead - but based on previous discussions the consensus is to merge. 16:19 sfan5 reformatting now doesn't have any advantages other than "it's done right now" 16:19 MTDiscord Reformatting will create rebase work for currently open PRs 16:20 MTDiscord The gltf loader at least is like 95% in separate files so a rebase should be survivable 16:20 Desour so the question is which PRs should be merged before the reformatting 16:21 sfan5 all that are ready by the time we move irrlicht into MT 16:21 srifqi ... or is it that we can import Irrlicht to MT after they are ready? 16:22 MTDiscord Of the 7 open PRs, two are CMake stuff which shouldn't be concerned. 16:23 MTDiscord 3, actually 16:24 srifqi so, there are 3 PRs needed to be merged before the formatting PR? 16:24 Desour sfan5: so, are you suggesting that #13642 should be made instantly merge-able, and as soon as that happens we merge it all? 16:24 ShadowBot https://github.com/minetest/minetest/issues/13642 -- Import Irrlicht by numberZero 16:24 Desour (which you claim won't happen before the other PRs are ready, which would then cause another redo of the reformatting/import PRs) 16:25 MTDiscord srifqi: yep, there's the gltf loader, irr#276 and irr#234 a rebase will hurt the former a bit, but we should survive it decently. not sure about the other 2 PRs. 16:25 ShadowBot https://github.com/minetest/irrlicht/issues/276 -- GL loader work by sfan5 16:25 ShadowBot https://github.com/minetest/irrlicht/issues/234 -- Change OpenGL 3 Renderer to Core Profile by calebabutler 16:25 sfan5 what I'm thinking: wait until desired date or event -> ensure relevant PRs have been merged -> rebase and merge irr#248 -> rebase and merge #13642 16:25 ShadowBot https://github.com/minetest/minetest/issues/13642 -- Import Irrlicht by numberZero 16:25 ShadowBot https://github.com/minetest/irrlicht/issues/248 -- Reformat the code by numberZero 16:25 MTDiscord Desour: these PRs should ideally be cheap to redo, or am I missing something? 16:26 sfan5 relevant PRs = ones to the Irrlicht repo, not the reformatting 16:27 Desour luatic: cheap yes, but not instantaneous. so a new PR could sneak in between 16:27 MTDiscord doesnt gltf make sense to wait till after irrlichtmt is merged into mt because of jsoncpp support? 16:27 srifqi can the "desired event" be relevant PRs have been merged? 16:27 sfan5 it could, but separate discussion 16:28 MTDiscord wsor: we figured that out using FetchContent, we don't really feel strongly either way currently 16:28 MTDiscord ah, thanks 16:29 Desour can we have a fixed event (e.g. a fixed set of PRs is merged), and then block any until the reformat/import PRs are done? 16:29 sfan5 sure 16:29 sfan5 I was hoping to sort out the GL linkage in irrlicht before we import it 16:30 sfan5 (doing so would get rid of another >1k LOC that we don't need to copy around) 16:32 srifqi so, is it just 276? the other PRs seem to be fine for a little rebase after the formatting PR 16:32 srifqi i'm not sure about the glTF, though 16:32 sfan5 yes 16:35 Desour ok, so when 276 is merged, we won't trigger another delay of import? will write that down then 16:35 Krock I currently have the meetings page on edit to put a date there 16:36 Krock perhaps try to achieve it by Feb 4? a milestone could be arranged too if that's more helpful 16:38 Krock adding a milestone for Feb 4 and adding the two PRs 276 and the line formatting thing into it 16:38 Krock done 16:39 Krock next up: irr$#281 16:39 ShadowBot https://github.com/minetest/minetest/issues/281 -- Update src/guiFormSpecMenu.h by RealBadAngel 16:39 Krock irr#281 16:39 ShadowBot https://github.com/minetest/irrlicht/issues/281 -- CMake: Replace generator conditional expressions by SmallJoker 16:39 Krock Q: how to fix it? 16:40 sfan5 I would prefer a fix that doesn't remove the generator expressions but dunno 16:41 Krock honestly I haven't spent any time into research as for why exactly this issue appears. A simple fix would be good enough for me to build Irrlicht properly 16:41 Krock or let's rest it for a while to look out for more opinions 16:42 sfan5 e.g. this http://sprunge.us/A06U9T?diff 16:42 Krock quickly testing ... 16:43 Krock works just fine. I could adopt this in the PR if that's fine for you 16:44 Krock also to make sure MSVC doesn't act strangely 16:44 sfan5 sure 16:45 Krock thank you. 16:45 sfan5 jumping ahead a point: #11391 and #13789 have been sitting at one approval for a long time. if you (plural) have time please look 16:45 ShadowBot https://github.com/minetest/minetest/issues/11391 -- Add support for static boxes in 'leveled' type; add node box types 'leveled_plantlike[_rooted]' by Wuzzy2 16:45 ShadowBot https://github.com/minetest/minetest/issues/13789 -- Fix liquid falling if in "float" group, fixes #13782 by kromka-chleba 16:47 Krock the leveled PR is so old, the image host went offline in the meantime 16:47 [MTMatrix] Can't se split Supported by core dev in two labels: "Concept approved" (for issues) and "Supported by core dev" (for PRs)? 16:47 [MTMatrix] *we split 16:48 Krock sorry, I'll be away for 30 minutes. Thank you all in advance for participating 16:48 Krock meeting page is updated up to this point 16:49 srifqi thank you for organising 16:49 srifqi Zughy: that was my suggestion 16:49 MTDiscord see you in 30 minutes :) 16:50 [MTMatrix] As a triager, I support that 16:50 MTDiscord fine by me 16:50 [MTMatrix] Next question is: what happens if I open a PR for a concept approved issue? 16:51 Desour do PRs then inherit "concept approved" from issues (or get it themselves if there's no issue)? 16:51 srifqi the "Concept approved" label should be used solely for issues (not for PRs) 16:52 srifqi the PR for a concept approved issue will be labelled the same way as regular PR 16:52 [MTMatrix] ^ 16:52 MTDiscord PRs shouldn't inherit concept approved. An issue being proper doesn't mean any PR that fixes it is proper. 16:52 srifqi should be labelled* 16:52 [MTMatrix] So we shouldn't close a PR after N time if it's for a concept approved one and not on the roadmap? 16:53 srifqi others can check whether the issue itself has a concept approval from reading the issue 16:54 srifqi Zughy: yes, it can be treated like that until it is reviewed 16:54 Desour ideally I'd like to have a fixed set of states that a PR can be in, and corresponding labels. so for closing, one can just filter by age plus missing label 16:55 [MTMatrix] But wouldn't it risk to sit there for months? 16:56 Desour from my former POV as non-coredev contributor, I find it more preferable to have a PR lying around for months, but be eventually merged, than to just have it closed even though it's not *bad* 16:57 Desour (but with clear communication that it's likely to lie around for a long time) 16:58 MTDiscord Agreed 16:58 MTDiscord We could just use the "low priority" label, no? 16:58 [MTMatrix] On PRs? Meeeh 16:58 Desour +1 on "low priority" 16:59 Desour if any coredev finds it important and wants to review soonish, they can remove it 16:59 srifqi ... and then assign themselves? 17:00 Desour maybe :thinking: 17:01 MTDiscord I'll start assigning myself to PRs. Up until now I just requested myself for review, but assigning seems more appropriate. 17:02 srifqi the "Low priority" label is already used as in "the issue that this PR fix has low priority" 17:03 srifqi and not low priority to review 17:03 Desour it's used for one open PR 17:03 [MTMatrix] Labelling them as "low priority" means sentencing them to the PR cemetery 17:04 Desour then add a new label "apparently low interest", and add it automatically after 30 days? 17:05 MTDiscord "low review priority"? 17:05 rubenwardy @luatic: use "request for review", don't assign 17:06 srifqi the assignment idea should already give context about interests in reviewing 17:06 srifqi (if it is agreed. has it been agreed?) 17:06 MTDiscord Reading the backlog, didn't we establish that we wanted to assign ourselves to PRs if we want to get them over the finish line (by means of reviewing until they have two approvals)? 17:06 rubenwardy huh, I thought by assignment people meant assign for review not actual assign? 17:06 Desour srifqi: it has been added to wiki 17:07 Desour https://dev.minetest.net/Git_Guidelines#Issue_and_Pull-Request_Management 17:07 srifqi oh, that is true. i just refreshed the page 17:08 srifqi can a request for review be canceled? 17:08 srifqi and can a user remove their review? 17:09 Desour rubenwardy: you can't request review or unassign reviewers in github anyways, it seems. and the author can request one reviewer, afaik 17:10 rubenwardy I can request review and unassign reviewers 17:10 rubenwardy is that an owner-only priv 17:10 srifqi i can not do that 17:10 rubenwardy https://rwdy.uk/djFRa.png 17:10 rubenwardy odd 17:11 MTDiscord request review has the downside that you need to do it again after every review, AFAIK; it doesn't persist 17:11 lhofhansl Are we still meeting? 15:00 UTC is quite early for me. 17:11 rubenwardy I do this from the "reviews" menu, it has a dropdown https://rwdy.uk/vI1nL.png 17:11 rubenwardy Well, using assignment is fine as long as people don't think it means they're doing the work 17:12 srifqi lhofhansl: yes 17:12 MTDiscord just tried, works for me too (and I'm not an "owner" AFAIK) 17:12 Desour rubenwardy: you can remove "request for review", but the feature is mixed with normal reviewers 17:12 srifqi rubenwardy: did you click the person again? 17:12 MTDiscord rubenwardy: yeah, that's why I used "request for review" before, because I think it makes the intent slightly more clear 17:12 Krock re. rubenwardy: I can assign people too. and request reviews. 17:12 rubenwardy yeah. Clicking the person again unassigns them 17:13 rubenwardy * yeah. Clicking the person again removes the request for review 17:13 MTDiscord we should work on contributor guidelines 17:14 MTDiscord in the repo, that is - something like a CONTRIBUTING.md, which can point to some dev wiki links and explain stuff like our review or labeling system etc. 17:14 srifqi is it this one: https://github.com/minetest/minetest/blob/master/.github/CONTRIBUTING.md ? 17:14 MTDiscord ah it's placed under .github 17:15 MTDiscord nevermind me then :) 17:15 Krock oh. my file search app ignores hidden directories. how convenient.... 17:15 srifqi i also was confused about it at first (was searching the docs/ folder 17:15 srifqi ) 17:16 MTDiscord let's hope new contributors find it. i assume github shows this to them when they open a PR? 17:17 srifqi yes: https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors 17:17 srifqi > When someone opens a pull request or creates an issue, they will see a link to that file. 17:17 MTDiscord good 17:18 lhofhansl * reading the backlog from today 17:18 srifqi ... and also here: https://github.com/minetest/minetest/contribute 17:18 Krock srifqi: where did you find that link? 17:19 srifqi from the docs page linked before 17:19 srifqi > For an example of a contribute page, see github/docs/contribute. 17:19 Desour we should put a link into the readme 17:19 Krock interesting how I cannot find a link for that in the repo itself 17:19 Desour how are ppl supposed to find this otherwise? 17:19 srifqi that is interesting indeed 17:21 srifqi i'm guessing that it meant to be used like for example Weblate engagement badge image, e.g. https://hosted.weblate.org/engage/minetest/ 17:22 srifqi so, are we fine with assigning core devs? (do we need more discussion?) 17:22 Krock there's currently doc/developing/*, doc/direction.md, .github/CONTRIBUTING.md and the dev wiki 17:25 srifqi they are scattered all over places 17:26 Krock srifqi: about assigning: I put it that way that it's each dev's own responsibility to stay involved in the PRs and try to get it through by assigning themselves 17:27 Krock would you suggest to force-assign, or did I misunderstand this question? 17:27 lhofhansl Many other projects have their own CONTRIBUTING.md in the top level directory. 17:28 srifqi Krock: i just want to check again because there were some discussion about core dev assignment after you left for a while 17:29 srifqi lhofhansl: ... or in a non-hidden folder (not prefixed by a full-stop [.]) 17:30 srifqi shall we move to the next point? 17:30 lhofhansl What was decided? 17:30 srifqi kept it as-is for now (?) 17:31 srifqi none actually ( ._.) 17:31 lhofhansl Regarding the contribution documentation? That's not a good outcome. 17:32 srifqi shall we move CONTRIBUTING.md file to doc/ instead? 17:32 lhofhansl I 17:32 lhofhansl I'd be in favor of that. Even top-level directory is fine. 17:32 Krock srifqi: there's review requests and assignments. assignments would show your dedication (assigned on your own) and review requests could be issued by anyone capable of doing so 17:32 srifqi the other document file outside the doc/ folder are readme and licenses 17:33 MTDiscord srifqi: does it still work if it's in the doc/ folder? 17:33 Krock +1 for CONTRIBUTING into doc/ and put a link into the current hidden folder 17:33 srifqi Krock: i understand the difference now. thank you 17:33 Krock :) 17:34 srifqi luatic: from the GitHub docs: 17:34 Krock in the process of moving stuff, sections of the wiki should be linked to github 17:34 srifqi > Decide whether to store your contributing guidelines in your repository's root, docs, or .github directory. 17:35 srifqi i'm not sure whether it supports doc/ instead of docs/ 17:35 Krock if it does not support doc/ then we can provide a dummy file with a huge link 17:36 srifqi what is a "huge" link? using headings? 17:36 Krock # [YOU'RE WRONG HERE, CLICK ME](https://www.youtube.com/watch?v=dQw4w9WgXcQ) 17:36 Krock something like this except that it goes to the correct file 17:37 srifqi the message's display is funny on Discord 17:37 MTDiscord https://cdn.discordapp.com/attachments/747163566800633906/1198682846391455835/image.png?ex=65bfcb65&is=65ad5665&hm=1a2d55dd5d869090a75ce19f6c44054c4171da1071583fc262540fad6a37a831& 17:37 Krock LGTM. merge as-is. 17:38 ROllerozxa forgot to put angle brackets around though so the video embed shows up on discord 8) 17:40 Krock srifqi: would you like to take care of the documentation, or rather have it done by someone else? 17:40 srifqi i might not be able to take care for that for now 17:41 Krock okay 17:41 Krock I'll put it on my mid-term TODO list 17:44 Krock oh right. if anyone's got questions on how to test #12595 - let me know so that I can update the PR with better instructions. Added it to the meeting points because it apparently solves another issue. 17:44 ShadowBot https://github.com/minetest/minetest/issues/12595 -- [NO SQUASH] Inventory: Fix order of callbacks when swapping items by SmallJoker 17:45 srifqi i am already sleepy, so i just want to add some points (if the discussion were to be continued): 1. how often is the contributors page in the MT website updated? 2. since the account is abandoned anyway, can the MT Twitter account have the plain profile picture? 17:45 srifqi see you 17:51 Krock cya. wiki page updated. 17:52 Krock at least the website is written in a pretty generic manner. moving the CONTRIBUTING file should not affect it 18:11 lhofhansl looks like we're done :) 19:36 sfan5 @zughy is this useful to you #14296 19:36 ShadowBot https://github.com/minetest/minetest/issues/14296 -- Allow fog color to be overriden properly by sfan5 19:55 MTDiscord merging #14244 in 5m 19:55 ShadowBot https://github.com/minetest/minetest/issues/14244 -- Fix waypoint precision wraparound, add bounds check by appgurueu 20:03 MTDiscord sfan5: "Friendly advice: Distro packagers will curse you for doing source downloads at build-time" - I have 2 questions: 20:03 MTDiscord (1) does that include configure time? 20:03 MTDiscord (2) why? this is effectively just "package manager at home" provided by cmake. i don't want to have to deal with dozens of different system package managers. 20:04 rubenwardy 1: In order to get reproducible builds, most packaging systems download everything first before building. So they can track all the source code 20:05 rubenwardy *2: 20:06 [MTMatrix] sfan5: sounda definitely useful, my issues with fog though are different, see #14176 20:06 ShadowBot https://github.com/minetest/minetest/issues/14176 -- Better fog API structure (visible, colours etc) 20:06 [MTMatrix] *sounds 20:07 [MTMatrix] It's pointless that I can customise fog colour if I can't force fog onto players. And I think we should have a set_fog(), maybe? 20:12 [MTMatrix] to sum up labels: 20:12 [MTMatrix] - Rename "Supported by core dev" into "Concept approved" 20:12 [MTMatrix] - Create a new "Supported by core dev" label to stick onto PRs that had it before the renaming 20:12 [MTMatrix] - Opening a PR for a "Concept approved" label might result in being ignored for a long time, as roadmap has the priority 20:13 [MTMatrix] Am I right? 21:01 [MTMatrix] ...well, I'll take it as a yes. I'll proceed 21:24 sfan5 @luatic due to what rubenwardy said every "go download this" solution (that is not something standard like a package manager) creates extra manual and project-specific work 21:24 sfan5 in this case it being part of cmake doesn't count as "standard" 21:26 sfan5 re "package manager at home": that would be less of a problem if the configure script transparently used a system-wide library install 21:26 sfan5 even in that case it can be tedious for the distro packager to ensure some dependency is not inadvertedly statically linked 21:28 sfan5 if there was a C++ package manager that is even remotely standard distro packagers could also create reusable tooling around it 22:40 [MTMatrix] since I was at it, I've changed labels palette to be coherent and a little bit better contrasted. If you think it's worse and you think I should really revert it, please tell me 22:47 Desour are you sure the contrast is higher now? it looks all so low saturated blueish to me now. where's the green? 22:47 Desour anyway, idk much about colors 22:54 [MTMatrix] that green was not matching the palette and the green in the palette made the text almost unreadable, so I lower it down one tone 22:55 [MTMatrix] for a variant I've temporarily changed "@ Startup / Config / Util" if you wanna have a look 22:55 [MTMatrix] but I think it's too much 22:57 Desour don't need to necessarily see it ¯\_(ツ)_/¯ 23:10 [MTMatrix] I'll keep "concept approved" on PRs because without any specific roadmap/supported label, we might think that someone simply forgot to label it (and we'll check the description every time, which is pretty annoying) 23:20 [MTMatrix] also, I'm assigning some people according to them sticking the old "Supported by core dev" label to a PR that wasn't linked to a previously supported issue. If you don't want to, feel free to remove yourself (and please warn OP)