Time Nick Message 00:05 Zeno` I'm more worried about 1527 heh 00:07 RealBadAngel i can see lighting code there 00:08 Zeno` doesn't seem to be related though. I'll remove that line and re-test 00:08 RealBadAngel please do 00:08 VanessaE RealBadAngel: I surmised this is why nore had to insert that extra check in technic to limit how many VM's it calls into use (a good check anyways since it reduces redundancy) 00:08 VanessaE it was also causing a massive memory leak 00:08 RealBadAngel it often happens that bug resides not where you suppose to 00:10 Zeno` well, the problem is still there: http://codepad.org/tbgNIQ83 00:19 RealBadAngel local vm, emin, emax = minetest.get_mapgen_object("voxelmanip") 00:19 RealBadAngel how this line is supposed to work? 00:19 RealBadAngel set 3 variables to one value? 00:20 RealBadAngel or just the 3rd? 00:21 Zeno` https://github.com/minetest/minetest/blob/master/src/script/lua_api/l_mapgen.cpp#L116 00:21 Zeno` all three are pushed onto the Lua stack 00:23 RealBadAngel i dont know who invented that, but for me that looks like not stack but pile of shit 00:23 Zeno` lol 00:24 Zeno` yeah well, that may be so 00:24 RealBadAngel that is against any coding logic 00:24 Zeno` it's convenient... apparently 00:24 Zeno` I don't like it myself, but *shrug* 00:24 RealBadAngel i do read that as def of three variables and 3rd is defined 00:25 RealBadAngel i mean set 00:25 Zeno` that's how I would (normally) read it as well (i.e. in a C-like way) 00:25 RealBadAngel 1st and 2nd are just set local 00:25 Zeno` or, heck, even pascal or modula-2 or normal basic 00:25 Zeno` but that's Lua for ya 00:25 Zeno` I think python does something similar but I've never used python 00:26 RealBadAngel can you do some tests for me while youre at it? 00:26 Zeno` sure 00:26 RealBadAngel add there before another variable 00:26 RealBadAngel and dump them all on call 00:27 Zeno` before vm? 00:27 RealBadAngel yes 00:27 Zeno` like: local test, vm, emin, emax = minetest.get_mapgen_object("voxelmanip") ?? 00:27 RealBadAngel yup, and try later on move test in between 00:29 Zeno` ok, it doesn't work when at the start: 10:28:48: ERROR[main]: ERROR: An unhandled exception occurred: ...est/worlds/ram-test/worldmods/voxelleaktest/init.lua:6: attempt to call method 'get_data' (a nil value) 00:31 Zeno` causes an unhandled exception with a extra at the beginning or the end 00:31 Zeno` actually, anywhere 00:33 Zeno` they're being assigned correctly (the return values) 00:34 RealBadAngel if so, then ok 00:34 RealBadAngel but anyway i still call it ugly as hell 00:35 Zeno` and I would agree :) But, alas, it's not the issue :( 00:35 RealBadAngel who knows if its using stacks 00:36 RealBadAngel one byte not taken out while pushed is a diseaster 00:37 RealBadAngel well, its pretending to be a stack 00:37 RealBadAngel so propably the issue is somwhere else 00:41 Zeno` the issue, I suspect, is the Lua garbage collector and https://github.com/minetest/minetest/blob/master/src/script/lua_api/l_mapgen.cpp#L105 00:41 Zeno` never being deleted properly 00:42 RealBadAngel *(void **)(lua_newuserdata(L, sizeof(void *))) = o; 00:42 RealBadAngel love such code :) 00:43 RealBadAngel you have to be twisted to get it and double twisted to write it 00:44 Zeno` although it *appears to be* here: https://github.com/minetest/minetest/blob/master/src/script/lua_api/l_vmanip.cpp#L34 00:45 Zeno` if (!o->is_mapgen_vm) is dodgy as crap though (any random object has a 50/50 chance of that being true due to the cast) 00:45 Zeno` if !? 00:45 Zeno` if not? 00:45 Zeno` err... does that seem right? 00:45 * Zeno` is confused now 00:46 Zeno` why would you delete it only if it's not NULL ? 00:46 Zeno` not true, rather 00:46 * VanessaE pokes hmmmm 00:48 RealBadAngel if !thing 00:48 RealBadAngel means if thing doesnt exists 00:49 RealBadAngel ie is NULL 00:49 RealBadAngel !NULL = true 00:50 Zeno` yeah, but it's a bool 00:50 Zeno` (I made a mistake when I said !NULL... !NULL would be correct( 00:53 hmmmm Zeno`: indeed, if (!o->is_mapgen_vm) is a typo 00:53 hmmmm that would definitely cause a memory leak. 00:53 hmmmm the casts are valid though 00:53 Zeno` changing it to if (o->is_mapgen_vm) {} causes segfault though heheh 00:54 hmmmm what's the cause of the segfault then? 00:56 RealBadAngel its a bool or a pointer then? 00:56 Zeno` double free (delete) I think... I'm still trying to get my head around what's going on 00:56 hmmmm OH 00:56 Zeno` it's a bool 00:56 hmmmm derp 00:56 RealBadAngel it negates o ? 00:56 hmmmm no 00:56 hmmmm shoot 00:56 Zeno` https://github.com/minetest/minetest/blob/master/src/script/lua_api/l_vmanip.h#L38 00:56 hmmmm I am confused now 00:56 hmmmm nevermind, that's not a typo 00:57 hmmmm if it's a mapgen vm, we don't want to delete that object because it's owned by the mapgens, NOT! lua 00:57 hmmmm LuaVoxelManip::is_mapgen_vm is a bool 00:57 Zeno` although valgrind points to https://github.com/minetest/minetest/blob/master/src/script/lua_api/l_mapgen.cpp#L105 00:57 Zeno` as being the source of the leaked mem 00:57 hmmmm we only want to delete VoxelManipulator objects that were allocated, not grabbed as an existing mapgen object 00:58 Zeno` Where is that memory deleted, is the question I guess 00:58 hmmmm okay 00:59 hmmmm that memory is deleted in Map::finishBlockMake() 00:59 Zeno` hmm ok 01:00 hmmmm ahhhh 01:00 Zeno` must be something else then. sorry for the red herring 01:00 hmmmm I see the problem 01:00 hmmmm Map::finishBlockMake deletes the underlying VoxelManipulator object 01:00 hmmmm regardless of creation method, the LuaVoxelManip must be deleted 01:01 hmmmm so it should be actually like 01:01 hmmmm if (!o->is_mapgen_vm) delete o->vm; 01:01 hmmmm delete o; 01:01 hmmmm sorry 01:02 VanessaE hmmmm: would this be the same thing that caused that big memory leak we had to patch over in technic? 01:02 VanessaE (using non-mapgen vm's to force-load blocks) 01:02 hmmmm i really don't know anything about that 01:03 VanessaE hmmmm: this, except line 83 didn't exist before the patch. https://github.com/minetest-technic/technic/blob/master/technic/machines/switching_station.lua#L82 01:03 hmmmm if it's a mapgen vm you're getting, then o would never be deleted. if it's your own vm you're creating, the LuaVoxelManip would be deleted but the VoxelManipulator would be leaked 01:03 hmmmm so in either case something is leaking 01:04 Zeno` yes, that seems to be the case, but I'm new to the code base so I'm battling to understand it :) 01:05 hmmmm alright i'll give an overview of what's happening 01:05 Zeno` you have the test script I am using? 01:05 Zeno` test Lua* 01:05 hmmmm no. 01:05 Zeno` https://github.com/minetest/minetest/issues/1527 01:05 hmmmm you can either create a new LuaVoxelManip object in order to do your own loading and whatever 01:06 Zeno` you can delete the set_lighting() and calc_lighting()... it's not related 01:06 hmmmm or you can grab the existing mapgen's voxel manipulator in order to make any changes to the terrain that has been generated 01:06 hmmmm the existing mapgen's voxelmanip is created in initBlockMake, and deleted in finishBlockMake 01:06 hmmmm as a result, the lua GC can't delete that VoxelManipulator 01:07 Zeno` yep 01:07 hmmmm therefore any LuaVoxelManips created as a wrapper around existing VoxelManipulators must ignore deletion of that VM 01:07 hmmmm but the GC must still delete the LuaVoxelManip wrapper object itself 01:07 Zeno` Ok, I understand the use of the bool now 01:08 hmmmm as for user-allocated voxel manipulators (e.g. used outside of on_generated), the VoxelManipulator is allocated within that same wrapper and thus must be destroyed within the GC 01:08 hmmmm so, indeed, changing that line to "if (!o->is_mapgen_vm) delete o->vm; delete o;" would fix this issue 01:08 Zeno` so line 35 should be delete o->vm and then outside of the condition delete o ? 01:08 Zeno` cool 01:09 Zeno` testing 01:09 hmmmm but 01:09 hmmmm there's more to it 01:09 Zeno` oh oh 01:09 hmmmm look at LuaVoxelManip::~LuaVoxelManip() 01:09 Zeno` ugh 01:09 hmmmm so you can see the destruction logic was poorly thought out at the time 01:10 hmmmm the best way to fix this would be to change l_vmanip.cpp:322 to if (is_mapgen_vm) delete vm; 01:10 hmmmm and then remove the conditional on line 34 entirely 01:11 hmmmm to the best of my knowledge, without testing anything at all, this should solve the issue 01:11 hmmmm i can't be arsed to open up my IDE 01:20 Zeno` testing 01:22 Zeno` so, LuaVoxelManip::gc_object() should simply delete o; ? 01:22 Zeno` after the cast of course 01:25 Zeno` ok, that resolves the issue 01:27 Zeno` I've updated the issue with the changes: https://github.com/minetest/minetest/issues/1527 01:27 Zeno` since I can't push 01:29 Zeno` thanks hmmmm 01:29 Zeno` thanks also for the explanation... apart from the bug fix I understand those source files a bit better now :) 02:20 Zeno` hmmmm, https://github.com/minetest/minetest/pull/1529 02:37 hmmmm yep looks good zeno` 02:37 hmmmm oh wait a minute, you don't have committer privileges 02:38 Zeno` no, you have to do it :) 11:03 kahrl RealBadAngel: wtf is wrong with multiple return values? 11:04 kahrl it's a completely documented feature of lua and a lot of other languages 11:04 kahrl even string.find uses it 11:04 RealBadAngel kahrl, well, nothing in fact. i just dont like how it looks 11:04 RealBadAngel if it works, then ok 11:04 kahrl ah, ok 11:22 Zeno` for a language like Lua, multiple return values /is/ the neatest solution 11:23 Zeno` the problem is that local a, b, c = func() *looks* like c is assigned and not a and b 11:24 Zeno` the only other solution would be to pass a table to the func, kind of like passing a struct in C, but due to Lua's stack mechanism that wouldn't be very efficient 11:25 Zeno` personally I'd like to see some kind of, I dunno, wrapping. Maybe (a, b, c) = func() 11:25 Zeno` but *shrug* 11:25 Zeno` it's not going to change heh 11:25 Zefram_Fysh returning a table is a nice solution when performance isn't tight 11:26 Zefram_Fysh parenthesised var list would have been a better syntax, but too late now. you've just got to rely on people knowing the language 11:26 Zeno` Zefram_Fysh, yes, or returning a table (same as passing a table, the performance is the same... not like C where you can pass a pointer to a struct) 11:26 Zeno` yep, that's what I mean by "it's not gonna change" 11:26 Zeno` because it's too late :) 11:29 Zefram_Fysh not too late if you're willing to implement your preferred language in lua. you can implement it to compile to lua code, which you evaluate with loadstring(). with LuaJIT, this will even have non-horrendous performance 11:30 Zeno` people are used to it now though 11:31 Zeno` and I don't really care enough to fork Lua heh 11:31 Zeno` it's not so bad 11:32 Zefram_Fysh if I were implementing a better language, it wouldn't be just a slightly-modified lua 11:40 Zeno` lua is pretty nice actually 11:40 Zeno` I hated it at first 11:40 Zefram_Fysh it's inoffensive 11:41 Zeno` I began to like it much more when I actually started writing C "extensions" heh 11:41 Zeno` the design it not without flaws, but it's pretty darn good 11:42 Zeno` I couldn't do better 11:42 Zeno` well, maybe I could but only in hindsight 11:47 Zefram_Fysh the 1-offset for numeric indices is obnoxious 11:47 Zefram_Fysh so is the implicit semicolon 11:48 Zefram_Fysh and the implicit creation of variables on typo 11:48 Zefram_Fysh there are many details that are more or less annoying 11:48 Zefram_Fysh but it gets the big things generally right 11:49 Zeno` not related to the language but the common practice of always putting a , after table entries annoys me for some reason 11:49 Zefram_Fysh it's nice to have lambda expressions just work. you wouldn't have got that in a corresponding language from 20 years ago 11:49 Zeno` lots of languages create implicit vars from typos 11:50 Zeno` well, most "basic like" ones 11:50 Zefram_Fysh putting a comma after the last entry of a table means that adding another entry after it doesn't edit that line, which makes editing easier and the diff less cluttered. that the language permits it is a desirable feature 11:51 Zeno` Yes, it seems to be desirable to most people. I don't like it though :) 11:51 Zeno` just one of those things I guess 11:51 Zefram_Fysh that many other languages make the same mistake doesn't make it less problematic, nor in this case is it an excuse. the problems with it are well known, as is the solution 11:51 Zeno` I don't like introducing compound statements in C when they're not necessary either heh 11:52 Zeno` which annoys many other people immensely 11:53 Zeno` I do for () for () for () {} and people hate me :( 18:44 Krock hmmmm, I'm almost sure, it has been asked before, but I can't find anything about this. Why does minetest call the function register_on_generated sometimes twice? 19:35 hmmmm minetest doesn't call that function, various games do