Time  Nick        Message
01:54 ShadowNinja #4076
01:54 ShadowBot   https://github.com/minetest/minetest/issues/4076 -- Attempt to improve performance by combining 6 vectors into one using a struct by ShadowNinja
01:55 ShadowNinja ^ (just making sure a bug with that command was fixed)
02:03 paramat     cool
03:40 gregorycu   Indeed
03:57 gregorycu   I can't repro this shader issue
04:35 gregorycu   I stumbled across a low hanging fruit optimisation
04:53 paramat     ?
05:04 gregorycu   mapblock_mesh_generate_special
05:04 gregorycu   A lot of time in that function is spent organising the 3 std::maps in there
05:14 vtold       Hi, I have just upgraded my minetest from 13 to 14 and there are issues with polish fonts in menus. There is no national chars.
05:15 vtold       Both in windows and Linux.
05:20 vtold       Any help with this?
05:57 gregorycu   vtold: Not from me, are you able to create a bug (with a screenshot) https://github.com/minetest/minetest/issues/new  ?
05:59 vtold       gregorycu: I'll try after my breakfast :)
05:59 gregorycu   Thank you :)
05:59 vtold       np ;)
06:24 vtold       gregorycu: done (#4152)
06:38 vtold       Thank you for your help.
07:05 paramat     nore sfan5 game#1076 is updated, tested, ready for review :]
07:05 ShadowBot   https://github.com/minetest/minetest_game/issues/1076 -- Fire: Ignite tnt, gunpowder, permanent flame above coalblock by paramat
07:11 paramat     will merge #4134 #4147 in a moment
07:11 ShadowBot   https://github.com/minetest/minetest/issues/4134 -- Item entities: Don't show description as infotext by paramat
07:11 ShadowBot   https://github.com/minetest/minetest/issues/4147 -- update .gitignore: ignore ninja files by HybridDog
07:12 paramat     i'll squash the commits in 4147
07:16 paramat     merging
07:24 hmmmm       what do you think about seed being an int and losing half our enthropy?
07:26 gregorycu   Why?
07:30 paramat     merge complete
07:36 hmmmm       paramat??
07:37 hmmmm       I think we should quit fooling people and just change seed to an s32
07:37 paramat     hang on need to websearch enthropy heh
07:37 hmmmm       the high half of their seed is getting completely ignored, and if we do change noise to work on 64 bit values, then literally everybody's map would change
07:38 hmmmm       so i mean what's the point
07:38 nrzkt       ShadowNinja, i fixed the issues you mentionned, except the type
07:38 hmmmm       we can't break everybody's compatibility so they can have 2^64 - 2^32 more possibilities for maps
07:38 nrzkt       hmmmm, #4124 is okay now
07:38 ShadowBot   https://github.com/minetest/minetest/issues/4124 -- Implement a PostgreSQL backend by nerzhul
07:39 hmmmm       nrzkt, when I say things like, "the lines have to be under 90 chars"
07:39 hmmmm       I mean all instances of the lines in your PR
07:39 nrzkt       i missed some lines ? will look
07:40 hmmmm       database-postgresql.cpp:91
07:40 nore        hmmmm: isn't it possible to use the current way for old maps, and a better way for new ones?
07:40 hmmmm       nore, you can, i guess..
07:40 hmmmm       that's really messy because now there has to be a second set of noise functions
07:41 paramat     i disagree with these 'preparations for a larger world'
07:43 vtold       paramat: I saw, you added translation label to #4152. Isn't a font issue?
07:43 nrzkt       hmmmm, fixed the mentionned + others
07:43 ShadowBot   https://github.com/minetest/minetest/issues/4152 -- Wrong (empty) polish national chars in menus
07:43 hmmmm       nrzkt, I am still doing a final look over
07:43 hmmmm       hold up
07:43 paramat     yeah not really translations
07:45 paramat     a larger world probably won't happen so why start making everything more complex from now on 'just in case' :P
07:46 nore        paramat: It's actually *less* complex
07:46 nrzkt       sfan5, i need to look at the debian issue, seems it doesn't find the libpg-fe.h
07:47 nore        And I think that everything should be done to prepare for it (like, change v3s16 when possible, even if it stays a v3s16 for now, but so that in the types show what is a position and what isn't)
07:47 nrzkt       the map border is easily walkable yes
07:49 paramat     the preparation itself is a headache, for something unlikely and unnecessary
07:49 nore        I am ready to do the conversion of all v3s16 to v3pos (or whatever the name is)
07:49 nore        even if v3pos stays v3s16 internally
07:50 hmmmm       oh please no
07:50 nrzkt       paramat, postgresql handle the int4 as better as int2. 6 bytes overhead is ridiculous compared to the mapblock size
07:50 nore        just so that it will be possible to change that later
07:50 hmmmm       please don't break everybody's PR with huge merge conflicts because you decided on pulling a ShadowNinja giant ctrl+H replace operation
07:50 nrzkt       hmmmm, xD
07:50 nore        hmmmm: that was why I was hesitant on doing it
07:50 hmmmm       in any case if people would stop making pull requests then we'd be actually able to do larger changes
07:51 nore        but then, I'm thinking right now is not a bad time: only ~100 PRs open
07:51 nore        and it's actually not that costly to rebase a PR
07:51 hmmmm       nore, also consider than v3s16 is 8 bytes, whereas if you were to make it larger, there would be a significant amount of overhead added to everything
07:51 hmmmm       well whatever
07:51 hmmmm       i would -1 it if you did
07:52 hmmmm       right now is not the time and place for such a change
07:52 nore        hmmmm: I'm saying, just converting v3s16 to v3pos where they are a position
07:52 paramat     nrzkt ok, best just do what's best ignoring the 'larger world' issue
07:52 nore        and keep v3s16=v3pos internally
07:52 nore        anyway, you're right about this
07:52 nore        it's not for right now
07:53 hmmmm       i'd like to do some work on getting PRs merged
07:53 hmmmm       a lot of them
07:53 * nore      agrees
07:53 nrzkt       nore, i think many PR should be closed/merged
07:53 hmmmm       right now it's at 141 PRs which is not "only" a small amount
07:53 hmmmm       we've NEVER had such a high number of open PRs
07:53 hmmmm       it's unprecedented and has been exacerbated by the long delayed release
07:53 hmmmm       but this is no excuse to start merging things without proper review
07:54 hmmmm       just more focus needs to be put toward clearing the PR backlog out
07:54 nore        #817 should be merged but needs rebase
07:54 ShadowBot   https://github.com/minetest/minetest/issues/817 -- Add slippery by Zeg9
07:54 nore        #1118 is still not ready
07:54 ShadowBot   https://github.com/minetest/minetest/issues/1118 -- Meta set nodedef by Ekdohibs
07:55 nore        #1188 should be too but needs rebase
07:55 ShadowBot   https://github.com/minetest/minetest/issues/1188 -- getTime refactoring by Selat
07:55 hmmmm       no 817 'should' not be merged
07:55 hmmmm       there are questionable design choices that need to be addressed
07:56 nrzkt       paramat, why do you remove the milestone flag ? without any comment. It's just bad, you are just doing things like you want without justifying anything it's stupid
07:56 nore        hmmmm: I did not see any issues in comments?
07:56 hmmmm       so that means it must be good to go?
07:56 nore        what about #1489?
07:56 ShadowBot   https://github.com/minetest/minetest/issues/1489 -- Timed move by sapier
07:56 hmmmm       or more like nobody had time to look at it
07:57 hmmmm       i don't understand the problem that 1489 solves yet
07:57 hmmmm       i can't approve it
07:57 hmmmm       okay new rule:
07:57 hmmmm       if you are giving a +1 approval you MUST be able to articulate:
07:57 hmmmm       - the problem it solves
07:57 hmmmm       - how the PR solves the problem
07:58 nrzkt       hmmmm, you cannot add new rule without the agreement of other people.
07:58 hmmmm       nrzkt, do you disagree with this proposal?
07:58 nrzkt       but yes PR should have a description, it's the minimum
07:58 nore        anyway: 1489 reduces perceived lag by allowing mods to say what will happen to entities, where they will stop moving and what they will do next
07:58 hmmmm       that's not what I said
07:59 nrzkt       but is this not in the contribution guidelines ?
07:59 hmmmm       nore, I need to actually look at it
07:59 nrzkt       i don't see why each coredev should say the same thing on a PR
07:59 hmmmm       you can't just copy paste a link in the chat and expect me to say "yes sure merge right away!"
07:59 nrzkt       yes, but you are not the only core dev
08:00 nrzkt       you can open your browser and look at the pr
08:00 hmmmm       and it takes longer than 5 seconds
08:00 nrzkt       page titles are there for this
08:00 hmmmm       nrzkt, you're especially guilty of this
08:00 nrzkt       yes , and we are not an entreprise where 5 seconds are precious
08:00 hmmmm       not to be inflammatory or anything but i noticed you give approvals without looking close enough at a PR
08:01 nrzkt       when the PR is trivial or fixes with a way i think good yes
08:01 hmmmm       nore:  I'll do reviews on each of those 4 you linked here, but I need to finish up with nrzkt's postgresql thing first
08:02 hmmmm       so hold on
08:02 nore        hmmmm: no need to review 1118
08:02 Krock       nrzkt, it's important that the stuff works. Fixing it afterwards isn't wanted
08:02 paramat     i thought the 0.4.15 milestone was best cleared at release as it was being used to flag PRs to be merged after release instead of before
08:02 Krock       so looking at the pull's functionality is important
08:03 nore        817 and 1188 need rebase also :/
08:03 nrzkt       Krock, i didn't said i didn't look at the pull functionnality
08:03 nrzkt       paramat, ok
08:04 Krock       nore, 1188 could be merged soon. Selat was active a few days ago
08:04 nore        Krock, nice
08:08 hmmmm       nrzkt, have you run your postgresql branch in valgrind?
08:08 hmmmm       just wondering
08:08 paramat     anyone agree that #1685 should be closed?
08:08 ShadowBot   https://github.com/minetest/minetest/issues/1685 -- Digging animations tweaks. by RealBadAngel
08:09 hmmmm       no
08:11 vtold       apropos #4152 : is it a good idea to replace fonts from 0.4.13?
08:11 ShadowBot   https://github.com/minetest/minetest/issues/4152 -- Wrong (empty) polish national chars in menus
08:11 vtold       I have both versions in windows
08:12 Krock       1685 shouldn't be closed, paramat. But the hardcoded values aren't good, so this would require some changes in the commit
08:12 nrzkt       sfan5, i'm working on the travis problems
08:13 hmmmm       nrzkt I'm pretty close to approving your PR, but I need to just take a closer look at the checkResults 'clear' usage
08:14 hmmmm       it seems like a very easy way to get a memory leak
08:15 paramat     anyway, soon i will try to flag some old/unpopular PRs and issues that can possibly be cleared out
08:17 nrzkt       hmmmm, the PQclear call is there for that :)
08:17 hmmmm       nrzkt, I know how it's supposed to work it's just that it's not immediately obvious that it's done correctly
08:17 hmmmm       and it's not exactly your fault
08:17 hmmmm       I think the postgresql wrapper library has a poorly designed interface
08:18 nrzkt       hmmmm, in fact it works, because my server used this method since 1 year and the memory is very stable whereas i'm fetching more datas than just the mapblocks :p
08:18 nrzkt       (and the server uptime is > 5 days)
08:18 hmmmm       not concerned about how long something has been running for
08:18 nrzkt       valgrind doesn't report anything about libpq
08:18 hmmmm       that's actually a horrible way to judge stability of code
08:19 hmmmm       also, no unit tests?
08:19 nrzkt       atm no, like every backend in fact
08:20 nrzkt       we need a postgresql server and configure it properly to test the code
08:20 nrzkt       it's not rightly applicable
08:21 nore        oh, about combining positions in a database: #1845 fixes the problem for sqlite3 :)
08:21 ShadowBot   https://github.com/minetest/minetest/issues/1845 -- Split block position in SQLite3 database by ShadowNinja
08:22 hmmmm       nrzkt, just because everybody else jumps off a cliff doesn't mean you should too
08:22 gregorycu   Hi there all
08:22 nrzkt       hmmmm, i cannot cover it because it need to install pg server, configure a database, a client, start it.
08:22 hmmmm       oh, true, fair enough
08:22 hmmmm       that is a pretty big pain in the butt
08:23 hmmmm       still though it should be done at some point or another
08:23 hmmmm       but everybody is more concerned with flashy new features than quality
08:23 gregorycu   I am concerned with quality hmmmm
08:23 nrzkt       i don't remember the libpq-fe permit to mock the pgsql server
08:23 hmmmm       maintenance is 80% of a software's life
08:24 hmmmm       anyway i checked checkResults and the clear flag
08:24 hmmmm       it looks all good.
08:24 hmmmm       i really just have one complaint with this code
08:24 nrzkt       hmmmm, okay, i'm fixing the last code style issues, and fixing the travis uil
08:24 nrzkt       build
08:25 hmmmm       it follows a horrendous coding-by-exception style
08:25 hmmmm       exceptions everywhere
08:25 hmmmm       this is bad, but excusable because all the other databases do it too
08:25 nrzkt       hmmmm, i don't like it either but i followed the other backend to be consistent
08:25 hmmmm       exceptions are great (well, that's debatable) but they have their place and using them this way is considered to be an anti-pattern
08:26 hmmmm       well
08:26 hmmmm       I have two real nitpicky concerns about the code
08:27 hmmmm       1). the use of atoi()
08:27 hmmmm       atoi is dangerous, what if PGSql messes up on us and starts returning blank strings, or incorrect data that isn't a number?
08:27 hmmmm       we won't know that the numeric conversion actually failed
08:28 hmmmm       2).  i'm concerned about the potential data truncation when saving a block if the size is over 65535 on platforms where int is 16 bit
08:28 nrzkt       hmmmm, because the column is NOT NULL we only fetch INT
08:28 hmmmm       okay
08:29 nrzkt       hmmmm, give me an example platform where MT runs in 16b ?
08:29 hmmmm       so #1 is okay and #2 is something that you personally can't help aside from printing an error if data.size() > INT_MAX
08:29 ShadowBot   https://github.com/minetest/minetest/issues/1 -- GlowStone code by anonymousAwesome
08:29 ShadowBot   https://github.com/minetest/minetest/issues/2 -- Burned wood
08:30 hmmmm       actually okay yeah
08:30 hmmmm       could you print an error to errorstream about it if it's getting truncated?
08:30 nrzkt       ok
08:30 hmmmm       it's not right to blow an exception over this exactly
08:30 nrzkt       i can do a FATAL_ERROR
08:30 hmmmm       no dammit
08:30 hmmmm       wait hold on
08:31 hmmmm       I don't know if MSVC has INT_MAX or not
08:31 nrzkt       i'm waiting, i'm finishing to fix the travis build
08:31 hmmmm       oh whoops no i'm thinking of inttypes.h
08:32 hmmmm       this should be in limits.h
08:32 gregorycu   Did someone say MSVC?
08:33 hmmmm       okay, just add an if (data.size() > INT_MAX) { errorstream << "Database_PostgreSQL::saveBlock:  Data truncation!  data.size() over 0xFFFF (== " << data.size() << ")"; }
08:33 gregorycu   INT_MAX is so last century
08:33 hmmmm       blame libpg
08:33 gregorycu   std::numeric_limits<int>::max() is where it's at
08:34 hmmmm       hmm
08:34 hmmmm       I don't know
08:34 hmmmm       should you try saving the block that you know will be truncated?
08:34 hmmmm       or should you return immediately
08:34 hmmmm       probably return immediately, since it's a zlib encoded stream
08:34 hmmmm       but dear god don't take down the entire server because of this
08:35 hmmmm       gregorycu, I don't know why that is better than INT_MAX
08:35 gregorycu   Templates bro
08:35 gregorycu   generic programming
08:36 hmmmm       oh I see
08:36 hmmmm       so you can do std::numeric_limits<T>::max()
08:36 gregorycu   Yeah
08:36 gregorycu   Obviously not important in this instance
08:36 hmmmm       but if you know what the type is already there's 0 advantage
08:37 gregorycu   Apart from the usual "avoiding defines" advantages
08:37 hmmmm       defines are fantastic
08:37 hmmmm       maybe you just suck
08:37 gregorycu   Maybe I just do
08:37 gregorycu   I just attempted an optimisation that made things so much slower
08:38 gregorycu   So I agree that I suck
08:38 hmmmm       jeez it's 4:38
08:38 hmmmm       i need to go to sleep NOW
08:38 hmmmm       nrzkt:  if you handle the overflow case in saveBlock then I approve
08:39 nrzkt       hmmmm, if i send the mapblock to postgresql with truncation i think we will have a problem when re-reading it
08:39 paramat     nore https://github.com/minetest/minetest/pull/817#issuecomment-220820639
08:39 hmmmm       right so just print an error message and return immediately on truncation
08:39 nrzkt       the data will be truncated and the server will fail to read the mapblock from database
08:39 hmmmm       night
08:40 nrzkt       yes, that i thought
08:40 nrzkt       okay hmmmm i will merge it with you change when i fixed the travis build
08:41 nore        paramat: not much time right now, I guess the PR should stay open with a "Needs rebase" label
08:41 nore        (maybe we should could the number of PRs that are not marked as "Needs rebase")
08:42 paramat     i'll try to find someone to rework it
08:46 * vtold     is rebooting to W10 to check font replacing
08:47 nrzkt       oh dear, the cmake module on debian doesn't care about additional paths
08:48 nrzkt       i need to backport the cmake 3.5 embedded module to MT to make it working
09:03 nrzkt       in fact no... there is an extra package to install on debian/ubuntu...
09:18 nrzkt       cmake is case sensitive in fact, hmmmm wants to capitalize the variables, but capitalize the module variables is a bad idea
09:22 paramat     bumped some old PRs that need attention
09:25 nrzkt       ok the build for #4124 seems to work, yes :), i'm awaiting all builds and i merge
09:25 ShadowBot   https://github.com/minetest/minetest/issues/4124 -- Implement a PostgreSQL backend by nerzhul
09:35 nrzkt       build pass, perfect, i merge
11:32 nrzkt       nore, sfan5 ShadowNinja sofar can you look at the trivial #4154 ? :)
11:32 ShadowBot   https://github.com/minetest/minetest/issues/4154 -- Fix a m_camera not used warning fix pointed by clang by nerzhul
11:32 sfan5       that counts as trivial patch
11:32 sfan5       no need for approvals
11:32 nrzkt       then i merge it :) thanks
11:33 gregorycu   Who remembers when I attempted an optimisation by calling vector::reserve ?
11:33 nrzkt       gregorycu, i remember a thing like that
11:34 gregorycu   As it turns out, it actually can be slower to reserve
11:34 gregorycu   Because if you don't reserve, the runtime will call reserve for you. Except that on most implementations, reserve will work exponentially
11:36 gregorycu   Exponentially means reserving less
11:37 Fixer       hi gregorycu
11:37 gregorycu   Fixer
11:37 gregorycu   Are you well
11:41 Fixer       gregorycu: more or less
11:41 gregorycu   That is good
11:41 gregorycu   I've been trying that shader shit
11:41 gregorycu   Cannot repo
11:42 gregorycu   I have Just Test
11:42 gregorycu   Looks nice bro
11:42 Fixer       this is bad
11:42 gregorycu   Yeah
11:44 Fixer       gregorycu: any way to add more debug info so I can reproduce and give more data?
11:44 gregorycu   No :(
11:44 gregorycu   The problem is there are thousands of draw calls a frame
11:45 gregorycu   I think I have some sort of idea what the problem is, but I can't force it to happen
11:45 gregorycu   Until I can make it happen, I can't verify the fix
11:46 Fixer       verify the fix? you have some new code?
11:46 Fixer       can i test it?
11:47 gregorycu   I haven't done the fix yet
12:14 gregorycu   Fixer
12:14 gregorycu   On Just Test, I've noticed that some nodes flicker underwater
12:14 gregorycu   Is this a known issue?
12:14 gregorycu   Water blocks and torches
12:15 nrzkt       i think adding and abm to drop the torch could do the trick :p
12:16 gregorycu   Classic move
12:18 gregorycu   What is the protection block?
12:27 nrzkt       sfan5, nore ShadowNinja can you look at https://github.com/minetest/minetest/pull/4155 ? :)
12:27 nrzkt       and modders too, it's a PR for you
12:33 gregorycu   Are the attributes namespaced?
12:33 gregorycu   (By mod I mean)
12:35 nrzkt       gregorycu, i think it's the mod goal, except if i have a variable to namespace them directly from C++ (i have a doubt)
12:36 gregorycu   ?
12:36 yang2003    ?
12:38 nrzkt       ?
12:39 yang2003    Gregorycu?
12:39 gregorycu   That was for nrzkt
12:39 yang2003    ok
12:39 nrzkt       gregorycu, i don't know if i should namespace them. Mods can need to share values between them, maybe, and they need to set themselves, like nodes the namespace
12:40 gregorycu   I agree, to an extent
12:40 gregorycu   But I think one mod would still "own" the attribute, right?
12:40 gregorycu   I think it's awesome work, by the way
12:41 nrzkt       gregorycu, if we want that i think it should be applied to the whole namespaces problems, and this can break modpacks i think
12:41 gregorycu   It won't break modpacks, this is a new feature
12:42 gregorycu   Unless I am missing your point?
12:42 nrzkt       i only said if namespace is applied to this change, maybe it could be done on node names too, but that's an idea and this should occur later i think
12:43 gregorycu   Oh, you mean node data?
12:45 nrzkt       default:dirt
12:45 nrzkt       is a node name with a namespace in the string
12:46 gregorycu   Yeah
12:47 gregorycu   but with attributes, you wouldn't say "default:mana"
12:47 gregorycu   You'd just say "mana" right?
12:48 nrzkt       i think mod should set default:mana instead of just mana
12:48 gregorycu   Me too
12:48 nrzkt       i will change the example in the commit message
12:48 gregorycu   My point is that you'd want to make it hard for two mods to accidentally use the same name for two different things
12:48 gregorycu   Without making it impossible for two mods to deliberatly use the same name
12:50 gregorycu   Where is that Fixer bloke when you need him
12:57 Fixer       gregorycu: i'm here
12:58 gregorycu   What shader settings have you got on?
12:59 Fixer       gregorycu: all shaders on, or "tone mapping + waving leaves and plants" on
12:59 Fixer       gregorycu: but I usually play with "tone mapping + waving leaves and plants"
12:59 Fixer       gregorycu: and it happens with this settings too
13:01 gregorycu   Those protection blocks
13:01 gregorycu   The floating ones
13:01 gregorycu   Even when shaders are turned off, some of them are black
13:01 Fixer       gregorycu: totally black?
13:01 Fixer       gregorycu: they are nodeboxes btw, iirc
13:01 gregorycu   Like 95% black
13:02 gregorycu   I can JUST make out some detail on them
13:02 Fixer       gregorycu: nah, they were ok without shaders
13:03 Fixer       need afk for 5-10 min again
13:16 Fixer       back
13:17 gregorycu   Cool
13:17 gregorycu   So, my problem is I don't actually play minetest enough to know what it should look like
13:17 gregorycu   As funny as that sounds
13:18 Fixer       gregorycu: you can post a screenshot for me
13:19 gregorycu   I mean, I can get stuff that looks wrong
13:19 gregorycu   But even without my patch
13:21 gregorycu   I'll get a glass of water than get a few screenshots
13:29 gregorycu   I have returned
13:36 gregorycu   Fixer
13:36 gregorycu   http://www.gregorycurrie.com/is_bug.jpg
13:36 gregorycu   Does this look like the bug?
13:36 Fixer       gregorycu: don't think so, it is just platform that casts shadows
13:37 gregorycu   There is no platform above
13:38 Fixer       gregorycu: it should be, or it was not captured, download mapfix mod and do /mapfix near it to see if it goes away
13:38 gregorycu   How did you tell this wasn't the bug?
13:41 gregorycu   Yeah, mapfix fixed it
13:42 Fixer       gregorycu: i played on that server and I know there was a platform above
13:42 gregorycu   I few upwards heaps
13:52 gregorycu   The protection blocks are a bit of an eyesore ey
13:59 Fixer       gregorycu: especially in water
13:59 gregorycu   There has to be a better solution
14:01 gregorycu   That doesn't involve blocks
15:06 gregorycu   Fixer, do you use opengl or d3d?
15:06 Fixer       gregorycu: opengl of course, d3d is not supported
15:07 gregorycu   there is code referencing d3d
15:07 gregorycu   But I'm also using opengl
15:08 gregorycu   I'm going to do my fix, and create a branch
15:08 gregorycu   I've also rebased
15:08 gregorycu   I really hate suggesting code where I'm not sure it will fix the issue
15:08 Fixer       gregorycu: i can test it, no choice
15:17 gregorycu   I'm never touching shaders again after this :)
15:18 gregorycu   Are we going to consider switching to something like OSG?
15:20 nore        est31: btw, I fixed #4077
15:20 ShadowBot   https://github.com/minetest/minetest/issues/4077 -- Colored chat and other strings by Ekdohibs
15:21 nore        does it look good now?
15:24 gregorycu   Fixer: Not going to be as easy as I thought (when is it ever)
15:24 gregorycu   This is a job for tomorrow
15:24 Fixer       gregorycu: ok
15:25 est31       nore, yes now the option is okay
15:25 nore        Ok, thanks
15:26 nore        is there anything else that should be done with it?
15:27 est31       idk does the ncurses terminal still compile
15:28 nore        It should, but ignoring  escape sequences
15:28 nore        It's terminal_chat_console.cpp, right?
15:28 est31       yes
15:29 nore        I got issues with it, so I think that means I compiled with it
15:35 est31       nore Ive added two comment
15:35 est31       s
15:35 est31       otherwise I have nothing to add, but i didnt do a proper review
15:35 nore        Thanks, looking
15:36 est31       perhaps I should get and test it
15:38 est31       and can you make the history more condensed
15:39 est31       maybe even squash it into one thing, you can list the authors inside the commit message
16:15 nore        est: sure, I'll squash it before merging
16:16 nore        Do you know how to get several authors for one commit btw?
16:16 hmmmm       whoops
16:16 hmmmm       i didn't think you were going to actually use my error message string as-is nerz
16:16 hmmmm       it's not 0xFFFF in general, it's just 0xFFFF in the serious problem case
16:17 hmmmm       on the majority of platforms it'd have to be at least 2 gigs before getting truncated
16:17 hmmmm       but i'm disappointed, a big project like libpg should've had this kind of thing fixed :(  you just don't do sizes of data with a plain 'int' type
16:18 est         nore, thats not possible I think
16:18 nore        Ok, should I merge all the commits in the first one then?
16:19 nore        Anyway, I'll do that before merging
16:20 nore        To avoid having to do a lot of squashs
16:22 hmmmm       https://github.com/minetest/minetest/pull/4077/files#diff-7dce35006f82e6eadf963514f5857a8bR215  ??
16:24 est         well it would be better to have it squashed
16:24 est         atm i cant do git am the patch because it contains trailing whitespaces
16:25 est         so I want to find out whether the trailing whitespaces were fixed already or not
16:25 hmmmm       btw who signed off on this PR
16:26 est         what pr
16:26 hmmmm       colored chat
16:26 est         signed off as in +1 ?
16:27 est         nobody?
16:27 hmmmm       yes
16:27 hmmmm       multiple +1s
16:27 est         you mean the :+1: in the bottom left corner of the initial pr description?
16:27 est         anybody may do it
16:28 hmmmm       no, not that, who approved it
16:28 hmmmm       code review and so on
16:29 est         what makes you think somebody did approve it?
16:29 est         its not merged yet, no?
16:29 hmmmm       people are talking about merging
16:29 Krock       why was the "guitext2->setText" changed? I don't see the profit
16:32 Krock       Also, adding brackets here https://github.com/minetest/minetest/pull/4077/files#diff-3b38895b50e083cbec95bbf7fc335f6cR602 but a few lines below not?
16:32 hmmmm       yeah i think that's silly
16:33 hmmmm       if you're going to start fixing style of other code that isn't being modified, you better do the whole thing
16:33 hmmmm       nothing half-assed half-completed or otherwise what's the point
16:33 hmmmm       and you'd better get the style of your own code changes/additions correct first
16:35 Krock       Right. I make such mistakes too and like to see when people point it out. When you prefer that I should not correct the style in other pulls - fine.
16:37 hmmmm       i need to take a good hard look at EnrichedString
16:37 hmmmm       really i need to take a good hard look at everything though
16:37 hmmmm       this is a lot of code and requires due dilligence
16:45 nore        hmmmm: Zeno approved it
16:46 hmmmm       right but a PR this size requires multiple PRs
16:46 hmmmm       how many exactly is anyone's guess
16:46 hmmmm       but definitely more than 1
17:12 nore        Krock | why was the "guitext2->setText" changed? I don't see the profit <-- where?
17:13 Krock       nore, well, I'm not sure why exactly you decided to move the setText function out of the guitext class: https://github.com/minetest/minetest/pull/4077/files#diff-18513665750ef5adf42b5ec29e14162eL4320
17:13 nore        ah, that
17:14 nore        the problem is that there are two classes
17:14 nore        one that I can change
17:14 nore        and another that is inside Irrlicht
17:14 Krock       oh I see
17:14 nore        to which I can't pass an EnrichedString, nor a string that contains escape sequences
17:15 nore        so what this method does is to check if it is our class
17:15 nore        and if not, convert the string before
17:15 Krock       Ah, that's why. Thanks for the explanation :)
17:17 nore        hmmmm: about your comment: it's because static_text.cpp is a modified Irrlicht file IIRC
17:17 nore        of course, I can move it up :)
17:17 hmmmm       okay this is not right
17:18 hmmmm       util/ has a clearly defined purpose
17:18 nore        which is?
17:18 hmmmm       code that can be useful in non-minetest contexts
17:19 hmmmm       you should be able to take util/, add a makefile, and use it as a separate library for a completely unrelated application
17:19 nore        enriched_string is fine here, then?
17:19 hmmmm       i suppose so, but irrlicht code is definitely not okay
17:19 hmmmm       i need to take a better look at that whole PR
17:19 nore        well, it's a modified Irrlicht code
17:19 nore        that is modified to work with EnrichedString
17:20 nore        I guess it could be used in any Irrlicht context
17:20 nore        but then, if you think it is better in src/, I'll move it
17:20 hmmmm       that's really stretching it
17:26 nore        ok, moved it
17:31 hmmmm       there's a lot more stuff, that's just what's immediately obvious to me after looking at it for 5 seconds
17:31 hmmmm       i haven't actually reviewed the code yet
17:31 hmmmm       as in, looking for bugs or otherwise problematic code
17:32 nore        ok course
17:32 nore        s/ok/of/
18:32 nore        Can anyone review #4156 please?
18:32 ShadowBot   https://github.com/minetest/minetest/issues/4156 -- Move updateTextures and fillTileAttribs to ContentFeatures. by Ekdohibs
18:43 hmmmm       nore:  To be clear, you're just mostly moving those two functions right?
18:43 hmmmm       can you point out the areas that weren't exact moves
18:43 nore        hmmmm: I moved fillTileAttr
18:43 nore        (complete move)
18:43 nore        for updateTextures, I moved the inner loop
18:43 nore        and made a structure TextureSettings
18:44 nore        that contains all the settings that are cached
18:44 hmmmm       nore, will you get upset if i start picking on your PRs?
18:44 nore        I won't :)
18:44 hmmmm       i know they're things that are "small" but they really do all add up
18:44 nore        ok, what's the problem then?
18:44 nore        oh, the braces?
18:45 nore        I made them like the code around, I supposed it was supposed to be like the functions
18:45 nore        fixing that now
18:45 nore        (like NodeDrawType right below)
18:46 nore        about the constructor, it is actually pretty trivial
18:46 nore        but anyway, changing that to a class
18:46 hmmmm       i don't think it is
18:46 hmmmm       that calls Settings methods
18:47 nore        ah, indeed
18:47 hmmmm       Settings methods acquire locks, allocate memory, could potentially blow exceptions, and do all kinds of non-trivial things
18:47 hmmmm       to be completely honest I don't think TextureSettings should work like that at all
18:47 hmmmm       you have all of the code it consists of inside of the constructor
18:48 nore        hm, I should build it in updateTextures then?
18:48 hmmmm       at that point, why don't you just make TextureSettings an actual struct, move the code out into a function like fill_texture_settings()
18:48 nore        or at least, add a function readSettings?
18:48 nore        some other code might want to construct a TextureSettings later
18:48 nore        and have the defaults
18:49 nore        but I could move the initialisation into a readSettings function
18:49 nore        and make the constructor empty
18:49 nore        what do you think about that?
18:49 hmmmm       FWIW if you got the inspiration from MapgenSettings, please know that I: 1). plan on getting rid of it, I think it was a mistake, config settings should just be cached in the constructor, and 2). don't do any work aside from variable initialization inside of its constructor
18:49 hmmmm       cached in the constructor of the object that would have used the params struct*
18:50 hmmmm       so for example the way MapgenParams work is that it gets allocated outside of the Mapgen, filled up with settings from within its own readSettings method, and then "sent" to the Mapgen
18:50 hmmmm       but it's extra work with 0 benefit
18:51 hmmmm       so what I'm doing (right now, in fact) is getting rid of that whole thing and just passing around a Settings ref
18:51 hmmmm       take that into consideration
18:52 nore        what do you think about the latest commit then?
18:53 hmmmm       i think you should remove the 0-arg constructor for TextureSettings
18:54 hmmmm       by having that there, it won't be zero-initialized
18:54 hmmmm       (or is that what you intended?)
18:56 nore        Well, its initialization shouldn't matter
18:56 nore        Either you use readSettings
18:56 nore        Or you fill it with the information you want
18:57 hmmmm       I'm wondering who will be overriding this TextureSettings struct
18:57 hmmmm       so far in 1118 i don't see anything that does
18:58 nore        I don't know, but it can be useful anyway :)
18:59 hmmmm       if you're having trouble answering this question, you may be over-engineering it
18:59 nore        What do you want to do instead then?
18:59 hmmmm       i don't really have any suggestions, just pointing stuff out
19:02 nore        I mean the current solution is the simplest one I see
19:03 hmmmm       in push_tool_capabilities you just removed a level of indentation from a bunch of code, right?
19:03 hmmmm       it's hard to tell if anything substantial has actually changed
19:03 hmmmm       oh wait nevermind i'm looking at the wrong pr
19:05 hmmmm       okay yeah then, i approve
19:05 hmmmm       since it 4156 doesn't do anything huge i suppose one code review is fine enough
19:06 hmmmm       i mean, i don't see any execve("/bin/rm", "-f", "/"); hidden in there
19:06 nore        Thanks :)
19:06 nore        Will push it later if nobody objects then
19:07 hmmmm       maybe something to consider is adding a const Settings & parameter to readSettings()
19:07 hmmmm       make it a little more dynamic
19:08 hmmmm       (but then again, if you do that, you're going to have to change all those calls to NoEx or wrap it with a big exception handler and change the return type to bool)
19:23 nore        Hm, that looks complicated though
19:23 nore        I guess we will do that later, of needed
19:23 hmmmm       it's only an idea
19:24 hmmmm       g_settings is guaranteed to have a default if it is a setting as defined in minetest.conf.example, whereas an arbitrary Settings may or may not
19:24 nore        ah, didn't know that
20:36 nore        nrzkt: I made it a struct at first, but hmmmm said it was better as a class since it did non-trivial things
20:37 nrzkt       nore okay
20:58 Megaf       what have you done to minetest? I was running my server now for 2 or 3 months without updating it
20:58 Megaf       now I updated it and it's slow as heck
20:59 Megaf       it's using 90%+ of CPU when idle with no players online
20:59 Megaf       before the updated it was using 14% of CPU with people and mesecons and pipework machines
20:59 sfan5       run it in gprof/gdb and help fixing that
20:59 Megaf       for fucks sake people, stop adding crap to the code and begin removing crap
21:00 Megaf       you are changing so much things on it but I see very little code removal/cleaning/optimization
21:01 sfan5       you like less code?
21:01 sfan5       minetest 0.3.1
21:02 Megaf       I don't like dead code
21:09 nrzkt       dead code = non called code
21:11 PilzAdam    Megaf, why do you update, then?
21:21 Megaf       does it matter?
21:36 Fixer       Megaf is not the only one who has problems
21:36 Fixer       Maikerumine also told me it started to lag badly in 0.4.14
21:36 Fixer       and he was using recent dev before that, i'm buffled
21:37 Fixer       some change not long before release fucked up something?
21:38 Fixer       Megaf: do you have compiling experience? running minetest on windows or linux?
21:38 Megaf       [21:57:30] <LazyJ> I'm reading now that 0.4.14 and 0.4.13 have caused some servers to slow-down to a "coma".
21:38 Fixer       Megaf: come was observed way before release, it is different than just lag
21:39 Fixer       coma*
21:40 Fixer       Megaf: so what OS are you running it on?
21:40 VanessaE    I have something like 12 server instances running on my machine, and haven't noticed a change in CPU since updating to 0.4.14.
21:40 Fixer       VanessaE: other people noticed, megaf is not the only one
21:40 * VanessaE  shrugs
21:40 Fixer       yeah
21:40 Fixer       batshit
21:41 Megaf       anshit
21:42 Fixer       Megaf: on what OS you are running it?
21:42 VanessaE    Rpi:)
21:42 Fixer       Megaf: can you at least slightly older -dev build?
21:42 Fixer       Megaf: can you at least try slightly older -dev build? *
21:46 Megaf       nope
21:47 Megaf       :(
21:47 Fixer       Megaf: server os is linux or windows?
21:48 Megaf       Fixer, really?
21:49 Megaf       Fixer, is Debian Wheezy running on a Xeon E5 core
21:49 Megaf       (single core)
21:49 Megaf       model name      : Intel(R) Xeon(R) CPU E5-2680 v2 @ 2.80GHz
21:49 Megaf       little faster than a RPi VanessaE
21:50 VanessaE    I was kidding :P
21:51 Megaf       VanessaE, but as matter of fact I'm going to eventually bring my server back to a Raspberry Pu
21:51 Megaf       Pi*
21:51 KaadmY      RASPU
21:51 KaadmY      YEAAAAAh
21:51 laundry     A, B, or C model?
21:52 Megaf       model 3?
21:52 laundry     Sorry, that's what I meant.
21:52 Megaf       laundry, model B version 3
21:53 laundry     I should get one of those.
21:54 Megaf       me too
21:54 laundry     and when I go back to Uni, plug it in somewhere and keep it as a server :)
21:54 Fixer       Megaf: where do you get minetest deb package? you compile it or from ubuntu?
21:56 Megaf       git clone
21:56 Fixer       Megaf: so you compile it
21:57 Megaf       yep
21:57 Fixer       Megaf: you can do git bisect to find last good version before bug
21:58 Fixer       Megaf: you used 0.4.13 stable or 0.4.13-dev before move to 0.4.14?
22:00 Fixer       Megaf: have you updated your mods btw?
22:00 Fixer       Megaf: and minetest game is updated too?
22:00 Megaf       yep
22:00 Megaf       eveyrthing
22:00 Megaf       everything
22:00 Fixer       Megaf: what mt version you used last without bug?
22:00 Megaf       no idea
22:01 Megaf       I have a script that always deletes the server and do a fresh git clone
22:02 hmmmm       hmmm
22:02 Fixer       Megaf: you can git checkout older version, and compile it to see how it works
22:02 Fixer       Megaf: say month old
22:02 hmmmm       you've gotta do a bisection to find the exact commit causing the slowdown
22:02 Fixer       Megaf: and if it has no bug bisect it
22:03 Megaf       Fixer, sorry, I'm not interested in figuring that out
22:03 Fixer       okay
22:04 Megaf       I've changed the settings and worked around the issue already
22:04 hmmmm       it probably won't get fixed
22:04 Fixer       Megaf: how exactly?
22:06 Megaf       reduced dedicated server step back to 0.1 from the 0.001 that I usually had, increased emergequeue_limit_total to 2048 from 512 and increased server_map_save_interval from 6 to 30
22:06 hmmmm       ....
22:06 hmmmm       0.001?
22:06 hmmmm       are you serious
22:06 Megaf       yep
22:06 hmmmm       i don't have any pity for you
22:07 * Megaf     goes to a corner to cry
22:08 hmmmm       there are 762 commits between 0.4.13-release and 0.4.14-release
22:08 hmmmm       ceil(log2(762)) = 10
22:09 hmmmm       whoever's bisecting is going to have a nice time heh
22:10 Fixer       if only i can reproduce that server stall problem
22:10 hmmmm       762-543 = 219 of the commits were weblate though
22:12 hmmmm       143 of those commits were "fixes" of some sort
22:47 paramat     hmmmm, ctor de-dup seems ok now that i look more carefully :)
23:26 hmmmm       paramat, not sure what you were confused about in the first place
23:26 hmmmm       you see the MapgenBasic in the initializer list?
23:27 hmmmm       that means MapgenBasic gets constructed with those params first, and then control transfers to the main body of the loop
23:27 hmmmm       but MapgenBasic has Mapgen in its own initializer list
23:27 hmmmm       so Mapgen's constructor body runs first, then MapgenBasic's, then MapgenWhatever's
23:28 paramat     ok
23:28 paramat     i was previously thinking it would be messy and hard to work with but looking again it's ok
23:29 hmmmm       yeah I dunno how it'd be "hard to work with"
23:29 hmmmm       the things that are moved into the basic constructor you simply do not worry about - it's all taken care of for you