| Time |
Nick |
Message |
| 00:06 |
|
AnotherBrick joined #minetest-dev |
| 00:33 |
|
Taoki joined #minetest-dev |
| 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 |
| 00:55 |
|
AcidNinjaFWHR joined #minetest-dev |
| 01:16 |
|
Tmanyo joined #minetest-dev |
| 01:24 |
|
Taoki joined #minetest-dev |
| 01:25 |
|
froike joined #minetest-dev |
| 01:26 |
|
ElectronLibre joined #minetest-dev |
| 01:34 |
|
BrandonReese joined #minetest-dev |
| 01:53 |
|
Grandolf joined #minetest-dev |
| 01:53 |
|
halt_ joined #minetest-dev |
| 01:54 |
|
Grandolf joined #minetest-dev |
| 01:54 |
|
halt_ joined #minetest-dev |
| 02:04 |
|
torgdor joined #minetest-dev |
| 02:13 |
|
hiradur joined #minetest-dev |
| 02:15 |
|
turtleman joined #minetest-dev |
| 02:28 |
|
Grandolf joined #minetest-dev |
| 02:50 |
|
Zeno` joined #minetest-dev |
| 03:08 |
|
halt_ joined #minetest-dev |
| 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 |
| 04:44 |
|
garywhite joined #minetest-dev |
| 05:08 |
|
Hunterz joined #minetest-dev |
| 05:18 |
|
nerzhul_ joined #minetest-dev |
| 05:46 |
|
paramat joined #minetest-dev |
| 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 |
| 06:18 |
|
jin_xi joined #minetest-dev |
| 06:47 |
|
red-001 joined #minetest-dev |
| 06:59 |
|
paramat left #minetest-dev |
| 07:18 |
|
Foghrye4_ joined #minetest-dev |
| 07:41 |
|
werwerwer joined #minetest-dev |
| 08:49 |
Zeno` |
closed #4530 |
| 08:49 |
ShadowBot |
https://github.com/minetest/minetest/issues/4530 -- Fix superflous shader setting updates by gregorycu |
| 09:12 |
|
troller joined #minetest-dev |
| 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:50 |
|
est31 joined #minetest-dev |
| 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 |
| 09:58 |
|
blaze joined #minetest-dev |
| 10:02 |
|
Fritigern joined #minetest-dev |
| 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:14 |
|
AcidNinjaFWHR joined #minetest-dev |
| 11:15 |
Zeno` |
common sense would say that b == 42, but *shrug*... that's not defined |
| 12:08 |
|
Fixer joined #minetest-dev |
| 12:21 |
|
proller__ joined #minetest-dev |
| 12:43 |
Fixer |
https://github.com/minetest/minetest/issues/4335#issuecomment-256335199 |
| 12:55 |
|
red-001 joined #minetest-dev |
| 13:35 |
|
garywhite joined #minetest-dev |
| 13:45 |
|
STHGOM joined #minetest-dev |
| 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 |
| 14:51 |
|
est31 joined #minetest-dev |
| 15:17 |
|
Fixer joined #minetest-dev |
| 15:17 |
|
hmmmm joined #minetest-dev |
| 15:22 |
est31 |
kahrl really knows his C++ |
| 15:22 |
est31 |
kudos |
| 15:39 |
|
Fixer_ joined #minetest-dev |
| 16:09 |
|
blaze joined #minetest-dev |
| 16:10 |
|
Hunterz joined #minetest-dev |
| 16:34 |
|
jin_xi joined #minetest-dev |
| 16:44 |
|
Foghrye4__ joined #minetest-dev |
| 17:01 |
|
AcidNinjaFWHR joined #minetest-dev |
| 17:02 |
|
Krock joined #minetest-dev |
| 17:02 |
|
Krock joined #minetest-dev |
| 17:38 |
|
proller__ joined #minetest-dev |
| 17:58 |
|
nrzkt joined #minetest-dev |
| 18:08 |
|
torgdor joined #minetest-dev |
| 18:27 |
|
est31 joined #minetest-dev |
| 19:32 |
|
torgdor joined #minetest-dev |
| 19:46 |
|
Anchakor_ joined #minetest-dev |
| 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:04 |
|
proller joined #minetest-dev |
| 20:10 |
|
indriApollo joined #minetest-dev |
| 20:13 |
|
Tmanyo joined #minetest-dev |
| 20:13 |
red-001 |
Its not as if they would lose a paying customer |
| 20:14 |
|
proller joined #minetest-dev |
| 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:27 |
|
Jellonator joined #minetest-dev |
| 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:40 |
|
Person joined #minetest-dev |
| 20:43 |
|
DI3HARD139 joined #minetest-dev |
| 20:57 |
|
torgdor joined #minetest-dev |
| 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? |
| 21:11 |
|
proller joined #minetest-dev |
| 21:43 |
|
AcidNinjaFWHR joined #minetest-dev |
| 21:49 |
|
twoelk joined #minetest-dev |
| 21:52 |
|
kaeza joined #minetest-dev |
| 22:16 |
|
Zeno` joined #minetest-dev |
| 22:45 |
|
proller joined #minetest-dev |
| 22:48 |
|
Hunterz joined #minetest-dev |
| 22:49 |
|
ElectronLibre joined #minetest-dev |
| 22:53 |
|
troller joined #minetest-dev |
| 22:56 |
|
ssieb joined #minetest-dev |
| 23:16 |
|
Zeno` joined #minetest-dev |
| 23:24 |
|
Tmanyo joined #minetest-dev |
| 23:24 |
|
twoelk|2 joined #minetest-dev |
| 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 |
| 23:57 |
|
paramat joined #minetest-dev |
