| Time |
Nick |
Message |
| 01:34 |
[MTMatrix] |
<Zughy> 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] |
<Zughy> *spend |
| 02:57 |
|
fluxionary_ joined #minetest-dev |
| 05:00 |
|
MTDiscord joined #minetest-dev |
| 06:39 |
|
json joined #minetest-dev |
| 07:05 |
|
YuGiOhJCJ joined #minetest-dev |
| 07:45 |
|
YuGiOhJCJ joined #minetest-dev |
| 07:57 |
|
calcul0n joined #minetest-dev |
| 08:32 |
|
YuGiOhJCJ joined #minetest-dev |
| 08:41 |
|
YuGiOhJCJ joined #minetest-dev |
| 09:22 |
|
Warr1024 joined #minetest-dev |
| 09:46 |
|
Warr1024 joined #minetest-dev |
| 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:36 |
|
appguru joined #minetest-dev |
| 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:45 |
|
Desour joined #minetest-dev |
| 14:55 |
|
srifqi joined #minetest-dev |
| 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 |
|
grorp joined #minetest-dev |
| 15:00 |
Krock |
pinging those who reacted. @Zughy sfan5 srifqi appguru / @appguru |
| 15:00 |
[MTMatrix] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> Krock: oh my bad, it was supposed to be a link to an IRC conversation, on it |
| 15:08 |
[MTMatrix] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> ^ |
| 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> *to review a related PR |
| 15:30 |
[MTMatrix] |
<Zughy> 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] |
<Zughy> *be assigned |
| 15:30 |
[MTMatrix] |
<Zughy> 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 |
|
Desour joined #minetest-dev |
| 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 |
<wsor4035> 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 |
<wsor4035> *already do it to a very limited extent |
| 16:01 |
|
Desour joined #minetest-dev |
| 16:02 |
MTDiscord |
<wsor4035> arguably the concept is more generic than the existing plantlike rooted |
| 16:02 |
Desour |
plantlike rooted works with arbitrary liquids though |
| 16:03 |
MTDiscord |
<wsor4035> 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 |
<wsor4035> 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:05 |
|
grorp_ joined #minetest-dev |
| 16:06 |
Krock |
the "inner_node" field is kinda hacky |
| 16:06 |
MTDiscord |
<wsor4035> 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 |
<luatic> 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 |
<grorp> 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 |
<luatic> Reformatting will create rebase work for currently open PRs |
| 16:20 |
MTDiscord |
<luatic> 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 |
<luatic> Of the 7 open PRs, two are CMake stuff which shouldn't be concerned. |
| 16:23 |
MTDiscord |
<luatic> 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 |
<luatic> 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 |
<luatic> 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 |
<wsor4035> 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 |
<luatic> wsor: we figured that out using FetchContent, we don't really feel strongly either way currently |
| 16:28 |
MTDiscord |
<wsor4035> 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] |
<Zughy> 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] |
<Zughy> *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 |
<luatic> see you in 30 minutes :) |
| 16:50 |
[MTMatrix] |
<Zughy> As a triager, I support that |
| 16:50 |
MTDiscord |
<luatic> fine by me |
| 16:50 |
[MTMatrix] |
<Zughy> 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] |
<Zughy> ^ |
| 16:52 |
MTDiscord |
<luatic> 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] |
<Zughy> 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] |
<Zughy> 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 |
<luatic> Agreed |
| 16:58 |
MTDiscord |
<luatic> We could just use the "low priority" label, no? |
| 16:58 |
[MTMatrix] |
<Zughy> 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 |
<luatic> 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] |
<Zughy> 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 |
<luatic> "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 |
<luatic> 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:10 |
|
lhofhansl joined #minetest-dev |
| 17:11 |
MTDiscord |
<luatic> 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 |
<grorp> 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 |
<luatic> 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 |
<luatic> we should work on contributor guidelines |
| 17:14 |
MTDiscord |
<luatic> 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 |
<luatic> ah it's placed under .github |
| 17:15 |
MTDiscord |
<luatic> 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 |
<luatic> 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 |
<luatic> 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 |
<luatic> 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 |
<srifqi> 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 |
|
appguru joined #minetest-dev |
| 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 :) |
| 18:27 |
|
Desour joined #minetest-dev |
| 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 |
<luatic> 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 |
<luatic> sfan5: "Friendly advice: Distro packagers will curse you for doing source downloads at build-time" - I have 2 questions: |
| 20:03 |
MTDiscord |
<luatic> (1) does that include configure time? |
| 20:03 |
MTDiscord |
<luatic> (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] |
<Zughy> 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] |
<Zughy> *sounds |
| 20:07 |
[MTMatrix] |
<Zughy> 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] |
<Zughy> to sum up labels: |
| 20:12 |
[MTMatrix] |
<Zughy> - Rename "Supported by core dev" into "Concept approved" |
| 20:12 |
[MTMatrix] |
<Zughy> - Create a new "Supported by core dev" label to stick onto PRs that had it before the renaming |
| 20:12 |
[MTMatrix] |
<Zughy> - 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] |
<Zughy> Am I right? |
| 21:01 |
[MTMatrix] |
<Zughy> ...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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> for a variant I've temporarily changed "@ Startup / Config / Util" if you wanna have a look |
| 22:55 |
[MTMatrix] |
<Zughy> but I think it's too much |
| 22:57 |
Desour |
don't need to necessarily see it ¯\_(ツ)_/¯ |
| 23:10 |
[MTMatrix] |
<Zughy> 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] |
<Zughy> 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) |
| 23:32 |
|
panwolfram joined #minetest-dev |
