| Time |
Nick |
Message |
| 00:05 |
|
Eragon joined #minetest-dev |
| 00:13 |
|
SFENCE joined #minetest-dev |
| 00:46 |
|
SFENCE joined #minetest-dev |
| 01:20 |
|
SFENCE joined #minetest-dev |
| 01:43 |
|
SFENCE joined #minetest-dev |
| 02:24 |
|
hwpplayer1 joined #minetest-dev |
| 02:58 |
|
SFENCE joined #minetest-dev |
| 05:00 |
|
MTDiscord joined #minetest-dev |
| 08:09 |
|
hwpplayer1 joined #minetest-dev |
| 11:43 |
sfan5 |
pushing https://github.com/minetest/minetest/compare/ci once CI runs |
| 11:53 |
[MatrxMT] |
<grorp> 👍 |
| 12:14 |
sfan5 |
now the test no longer fails. very mysterious. |
| 13:39 |
|
cx384 joined #minetest-dev |
| 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:48 |
|
wrrrzr joined #minetest-dev |
| 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 |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> wrrrzr, before you open the PR, I have some feedback. |
| 17:23 |
wrrrzr |
josiah_wi: ?? |
| 17:24 |
MTDiscord |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> Just something you can consider. |
| 17:34 |
wrrrzr |
I sent PR |
| 17:36 |
MTDiscord |
<josiah_wi> 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 |
<josiah_wi> Have you done any microbenchmarks? |
| 17:40 |
MTDiscord |
<josiah_wi> (or other performance testing before/after the change) |
| 17:40 |
wrrrzr |
No... |
| 17:40 |
|
fluxionary joined #minetest-dev |
| 17:41 |
MTDiscord |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> And thank you for your contribution, wrrrzr. I approved it, hopefully it gets merged soon. |
| 17:53 |
|
wrrrzr left #minetest-dev |
| 19:11 |
|
Desour joined #minetest-dev |
| 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 |
<luatic> ^ this is a good heuristic |
| 19:22 |
MTDiscord |
<luatic> (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 |
<luatic> 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 |
<luatic> 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. |
| 20:04 |
|
hwpplayer1 joined #minetest-dev |
| 20:12 |
|
Desour_ joined #minetest-dev |
| 23:33 |
|
panwolfram joined #minetest-dev |
| 23:59 |
MTDiscord |
<exe_virus> 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. |
