| Time |
Nick |
Message |
| 00:28 |
|
Zeno` joined #minetest-dev |
| 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 |
|
est31 joined #minetest-dev |
| 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 |
<hmmmm> 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 |
| 01:24 |
|
proller joined #minetest-dev |
| 01:25 |
|
Void7 joined #minetest-dev |
| 01:32 |
|
STHGOM joined #minetest-dev |
| 01:38 |
|
electrodude512 joined #minetest-dev |
| 02:10 |
sofar |
LOL |
| 02:11 |
sofar |
Zeno`: est31 does that to me, we're the ones who are getting picked on :D |
| 02:49 |
|
DI3HARD139 joined #minetest-dev |
| 03:05 |
|
electrodude512 joined #minetest-dev |
| 05:04 |
|
paramat joined #minetest-dev |
| 05:04 |
* paramat |
reads logs |
| 05:11 |
|
Hunterz joined #minetest-dev |
| 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 |
|
jin_xi joined #minetest-dev |
| 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:24 |
|
nrzkt joined #minetest-dev |
| 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:34 |
|
jin_xi left #minetest-dev |
| 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 |
| 06:58 |
|
jin_xi joined #minetest-dev |
| 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 |
|
Darcidride joined #minetest-dev |
| 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:54 |
|
Megal_ joined #minetest-dev |
| 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 joined #minetest-dev |
| 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 |
|
jin_xi joined #minetest-dev |
| 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:30 |
|
pozzoni joined #minetest-dev |
| 09:47 |
|
paramat left #minetest-dev |
| 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? |
| 09:52 |
|
MoNTE48 joined #minetest-dev |
| 10:02 |
|
pozzoni joined #minetest-dev |
| 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:09 |
|
pozzoni joined #minetest-dev |
| 10:17 |
|
pozzoni joined #minetest-dev |
| 10:26 |
|
Fixer joined #minetest-dev |
| 10:33 |
|
rubenwardy joined #minetest-dev |
| 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:48 |
|
pozzoni joined #minetest-dev |
| 10:55 |
VanessaE |
heh |
| 10:57 |
Zeno` |
have a good night |
| 10:57 |
Zeno` |
err day? |
| 10:58 |
Zeno` |
oops, wrong place. bbl :) |
| 10:58 |
|
Zeno` left #minetest-dev |
| 11:01 |
|
pozzoni joined #minetest-dev |
| 11:16 |
|
rubenwardy joined #minetest-dev |
| 11:21 |
|
pozzoni joined #minetest-dev |
| 11:29 |
|
neoascetic joined #minetest-dev |
| 11:32 |
|
damiel joined #minetest-dev |
| 11:36 |
|
MoNTE48 joined #minetest-dev |
| 11:37 |
|
iangp joined #minetest-dev |
| 11:37 |
|
pozzoni joined #minetest-dev |
| 11:47 |
|
rubenwardy joined #minetest-dev |
| 11:58 |
|
Darcidride joined #minetest-dev |
| 12:03 |
|
paramat joined #minetest-dev |
| 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 |
| 12:11 |
|
proller joined #minetest-dev |
| 12:12 |
|
MoNTE48 joined #minetest-dev |
| 12:16 |
|
pozzoni joined #minetest-dev |
| 12:31 |
|
pozzoni joined #minetest-dev |
| 12:38 |
|
pozzoni joined #minetest-dev |
| 12:50 |
|
STHGOM joined #minetest-dev |
| 12:50 |
|
fishandchips joined #minetest-dev |
| 12:56 |
|
pozzoni joined #minetest-dev |
| 13:04 |
|
pozzoni joined #minetest-dev |
| 13:05 |
|
MoNTE48 joined #minetest-dev |
| 13:10 |
|
pozzoni joined #minetest-dev |
| 13:16 |
|
pozzoni joined #minetest-dev |
| 13:23 |
|
pozzoni joined #minetest-dev |
| 13:28 |
|
fishandchips joined #minetest-dev |
| 13:29 |
paramat |
bbl |
| 13:29 |
|
paramat left #minetest-dev |
| 13:42 |
|
jin_xi joined #minetest-dev |
| 13:58 |
|
davisonio joined #minetest-dev |
| 14:08 |
|
yang2003 joined #minetest-dev |
| 14:14 |
|
Hunterz joined #minetest-dev |
| 14:33 |
|
Player_2 joined #minetest-dev |
| 14:44 |
|
kaadmy joined #minetest-dev |
| 14:45 |
|
Void7 joined #minetest-dev |
| 14:54 |
|
hmmmm joined #minetest-dev |
| 15:10 |
|
Foghrye4 joined #minetest-dev |
| 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:25 |
|
proller joined #minetest-dev |
| 15:29 |
|
davisonio joined #minetest-dev |
| 15:35 |
|
paramat joined #minetest-dev |
| 15:39 |
|
Krock joined #minetest-dev |
| 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:51 |
|
thePalindrome joined #minetest-dev |
| 15:54 |
|
Hunterz joined #minetest-dev |
| 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 |
| 16:33 |
|
DFeniks joined #minetest-dev |
| 16:44 |
|
paramat left #minetest-dev |
| 16:53 |
|
celeron55_ joined #minetest-dev |
| 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:15 |
|
turtleman joined #minetest-dev |
| 17:19 |
|
hmmmm joined #minetest-dev |
| 17:22 |
|
Zeno` joined #minetest-dev |
| 17:24 |
Zeno` |
It looks like I will see the sunrise yet again |
| 17:25 |
|
hmmmmm joined #minetest-dev |
| 17:25 |
Zeno` |
hi hmmmm and hmmmmm |
| 17:27 |
|
hmmmmmm joined #minetest-dev |
| 17:30 |
|
Darcidride joined #minetest-dev |
| 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:38 |
|
hmmmmm joined #minetest-dev |
| 17:39 |
hmmmmm |
- can you explain exactly what all the differences between generateBiomes() implementations are, and why they diverge? |
| 17:43 |
|
hmmmm joined #minetest-dev |
| 18:04 |
|
davisonio joined #minetest-dev |
| 18:18 |
|
Void7 joined #minetest-dev |
| 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 |
| 18:55 |
|
lisac joined #minetest-dev |
| 18:59 |
|
Void7 joined #minetest-dev |
| 19:25 |
|
DI3HARD139 joined #minetest-dev |
| 19:27 |
|
paramat joined #minetest-dev |
| 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 |
|
Topic for #minetest-dev is now Minetest core development and maintenance. Feature freeze (2 weeks): sometime later than 2016-04-27. Last release: 0.4.13, Aug 20 2015. Chit-chat goes to #minetest. Consider this instead of /msg celeron55. http://irc.minetest.net/minetest-dev/ http://dev.minetest.net/ |
| 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:44 |
|
hmmmm joined #minetest-dev |
| 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:54 |
|
ElectronLibre joined #minetest-dev |
| 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 |
|
electrodude512 joined #minetest-dev |
| 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 |
|
proller joined #minetest-dev |
| 19:59 |
paramat |
ok something to consider |
| 19:59 |
paramat |
i can work on it after release |
| 20:00 |
|
ssieb joined #minetest-dev |
| 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 |
|
sofar joined #minetest-dev |
| 20:03 |
|
sofar joined #minetest-dev |
| 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 |
|
technics joined #minetest-dev |
| 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:34 |
|
MoNTE48 joined #minetest-dev |
| 20:37 |
|
Amaz joined #minetest-dev |
| 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 |
|
Void7 joined #minetest-dev |
| 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:36 |
|
proller joined #minetest-dev |
| 21:39 |
paramat |
merged! |
| 21:40 |
paramat |
TNT is really good now. all game milestones closed :D |
| 21:44 |
|
paramat left #minetest-dev |
| 22:08 |
Fixer |
MTG ones? |
| 22:10 |
|
DFeniks joined #minetest-dev |
| 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:29 |
|
electrodude512 joined #minetest-dev |
| 22:46 |
|
paramat joined #minetest-dev |
| 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 |
| 22:56 |
|
paramat left #minetest-dev |
| 23:43 |
|
yang2003 joined #minetest-dev |
