Time Nick Message 09:24 nrz sfan5, thanks for the review first. I fixed all points pointed by everyone, it make things clearly better, and you catched nice points 🙂 #12885 is now ready for a merge if you can take a last look 🙂 09:24 ShadowBot https://github.com/minetest/minetest/issues/12885 -- [NOSQUASH] Reduce exposure of various internals by nerzhul 17:35 Krock meeting in 25 minutes 18:00 Krock nrz jwmhjwmh rather quick meeting today 18:01 Krock > #12772 18:01 ShadowBot https://github.com/minetest/minetest/issues/12772 -- Add `vector()` constructor by TurkeyMcMac 18:02 Krock unfortunately Zughy does not seem to be available for this meeting 18:03 sfan5 I'm here too 18:03 Krock great 18:04 Krock so apparently the topic is to decide whether or not we want to add this constructor? 18:04 jwmhjwmh Yes. 18:04 Krock I have my concerns, because vectors are loosely defined; many times they are a simple table without a metatable 18:06 Krock so what's been used are helper functions, without any class association 18:06 Krock what's the goal in the future? to replace the old vector notation, or is it just about adding another shorthand command? 18:07 jwmhjwmh I think it's good to have a declarative way to create vectors (vector.new has an imperative vibe IMO.) 18:08 jwmhjwmh It's more consistent with stuff like ItemStack, though the constructor isn't overloaded. 18:08 jwmhjwmh Like an item stack, a vector is conceptually a class of object. 18:08 sfan5 adding that sounds fine to me 18:08 Krock then why not make it equivalent to vector.new(), accepting other parameters too? 18:09 jwmhjwmh Overloading it could be a good idea, although that functionality of vector.new is deprecated. 18:09 Krock the three parameters x/y/z seems to be an artificial restriction where it could be as well made general-purpose like ItemStack where you can either pass strings or another itemstack ot copy 18:09 jwmhjwmh That sounds good to me. 18:11 sfan5 the constructors were split on purpose 18:11 Krock also a version without arguments. that would then replace all of the other functions 18:11 Krock what was the intend of splitting it? 18:12 sfan5 performance, I think 18:12 sfan5 Desour will more and/or check the relevant PR(s) 18:12 sfan5 know more* 18:13 Desour performance, and more importantly readability. vector.new() is less readable than vector.zero() 18:13 Desour iirc 18:13 Krock at least for my part, I'd prefer slightly slower multi-purpose functions over cluttered API 18:13 Desour performance isn't the main goal of the vector helpers anyway 18:13 jwmhjwmh In LuaJIT traces overloading the function is basically zero cost, too. 18:14 jwmhjwmh IMO vector() with no arguments would be a confusing constructor. 18:14 Krock is ItemStack() with no arguments confusing? 18:15 Desour Krock: is ItemStack() valid? if yes, what does it do??? 18:15 Krock empty stack 18:15 Desour ah, those exist 18:15 Krock well I guess for vectors it's less obvious 18:15 jwmhjwmh I guess it could be supported for consistency with C++. 18:16 jwmhjwmh And 0 is "the default". 18:16 Desour modders don't need consistency with c++ 18:16 Krock right 18:17 pgimeno I'm quite annoyed with vectors being a hash rather than an array, but that's not changeable now 18:17 Desour does ItemStack(string) work btw? 18:17 Krock pgimeno: which hash? 18:17 Krock Desour: sure 18:18 Krock that's 99% of the purposes for ItemStack, in first place. 18:18 Desour and if yes, would we also want vector(string) ? (imo not nice api) 18:18 pgimeno Krock: {x = 1, y = 2, z = 3} rather than {1, 2, 3} 18:18 jwmhjwmh Perhaps vector() should be supported for consistency with ItemStack(). 18:18 jwmhjwmh I think vector(string) is too much overloading. 18:18 Krock pgimeno: with the new metatable format, both should be accessible, to my knowlege 18:19 Desour v[num] is slow though 18:19 Krock it also improves readability when you index by x/y/z in your code, rather than messing with indices 18:19 pgimeno it seems I've missed a lot in the last few months then 18:20 Krock jwmhjwmh: I'd suggest to alias vector() to vector.new(), after all it's a constructor, and new() is basically the constructor 18:20 Krock so these two doing the same would make sense to me 18:20 Desour jwmhjwmh: >In LuaJIT traces overloading the function is basically zero cost, too. do you have a source for that claim? I'd like to know more details 18:21 jwmhjwmh Desour: traces specialize to types, and nil/false/true are all separate types. Checking if nil is truthy is a no-op in a trace. 18:21 jwmhjwmh Krock: That would work, although vector.new(v) is deprecated I believe. 18:22 jwmhjwmh I'll try to find a source on LuaJIT traces. 18:22 Desour ah, I see thx 18:23 jwmhjwmh Sort of covers it: https://en.wikipedia.org/wiki/LuaJIT#Tracing 18:27 jwmhjwmh So, final thoughts on adding vector(v) and vector() constructors? 18:27 jwmhjwmh And on the vector(x, y, z) constructor. 18:27 sfan5 don't mind it either ¯\_(ツ)_/¯ 18:28 Krock I don't mind the zero argument constructor, but would prefer to keep consistency with vector.new 18:28 Krock not like that it would matter too much 18:28 jwmhjwmh Should the overloaded functionality of vector.new be un-deprecated? 18:28 Krock the API is already huge so there's not much impact 18:29 Krock I'd be in favour of un-deprecating, but idk about others 18:33 jwmhjwmh Anything for "One Approval" or "Roadmap: Needs Approval" PRs? 18:35 Krock hmm 18:39 Krock why does #12757 have the roadmap approval label? 18:39 ShadowBot https://github.com/minetest/minetest/issues/12757 -- Add dynamic media without loading from disk by GoodClover 18:39 Krock sfan5: what do you think about this PR? 18:40 sfan5 rasonable 18:40 sfan5 +e 18:41 Krock yes, I think so too. thanks. I'll then remove the label 18:42 Krock although I think it's rather a niche-case 18:45 Krock and #12749 is being reviewed even though it's still pending for approval 18:45 ShadowBot https://github.com/minetest/minetest/issues/12749 -- Move code style guidelines into repo by Desour 18:45 Krock so I assume the label was just forgotten? 18:47 Desour could I get roadmap approval for this great feature: #12720 18:47 ShadowBot https://github.com/minetest/minetest/issues/12720 -- [nosquash] Add a worlds_here.txt file in the worlds folder and update .gitignore by Desour 18:49 Krock hmm I see. the worlds/ directory was not generated 18:50 Krock I don't like the wording but it seems useful, thus removing the label. 18:51 Desour thx 18:56 Krock so if there's nothing more, we can end this meeting earlier then usual. thanks for attending. 18:58 sfan5 sure 18:59 sfan5 one thing: it'd be good if someone could review #12844 so we can get it in, it's quite big 18:59 ShadowBot https://github.com/minetest/minetest/issues/12844 -- DevTest: Remove `experimental`, major refactor by Wuzzy2 19:01 nrz sorry team i was with children, this schedule is not so compatible with me 😒 19:01 nrz i have nothing special to add, all sounds said 🙂 19:02 nrz i just want to continue working on the refactor to help the code to be more maintenable and possibily performant, i have no special feature for now to propose 19:02 nrz (maybe some SAO new lua calls at a point, but not ready yet) 19:04 Krock sfan5: I had it opened a few times and the size is indeed scary. seems like a short playtest should be fine for this. it's devtest after all... 19:08 sfan5 yes 19:09 Krock is this to quash or not? 19:10 Krock the commits are very clean. would be a shame to squash 19:48 sfan5 merging #12844, #12889, #12891 in 10m 19:48 ShadowBot https://github.com/minetest/minetest/issues/12844 -- DevTest: Remove `experimental`, major refactor by Wuzzy2 19:48 ShadowBot https://github.com/minetest/minetest/issues/12889 -- Check `sizeof(int)` and `sizeof(size_t)` by TurkeyMcMac 19:48 ShadowBot https://github.com/minetest/minetest/issues/12891 -- Fix some outdated stuff about falling node docs by Wuzzy2 19:59 proller and this plz https://github.com/minetest/minetest/pull/12142 20:21 sfan5 https://i.imgur.com/Vykuorz.png haha no 20:27 Krock is there currently someone paying for the pipelines? 20:27 sfan5 I don't think so 20:29 sfan5 looking at usage quotas there's a huge spike of what gitalb appears to be consider "CI minutes": 90 in Sept, 1400 in Oct so far despite the "Shared runner duration" remaining mostly constant 20:31 sfan5 if I had to guess I'd say gitlab changes how they count CI use 20:31 sfan5 changed 20:31 sfan5 *