Time Nick Message 04:58 Zeno` very odd commit message for last addition to master lol 04:59 Zeno` it's like a tweet 05:01 Zeno` https://gist.github.com/Zeno-/960b82defa1f68645331 05:02 Zeno` profiling now but it "feels" faster (also solves the changing a global value to solve a local problem issue), as discussed yesterday 05:09 Zeno` getting 10 fps in valgrind with instrumentation on :D 05:15 Zeno` That's a bit better than 1-3 05:24 Zeno` #2281 for feedback please 05:24 ShadowBot https://github.com/minetest/minetest/issues/2281 -- Remove shader workaround in itemdef.cpp by Zeno- 05:35 kahrl Zeno`: lgtm 05:39 Zeno` will merge in about an hour then. Thanks kahrl 05:39 Zeno` Just doing some more tests and profiling in the meantime 05:57 Zeno` will also merge 2164. I can't test on Hurd, but will just have to trust apoleon that it fixes the Hurd issue 06:30 nrzkt hi 06:32 nrzkt i fixed the android build 06:32 nrzkt #2277 06:32 ShadowBot https://github.com/minetest/minetest/issues/2277 -- Main.cpp cleanup: Move ClientLauncher and InputHandlers to dedicated files by nerzhul 06:35 nrzkt i think we must use subdirectories for sources, this PR add src/client and src/shared for client sources and shared sources, this will permit to have a better view of the project, and to get a separation between client, server and shared sources. I think we need, in the future, migrate some common code to a shared lib, it's better for compilation time and project overview/maintenance. 06:38 hmmmm i don't quite like that idea... 06:38 nrzkt why ? 06:38 hmmmm there are already many subdirectories 06:38 nrzkt at this time all sources are in a common directory, it's a pure bazaar :) 06:39 hmmmm just adding more might make things more confusing 06:39 nrzkt if they are per theme, for example: gui, network, graphics, client, server 06:39 hmmmm and make a common library is bad because then whole program optimization wouldn't work as well 06:40 hmmmm i like organization just as anybody else, but it's a slippery slope 06:40 hmmmm i don't want minetest to end up having a source tree like tersasology 06:40 nrzkt for network merge i already create a network directory, because there is many sources. But i don't like java directories, then 1 or 2 subdirs is the maximum :) 06:40 hmmmm (i realize they take it to an absurd extreme and it's not normal, but I can see overzealous organization doing that) 06:41 hmmmm also obviously this would break ALL pending pull requests 06:41 hmmmm this is the largest issue here 06:42 hmmmm I know you're wanting to make really BIG changes but you need to be mindful of all the issues they could cause instead of only focusing on positives 06:42 hmmmm maybe if the number of active pull requests is reduced to a certain acceptable level first 06:43 nrzkt of course, it's why the split must be done step by step. in #2277 i only move some code and there is only one conflicting PR, then it's possible, i don't say we must move all code today, but move it when we refactor some things, step by step may be good 06:43 ShadowBot https://github.com/minetest/minetest/issues/2277 -- Main.cpp cleanup: Move ClientLauncher and InputHandlers to dedicated files by nerzhul 06:43 hmmmm but please... don't do the common shared library thing... it'd ruin global optimizations 06:43 nrzkt no problem, it's just an idea 06:44 hmmmm which is the pull request that conflicted? 06:44 nrzkt hmm i can't remember and i msut go to work, it's a move on timegetter in main.cpp because my issue move the same thing :) 06:44 hmmmm well, if it's your own PR then I see no problem 06:44 nrzkt see you 06:44 hmmmm but you need to be curteous to other contributers 06:44 hmmmm contributors* 06:45 nrzkt no, the other pr is not mine, but i can discuss with contributor, i'll speak with him today, promice 06:45 nrzkt i must go :) 06:45 hmmmm okay 06:45 hmmmm we really need to merge PRs 06:46 hmmmm maybe tomorrow I'll take a break from work and focus on minetest all day long 06:46 Zeno` sounds... riveting 06:47 hmmmm i sware i have ADD, i'm only able to focus on one coding project at a time 06:49 Zeno` I do like the idea of a limited number of subdirectories but as I mentioned in the PR I think it needs discussion 06:50 Zeno` e.g. for the points you raised 06:50 Zeno` I think the naming (taxonomy) must be agreed upon as well if it goes ahead 06:51 Zeno` e.g. I don't like src/shared/ (it makes me think of a library or something) and would prefer src/common/ 06:51 hmmmm how about leave the common things in the main directory and put client-specific things in client and server-things in server 06:51 Zeno` could take years... just like botanists spend years moving plant names around :D 06:51 hmmmm i don't understand the point of having the main directory contain nothing but subdirectories 06:52 Zeno` I don't understand that either 06:52 hmmmm it's a java thing 06:52 hmmmm I start to sneeze when I see people attempting to inject java-isms into minetest 06:52 Zeno` so, yeah, your idea is better. the subdirectories should be based on clearly separable "sub 'modules'/'systems'" 06:53 hmmmm because java isn't just a language, it's an entire mindset 06:53 Zeno` I can never browse java projects 06:53 hmmmm and a disasterous one at that 06:53 Zeno` it's ridiculous... you spend an hour traversing 1000 directories to find it contains one function and then you backtrack heh 06:54 Zeno` the function 1000 levels deep is probably 3 lines long as well 06:54 sofar problems with leaves not showing up properly in the inventory? just pulled git... 06:54 * Zeno` looks 06:55 sofar yeah, some leaves in inventoryplus are only half visible, others are completely gone 06:55 Zeno` they seem to be working for me 06:55 Zeno` hmm 06:55 sofar like the texture moved left 06:55 Zeno` and it just started happening? 06:56 sofar just did a git pull and build 06:56 sofar can't say I ever saw it before 06:56 sofar it's with an 0.4.11 server tho 06:57 Zeno` screenshot? 06:58 Zeno` also, is inventoryplus up to date? 06:58 Zeno` actually 06:58 Zeno` hmm 07:01 Zeno` sofar, update git again please? 07:34 Zeno` ok, we re-merge #2281 shortly 07:34 ShadowBot https://github.com/minetest/minetest/issues/2281 -- Remove shader workaround in itemdef.cpp by Zeno- 07:34 Zeno` err s/we/will 07:36 Zeno` Anything you'd like me to add to the commit message, Tesseract? 07:38 sofar Zeno`: sorry, was afk a sec 07:38 sofar pulling 07:38 Tesseract Zeno`: Nope, seems fine. 07:41 sofar Zeno`: all good now, all leaves show up fine in the inventory :D 07:42 Zeno` sofar, thanks for bringing it up; was a (sort of) typo on my part (not really a typo, a 10 char change that I meant to add and missed... oops) 07:43 sofar that's why I like running git... I'm a software developer myself and I only have 2 eyes.... 07:47 Zeno` sofar, sometimes even 6 eyes miss the small things... :D 07:48 Zeno` sofar, if you feel inclined you may want to test the latest git (again) 07:48 sofar I usually pull once a week or so 07:48 sofar especially around release time ;) 07:50 Zeno` I'm glad that we were talking about git and context is available 07:53 hmmmm #justaustraliathings 07:55 nrzkt hmmmm: okay with #2277, then ? i won't move every file and create a library, source move only allowed when refactor ? 07:55 ShadowBot https://github.com/minetest/minetest/issues/2277 -- Main.cpp cleanup: Move ClientLauncher and InputHandlers to dedicated files by nerzhul 07:56 hmmmm ermm.. sure, but why don't you wait until some PRs are cleared out 07:56 nrzkt i looked at PR and i haven't found the PR i mentioned yet, it seems it is very old 08:00 nrzkt hmmmm: what about #2267 ? 08:00 ShadowBot https://github.com/minetest/minetest/issues/2267 -- Mgv7: Add lower limit of zero to mountain height noise by paramat 08:01 hmmmm i haven't looked at that myself but i trust paramat's judgement 08:01 hmmmm at least we know it's not going to cause minetest to implode 08:02 nrzkt no, it only add MYMAX then the height could not be zero. But is the Perlin2D only generate negative numbers the map will be a flat zone 08:02 nrzkt could not be under zero* 08:06 sofar Zeno`: furnaces are not working for me atm... checking server logs 08:06 nrzkt Tesseract: for #2269 : https://github.com/nerzhul/minetest/commit/cc343292ebd42add30f12b078b0fb7af146ddf87 is correct ? 08:06 ShadowBot https://github.com/minetest/minetest/issues/2269 -- string.cpp error on android build make 08:08 Zeno` furnaces? 08:09 sofar looks like any furnace, grinder, generator isn't working 08:09 * sofar tries 0.4.11 build 08:10 sofar no difference, huh 08:10 nrzkt is http://dev.minetest.net/Android#Minetest right ? or we can use minetest main repo ? 08:12 Zeno` seems the wiki might be outdated 08:13 sofar nvm - was 'mod gauges' that broke furnaces 08:13 * sofar kills it 08:23 nrzkt hmmmm: i reread irc logs because i forget you suggestion. that's ok for common into src/, i change the PR design 08:30 nrzkt #2277 rebased. I removed the src/shared like you mentionned hmmmm . Are you find with it ? 08:30 ShadowBot https://github.com/minetest/minetest/issues/2277 -- Main.cpp cleanup: Move ClientLauncher and InputHandlers to dedicated files by nerzhul 08:31 Zeno` InputHandlers should never have been in main.cpp in the first place :/ 08:32 nrzkt ofc, now they are in a good place 08:32 nrzkt with this change main.cpp ~1000 lines instead of ~2400 lines 08:32 nrzkt this also permit to remove main #ifdef/#ifndef SERVER 08:32 Zeno` I agree that the PR is required and useful but don't see the rush for it to be merged. It's unlikely that it will need much rebasing if things wait a bit (main.cpp does not get touched much; when I refactored main.cpp from a 3000 line function it was a PR for over a month and I didn't need to rebase once heh) 08:33 nrzkt here it's not really a refact, it's only a class move 08:33 nrzkt refactor* 08:33 Zeno` Oh, I know... I was just using my experience as an example 08:33 Zeno` i.e. not many people will change main.cpp and cause your PR to break if you wait a little bit 08:34 Zeno` I doubt anyone will make a PR that affects main.cpp ;) 08:34 Zeno` I like it, btw 08:34 Zeno` So I support it, I just think maybe let the other devs have a say (their timezones seem very different to ours) 08:34 nrzkt i'll add a comment: merged on sunday if no objection 08:35 Zeno` Yeah I think waiting a little bit cannot hurt 08:45 Zeno` #2273 is now running on my server 08:45 ShadowBot https://github.com/minetest/minetest/issues/2273 -- [Patch 2/4] Network rework: packet writing, sending and cleanups by nerzhul 08:45 VanessaE and mine. 08:46 nrzkt #2279 confirmed. If the server is slow and we are doing F7, assert fail, i look at this 08:46 ShadowBot https://github.com/minetest/minetest/issues/2279 -- Minetest crashes after hitting F7 in GunshipPenguins PvE server 08:49 nrzkt for this issue, the assert was here: assert(playercao != NULL); (game.cpp L3224) 08:49 nrzkt why not simply return; if the pointer isn't good. This mean the player isn't loaded yet, then we don't permit to change the camera 08:49 nrzkt Zeno`, kahrl ? 08:50 Zeno` hmm 08:51 nrzkt it's triggered by playerinput, then if the SAO isn't here, we mustn't crash the player. 08:52 Zeno` should be just a condition rather than an assert then 08:52 * Zeno` looks 08:52 nrzkt i compile and test 08:55 Zeno` so... hmm 08:55 Zeno` GenericCAO::initialize() has not been called yet? 08:55 nrzkt no 08:55 nrzkt because the getCAO return null :) 08:56 nrzkt it needs more than changing the test 08:56 Zeno` I don't know enough about the protocol to know how that happens 08:57 nrzkt okay 08:57 nrzkt it's fixed 08:58 nrzkt https://github.com/nerzhul/minetest/commit/a42418ac969d5bb647ec6099dad3a9f8161e8d88 08:59 nrzkt we test the player CAO and then change the camera mode, else it permit to change camera mode whereas no playercao 08:59 nrzkt because current code change the camera mode and crash. 09:00 nrzkt are you okay with it ? 09:01 Zeno` yep, go for it 09:01 Zeno` be back later 09:01 celeron55 i think it's fine to have one level of subdirectories 09:01 celeron55 but no more than that 09:01 nrzkt hi celeron55, okay 09:02 celeron55 for example in my opinion the organization of the scripting API is kind of annoying because there's a first level that doesn't contain really anything 09:03 celeron55 (but whatever) 09:07 celeron55 all of this really depends on the tools that people edit the code with, though 09:07 celeron55 for me it's easiest to have no subdirectories 09:08 celeron55 really ideally there shouldn't be subdirectories but instead files should be tagged 09:08 celeron55 but our filesystems don't support tagging like that 09:08 celeron55 nor version control 09:09 nrzkt the problem is we don't have a good idea which source is client only, which is server only and which is common. Minimum will be to have clientonly and serveronly sources in a subdir, i think 09:10 nrzkt for new developpers or contributor it's simplier to have a little hierarchy (1 subdir) 09:11 celeron55 yeah i can see that 09:12 celeron55 some things are things though that just happen to be used on only the server or only the client while both could use it in the future; such subdirectories add unnecessary work when changes happen to those 09:13 celeron55 i guess that's not a major issue but the point is, it has downsides 09:14 celeron55 it seems like client, server and common subdirs would make the most sense 09:14 nrzkt ofc. server may not have a separated directory because singleplayer need every server api at this time (but maybe server socket listeners are useless and can be added to server instead of common) 09:14 celeron55 but that's not the only way to make the subdirs 09:14 celeron55 one way would be to split the code in layers instead 09:14 nrzkt no, we can have, like now, specialized dirs, network/ lua_api/ ... 09:17 celeron55 certainly some things should be moved to util/ to begin with 09:18 celeron55 (that's for self-contained things that don't depend on anything else in the project) 09:27 celeron55 i tried to figure out a set of broad-stroke subsystems that aren't client or server; it's impossible to have less than like five of them 09:27 celeron55 five of them more than currently, i mean 09:28 celeron55 otherwise you're dumping together things that are very much not the same thing 09:28 nrzkt broad-stroke ? 09:28 celeron55 i mean like client, server and common are three subdirectories 09:29 nrzkt oh, i see 09:30 gregorycu How are we all 09:30 Zeno` common probably isn't necessary (as hmmm inferred) 09:30 celeron55 trying to split things into three subdirs otherwise, you end up with something like gfx+sfx+gui, the game environment including voxels and mapgen, and then you'd need a place where you can put client and server specific things and various other weird bits and pieces 09:31 nrzkt i agree with hmmm and Zeno 09:31 gregorycu Various IDEs have the ability to tag files 09:31 gregorycu Well, I know visual studio has that 09:31 nrzkt i'm using a mixup of eclipse, geany and vi 09:32 nrzkt and this doesn't work :) 09:32 celeron55 and after that you still need a name for all of those, which is basically impossible 8) 09:33 celeron55 so the only way to have subdirectories is to have lots of them, which is arguably inconvenient 09:33 gregorycu Maybe just very general tags would work, server, client, and shared 09:33 Zeno` I think that stuff in server should be stuff that the client *never* uses 09:33 nrzkt Zeno` prefers common over shared :) 09:33 gregorycu Common is fine 09:33 Zeno` yeah that's just a nomenclature issue hehe 09:33 gregorycu Shared implies shared object I suppose 09:33 gregorycu Which is bad 09:33 Zeno` yeah, that was my concern 09:34 Zeno` but common is not necessary if both server and client use the file/unit then it can just stay in src/ 09:34 celeron55 are you aware that that way common ends up containing basically everything that src contains now? 09:35 Zeno` celeron55, yes. That's why I'm suggesting not having common/ at all 09:35 nrzkt +1 09:35 gregorycu Why do you say that? 09:35 Zeno` src/ is by "proxy" common 09:36 celeron55 maybe we could say that everything stays in src unless it is specifically a stable and substantial part which can be cleanly moved to a subdirectory 09:36 celeron55 in which case it can be moved 09:36 Zeno` but stuff that is currently surrounded by (e.g.) #ifdef SERVER ... #endif would probably be better in a separate file and put in server/ 09:36 celeron55 then we don't get any kind of haphazard explosion of subdirs that nobody really agrees on 09:37 Zeno` oh oh... MKR is on TV 09:37 Zeno` (sorry wrong channel :/_ 09:37 nrzkt yes Zeno`, but it's not very simple, because it was used on main.cpp and reduce the code duplication, but sometimes there are large parts of code which could be exported 09:37 nrzkt xD 09:37 Zeno` nrzkt, I know 09:38 celeron55 (altough i'm not sure if that would work either; it might (maybe rightfully so) encourage not changing much) 09:38 Zeno` I was very aware of the "issue" when I was working on main.cpp :) 09:38 Zeno` bbiab 09:49 gregorycu celeron55: I found something interesting with our graphics rendering 09:51 celeron55 finding interesting things in it isn't very hard because it's full of all kinds of weird crap, but what is it? 09:52 gregorycu On my system, half the render loop is sending shaders the shader constants 09:52 celeron55 how do you measure that 09:54 gregorycu profiling using time 09:54 gregorycu Or sampling 09:54 celeron55 screenshot? 09:54 gregorycu One moment, and I'll get one for you 09:55 celeron55 i find this very hard to believe because the code in minetest doing that is extremely lightweight 09:55 celeron55 maybe the GPU is some kind of busy state that isn't visible in your profiling 09:55 gregorycu It's sending the data for every mesh 09:56 gregorycu 1.7 million sends per 6000 frames 09:56 gregorycu There are 16 shaders 09:56 celeron55 are you sure it's every mesh, or is it every material switch? 09:56 gregorycu Probably every material switch 09:57 celeron55 this is a known problem that everyone is too lazy to solve 09:57 celeron55 we basically need a texture atlas system 09:57 celeron55 there was one but it was removed because it broke and everyone was too lazy to fix it 09:58 celeron55 there's your work cut out for you 8) 09:58 gregorycu The other work around is to have a callback per shader, and remember when we send the globals and don't send them again 09:58 gregorycu As they appear to be cached on the GPU 09:58 celeron55 so is it partly an irrlicht bug? 09:58 gregorycu Well... 09:58 kahrl would it help to not send mTransWorld and mInvWorld? they seem to not be used in the shaders 09:59 celeron55 i would assume that it won't send the same shader multiple times if it doesn't change in the material 09:59 gregorycu Yes kahrl, it would 09:59 gregorycu But it's part of the problem 09:59 kahrl well computing the inverse of a matrix is expensive so it might help a lot 09:59 gregorycu The docs for irrlicht suggest the shaders set their global once per frame, but it's once per material switch 10:00 gregorycu Most of the time is spent sending, not on the calculations 10:00 gregorycu I'm going to shutup for a few min and get a screenshot of my data 10:02 celeron55 well in any case the core problem is that the material is switched way too often, and the cause of that is that everything uses a different texture, and the cause of that is that there are no texture atlases - i would imagine though that most materials use the same shader and thus the shader parameters don't need to be re-uploaded so that's an optimization issue in irrlricht 10:02 celeron55 irrlicht* 10:03 celeron55 but i don't know whether it is the case that the shader doesn't change; if it does, then irrlicht may be doing the right thing 10:03 gregorycu I believe the stuff is cached per shader 10:03 gregorycu So we should only send changes when they occur 10:04 celeron55 i believe we have exactly two shaders, one for regular nodes and one for water 10:04 celeron55 so those cannot switch *that* often if the thing is operating correctly 10:05 gregorycu hmm... 10:05 gregorycu I started to look into this, then I backpedled severely 10:05 gregorycu I thought it was 16 10:05 celeron55 the material *does* switch due to texture changes, and there may be things that are rendered without a shader 10:05 celeron55 in case those can cause something 10:06 celeron55 wait do we actually have 16 of them 10:07 gregorycu I believe that to be the case 10:07 kahrl celeron55: yeah, generate_shader() creates different ones from the same source 10:09 kahrl by prepending a header containing things like "#define MATERIAL_TYPE ..." 10:09 celeron55 lol what, it can create like 84 shaders 10:10 gregorycu I'm a bit out of my depth here to be honest, I don't do graphics 10:10 gregorycu But I understand profiling 10:12 kahrl why it creates different shaders for different draw types is beyond me (other than the normal vs. liquid distinction) 10:12 kahrl the shaders don't even use the DRAW_TYPE constant 10:12 celeron55 i guess someone might want to optimize it a bit 10:13 celeron55 also a github issue should be created about this 10:14 gregorycu http://i.imgur.com/kJTSod9.png 10:14 gregorycu Second last column is time spent 10:14 gregorycu In %, including children 10:15 gregorycu renderMap is 57.37% of the time, updating shaders is 27.49% of the time 10:15 gregorycu Which means updating shaders is 48% of the time of renderMap 10:18 gregorycu Does this table make sense to you two? 10:19 gregorycu https://github.com/minetest/minetest/issues/2283 10:19 gregorycu Feel free to pad that with comments and useful things 10:27 Zeno` it's amazing how different the Linux and Windows profiles look 10:27 gregorycu I'm all for low hanging fruit 10:27 gregorycu Would the atlas idea be feasible, because mods "could" add textures at any stage 10:27 nrzkt what do you think about #2213 ? it seems good 10:27 ShadowBot https://github.com/minetest/minetest/issues/2213 -- Better Design for Dialogs ( Create World, Delete World, Rename Modpack, ... by fz72 10:28 nrzkt What do you think about locking inventory mapping if playerCAO isn't good ? 10:28 kahrl gregorycu: having atlases for the node tiles would already help a ton, and those are static 10:29 nrzkt i looked at issue #831 (which is solved now, but not closed) and this opens the inventory, but an empty inventory because it's not loaded. 10:29 ShadowBot https://github.com/minetest/minetest/issues/831 -- Inventory crash during map load 10:29 nrzkt if we permit only inventory key when playerCAO is okay, inventory is shown properly 10:29 crazyR there used to be a texture atlas didnt there? 10:29 kahrl crazyR: yes 10:30 Zeno` I can't even see shader updating as part of the callgraph for renderMap() 10:30 celeron55 Zeno`: it probably depends a lot on the GPU and its driver 10:30 crazyR kahrl: why was it removed 10:30 Zeno` celeron55, yeah 10:31 celeron55 altough, even if you don't see it, it might still be causing performance degradation 10:31 kahrl crazyR: bugs in several other parts of the engine where for example texture coords weren't set correctly 10:31 celeron55 if the GPU includes the context switches in the actual rendering call instead 10:31 kahrl iirc 10:31 crazyR ahh 10:45 Zeno` I noticed the difference between drivers (even on Linux) when I was looking at the profile Wayward_One prepared 10:45 Zeno` big differences 10:45 Zeno` s/profile/profiles 10:52 Zeno` I just wish gregorycu's screenshots showed the headings hehe 10:52 Zeno` bit hard to comprehend without them, gregorycu :P 10:55 Zeno` seems I'm talking to myself again 10:57 nrzkt xD 10:58 gregorycu Who is kwolekr? 11:03 Zeno` That's for me to know and you to find out! :D 11:03 Zeno` It's a big secret known to only a few people 11:04 nrzkt it's hmmmm 11:04 nrzkt he's :) 11:04 Zeno` oh, go on, ruin it 11:05 Zeno` gregorycu, do you still have me on ignore? 11:05 nrzkt i think yes 11:05 Zeno` lol 11:07 Zeno` Well, I won't bother spending an hour applying, testing, and checking relevant PRs then 11:07 Zeno` *sigh* 11:09 Zeno` #2244 seems good even if const MapNode VoxelManipulator::ContentIgnoreNode = MapNode(CONTENT_IGNORE); was the only change 11:09 ShadowBot https://github.com/minetest/minetest/issues/2244 -- Fix rebase bug, make render loop use cache setting by gregorycu 11:10 Zeno` I mean it would still be good if that was the only change and no others made 11:11 Zeno` But I won't spend (up to) hour reading and testing the whole thing. Which is a pity 11:14 nrzkt Zeno`, celeron55 what do you think about #2284 ? 11:14 ShadowBot https://github.com/minetest/minetest/issues/2284 -- Open inventory only when inventory could be loaded by nerzhul 11:15 nrzkt this change doesn't let the user to toggle inventory if it's not loaded. 11:15 Zeno` nrzkt, why are these issue popping up now? 11:15 nrzkt which issue ? 11:16 Zeno` the player->CAO() not loaded related ones 11:16 nrzkt because i looked at it and test some old issues ? 11:16 nrzkt this fix doesn't prevent a crash, he prevent a bad user experience at connection 11:17 Zeno` yeah, I'm just wondering why they've never been experienced or reported before. Are they caused by a race condition or something related to optimised code? 11:17 nrzkt i don't know, the crash pr was done 2 years ago 11:17 nrzkt now i'll test the same thing with console, bug i think there is the same issue 11:17 Zeno` 2 years? 11:18 Zeno` I think I must have been looking at the wrong report :D 11:18 nrzkt maybe a duplicate, no ? 11:18 Zeno` maybe 11:18 nrzkt #149 is like issue i fixed in master 11:18 ShadowBot https://github.com/minetest/minetest/issues/149 -- Segfault when opening console 11:18 Zeno` But I don't mind that PR; nothing wrong with defensive (and proper) 11:19 nrzkt i test the crash now, but i think we mustn't open console if playercao not present too 11:19 Zeno` #2284 looks fine to me 11:19 ShadowBot https://github.com/minetest/minetest/issues/2284 -- Open inventory only when inventory could be loaded by nerzhul 11:19 nrzkt okay, i merge it 11:20 gregorycu What's the deal with 149? 11:21 Zeno` maybe it's just been overlooked for so long because the_game() used to be so large 11:21 Zeno` and now things can be narrowed down 11:22 Zeno` gregorycu, it's probably related to the player->CAO() issue 11:23 nrzkt it's an old issue, i'll look at it, to see if it works and fix it, or fix the console like 2284 11:27 Zeno` Related to mapgen: I don't know if it's because I'm aware of the issue but I'm noticing the shadow issues much more recently 11:28 Zeno` e.g. shadows on the ground when there is nothing above 11:28 Zeno` I am supposing it's more related to cavegen 11:28 Zeno` maybe I'll bisect later and see if it's my imagination or not 11:30 Zeno` nrzkt, I don't think it's necessary to change commit messages to include "@dev" at the end 11:31 nrzkt sorry, i like the OpenBSD commits :p 11:31 nrzkt you receive a mail for each commit ? 11:31 Zeno` Nah, it just looks funny when browsing history 11:31 Zeno` I do receive emails already 11:32 Zeno` If you feel it's necessary maybe add it in the PR? 11:32 Zeno` e.g. commit 28492849284928492 (@Zeno- approved) or something? 11:33 nrzkt i don't know 11:34 nrzkt as you want 11:34 Zeno` I don't know either; I'd rather see commit messages include only what has changed rather than political issues as well :) 11:35 Zeno` Maybe the other devs feel differently 11:35 Zeno` But there is another issue... say I discuss a commit with hmmmm, c55, RBA, kahrl and you... would all be in the @ ? 11:36 nrzkt stop talking to everybody xD 11:36 Zeno` I think there must be some trust that devs will commit following the rules/guidelines 11:36 Zeno` rofl 11:36 Zeno` ok :D 11:36 nrzkt i laugh :p 11:38 nrzkt you are right :) 11:39 * Zeno` stops talking to everybody 11:40 nrzkt oh no ! 11:40 Zeno` lol 11:40 Zeno` xD 11:40 Zeno` but I will. Be back later... going onto the Windows to see how the build works :-o 11:40 Zeno` (yeah I could use a VM) 13:41 PilzAdam nrzkt, bug! 13:41 PilzAdam [14:33:28] something is very worng with VanessaE's Vanilla minetest server 13:41 PilzAdam [14:38:47] doesn't she run a new network patch? 13:41 PilzAdam [14:38:51] CWz, whats wrong? 13:41 PilzAdam [14:39:33] http://pastebin.com/Zcy6BBXN 13:41 PilzAdam [14:40:29] seems like a network error 13:47 Wayward_One just tested, I got something similar: http://pastebin.com/xdFN6zTm 13:47 Wayward_One completely locked up my client 13:48 kilbith i confirm this, disastrous slowdown and heavily-spammed debug with ERROR[main]: 13:53 kilbith this seems occurs when snowfall mod is installed, others VE servers without this one are OK, and Zeno's server too. 13:54 Wayward_One yeah, just noticed it on VE-Survival and Creative too 13:54 shadowzone I have no idea what is going on...so I'm about as confused as confused gets. 14:02 nrzkt what is that ? 14:02 nrzkt where does this error come from 14:02 nrzkt sorry for to be late i'm working at work 14:03 nrzkt snowfall mod you say ? 14:03 nrzkt when does it come ? at start ? can you be more precise 14:03 Wayward_One for me, almost as soon as i join the servers 14:03 shadowzone Hello, Zeno`. 14:03 nrzkt it's a problem on client side ? 14:03 Zeno` hello 14:05 Wayward_One not sure if it's also server-side, all i know is it floods my chat and console and locks up my client 14:06 nrzkt ok, strange i haven't seen this issue before, but i haven't tried with this mod 14:06 nrzkt what client are you using ? 0.4.11 stable ? master ? 14:07 Wayward_One tried with both latest git and a month-old build 14:11 Zeno` save me looking at the logs. What is the problem? 14:12 Wayward_One http://pastebin.com/xdFN6zTm 14:12 Zeno` does it happen if you connect to my server? 14:13 kilbith no 14:13 Wayward_One upon joining VE-Vanilla, Creative, and Survival, chat and console get flooded with that 14:13 kilbith where 'snowfall' is installed -> bug 14:13 nrzkt hmmm... let me a minute 14:14 Zeno` ridiculous error message in the first place, heh 14:15 Zeno` ERROR[main] .... thanks for nothing silly client 14:15 Wayward_One lol 14:16 nrzkt hmmm 14:16 nrzkt the bug is not network 14:16 nrzkt i think, wait a second 14:16 nrzkt very difficult to catch in my window :p 14:18 nrzkt same issue, but i saw a problem with CURL 14:20 nrzkt server has problem to download index.mts file, i saw a 405 14:22 nrzkt wait a minute, i test 0.4.12 frozen version with this mod 14:23 nrzkt and next i install it on a local server with 0.4.12-freeze, and network part 1 and next network part 2 14:32 shadowzone Zeno`, CWz is killing your server. 14:32 shadowzone XD 14:33 nrzkt at this time: 0.4.12 -> 0.4.12: no problem. 0.4.12 -> master: no pb. master -> master: no pb 14:33 nrzkt now i test with patch 14:41 nrzkt right there is a bug with the particles or other thing 14:43 nrzkt network 2/4 client also have the problem 15:06 nrzkt ok 15:06 nrzkt i have found an interesting thing 15:06 nrzkt it seems the texture loading error is on packet 0x46, because i have 7 0x46 packets receive and 7 image error. 15:07 nrzkt but before fixing the packet bug, i look at the ugly output on the console 15:08 nrzkt oh... it uses a strange deserialization mode, i look at the last PR (network P4) maybe it's properly fixed 15:13 nrzkt hmmm with current master the reading is done with same way than master, i look at server packet sending 15:18 nrzkt oh 15:18 nrzkt i found the issue.... a duplicate insert into the packet into server side... 15:18 nrzkt *pkt << pos << velocity << acceleration << expirationtime 15:18 nrzkt << expirationtime << size << collisiondetection; 15:18 nrzkt pkt->putLongString(texture); 15:18 nrzkt *pkt << vertical; 15:18 nrzkt double expiration time... 15:19 nrzkt i test the fix and commit it 15:22 Zeno` nrzkt, what patch # (of yours) was I testing? 15:23 nrzkt same as vanessaE 15:23 Zeno` yeah, I can't find it 15:23 Zeno` anyway there is something wrong 15:23 Zeno` with it applied client's can crash the server 15:24 Zeno` ask CWz for more info 15:24 Zeno` I've removed the patch from my server for now 15:24 nrzkt okay. 15:24 Zeno` Wayward_One knows how to reproduce as well I think 15:25 nrzkt no problem , we must find all the issues. TOCLIENT_SPAWN_PARTICLE now works properly, i commit & rebase 15:25 nrzkt do you have the crashlog ? 15:25 nrzkt or the assert, or something other ? 15:26 Zeno` I don't 15:26 Zeno` it's not as simple as that 15:26 nrzkt CWz do you have informations ? 15:27 Zeno` nrzkt, one sec 15:28 Zeno` ok, sending PM 15:40 Zeno` nore, please allow ShadowNinja to approve and merge PRs :( 15:40 nore Zeno`, I thought I did that a few days ago... 15:40 Zeno` ok 15:41 Zeno` does he know? :) 15:41 kilbith Zeno`: http://dev.minetest.net/minetest_game_Development 15:41 nore well, I sent the message to ShadowNinja since I didn't know that ShadowNinja == Tesseract 15:41 nore so I'm not sure he knows 15:42 celeron55 nrzkt's network changes are really unsafe 15:42 celeron55 they don't do proper bounds checking when reading the data array 15:42 Zeno` celeron55, I've notified him of that 15:43 celeron55 most of what we did previous was using ostringstream (not all), which was always safe 15:43 Zeno` and also removed the patch from my server 15:43 celeron55 previously* 15:43 celeron55 i mean, istringstrea 15:43 nrzkt celeron55 i am fixing a use after free 15:43 celeron55 +m 15:44 nrzkt celeron55 can you give me an example of missing bound checking, i'll fix it 15:44 celeron55 i'm not going to list them all 15:44 nrzkt give me only one example and i fix all 15:45 celeron55 https://github.com/minetest/minetest/blob/master/src/network/networkpacket.cpp#L185 15:45 celeron55 the check is for 1-byte data while it reads 8 15:45 celeron55 also it shouldn't use assert 15:46 celeron55 it probably should throw a SerializationError or whatever it was called 15:46 nrzkt https://github.com/nerzhul/minetest/blob/packet-handling_2/src/network/networkpacket.cpp 15:47 nrzkt please look at this, it's patch 2/4 15:47 celeron55 it has the same problem 15:47 nrzkt you are right, sorry, i'm tired 15:48 hmmmm for what it's worth, I am not a fan at all of operator overloading 15:48 hmmmm if you're writing a u16 to a packet, why hide that... 15:48 hmmmm i think that's very noteworthy information 15:49 celeron55 well that overloading is in the bounds of sanity, because it's streaming data into the packet just like we previously used std::i/ostringstream 15:49 celeron55 altough, hiding the data type is a very valid problem 15:49 nrzkt we can show it by adding the type when we stream it 15:49 celeron55 it may cause unwanted protocol breakage when internal types change 15:50 nrzkt i can prefix each insert 15:50 celeron55 is it possible to make an explicit template parameter mandatory in C++? 15:50 celeron55 that would be a good way here 15:51 nrzkt do you want a thing like pkt->insert(data); ? 15:51 hmmmm are templates actually necessary for this 15:51 celeron55 yes i mean allowing to leave the out is a bad thing when formatting and reading packets 15:52 celeron55 unless the packet contain type information, but we don't want that 15:52 celeron55 or, well, we can decide to want it, but i think many would think it's too bloated 15:52 hmmmm we shouldn't be using (bad) C++ features like that for little to no reason when there is a non-C++y alternative that works even better 15:52 celeron55 (i've suggested it previously) 15:54 celeron55 C++ has many well-intended features that aren't applicable to a lot of cases due to practical reasons 15:54 hmmmm C++ is simply a bad language 15:54 hmmmm i can't wait until we get something better 15:55 celeron55 well... rust 1.0 is out soon, it's the closest to sanity out of what's out there now 15:55 Zeno` c'mon hmmmm... they're adding lots of cool stuff like Cairo 15:55 celeron55 8) 15:55 nrzkt then celeron55 i'll study a solution to have the right reading/writing value, but a useless cast in streaming interface could be good, no ? 15:56 hmmmm heh 15:56 celeron55 what do you mean by "a useless cast in streaming interface" 15:56 hmmmm he means like 15:56 nrzkt *pkt << (u8) value; 15:56 hmmmm yeah 15:56 hmmmm it's useless only in most cases until you accidentally use the wrong datatype 15:57 celeron55 what i want is something where breaking the protocol is maximally difficult to do 15:57 celeron55 automatic type inference does not accomplish that 15:57 celeron55 it makes it easier to break 15:58 nrzkt 0.4.12-freeze permit to break the protocol too 15:58 celeron55 it can be broken only once; after it, it should be solid as a rock so that people are unlikely to break it accidentally 15:59 nrzkt what do you suggest ? 15:59 Zeno` backpedal... the issue I have is that two users worked out how to crash my server 15:59 Zeno` in less than 5 hours and they didn't even know I had a patch applied 16:00 celeron55 well as a fact, if you don't use templates in the packet-reading and writing interface, there is no automatic typing of packet data, which solves that problem (then you have to explicitly change the method name to change what goes in and out of the packet) 16:00 celeron55 of course it isn't using so much C++ features then, but that's not the goal anyway 16:00 nrzkt no but we loose elegant working code 16:01 celeron55 elegance is not the main concern when packing and unpacking packets 16:01 celeron55 the main concern is making correct packets 16:02 celeron55 and reading them correctly 16:02 nrzkt the current interface permit it, but we are not catching the error, we assert the receiver/transmitter, which doesn't permit protocol modifications 16:03 celeron55 something like google's protocol buffers is one elegant-ish way that also makes it easy to keep the protocol from breaking 16:03 nrzkt do you have source ? it would be interesting 16:03 celeron55 https://code.google.com/p/protobuf/ 16:03 celeron55 it has a separate packet description file which determines what packets are allowed to be made 16:04 celeron55 there are other similar implementations out there and rolling our own isn't impossible either 16:05 celeron55 really the PR that you closed that had some stuff from me and sfan5 was one way to roll our own (an incomplete one though) 16:06 celeron55 anyway my main point is: the protocol has been too easy to accidentally break previously; we need something that is harder to accidentally break 16:07 nrzkt then you meen we must have a struct for each packet type 16:07 nrzkt mean* 16:07 celeron55 in the protobuf style, there is only one canonical definition which determines the packet's format 16:07 celeron55 which is kind of a struct 16:08 celeron55 and then there are checks that make it so that the sender cannot make anything different, and the receiver will not accept anything different 16:09 nrzkt it's perfect 16:09 nrzkt then i can use it if you prefer it. 16:10 celeron55 i'd like to hear if someone else has thoughts or experiences on this 16:10 hmmmm oh god no please don't use protobuf 16:11 hmmmm say 'no' to generated code spew 16:11 hmmmm say 'no' to bloated interfaces 16:11 hmmmm serializing data isn't a difficult problem, and it's one that's already been solved here 16:12 hmmmm why are you adding more SHIT for no reason 16:12 hmmmm all you need to do is create a good interface 16:12 celeron55 it's the KISS way for sure 16:13 nrzkt i agree with hmmmm, but i understand celeron55 16:13 celeron55 and not using templates allows it to be more resistant to breaking 16:13 hmmmm we use protobuf at work and it's painful for sure 16:13 celeron55 protobuf doesn't have any mechanisms for versioning, right? 16:14 hmmmm it doesn't 16:14 nrzkt Zeno` could you use #2273, i fixed the use after free (and also the particle packet) 16:14 ShadowBot https://github.com/minetest/minetest/issues/2273 -- [Patch 2/4] Network rework: packet writing, sending and cleanups by nerzhul 16:14 celeron55 versioning is one thing that's a bit of a mess in MT, so that's one goal 16:15 celeron55 i think the obvious way to go for now is pkt->insertU8(data); 16:15 nrzkt ouch :( 16:15 celeron55 that allows cleaner code but does not cause the extra hazards of templates 16:16 nrzkt what is the difference between pkt->insert() and insertu8 ? 16:17 celeron55 templates would be fine if the type inference could be disabled, but it looks like it cannot be 16:17 celeron55 ^ 16:17 hmmmm they also make compilation slower 16:17 celeron55 the compilation overhead isn't that much in a simple case like that 16:18 hmmmm i agree this is the kind of stuff that templates were made for 16:18 hmmmm but there are still problems 16:19 celeron55 it's sad but also the reality 16:20 celeron55 i love it how people come from programming schools knowing all this template stuff, and then they can't ever actually use it because there are practical problems with it 8)) 16:22 nrzkt then the problem here is we hide the insert type, right ? 16:24 nrzkt i can change the NetworkPacket API to show it 16:25 celeron55 how would you do that 16:25 nrzkt if you don't like my streaming operator 16:25 nrzkt we could use a ugly and repertitive insert 16:25 nrzkt of we could add a cast 16:27 celeron55 this wouldn't be a problem if we had a test system with proper cross-version testing with complete network packet coverage 16:27 celeron55 but... that's very far fetched 16:27 celeron55 never going to happen 16:27 nrzkt and this doesn't prevent hacking clients which tried to crash servers, like a bug i fixed 1 month ago 16:28 nrzkt i thing the API can be improved 16:32 Zeno` the currently discussed crash has nothing to do with hacking 16:33 Zeno` even if a client was hacked it should not be able to crash a server 16:34 nrzkt i agree with you, and i must improve the NetworkPacket to delete this issue. 16:34 nrzkt but i think try catch is the only path :s 16:35 Zeno` that's unfortunate :( I don't like exceptions much at all 16:35 nrzkt i agree but it's a special layer which needs it i think, we don't have choice 16:35 celeron55 what, that's exactly the kind of situations where exceptions do make sense 16:35 celeron55 if you never use exceptions elsewhere, that's where you *do* want to use them 16:35 nrzkt then i'll replace all asserts with throw 16:36 Zeno` celeron55, it cannot be solved without using exceptions? 16:36 nrzkt not with elegant code, adding a boolean could present using a throw, but it's very very fat 16:38 Zeno` I disagree (and so do a lot of other people), but anyway 16:38 Zeno` I noticed a reference to some Google code earlier. Google pretty much forbid their programmers using exceptions 16:40 celeron55 guys 16:40 celeron55 we have used exceptions to succesfully handle these errors previously 16:40 celeron55 we can do it in the future too 16:40 celeron55 exceptions have been a problem elsewhere but not here 16:40 celeron55 just don't even start messing with it; it's unproductive 16:42 Zeno` I'm not going to mess with it; I'm just saying that elegant code does not *require* exceptions 16:42 Zeno` and that many people agree. *shrug* 16:43 celeron55 i asked #elsewhere about disabling the type inference 16:43 celeron55 looks like it can be done 16:44 celeron55 template void insert(T t, std::enable_if::value>::type* = nullptr) 16:44 celeron55 you do something like this 16:44 celeron55 U is given explicitly, T is inferred, and then there's just a compile time check 16:46 nrzkt if types are different, what happens ? 16:46 celeron55 compilation fails if the inferred type is different than the first (only) supplied template parameter 16:47 nrzkt then, how we call the function in reality ? 16:47 nrzkt pkt->insert(toto) ? 16:47 celeron55 yeah, it's just that 16:47 nrzkt it could be good then 16:47 celeron55 (i didn't test that code; but it's something close to what would work) 16:50 nrzkt it could be a good improvement for code 16:50 nrzkt i thought about it. 16:55 nrzkt i must go. celeron55 i replace asserts by throw SerializationError like you suggest to prevent special crafted packet which could crash server. 16:57 nrzkt i will improve the throw test this evening or tomorrow. For your template idea i will think about it in the future but i think i will not merge it for PR2. Maybe an intermediate between PR2 and PR3 (PR3 repairs the protocol and change some bad handled packets, and unify some APIs) 16:57 celeron55 looks like here's another way of doing it http://paste.dy.fi/2BS 16:57 nrzkt for packet writing it's dynamic 16:58 celeron55 that is probably better because it doesn't compare types but instead just requires a parameter 16:58 celeron55 i.e. it allows making an implicit cast 16:58 nrzkt it cannot be used, because we have the endianess problem after 16:59 nrzkt it's difficult to handle write and read with template... impossible in fact, because of endianess 16:59 nrzkt some types needs special handling. I must go, see you later 17:01 celeron55 no that's not a problem because the underlying functions can support endianess directly 17:02 celeron55 you just have to avoid copying memory as-is and instead create template implementations for the primitive types 18:22 nrzkt #2286 seems to be good. Nobody have problem with it ? 18:22 ShadowBot https://github.com/minetest/minetest/issues/2286 -- Fix Download complete dialog in the mods store by ngosang 18:42 MattJ Is there any particular reason that most mods supply their own (and all different!) config system (config.txt, settings.txt, config.lua, etc....) rather than using the core settings API? 18:46 nrzkt the liberty ? 18:47 MattJ If that's the only reason, ok :) 18:47 MattJ I wondered if there was some non-obvious reason that I couldn't/shouldn't use it in my own mods 18:48 nrzkt i think, yes. But maybe everybody doesn't know about it 18:55 celeron55 well, i'd imagine if you have hundred mods and all of them would use the centralized settings, it becomes bit of a mess 18:56 celeron55 but dunno, i mean a config.lua is effectively a non-cost solution for mods as they have to have the magic values somewhere in any case and that's just a source file 18:56 celeron55 .txt files are weird as they're way more work than either of those other solutions 18:57 nrzkt what about #2227 ? (non core-dev people can talk for design changed) 18:57 ShadowBot https://github.com/minetest/minetest/issues/2227 -- Small changes in the style of controls by ngosang 18:57 nrzkt changes* 19:01 nrzkt celeron55 can i get the right status on forum.minetest.net please ? :) 19:03 Calinou do we have a standard config API? 19:06 fireglow use https://github.com/vstakhov/libucl 19:08 nrzkt lua/build/liblua.a(loslib.o): dans la fonction « os_tmpname »: 19:08 nrzkt everybody has this ? 19:09 Calinou use LANG=C for English messages :P 19:09 Calinou yes I do 19:09 nrzkt the use of `tmpnam' is dangerous, better use `mkstemp' 19:09 est31 yes /me too ! 19:09 nrzkt i'll look at it when i get a little bit more ime 19:12 est31 I can look at it too 19:12 * est31 looks at it 19:15 celeron55 mkstemp is posix, thus lua won't use it 19:16 celeron55 tmpnam is in the C standard so that's what it will use 19:16 celeron55 it's not our concern 19:19 est31 man tmpnam sais Note: Avoid use of tmpnam(); use mkstemp(3) or tmpfile(3) instead. 19:19 est31 tmpfile is bare c 19:21 celeron55 umm wait... "By default, Lua uses tmpnam except when POSIX is available, where it uses mkstemp" 19:22 celeron55 this means our lua cmake stuff is missing the setting of LUA_USE_MKSTEMP when posix is supported 19:22 est31 yes 19:22 celeron55 should be easy to add 19:23 nrzkt it's just adding a variable for POSIX ? 19:24 nrzkt then in every supported UNIX but not windows, right ? 19:24 celeron55 yeah i guess something like if(POSIX) add_definition(whatever) endif() 19:25 celeron55 regular lua uses an autotools based system and ours is a rather minimalist cmake setup so it's not surprising that it's missing a thing or two 19:25 est31 there is already a if (DEFAULT_POSIX) there 19:27 celeron55 it's missing the setting of LUA_USE_POSIX if it detects being on posix 19:27 celeron55 it just sets some DEFAULT_POSIX cmake variable... with which it does nothing while there is an if() for it 8) 19:28 est31 there is LUA_USE_{LINUX, MACOSX} 19:28 est31 rather set that than DEFAULT_POSIX 19:28 celeron55 there are no add_definitions() calls in there so nothing will end up in C 19:29 celeron55 and no cmake-generated configuration files either 19:30 nrzkt if(DEFAULT_POSIX) 19:30 nrzkt option(LUA_USE_POSIX ON) 19:30 nrzkt else() 19:30 nrzkt endif() doesn't work :) 19:30 est31 perhaps s/ON/"" ON/ ? 19:30 est31 or better, s/option/set& 19:31 nrzkt doesn't work 19:31 est31 gonna try another method... 19:31 nrzkt i must go away. If you solve the issue please PR 19:32 est31 ok :) 19:54 est31 you have to actually pass that config value 19:54 est31 I'll do a PR 20:07 est31 pr is out #2290 20:07 ShadowBot https://github.com/minetest/minetest/issues/2290 -- Fix two gcc warnings for lua by est31 20:13 est31 pr is out nrzkt 21:04 nrzkt thanks est31, i finish my wow raid and i look at this, or tomorrow :) 21:08 est31 ok 22:37 est31 what do you think, what is better 1) change bundled lua code (only a comment change), 2) disable Wall warnings for bundled lua or 3) live with the warning 22:37 est31 I'm talking bout the *.c change in #2290 22:37 ShadowBot https://github.com/minetest/minetest/issues/2290 -- Fix two gcc warnings for lua by est31 22:43 fireglow est31: dang I forgot: Really big thank you for fixing Pipeworks! :) 22:43 fireglow you are our servers saviour, again :) 22:44 est31 lol great :) 22:46 fireglow commented on the pull-req