Time  Nick      Message
00:52 paramat   est31 sofar how about we close #3623 ? the commits will still be preserved and findable if someone wants to take over
00:52 ShadowBot https://github.com/minetest/minetest/issues/3623 -- Postprocessing (WIP) by RealBadAngel
03:11 paramat   Zeno` is it possible you could check #4530 for memory leaks? btw if it makes you feel better i doubt gcu will be working on this anymore
03:11 ShadowBot https://github.com/minetest/minetest/issues/4530 -- Fix superflous shader setting updates by gregorycu
03:15 paramat   hm i can't find the comment about a possible memory leak, this seems fairly close to mergeable
03:15 Zeno`     The comment was made my hmmmm 29 days ago
03:16 Zeno`     new ShaderCallback(setter_factories),
03:16 paramat   ah that
03:16 paramat   yeah as sfan5 commented
03:21 Zeno`     well, I'll run it
03:21 Zeno`     I dunno why gcu doesn't answer questions though :/
03:22 paramat   yeah frustrating. we're finding it difficult to find anyone who can work on it
03:23 Zeno`     well, I personally don't want to touch it with a 50km long pole
03:23 Zeno`     seeing what valgrind says about it
03:25 Zeno`     might take awhile... I should have generated the world before running valgrind lol
03:26 Zeno`     I'm gonna abort and try with an existing world, like I should have in the first place :D
03:26 paramat   heh ok i'll continue to bug hmmmmm about it
03:27 Zeno`     geez
03:27 Zeno`     even without the world generated I get heaps of shader errors
03:27 Zeno`     if (std::equal(m_sent, m_sent + count, value) && has_been_set) <--- depends on unitialised value(s)
03:27 Zeno`     about 20 of them
03:29 Zeno`     new ShaderCallback(setter_factories),   <--- definately lost memory (3 of those)
03:29 Zeno`     oops 4
03:29 Zeno`     actually when was the last time anyone checked for leaks etc?
03:29 Zeno`     but most of them are shader anyway
03:29 paramat   for this PR? possibly never
03:29 Zeno`     let me see if they're there before the PR
03:30 paramat   ah
03:32 Zeno`     I'm more worried about the unitialized values then the memory "leak"
03:32 Zeno`     (valgrind still running... I'll be back in a few)
03:34 Zeno`     there are leaks before the PR
03:34 Zeno`     but different ones lol
03:34 Zeno`     but before the PR there is none of the  depends on unitialised value(s)
03:34 paramat   =/
03:35 Zeno`     so those unitialized things need to be fixed
03:35 Zeno`     I'll generate a proper report
03:35 paramat   thanks, this is useful
03:39 Zeno`     the PR introduces 1 *new* memory leak that wasn't there before
03:39 Zeno`     the others might actually be a bug in irrlicht, but the new one isn't (the one that hmmmm pointed out)
03:40 hmmmm     well first of all
03:40 hmmmm     i think this entire factory setup is confusing and i just don't understand the need for it
03:40 Zeno`     hmmmm, that's why I can't work out where the unit var. is coming from without looking at the code... it's confusing
03:40 Zeno`     normally it's "yeah, it was declared he and used there", but this is all over the place
03:41 hmmmm     i need to spend a bit of time on trying to understand the interfaces irrlicht provides for us to work with, understand the problem this PR is solving, how it's been solved, and what a better way to do it is
03:41 hmmmm     so
03:41 hmmmm     i love the fact that this PR boosts performance for quite a few people a significant amount
03:41 hmmmm     but this code is frankly crap
03:41 Zeno`     the backtrace isn't much help either because it goes through the factory through a template through a... I dunno what it's doing lol
03:42 hmmmm     i'm absolutely certain there's a cleaner way to do this without memory leaks, so that it's understandable
03:42 hmmmm     i really, really hate to say this because there's a huge benefit but
03:42 hmmmm     i code review -1 that PR for the mentioned reason
03:42 hmmmm     the structure of it is just a total mess
03:42 hmmmm     i would recommend re-solving the problem with an entirely new PR
03:42 est31     can the uninit thing be put on the github log of the PR as well?
03:43 est31     <Zeno`> if (std::equal(m_sent, m_sent + count, value) && has_been_set) <--- depends on unitialised value(s)
03:43 est31     <Zeno`> about 20 of them
03:43 Zeno`     est31, yeah I'm getting a proper report now for them
03:43 hmmmm     so
03:43 hmmmm     what do we say
03:43 Zeno`     it's shader.h line 98 btew
03:43 hmmmm     re-write the effect of the PR?
03:43 hmmmm     or try to fix it and merge that
03:43 hmmmm     we'll be stuck with code we're not happy with
03:44 paramat   higher quality is worth a wait
03:45 paramat   perhaps the 0.4.15 milestone can be removed
03:50 Zeno`     well I remove my approval from it at least heh
03:52 Zeno`     hmmmm, well I've never looked much at the shader code but I just don't understand this new "style"
03:52 Zeno`     it seems more complicated than it should be to me
03:52 Zeno`     I dunno
03:52 hmmmm     it's way more complicated than it should be
03:52 hmmmm     you see that double pointer to Sky?
03:52 hmmmm     I literally can't see where Sky is being modified
03:52 Zeno`     the one that he doesn't know why it's a double pointer?
03:53 hmmmm     but then again, i haven't really dug into the code a significant amount
03:53 hmmmm     i feel like you shouldn't have to fully immerse yourself in the code in order to understand it
03:54 hmmmm     if code requires you to put that much effort into understanding what it does and how it works, i think the developer completely failed at an important aspect of writing software
03:54 hmmmm     not to mention that we've already confirmed it's hiding several memory leaks
03:54 hmmmm     this makes it much more difficult to review and more error prone
03:55 Zeno`     the backtrace for those unitialized vars leading to shader.h line 98 is obscene lol
03:55 hmmmm     so yeah
03:55 hmmmm     we'll dump it and start a new one
03:55 Zeno`     I added it to the PR
03:56 Zeno`     the entire atrocity that I am having trouble parsing properly in the source code
03:56 hmmmm     our ultimate solution for reducing shader variable setting will use the solution this PR proposes, but implemented in its own way
03:56 Zeno`     those are *all* from this PR btw. The memory leaks are a different matter...
03:59 Zeno`     Are the benefits as huge as they're made out to be?
04:00 Zeno`     I might profile it if my headache goes away
04:00 Zeno`     but yeah I think it might be better started from scratch in any case
04:01 Zeno`     I don't really know what's going on... I can't keep up with where I am in the new code
04:01 Zeno`     'cause of the template and factory "pattern"
04:01 Zeno`     it seems to jump back and forth... *sigh*
04:01 hmmmm     yeah it really doesn't help
04:02 hmmmm     the jumping back and forth is the bane of my existence
04:02 hmmmm     it reminds me of java code
04:07 Zeno`     the sky pointer (where it's pointing to) isn't changed in C++
04:08 Zeno`     even if it was something Lua related there'd have to be something in C++ to actually make the change and it doesn't exist
04:09 Zeno`     weird
04:11 Zeno`     hang on
04:12 Zeno`     gcu changed it to a pointer to a pointer
04:12 Zeno`     and he doesn't know why?
04:12 Zeno`     that seems a bit strange
04:13 paramat   that may have been ShadowNinja in #3770 "with some changes like replacing SkyBackgroundColorProvider with a pointer to a pointer"
04:13 ShadowBot https://github.com/minetest/minetest/issues/3770 -- Fix superflous shader setting updates by ShadowNinja
04:13 hmmmm     look if that's not a sign that says "abandon all hope ye who enter here" then i don't know what is
04:13 Zeno`     it's used in exactly the same 3 places but as a ** not a *
04:14 Zeno`     paramat, is 3770 in master?
04:14 Zeno`     oh I see
04:14 Zeno`     it's a modified version of 3770
04:14 paramat   yes
04:15 Zeno`     I wonder why he did that
04:15 Zeno`     close it!
04:17 Zeno`     I have abandoned all hope and am exiting there hehe
04:18 sofar     paramat: I don't mind closing old PRs like that, but maybe label them "not merged" so that they're easy to find later on?
04:18 sofar     or some other label that is easily usable
04:19 Zeno`     i meant close 4530 as hmmmm suggested
04:19 Zeno`     but yeah I agree
04:20 paramat   for #3623
04:20 ShadowBot https://github.com/minetest/minetest/issues/3623 -- Postprocessing (WIP) by RealBadAngel
04:21 Zeno`     i see why it's done btw
04:22 Zeno`     it's because the sky background colour can be changed before sky exists... but I just fixed it (without using a **) by adding an if (m_sky) { } around the offending code
04:23 Zeno`     which is a whole lot clearer IMO than having a ** to deal with something that happens once during program init
04:23 paramat   well i'm in no rush to close 3623
04:24 hmmmm     Zeno`:  ha good on you, but is knowing that going to fix the template shit and the factory shit?
04:26 hmmmm     you know what, lately i've been -1ing more PRs than +1ing
04:26 Zeno`     hmmmm, no. But I now know that the sky** is a kludge :)
04:26 hmmmm     maybe i'm too negative of a person, but maybe a lot of these PRs aren't really that great
04:27 Zeno`     it's called once
04:28 Zeno`     why would you solve the problem by using a ** heh
04:28 Zeno`     oh well
04:28 * Zeno`   puts it on his list of weird things he's seen today
04:28 * Zeno`   gets out of the branch now and deletes it from his local repo
04:33 Zeno`     delayed server shutdown
04:34 Zeno`     ok, -1 on that idea :)
04:38 Zeno`     closed #3354
04:38 ShadowBot https://github.com/minetest/minetest/issues/3354 -- Add ring buffer by dandelion-james
04:38 Zeno`     it doesn't do anything
04:39 sofar     was there even unit tests in that?
04:39 Zeno`     no
04:39 Zeno`     my comment on closing is "If it's intended "to be used with ProfilerGraph, to replace deque m_log" then that should be done and this change part of that."
04:39 sofar     well that's surely a sign that it doesn't do anything :)
04:39 Zeno`     as that was the author's justification for the PR
04:43 Zeno`     hehe
04:44 Zeno`     well, yeah. Not much point in adding dead code before it even lives
05:58 paramat   discovered i left an unnecessary alpha channel in acacia tree texture, fixing
06:08 paramat   game#1344
06:08 ShadowBot https://github.com/minetest/minetest_game/issues/1344 -- Default: Remove alpha channel from acacia tree textures by paramat
06:10 paramat   will merge in a moment
06:14 paramat   merging
06:17 paramat   complete
08:49 Zeno`     closed #4530
08:49 ShadowBot https://github.com/minetest/minetest/issues/4530 -- Fix superflous shader setting updates by gregorycu
09:49 Zeno`     I'm sick of gcu's accusations against me
09:49 Zeno`     apparently I am trying to sabotage his work (according to his crazed mind)
09:51 Zeno`     I didn't "close" his stupid and shitty PR (apart from closing it)
09:51 Zeno`     it was closed at the point that the "Won't add" label was added
09:51 Zeno`     geez
09:53 Zeno`     and I had nothing to do with that apart from participating in a discussion
09:53 Zeno`     ugh
10:21 Zeno`     someone should add "the result of a comparison is discarded in the case one of the operands is uninitialised" to the topic
10:41 jin_xi    as far as i can tell the result of this comparison is discarded based on wether a bool is set
10:50 Zeno`     no, it's checked
10:50 Zeno`     i.e. accessed
10:50 Zeno`     and the value of that is undefined
10:53 jin_xi    but that value is never used? so this code allows for some undefined behaviour but does not rely on it being anything specific at all
10:53 jin_xi    well, idk. thats what i get from a quick glance
10:54 Zeno`     it's used in a conditional comparison (if)
11:05 jin_xi    i'd say its used in a comparison (&&) the result of which is always well defined due to has_been_set
11:07 Zeno`     no, because it's && both have to be initialised
11:07 Zeno`     even if it's || both *should* be initialised to be safe
11:08 Zeno`     if one of the operands involved in an && is not initisialised then the entire expression is undefined
11:08 jin_xi    but the comparison of random memory just gives non reliable bool, not uninitialized
11:10 Zeno`     no, the result of the expression is undefined because the evaluation of the expression relied on unitialised (random) "memory"
11:10 Zeno`     so the expression could be true, or it could be false. Neither is correct because the result of the && is undefined
11:12 Zeno`     int a = 0, b; int c = a && b;
11:12 Zeno`     the value of c is undefined because 'b' is not initialised
11:12 Zeno`     it might be 0, or it might be 1. Neither is more correct
11:13 Zeno`     and that's what's happening (in essence, but in an if statement) in the PR
11:14 Zeno`     who knows what the value of 'b' is?
11:14 Zeno`     I don't
11:15 Zeno`     common sense would say that b == 42, but *shrug*... that's not defined
12:43 Fixer     https://github.com/minetest/minetest/issues/4335#issuecomment-256335199
14:15 Calinou   https://github.com/minetest/minetest/issues/4335#issuecomment-256360437
14:15 Calinou   ran a benchmark, please review
14:15 Calinou   even on my hardware, I lose 20 FPS with shaders enabled and no special features, 30 FPS more when enabling all features
14:15 Calinou   at this point Terasology runs faster… :P
14:16 Calinou   so there's a lot of optimization work to do, even more so with shaders
14:16 Calinou   Minecraft performs quite better, last time I played it
14:20 sfan5     it depends on the machine how MT & MC perform
14:21 Calinou   sfan5: Minecraft almost universally performs better in my experience, too :/
14:21 Calinou   even on mediocre hardware
14:21 Calinou   they really did optimize it a lot
14:34 Fixer     same for me
14:34 Fixer     MC runs a lot faster and smoother
15:22 est31     kahrl really knows his C++
15:22 est31     kudos
19:58 nrzkt     sfan5, a user reports this in serverlist +> https://lut.im/mWND9f1wXm/7i0EPRLKiTR9U1tJ.png can you do something please ?
20:00 sfan5     well they can name their servers however they want
20:01 sfan5     also individual server banning is not implemented yet
20:01 sfan5     i could only ban the whole of mthosting.com right now
20:01 garywhite You could try emailing minetesthosting
20:02 sfan5     i could
20:02 est31     and if they don't ban it you ban the whole site :)
20:13 red-001   Its not as if they would lose a paying customer
20:15 red-001   I don't think they would mind taking it down
20:16 sfan5     well send them a mail
20:16 sfan5     it's not my responsibility to do so
20:34 garywhite Emailed, he said:
20:34 garywhite https://www.irccloud.com/pastebin/9sAWvDMo/
20:36 Fixer_    i don't see it in serverlist currently
20:36 garywhite The guy in charge of minetesthosting shut it down
20:58 Hijiri    nrzkt: in https://github.com/minetest/minetest/pull/4669#issuecomment-256161297 you meant that making it reliable would cause lots of lag, right?
20:58 Hijiri    the "sorry but i'm wrong" at the beginning confuses me
20:59 Hijiri    I was thinking to instead append the stuff that normally gets sent in a TOSERVER_PLAYERPOS to the end of interact packets, and then use that to set player control stuff
20:59 Hijiri    does that sound good?
23:36 Zeno`     est31, am I using "Won't Add" tag incorrectly?
23:55 est31     Zeno`: ??
23:56 Zeno`     it's just that you removed the two I added yesterday
23:56 Zeno`     so I thought/think I must be using it wrong
23:57 Zeno`     I probably misunderstand what the label is supposed to be used for