Time Nick Message 11:43 sfan5 pushing https://github.com/minetest/minetest/compare/ci once CI runs 11:53 [MatrxMT] 👍 12:14 sfan5 now the test no longer fails. very mysterious. 13:48 cx384 merging #15621 and #15563 in 10 min 13:48 ShadowBot https://github.com/minetest/minetest/issues/15621 -- Rename getMapSettingNoiseParams to getNoiseParams by wrrrzr 13:48 ShadowBot https://github.com/minetest/minetest/issues/15563 -- Main menu server tab search improvements by cx384 14:00 cx384 on no forgot to squash need to force push to master :/ sorry 14:01 sfan5 go ahead 14:09 cx384 ok I removed the commits 14:19 cx384 pushing squashed #15563 in 5 min 14:19 ShadowBot https://github.com/minetest/minetest/issues/15563 -- Main menu server tab search improvements by cx384 14:25 cx384 done, sorry again 14:29 sfan5 don't worry. it happens. 16:49 wrrrzr Hello, i found file in source what can be deleted if move constructor to header. 16:49 wrrrzr Can i send PR with delete this file and move constructor to header 16:59 Krock wrrrzr: question is whether there will be more content in that source file in a few years. I'd rather focus on cleaning up dead code and fix up bad practices. 17:01 wrrrzr Krock: i think not, this file is lighting.cpp 17:13 sfan5 Krock: silencing errors in unit tests doesn't actually seem that useful 17:14 Krock sfan5: well idk. I think we can just unsilence the thing and highlight failures better 17:14 MTDiscord wrrrzr, did you run into a particular problem you're trying to solve with that change? 17:15 Krock I have to admit that it took me about 5 minutes to find out why it's not logged 17:15 wrrrzr josiah_wi: no, just refactoring 17:15 Krock (even though I already knew that there's a caveat) 17:18 MTDiscord Ok. It might be worth looking at the blame to see why that file is so short; it's more important to have everyone on the same page than to have perfect code. 17:18 MTDiscord But I wish you success with getting that merged. 17:18 Krock afiak it used to contain light curves and such which were then moved to light.cpp or so 17:20 Krock nvm no. That's another file. 17:21 wrrrzr Ok, i try to send PR with remove of lighting.cpp 17:22 MTDiscord wrrrzr, before you open the PR, I have some feedback. 17:23 wrrrzr josiah_wi: ?? 17:24 MTDiscord I looked at the file. x2048 created it pretty much as-is as part of a lighting fix. He thought it was worthwhile to make a source file for just the constructor - I knew a C++ expert who also taught that as good practice. 17:25 MTDiscord However, all the constructor does is initialize some floats to default values. It's not necessary to define a constructor at all for that purpose. 17:25 MTDiscord You can do more than remove a file - you can put the default values directly in the struct definitions and remove the constructor completely. 17:25 wrrrzr I think it be better to make constructor in header, and i want to make it constexpr 17:25 MTDiscord Just something you can consider. 17:34 wrrrzr I sent PR 17:36 MTDiscord I'm not going to complain about it, but if you go a little farther and explain why you think this is better than before I'll even approve it (although my approval doesn't count towards unlocking a merge). 17:38 wrrrzr I make it constexpr, i think it little better for perfomance 17:40 MTDiscord Have you done any microbenchmarks? 17:40 MTDiscord (or other performance testing before/after the change) 17:40 wrrrzr No... 17:41 MTDiscord It might be good to explain the intent in the PR. It seems a little random otherwise. I'll go ahead and approve; thank you for explaining. 17:41 wrrrzr I think better make more constexpr things 17:43 MTDiscord Do you know whether the constructor is used anywhere in a context where it would benefit from constexprness? 17:45 sfan5 I think the change is simple and harmless enough to not require must explanation or microbenchmarking 17:46 MTDiscord The change is harmless. I'm encouraging him to think through these things for his own benefit - I am a CS tutor. xD 17:47 MTDiscord And thank you for your contribution, wrrrzr. I approved it, hopefully it gets merged soon. 19:11 jonadab Never make any change for performance reasons without doing benchmarks, unless there's an extreme difference in Big-O value (like, O(log n) vs O(n^2), is a no-brainer if the data set can be large). 19:21 MTDiscord ^ this is a good heuristic 19:22 MTDiscord (longer term) refactoring idea: let's get rid of non-animated meshes (SMesh). they are just a special case of animated meshes with no animation and should be implemented as such. there is no need to duplicate SkinnedMesh logic. SkinnedMesh could be the only mesh class, with everything related to skinning in an optional (or behind a pointer if we don't want to bloat non-animated meshes). 19:24 MTDiscord as for the constexpr constructors, i don't think it matters. our style guidelines should say one thing or the other and then we should roll with it. 19:25 MTDiscord i doubt that there is a performance difference at all. i would bet that these generate the exact same assembly code. the constructor is in the header and will be inlined. 23:59 MTDiscord Agree simplify to one animated mesh type. Also, this is why entity class is a god class in size. Typically in software the one god class allows for ever better optimization.