Time Nick Message 00:29 Zeno` hmmmm, #2602 00:29 ShadowBot https://github.com/minetest/minetest/issues/2602 -- move particle spawners to env by obneq 00:29 * sofar yells at est31 00:29 hmmmm ah this pr 00:29 * sofar yells at est31 some more 00:29 * sofar yells at est31 some more waving fist 00:30 sofar why does est31 always manage to turn a simple PR into a lesson into C++ object inheritence ? And not because my patch is bad, but he just abstracted things behind another object layer away himself 00:31 hmmmm Zeno`, no, remember that it++ makes a copy of it and increments it, passes that along as the parameter to erase, and then sets it to the copy after all other actions on the line have executed 00:31 Zeno` hmmmm, well the original has the bug as well in that case 00:31 hmmmm foobar.erase(it++) is a common idiom for this 00:31 Zeno` huh 00:31 Zeno` yes, I changed that 00:32 Zeno` m_particle_spawners.erase(it++); 00:32 hmmmm but you can't do for (it = blah.begin(); it != blah.end(); it++) { if it is going to get invalidated at any point within the loop 00:32 Zeno` my revision doesn't 00:33 hmmmm what's wrong with what obneq has there already? 00:33 hmmmm didn't he have the idiom 00:33 hmmmm i think i told him about this too 00:33 Zeno` it's a mess 00:33 Zeno` continue and then 3 conditions 00:33 hmmmm oh 00:34 hmmmm the rest of the loop control is the problem 00:34 hmmmm kinda hard to read the code with all the notes on it 00:34 hmmmm but then you need the notes to compare with your version :D 00:35 Zeno` browser tabs :P 00:36 hmmmm so the original code is supposed to ignore non-expiring spawners, that is, those with a time less or equal to -1.f 00:36 hmmmm then for all that it doesn't ignore, it decrements a second from the timer 00:37 hmmmm if the timer is leq 0 then it erases it, otherwise advances along 00:37 hmmmm then what your revision dos is... 00:37 hmmmm if there's a positive time, (by definition non expiring timers have a negative time so they're never included), you decrement the time 00:38 hmmmm if it's < 0 then, you delete 00:38 hmmmm however if it didn't expire yet, you get caught in an infinite loop 00:38 hmmmm also should it really be < 0.0f? or should it be <=? 00:39 Zeno` well, if it was <= then that should solve the first issue also 00:39 hmmmm i think obneq's version is clumbsier but what it's written in a more intuitive manner 00:40 hmmmm your version has a slight bug, but looks nicer 00:40 Zeno` I find obneq's version way more confusing heh 00:40 hmmmm well it's more obvious when you say what each block of code is performing out loud 00:40 Zeno` but I don't really mind 00:41 Zeno` maybe.... but it kind of interrupts flow. I don't feel that is *has* to change. I really just wanted to discuss it. Discussion done ;) 00:42 hmmmm also, with your version, an expired timer that wasn't deleted would get stuck in the list 00:42 hmmmm so in other words if arithmetic was done with no floating point error, then you have a bug. ironically enough. 00:42 Zeno` I don't see how if the < 0 was changed to <= 0 but *shrug* 00:42 hmmmm right, you need to change the <= 00:42 hmmmm i'd change the other comparison with 0.0f to >= also 00:43 Zeno` I'll just leave it. Too hard (and I made that comment a year ago anyway lol) 00:43 hmmmm obneq's version: 0 bugs. zeno's version: 2 bugs! 00:43 hmmmm gasp 00:44 hmmmm each of them quite fatal and subtle 00:44 hmmmm playing with iterators and floating point arithmetic is serious business 00:45 Zeno` well, hopefully obneqs version never ends up i->second == 0 00:45 Zeno` otherwise it's going to be stuck there 00:45 hmmmm oh whoops, obneq bugs: 1 00:45 Zeno` or hopefully it's never -0.5f 00:46 Zeno` obneq bugs: 2 00:46 Zeno` 1 00:46 Zeno` heh 00:46 hmmmm how can such a simple piece of code have so many problems 00:46 Zeno` well it's lucky that a year ago I decided to refactor it for some reason! 00:46 hmmmm could this piece of code be written in a manner that would make it less error prone? 00:47 hmmmm not without making it bloatier i think 00:47 hmmmm it all comes down to cold, hard competence, and peer review 00:49 Zeno` I disagree that mine has two bugs: It has one (the same as obneqs) 00:49 hmmmm ?? 00:49 hmmmm what about the infinite loop if there's a timer with 2 seconds 00:50 hmmmm er particle with 2 seconds left 00:50 Zeno` yeah the first condition will get that if it's changed to >= 00:50 est31 never use it++ 00:50 hmmmm no, the iterator never advances 00:50 est31 always do ++it 00:50 Zeno` because straight after that it will be -1 seconds 00:50 hmmmm oh wait a minute 00:51 Zeno` (next time the loop runs that is) 00:51 hmmmm oh so the time left to expiry would always be effectively 1 second 00:51 hmmmm but it wouldn't have an infinite loop 00:52 est31 sofar, dont fear, you need no object inheritance knowledge to fix your patch :) 00:53 est31 just look six lines below 00:53 Zeno` so the timer would go 2.0 --> 1.0 --> 0.0 --> -1.0 (erase) 00:53 hmmmm right so to fix you'd need to change the > < to >= <= and then add a else ++it; to the erase if statement 00:53 Zeno` ok 00:53 hmmmm btw 00:53 Zeno` but why is it++ not ok on a line by itself? 00:54 hmmmm what do you mean 00:54 sofar est31: sorry, was the first thing I looked at after I got home from a long day at work :) 00:54 est31 Zeno`, it makes a copy of the iterator, which nobody needs 00:54 hmmmm if (it >= 0.0f) { // spawners that can expire 00:54 est31 ++it makes no copy 00:54 Zeno` I mean on line 1277 in the original patch 00:55 hmmmm it->second -= 1.0f; 00:55 est31 Zeno`, it++ is less heavyweight 00:55 hmmmm if (it->second <= 0.0f) m_particle_spawners.erase(it++); 00:55 hmmmm else ++it; 00:55 Zeno` ok, so I fix that bug also 00:55 hmmmm } else { 00:55 hmmmm ++it; } 00:56 hmmmm if that "else ++it;" isn't added you'll have this situation: 00:56 est31 commit 34b7a147dcf9831f3b4d81599c473ba01ff5da00 has eliminated every it++ 00:56 hmmmm particle with it->second == 2.0 encountered 00:56 est31 @ least i thinks so 00:56 est31 think* 00:56 hmmmm it->second -= 1.0f;, now it->second == 1.0f 00:56 est31 sofar, no problem 00:56 hmmmm it->second <= 0.0f condition isn't satisfied, so it does not increment it 00:56 Zeno` est31, https://github.com/minetest/minetest/pull/2602/files#diff-baa4d2d0ffafdec2b86baf065fba252bR1277 <---- obneq added a note 3 hours ago 00:57 hmmmm so the next time it loops around it's stuck at the same iterator 00:57 Zeno` to his original PR (I was just comparing the two) 00:57 hmmmm and it'll again be >= 0.0f, get decremented, now it's 0.0 and will be deleted 00:57 hmmmm so the loop gets stuck at that iterator until it's either expired or was negative to begin with 00:57 Zeno` I have the else! 00:58 hmmmm which means that you can set a timer for 5000 and it'd execute in 1 second 00:58 Zeno` I just need to change it to ++it 00:58 Zeno` instead of comparing the two PRs... this is confusing 01:00 Zeno` Added <= to my code in the comment 01:01 Zeno` oh and updated to ++it instead of it++ 01:01 Zeno` second not a bug, but a useless copy so dumb 01:02 hmmmm wait, where is your PR exactly 01:03 est31 he has a branch 01:03 est31 no pr 01:03 est31 linked in the pr by obneq i think 01:03 Zeno` All I did was rebase it because for some reason paramat asked me to 01:04 est31 https://github.com/minetest/minetest/pull/2602#issuecomment-214298812 01:04 Zeno` my rebase doesn't have my changes 01:04 Zeno` it's just rebased 01:04 hmmmm wait 01:05 hmmmm so is the version you're going with the one under "Revised after IRC discussion"? 01:05 Zeno` refresh now 01:05 Zeno` I'm not going to do anything. It's still just a code note question 01:05 hmmmm what about the it->second < 0.0f? why not change that to <=? 01:05 Zeno` I had no intention of changing it 1 year ago and I have no intention of changing it now 01:06 hmmmm also you're still missing the else ++it; 01:06 Zeno` no I'm not. you imagined it 01:06 hmmmm yes you are 01:07 Zeno` :P 01:07 Zeno` http://dpaste.com/1M01YD7 01:07 sofar est31: I must be blind 01:07 hmmmm yea 01:07 hmmmm that's good 01:08 hmmmm buthold up 01:08 hmmmm if you don't make the it->second < 0.0f into <= that's going to cause an off-by-one error 01:08 hmmmm you add a particle with an expiry of 5 seconds 01:08 hmmmm each dtime of 1.0, 1.0 seconds gets deleted from that 01:09 Zeno` Oh dammit! Now I'm posting the wrong code... getting so confused with all these windows open 01:09 hmmmm in order to finally get deleted, it needs to go another 1.0 seconds 01:09 hmmmm so that it's *under* 0.0f 01:09 Zeno` http://dpaste.com/20VA3RQ 01:09 hmmmm again, assuming that we're on a platform where -= doesn't cause floating pointe rror 01:09 hmmmm so it'd take 6 seconds to expire 01:09 Zeno` I've closed all my browser tabs... it was getting too confusing 01:09 hmmmm yes that's good 01:10 Zeno` ok, the note in the PR is updated as well 01:10 Zeno` my job done! 01:10 hmmmm but i'd still make the first if statement >= instead of > 01:10 Zeno` wtf 01:10 Zeno` it didn't update 01:11 hmmmm because you're effectively changing the convention to "0 or below" instead of "a negative number" to mean a non-expiring particle 01:11 hmmmm so you'd need to update the docs 01:11 sofar est31: alright, sheep still path OK with your patchset 01:12 est31 yeey 01:12 Zeno` that's changed in the PR note as well 01:12 sofar btw, I've caught a few sheep that "fell" into a node 01:12 sofar I doubt it's the find_path code, though 01:13 est31 probably more likely the collision code 01:13 sofar hmm, possibly 01:13 Zeno` hmmmm, code as in comment: http://dpaste.com/2TKWPGP 01:13 sofar wondering whether I should just let the animal die or escape 01:13 hmmmm OK 01:14 Zeno` If someone wants to update the PR with the suggestion they can. My only job was to rebase heh 01:14 hmmmm wait a minute jesus christ 01:14 hmmmm now you're subtracting a double from a float 01:14 hmmmm do you re-introduce minor errors on purpose to test if i'm paying attention?? 01:15 Zeno` maybe 01:15 hmmmm or nvm that was there the whole time 01:15 hmmmm please make that 1.0f 01:15 Zeno` done 01:16 hmmmm OK that chunk looks good to me 01:16 hmmmm did you have something else you wanted me to look at with that PR? 01:16 est31 do you re-introduce minor errors on purpose to test if i'm paying attention?? <----- Zeno` is a bad guy 01:19 Zeno` I don't even know why it was ME who was nominated to rebase it lol 02:10 sofar LOL 02:11 sofar Zeno`: est31 does that to me, we're the ones who are getting picked on :D 05:04 * paramat reads logs 05:20 paramat please can anyone review milestone #4031 ? i'll do a code review myself soon 05:20 ShadowBot https://github.com/minetest/minetest/issues/4031 -- Android menu: Unified serverlist by kilbith 05:21 paramat i'll merge #4019 soon 05:21 ShadowBot https://github.com/minetest/minetest/issues/4019 -- Make ItemStack with different metadata not stackable by hunterdelyx1 05:22 paramat i'll soon test #4032 then that will be ready for review/merge 05:22 ShadowBot https://github.com/minetest/minetest/issues/4032 -- Mapgen: Make 3D noise tunnels' width settable by paramat 05:25 paramat i'll also merge #4033 soon 05:25 ShadowBot https://github.com/minetest/minetest/issues/4033 -- Fix issue #4005 and update depends by MoNTE48 05:27 paramat in fact 4032 could be reviewed now, PR is complete, just needs a test 05:42 sofar paramat: please check out the alternative way of enlarging the boom particle effect :) 05:42 sofar I think you'll like it 05:42 paramat ok 05:43 paramat i'll test it soon 05:44 paramat thanks for working on it 05:46 paramat *** freeze is tuesday night, hopefully *** 05:51 paramat now testing 05:53 paramat seems good but the boom particle is too short lived, it tends to be obscured by the smoke 05:53 paramat i'll try 0.3/0,4 05:55 sofar game#1054 updated - "keys" mod is now a full-fledged API that mods can re-use to add key-functionality to their nodes 05:55 ShadowBot https://github.com/minetest/minetest_game/issues/1054 -- Keys - allow easy sharing of access without commands by sofar 05:55 sofar yeah, 0.2 isn't long, size is OK I think though 05:55 sofar maybe even too big :D 05:58 paramat i actually prefer 0.4 or 0.5, and matching the duration of the lighting 05:58 sofar yes, that needs to be matched 05:58 sofar without light the dark particle looks weird 05:58 paramat this is fun 05:59 sofar try several explosions in a row though 05:59 sofar not sure we want to make them too long 05:59 sofar but, I'll try as well 06:00 paramat well 0.4 is ok for me 06:01 paramat i now have a beach full of holes and drops i can't be bothered to pick up 06:02 sofar my test server is like a nuclear test zone 06:02 sofar I'm actually thinking of doing something even bigger 06:02 sofar later, though 06:02 sofar but here's the idea 06:02 sofar if tnt explodes, take a vmanip and count all tnt in a 5x5x5 radius around the tnt exploding 06:03 sofar make the explosion exponentially larger the more TNT is stacked close together 06:03 sofar then explode them all at once 06:03 paramat aha 06:03 sofar and make the radius larger the more tnt blocks are present 06:03 sofar that will make bigger bangs but less chance of chains, which is good 06:04 sofar you can still get chains but you'd have to place them very carefully 06:04 sofar anyway, 0.4 + 0.4 is ok for me too 06:04 paramat ok 06:07 sofar yesterday I blew some of my sheep away :o 06:07 paramat testing, i prefer size 40, this makes it exactly 3 nodes across, this makes it more visible 06:08 paramat corresponding to a radius 1 which would be the minimum used 06:09 paramat it's more like a decent sized fireball 06:09 paramat everything else is fine 06:10 sofar 40 should be 4 nodes across 06:10 sofar it's sized by BS, so divide by 10 06:10 paramat well the flame pixels don't span the texture 06:10 sofar ah 06:11 sofar well, hm, I already think it's really big 06:11 paramat i set expirationtime 20 and measured it 06:11 sofar I don't want to obscure a ton of other particles in the process 06:11 sofar and, it's already a huge step up from the original size 06:12 paramat ok well 30 is ok for me 06:12 paramat jin_xi please could you test #2602 ? 06:12 ShadowBot https://github.com/minetest/minetest/issues/2602 -- move particle spawners to env by obneq 06:13 paramat so i'll try to get a mtgame dev to review the tnt PR and hopefully merge it tues night 06:22 paramat sofar have you tested mob damage with that tnt PR? 06:23 paramat oh the sheep 06:23 sofar yes, it works 06:23 sofar I blew a sheep away and it took 8 damage 06:23 sofar which is a lot 06:28 paramat yeah, i only ask because that's the only thing i didn't test 06:29 paramat good work on tnt it's no longer embarassing 06:29 sofar I'll actually write an on_blast() API for entities as well 06:30 sofar tnt is a riot now :) 06:34 jin_xi paramat: idk what to do with this PR. Given how badly i suck at these i wont open any new ones. 06:45 paramat jin we're taking care of it 06:45 paramat but it needs testing and you're ideal :) 06:48 Zeno` 2602 is fine if my recommendation is added 06:49 Zeno` (because the recommendation contains the bug fix that hmmmm and I discussed) 06:49 Zeno` but it still needs to be all tested i.e. rebase applied, the adjusted code applied and then tested 06:51 Zeno` it wasn't exactly a straightforward rebase is what I'm saying, but I can't see anything incorrect. Someone else must check though 06:52 Zeno` paramat, should I close #2602 and open a new PR? 06:52 ShadowBot https://github.com/minetest/minetest/issues/2602 -- move particle spawners to env by obneq 06:52 Zeno` the author will still be obneq, it'll just be from my repo 06:53 Zeno` although that's not really necessary... the .patch is there in my comment :/ 06:54 paramat yeah a new PR seems good, perhaps cleaner 06:54 paramat whichever is easiest for you 06:54 Zeno` ok I'll apply the changes discussed as also 06:55 paramat yeah 07:01 Zeno` there may be another bug in that file not related to the PR heh 07:02 paramat jin_xi so yes we should obviously wait for zeno's version before testing 07:02 jin_xi please take this opportunity to do one good PR and just close mine, ok? 07:02 jin_xi my repo is still shot. and yes, testing i can do, to some extent 07:03 paramat ok 07:06 Zeno` just building and doing a cursory test etc 07:08 Zeno` actually 07:08 Zeno` hmmmm's comments were wrong :( 07:08 Zeno` // Timers with lifetime 0 do not expire 07:09 Zeno` I need to look at that code more 07:09 hmmmm i don't think it's smart to make 0.0 the cutoff for non-expiring particles 07:09 Zeno` yeah I'm looking at the Lua API docs 07:09 hmmmm floating point error and all 07:10 jin_xi its stupid from the get go to complicate time and expiration 07:10 Zeno` https://github.com/minetest/minetest/pull/2602/files#r29430826 07:10 sofar right, I'd like to see that set to e.g. -1, and then test for < -0.5 or something 07:10 hmmmm i don't think so, you just need to go about it a smarter way 07:10 hmmmm how about 07:10 Zeno` hmmmm ^--- I'm talking about the comment on 1284 07:10 Zeno` the comment should also be in the expiration code 07:10 hmmmm #define PARTICLE_DOESNT_EXPIRE FLT_MIN 07:11 hmmmm then compare == exact 07:11 hmmmm what is 1284? 07:11 hmmmm #1284 07:11 Zeno` line 1284 in u32 ServerEnvironment::addParticleSpawner(float exptime) 07:11 ShadowBot https://github.com/minetest/minetest/issues/1284 -- Errors appearing while using the chat formspec and pressing up/down arrow 07:12 Zeno` so the "rather serious bug" was more than one bug 07:12 hmmmm oh the thing about the variable time 07:12 hmmmm i don't know man 07:12 Zeno` because if a timer starts at 2.0 it should never expire according to that comment 07:12 hmmmm the particle spawner code and things related to it seem to be some of the lowest quality buggiest stuff 07:13 Zeno` so the true source of the "bug" is that there are bugs elsewhere 07:13 hmmmm perhaps 07:14 Zeno` if I apply the bug fixes discussed this morning to the expiration code then this comment and code in addParticleSpawner() becomes invalid 07:15 Zeno` Can someone at least check the rebase? 07:16 hmmmm i'm thoroughly confused but i'd like to help 07:17 hmmmm can you link me to the specific chunk of code that needs checking? 07:18 Zeno` https://github.com/obneq/minetest/blob/movespawner/src/environment.cpp#L1284 07:18 jin_xi Zeno i think this comment refers to explain the lua-api rather than what the code does 07:19 hmmmm Zeno`, it's not really clear but it does indeed state that 0 is an infinite particlespawner lifetime 07:19 hmmmm in the docs 07:19 Zeno` hmmmm, now look at our revised expiration code! The bug we fixed causes: a) all these invincible particles to actually be deleted; and b) if we *don't* fix it as discussed something starting with a timer of 2.0f will go, 2.0 --> 1.0 --> 0.0 (invincible) 07:20 Zeno` and never be deleted 07:20 hmmmm but what about the line of code on 1285 07:20 hmmmm doesn't that resolve the issue? 07:21 Zeno` no because the revised deleted code checks for >= 0.0 07:21 Zeno` revised deletion code* 07:21 hmmmm the way i understand it, internally, a negative number means it doesn't expire. however an expiry of 0 for the *interface* means infinite lifetime 07:22 Zeno` yes 07:22 hmmmm why would it matter, unless particlespawners somehow got added without calling addParticleSpawner 07:22 Zeno` yeah you're right 07:22 hmmmm if what you're saying is right, then that implies there's some piece of code that bypasses the interface provided 07:22 hmmmm that's bad 07:23 Zeno` yeah I just checked usages of that function 07:23 hmmmm in any case 07:23 Zeno` so you're right 07:24 hmmmm this is why i like obneq's version of that function better 07:24 jin_xi i think given that mt is a game and many beginners make mods it would be easier to just have a field 'expires' in the lua api, default to yes. this is too complicated for such a small gain 07:24 hmmmm it explicitly handles the case of infinite particlespawners 07:24 hmmmm jin_xi: fair enough, but you still need the old way for reverse compatibility 07:24 Zeno` hmmmm, well apply my rebase and fix jin_xi's (obneqs) version :) 07:25 hmmmm jin_xi is obneq? 07:25 hmmmm oh 07:25 Zeno` make new PR and profit! 07:25 hmmmm gosh i'd love to but i've got stuff TODO 07:25 hmmmm you should give it to RefactorNinja 07:26 paramat heh 07:26 jin_xi Shadow_Ninja please 07:27 hmmmm as long as you don't perform any arithmetic on a floating point number you'll always be able to make an exact comparison against its literal 07:27 hmmmm float foobar = 5.4f; (foobar == 5.4f) == true always 07:27 hmmmm float foobar = 5.4; foobar == 5.4f == not necessarily true 07:27 Zeno` https://github.com/Zeno-/minetest/commit/b619aff7c7339d408e7b2d53d5b47ca03b8eefa7 07:27 hmmmm float foobar = 5.4; foobar == 5.4 == not necessarily true 07:28 Zeno` hmmmm, I think we all know that 07:28 hmmmm so as long as you don't screw up and assign doubles to floats you should be ok 07:28 hmmmm right 07:28 hmmmm so my point is, use an actual constant to represent "this is an infinite particlespawner" 07:28 Zeno` oh I should change that to ++i asl well 07:28 hmmmm not some kind of floaty point comparison 07:28 hmmmm you can use a precise constant value as a flag 07:29 hmmmm if (i->second == PARTICLESPAWNER_NO_EXPIRY) { i++; continue; } ... blah blah blah 07:30 Zeno` https://github.com/Zeno-/minetest/commit/153006e903ea035f3ffb2f5b5e705ba9b4c16f80 07:30 hmmmm this would avoid the whole ambiguity we discussed 07:30 Zeno` this is the *last* time I rebase a PR for somebody 07:30 hmmmm erofl 07:30 paramat sorry 07:30 hmmmm this is an old pr though 07:30 hmmmm IT SIMPLY CAN'T BE HELPED 07:31 Zeno` I get pulled into fixing stiff that I never properly reviewed in the first place :P lmao 07:32 hmmmm btw why do you keep making commits 07:32 hmmmm why not just amend the last one 07:33 Zeno` Because it's my repo and I want people to be able to SEE what I changed 07:33 Zeno` I will squash once it's seen 07:34 Zeno` because the previous commit is rebase only (I made no changes apart from rebasing) 07:35 Zeno` if I change somebody else's code I want my changes look at and approved of first :) 07:35 Zeno` (i.e. this is not my PR) 07:35 hmmmm ok fair enough 07:35 hmmmm i always hated squashing because i have to actually do editing with vi 07:35 hmmmm with amend you just :w :q 07:36 Zeno` that was just a single commit btw... github changed the hash 07:36 Zeno` (I did amend commit for changing the ++I) 07:37 Zeno` if you an jin_xi are ok with those small changes to the original PR I will squash and open a new PR closing the old one 07:38 Zeno` because jin_xi's repo is messed up and he cannot apply the patch to test :( 07:38 jin_xi please do, i do not care about authorship, but don't mind if you do care about me taking the blame for future issues... 07:38 Zeno` jin_xi, the authorship will be the same 07:39 Zeno` it's just that the PR will be coming from my repo not yours 07:39 Zeno` (and it will be me who opens the PR of course which is a github thing only) 07:42 Zeno` so am I doing this, or not? :) 07:42 Zeno` paramat, how about I make a new patch? You can apply it and make the new PR :) 07:48 hmmmm ugh i'm upset 07:49 hmmmm i'm having trouble writing this new biome code in a manner that is sufficiently generic 07:51 Zeno` new patch is in original PR 07:56 Zeno` sofar, paramat, hmmmm, can someone apply the patch, test and open a fresh PR if required? 07:56 hmmmm i'd love to but my bedtime is in 4 minutes 07:56 * hmmmm runs 07:57 Zeno` heh 08:18 paramat uh maybe, sorry i'm incredibly busy with release stuff plus mtgame 08:18 paramat we need help from our elusive devs 08:20 paramat but yeah i'll look at it later 08:21 Zeno` you don't have to merge 08:21 Zeno` (into minetest) 08:22 Zeno` just make a new PR :) 08:22 paramat ok 08:23 Zeno` it's still wrong 08:24 Zeno` oh that's in a different part of environment.cpp not related to the PR 08:24 Zeno` nvm 08:42 Zeno` #4038 08:42 ShadowBot https://github.com/minetest/minetest/issues/4038 -- Handle particle spawners in env and delete expired ids by Zeno- 08:42 paramat nice 08:44 paramat oops didn't mean to remove your label, that was weird 08:44 Zeno` it happens :) 08:44 Zeno` you were probably editing labels at the same moment I was lol 08:44 Zeno` github race condition! 08:45 paramat so to be clear this is apparently good for merge? (according to others) 08:45 paramat i mean the implementation 08:46 paramat ah no, it's mostly unchanged 08:46 paramat ok 08:46 Zeno` I fixed the bug discussed 08:46 Zeno` I will add an additional note to the PR 08:47 paramat no problem we can read IRC logs 08:47 Zeno` better in the comment just in case it's not obvious that I actually did modify the original PR 08:49 Zeno` there, added a reference to the exact line I changed 08:52 est31 Anybody wants to review #4037 ? 08:52 ShadowBot https://github.com/minetest/minetest/issues/4037 -- Pathfinder cleanup by est31 08:53 paramat nope heh 08:53 Zeno` this is what I wanted to avoid 08:54 Zeno` but I fixed est31's comment anyway 08:54 Zeno` est31, I am not going to keep fixing somebody else's PR lol 08:54 est31 hehe 08:54 Zeno` I'll fix any minor comments you make in the next 5-10 minutes though 08:54 Zeno` as long as they're minor 08:54 Zeno` :) 08:55 paramat now should 4037 be in release? 08:55 paramat we're a bit overstretched 08:56 Zeno` est31, I reformatted your space after for a little differently 08:57 est31 paramat, it adds a great speedup for long paths, and it has another patch for fixing behaviour that most people consider a bug 08:57 est31 esp. stuff like grass being treated non walkable 08:57 paramat hm perhaps it should then ... 08:57 est31 I kept it separated into multiple commits 08:58 est31 you can only review the first two, they require no deep knowledge of the code 08:58 est31 only code style guidelines 08:58 est31 the third commit is also comparatively easy 08:59 paramat ok well lets add it to milestones to at least consider it 09:04 paramat will merge #4019 #4033 in a moment 09:04 ShadowBot https://github.com/minetest/minetest/issues/4019 -- Make ItemStack with different metadata not stackable by hunterdelyx1 09:04 ShadowBot https://github.com/minetest/minetest/issues/4033 -- Fix issue #4005 and update depends by MoNTE48 09:07 paramat now merging 09:17 nrzkt paramat for #4019 are you sure there is no side effect ? this PR is recent, could affect many mods we didn't know 09:17 ShadowBot https://github.com/minetest/minetest/issues/4019 -- Make ItemStack with different metadata not stackable by hunterdelyx1 09:18 paramat no i'm not sure but it was reviewed by another dev 09:19 paramat freeze will show any problems 09:19 nrzkt i'm not sure in two weeks we could see really the mods effects 09:20 jin_xi wouldn't any mod who intents to maintain 'individual' items have stack_max=1 in its definitions anyway? 09:23 paramat merged 09:50 Zeno` est31, see comment in #4038 09:50 ShadowBot https://github.com/minetest/minetest/issues/4038 -- Handle particle spawners in env and delete expired ids by Zeno- 09:50 Zeno` does that make sense? 10:07 est31 Zeno`, i dont think so 10:07 est31 err 10:07 est31 you are right 10:07 est31 sorry you are right :) 10:08 * VanessaE looks at est31 and Zeno` ... ok you two, shake hands. :P 10:42 * Zeno` shakes his hands in the air 10:43 Zeno` hokey pokey? 10:43 * Zeno` wonders why he and est31 are doing this silly dance 10:55 VanessaE heh 10:57 Zeno` have a good night 10:57 Zeno` err day? 10:58 Zeno` oops, wrong place. bbl :) 12:05 paramat nore the only current milestone for mtgame is ready for review game#1039 12:05 ShadowBot https://github.com/minetest/minetest_game/issues/1039 -- Tnt+: Changes for TNT mod after #862 by sofar 13:29 paramat bbl 15:12 Foghrye4 Hi! Does someone know, why CCraftDefManager has virtual functions? It seems there is no classes, who inherit CCraftDefManager. 15:18 hmmmm it may be the convention to mark implementations or overrides of virtual functions from a base class as virtual only to serve as a reminder 15:18 nore sofar: what happens with your keys patch if you use a stack of several skeleton keys? Shouldn't they have stack_max=1? 15:24 Foghrye4 Got it, thanks. 15:43 paramat was #4019 necessary since items with metadata usually have 'stack_max = 1'? written books do 15:43 ShadowBot https://github.com/minetest/minetest/issues/4019 -- Make ItemStack with different metadata not stackable by hunterdelyx1 15:59 sofar paramat: yes, it's necessary because with #3984 you may end up having items that are stackable until the metadata gets modified 15:59 ShadowBot https://github.com/minetest/minetest/issues/3984 -- Turn item metadata into a key-value store while keeping compatibility to the old interface via the empty string field by orwell96 16:00 paramat ok thanks 17:05 hmmmm oh, paramat 17:05 hmmmm whenever you're initializing a NoiseParams in an initializer list, you do not need NoiseParams() around it 17:24 Zeno` It looks like I will see the sunrise yet again 17:25 Zeno` hi hmmmm and hmmmmm 17:36 hmmmmmm hey paramat, when you get back i have a couple of biome related questions for you: 17:37 hmmmmmm - currently in all biomemanager-supported mapgens, biomes are computed in bulk (calcBiomes) and individually *for each column, not just when there's an overhang* 17:37 hmmmmmm is this intended? 17:39 hmmmmm - can you explain exactly what all the differences between generateBiomes() implementations are, and why they diverge? 18:25 Fixer i think I catched the bug with favourites with proc mon 18:28 Fixer anyone wants to see *.csv of file monitoring? 18:35 Krock I want to see a *.csv file about *.csv file monitoring 18:35 Zeno` yeah, sure 18:36 Fixer thread was the same btw 18:36 Fixer let me prepare files 18:36 Zeno` do different threads actually modify the file? 18:37 Zeno` (or read) 18:38 Fixer i think no, in my testing 18:38 Fixer same thread id 18:38 Fixer it was strange 18:44 Fixer Zeno`, https://github.com/minetest/minetest/files/237248/log.zip 18:45 Fixer !tell gregorycu https://github.com/minetest/minetest/files/237248/log.zip possible loss at 21:21 18:45 ShadowBot Fixer: O.K. 18:48 Fixer Zeno`, loss of file probably occured at 21:21 when joined server 18:50 Zeno` "21:21:24,6027327" and "21:25:20,3477869" doesn't make sense to me 18:50 Zeno` I'll study more closely after I sleep 18:51 Zeno` oh! 18:51 Zeno` ok, cool. I may have a clue 18:51 Zeno` let me sleep first, (it's 5AM) 18:51 Zeno` thanks for the logs 18:51 Fixer np :} 18:51 Fixer finally catched it 18:52 Fixer gn 19:28 paramat hmmmm i'll answer your questions in a moment 19:29 paramat nore sfan5 ShadowNinja please can anyone review game#1039 ? last milestone PR 19:29 ShadowBot https://github.com/minetest/minetest_game/issues/1039 -- Tnt+: Changes for TNT mod after #862 by sofar 19:29 paramat sofar and i have tested it 19:30 paramat freeze will be pushed back 19:31 paramat still a few engine milestones being worked on 19:31 paramat oh i chased SN away 19:33 sofar he's been swamped I guess 19:34 sfan5 paramat: game#1039 code looks good to me 19:34 ShadowBot https://github.com/minetest/minetest_game/issues/1039 -- Tnt+: Changes for TNT mod after #862 by sofar 19:34 sfan5 didn't test it though 19:35 paramat thanks 19:35 sofar it's all very small changes 19:35 paramat heh thanks 19:35 kaadmy :} 19:35 paramat hopefully in a couple of days 19:35 paramat weekend at latest 19:36 paramat hmmmm, calcBiomes calculates the 2D biomemap at the heightmap surface, the highest surface in the mapchunk. the biomemap is used when decos are placed because decos are only placed on the heightmap surface 19:37 paramat however 19:39 paramat in generateBiomes the biome surface material and biome stone and biome water is placed. see the notes in the function, while working down each column biome is (re)calculated for each surface encountered (air-solid, air-liquid, liquid-solid) 19:41 paramat so that biome materials respect biome y-limits. multiple layers of biomes within a mapchunk are possible, essential for beaches, ocean biomes etc 19:41 paramat so yes it's intended 19:45 paramat for dry land with no overhangs we could theoretically use the biomemap entry in generatebiomes, but to do that we would need to know that there are no overhangs, which would involve searching through a column 19:46 paramat it's simpler to just calculate it as it's currently done as surfacces are encountered 19:46 paramat (surfaces) 19:47 paramat during the actual biome material generation 19:48 paramat generateBiomes is identical for mgv5, mgv7, mgflat, mgfractal 19:48 paramat but different in mgvalleys because of river_water 19:49 paramat and mgwatershed has a generateBiomes similar to mgvalleys due to river_water, but probably different again due to other additional features 19:50 paramat :) 19:53 paramat i'll merge tnt PR soon 19:55 Fixer added more data to issue #4023, log of file operations when bug appears (i hope I catched it) 19:55 ShadowBot https://github.com/minetest/minetest/issues/4023 -- New unified favourites can disappear after game exit 19:55 paramat good 19:56 hmmmm yea but biomes are being calculated twice though 19:57 paramat hm, i guess if a surface is at heightmap we can use the biomemap result instead 19:59 paramat ok something to consider 19:59 paramat i can work on it after release 20:00 hmmmm the biomemap is used in decorations, ores, caves, and dust 20:00 hmmmm all of which come after generateBiomes() 20:00 hmmmm paramat, I'm working on it 20:01 paramat ok 20:02 hmmmm just need to remove bmgr->calcBiomes and have generateBiomes construct the biomemap 20:02 paramat it could be a 3D biomemap 20:02 hmmmm as i understand it, biomes in generateBiomes go from the top down, so the last biome to be selected should be the surface area 20:03 hmmmm erm i mean the surface level 20:03 paramat it could be seabed 20:04 Fixer offtop, but could you guys add more lakes on different heights than 0? 20:05 paramat the last biome selected is not necessarily significant 20:05 paramat Fixer mgwatershed does that 20:05 paramat as part of the rivers 20:07 paramat i worked for ages on various methods for highland lakes, very difficult 20:12 paramat hmmmm the first biome calculation in a column (in generateBiomes) would be equivalent to the current biomemap as that would be the highest surface 20:13 hmmmm the seabed is considered the surface level 20:14 paramat yes heightmap records seabed 20:14 hmmmm what you're thinking of is surface elevation from sea level, which is essentially MIN(heightmap[i], water_level) 20:14 hmmmm err, MAX 20:14 hmmmm but my point is 20:15 hmmmm the last biome calculated for a given column will always be equivalent to the results of calcBiomes() with the heightmap generated by generateTerrain() 20:15 paramat yes for generateBiomes an air-liquid surface triggers a biome recalculation, because we have biome-specific water 20:16 hmmmm right, so the only exception to this rule is if the seabed is below the current chunk 20:16 paramat no the first, generateBiomes works top-down 20:16 hmmmm right it works from the top down 20:16 hmmmm so the highest ridge would be the first biome 20:17 hmmmm so on and so forth 20:17 hmmmm then it might hit the air-water boundary 20:17 hmmmm then it might hit the water-stone boundary 20:17 hmmmm in either case, as long as the seabed is contained in the current chunk that'd be the last biome calculated for that specific column 20:17 paramat yeah 20:18 paramat heightmap is the highest surface in a mapchunk, so that's the first biome calculation 20:18 paramat oh apart from sea 20:18 hmmmm the heightmap generated by generateTerrain works purely off of noise (not probing) so the results will always be the absolute terrain height 20:18 hmmmm so let's say we're really high up in the sky 20:19 hmmmm the biome for that column, according to calcBiomes(), would be the same biome as at surface level 20:19 paramat no, updateHeightmap is used which searches down through columns 20:19 hmmmm oh you're right, i didn't realize updateHeightmap was run before calcBiomes 20:20 hmmmm but doesn't updateHeightmap keep the previous result if it can't find a node in the current chunk? 20:20 paramat erm 20:20 hmmmm so if the heightmap has -50 for one of the columns, the current chunk is ranging from y=200 to y=150, updateHeightmap will scan through that column from 200 to 150 and find no node 20:21 hmmmm so because it can't find a node it'd trust the old result of -50 20:21 paramat not sure 20:21 hmmmm that is, unless you changed it since i wrote updateHeightmap 20:22 paramat Returns -MAX_MAP_GENERATION_LIMIT if not found 20:22 hmmmm you changed it 20:22 hmmmm :| 20:22 hmmmm okay this blows away some of my assumptions 20:22 hmmmm see what happens when you have two chefs in the kitchen 20:24 hmmmm and you never updated calcBiomes 20:24 paramat i changed it, but before it returned ymin 20:25 paramat https://github.com/minetest/minetest/commit/d463000595ee2b8bce94e5b99e764be6e1fd52f6 20:25 hmmmm so this means calcBiomes is computing incorrect biomes if part of the ground level is outside of the current chunk 20:25 hmmmm yes, and you changed it in a commit before tat too 20:26 paramat where? can't find it 20:26 hmmmm idk but that's not important right now 20:26 hmmmm so in other words, everything that relies on the biomemap got the wrong biomes 20:28 hmmmm okay before all this manipulation happened, the heightmap would always have the results from the perlin noise, and thus the biomes would be partially correct every time a chunk is generated in the air 20:28 hmmmm i say partially correct because in the same situation before the change was made, this didn't account for the 3d noise 20:29 paramat i guess an incorrect biome is not a problem if the terrain is not found in a column 20:29 hmmmm paramat, i think the current setup is too complicated 20:29 paramat nothing is generated 20:30 hmmmm it's not that we're too dumb to understand it, it's just that nobody is smart enough to get it right and keep it right without errors throughout the entire SDLC 20:30 hmmmm lots of regressions 20:30 paramat it works fine 20:30 hmmmm i'm sorry but it works fine right now but it's a bitch and a half to maintain 20:31 paramat not for me, i understand it well 20:32 paramat i spent a year of full time work finishing and tuning the system you started 20:32 hmmmm then how come calcBiomes returns bogus results for chunks in the sky 20:32 hmmmm my point is that nobody can get every detail 100% right' 20:32 hmmmm now i hate to keep referring back to minecraft, but seriously, what about minecraft? 20:33 hmmmm their biome system is vastly less complicated and works fantastic 20:33 hmmmm just a simple 2d map over everything, no concern given to ridges or anything of the sort 20:33 paramat i doubt it's less complex, and it's inferior 20:33 hmmmm and if you're concerned about beaches or whatever 20:33 hmmmm i have a solution for that 20:42 paramat oh poo i did remove the fallback to previous heightmap when ground is not found, sorry 20:43 paramat just saw it 20:45 paramat hmmmm please don't trash a good system in a huge change, we can improve it in small steps 20:45 hmmmm i don't want to trash anything 20:45 hmmmm if it's done right, it'll be one option of several others 20:45 hmmmm that's why it's BiomeGenOriginal 20:45 paramat good 20:46 paramat so yes we should fallback to the original heightmap, i'm not sure why i changed that 20:47 paramat i really don't like the idea of long-lived biome caches 20:48 paramat that's overcomplex in anticipation of overcomplex needs in the future 20:48 hmmmm i'm not even thinking about biome caches right now 20:48 hmmmm i just wanted to go forth with an idea so i'm not required to rewrite everything once i do add it 20:49 paramat i see 20:49 hmmmm so here's what i want to do: 20:49 hmmmm put generateBiomes() into BiomeGen 20:49 hmmmm like DungeonGen places its own nodes, generateBiomes will do the same 20:50 hmmmm this makes it so that choosing a different biome gen selects a different way of placing the biomes, in addition to figuring out which biome goes where 20:50 hmmmm so you could in theory have a 3d biome system in addition to a 2d one 20:50 paramat with the option of generateBiomes being different if needed for a certain mapgen? 20:50 hmmmm no 20:50 hmmmm why does it need to be different? 20:51 paramat i explained in my 2nd answer earlier 20:51 hmmmm the more exceptions you make, the more difficult things become to code and maintain 20:51 hmmmm let me read up. 20:51 paramat that would be very inflexible and would make more specialised mapgens impossible 20:52 hmmmm is mgwatershed part of minetest right now? 20:52 paramat it will be very soon 20:52 paramat and mgvalleys is different 20:53 paramat the ability to have generateBiomes different if needed is very useful 20:53 paramat however yes it is identical in 4 maogens 20:53 paramat (mapgens) 20:54 paramat (chairman mao gens) 20:54 hmmmm oh i agree, it's just that it's hard to work with when you have a potentially different generateBiomes for each biome generator && each mapgen 20:54 hmmmm so that implies number_of_mapgens * number_of_biomegens is the number of different generateBiomes there'd need to be 20:55 hmmmm most of them should be the same, it's just the instance where one specific combination of mapgen and biomegen is different 20:55 paramat although generateBiomes differs, they all still just call getBiome(x, y, z) 20:55 hmmmm yes but 20:55 hmmmm getBiome(x, y, z) 20:56 hmmmm what if a different biomegen gets implemented, and this getBiome only takes (x, z) 20:56 hmmmm what if it takes a new type of input that isn't height, heat, or humidity 20:56 hmmmm maybe mapgen is too ambitious 20:57 hmmmm modularizing is always good, but maybe i ought to scale back the scope of my plans 20:58 paramat this is what i mean by 'trashing' in anticipation of a distant ideal that may never happen 20:58 paramat it's better to gradually work with what we have 20:59 paramat in an effort to generalize we would be throwing away the good things we have now 21:00 hmmmm honestly, i can't help but look at the current architecture and look at how incredibly limited it is 21:00 hmmmm there's so much wasted potential 21:00 hmmmm if you creep by with marginal changes forever, though, there will never be any kind of big development 21:00 hmmmm nothing noteworthy, or compelling, rather 21:01 paramat over time the mapgens and systems you started have become awesome because i gradually refined them 21:02 hmmmm yea, that's what refinement does 21:02 paramat i don't mind new potential but we should preserve what we have 21:03 hmmmm on another note i do way too much talking and not enough development 21:05 paramat so anyway, my request is, the ability for generateBiomes to be different for a mapgen if desired 21:21 paramat hmmmm here's why i changed updateHeightmap https://github.com/minetest/minetest/pull/2451 possibly not a good move, sorry. i'm happy to work on returning to the previous functionality 21:28 paramat now merging game 1039 at last 21:39 paramat merged! 21:40 paramat TNT is really good now. all game milestones closed :D 22:08 Fixer MTG ones? 22:18 sofar Fixer: yes, mtg 22:28 hmmmm paramat: i don't think reverting it will help that much 22:28 hmmmm the regular heightmap is still going to be wrong due to the lack of 3d noise so what's the difference? 22:46 paramat hmmmm, heh yeah i just realised that 22:56 paramat milestone #4032 is tested and ready for review, but hmmmmm i don't expect you to, since you're busy and focussed :] 22:56 ShadowBot https://github.com/minetest/minetest/issues/4032 -- Mapgen: Make 3D noise tunnels' width settable by paramat