Time  Nick      Message
00:03 Niebieski !seen zat
00:03 ShadowBot Niebieski: I haven't seen zat in #minetest-dev.
03:35 est31     paramat, don't forget to inform all devs of feature freeze dates. e.g. set the topic: do something like /msg ChanServ TOPIC #minetest bla bla
03:35 paramat   ok
03:36 est31     sofar i keep my mtgame contributions near-minimal, so you dont have to wait for me to replace ABMs with LBMs :)
03:37 paramat   hm #3605 is mergeable?
03:37 ShadowBot https://github.com/minetest/minetest/issues/3605 -- Play tool breaking sounds. by sofar
03:38 est31     lots and lots of discussion
03:38 paramat   and just found #3652 which has 2 approvals
03:38 ShadowBot https://github.com/minetest/minetest/issues/3652 -- Update Android depends, -O3 optimization, remove old arm config by MoNTE48
03:39 est31     when did it get its second approval
03:39 est31     aha...
03:39 est31     I think I'll merge that
03:39 est31     first I test whether android still works with it...
03:46 est31     hehe its already outdated
03:46 est31     there is openssl 1.0.2g lol
03:47 paramat   as you can see i've added 'one approval' 'two approvals' labels. if a PR is authored by a core dev perhaps that can be automatically counted as one approval?
03:48 est31     there was discussion about it in the past
03:48 est31     whether dev being author counts as approval
03:48 est31     I was on the side of people giving the author the approval badge :)
03:49 est31     man that was shit english
03:49 paramat   yeah many PRs are merged on author plus one approval, so ..
03:49 est31     I favoured counting the author as approval *
03:53 est31     I feel this PR is a bit inpolite against sapier
03:53 est31     because he proposed the same change as well
03:53 est31     but he didn't voice disapproval with the PR
03:53 est31     so I guess its okay with him
03:57 paramat   but then, there are cases where a core dev authors a PR but doesn't yet approve of it themselves, so it can't be automatic
03:57 paramat   depends on circumstances
03:57 est31     yeah
03:58 est31     I always wait until the author of a PR to merges it themselves if they haven't made it clear
03:59 est31     unless the PR is trivial in my eyes
04:09 ssieb     I'm seeing a bug where items with a formspec on right-click (e.g. furnaces) don't work right after you place them.  If I disconnect the client, then reconnect, they work.  Anyone else see this?  It's only started recently.
04:14 srifqi    it works for me
04:15 ssieb     how recent is your build?
04:15 srifqi    minetest build on af714c
04:17 est31     ssieb, that's general server lag
04:17 est31     not specific to furnaces I think
04:17 est31     the client predicts that you place something
04:17 est31     so it appears in the world
04:17 srifqi    that's also happened to me when the connection is slow
04:17 est31     but it cant predict that the something has a formspec
04:18 est31     it needs the server to send over the actually updated map
04:18 ssieb     that's pretty massive lag, because it never resolves :-)
04:18 srifqi    Why there are no valleys mapgen in settingtypes.txt ?
04:20 ssieb     also, the server is on the same network as me...
04:21 srifqi    change your server's configuration may helps you reduce lag
04:21 ssieb     the server log shows me placing the furnace
04:23 est31     well if you can reproduce the issue
04:23 est31     then its very easy to find out the cause
04:24 est31     ssieb, do you know how to git bisect?
04:24 ssieb     yes
04:24 est31     do it, finding out the causing commit helps alot in finding the cause
04:46 ssieb     hmm, it doesn't happen in singleplayer or local server, so it must be a mod or something on my server...
04:47 srifqi    did you test it in vanilla minetest game (no mod)?
04:47 ssieb     srifqi: the valleys mapgen isn't in the docs yet.  paramat said he was going to do something about that
04:47 ssieb     yes, that's what I mean.  It works there
04:48 srifqi    what are those mod installed?
04:49 srifqi    i'm working for #2561 now, so i asked for valleys mapgen
04:49 ShadowBot https://github.com/minetest/minetest/issues/2561 -- Add mapgen settings to create world dialog by srifqi
04:50 srifqi    *aksed about
05:08 paramat   oh yeah mg_name = valleys is missing from minetest.conf and settingtypes.txt docs, will fix that sometime
05:12 sofar     paramat: #3605 is mergable, yes.
05:12 ShadowBot https://github.com/minetest/minetest/issues/3605 -- Play tool breaking sounds. by sofar
05:12 sofar     paramat: note the linked patch to minetest_game as well
05:13 paramat   i was just wondering if the objections are reasonable enough for it to need more consideration
05:16 sofar     so the alternative patch
05:17 paramat   anyway i feel it can probably be merged
05:17 sofar     would require exactly the same changes to minetest_game to actually hook up default sounds
05:17 sofar     and more (the callback code)
05:17 sofar     so this is a smaller subset
05:18 sofar     making a callback for the sole purpose of playing an audio sample ... doesn't have my preference in this case
05:18 est31     yes but its more general
05:19 sofar     only hardly
05:19 est31     you can have the default callback be precisely that: play the audio sample
05:19 sofar     it would still require sound.break to be set
05:19 paramat   the objecter is not around anymore. i think we can merge it, i might do that tomorrow
05:20 est31     sofar, yes but you now have the option to override
05:20 sofar     there still is
05:20 sofar     and it's even further generic
05:20 paramat   (but then i don't understand it)
05:20 sofar     a chain of overridable callbacks isn't good design
05:21 sofar     the most generic callback is already implemented
05:21 est31     more generic callbacks often have a problem
05:21 sofar     it's not that hard to calculate the wear
05:21 est31     you have to guess what the engine code does
05:21 est31     but you still have to do it
05:21 est31     thats the point
05:21 est31     you have to keep track of the engine doing things
05:21 est31     if it changes, you have to change as well
05:21 sofar     that's always a risk
05:22 est31     thats why a callback as suggested by blockmen is better IMO
05:23 est31     similar to https://github.com/minetest/minetest/issues/3133
05:24 sofar     yes, but that's sensible and multipurpose
05:25 sofar     I'm not saying the argument is incorrect
05:25 sofar     I'm just saying that there's a place for callbacks, and I feel like this isn't one of them
05:25 est31     well idk, if its not requested, and nobody is coming up with any use cases except the one you implement in 3605, then we should merge 3605 and close the other PR
05:26 sofar     if people do come up with use cases, then we can expand/correct without much issues
05:26 srifqi    paramat: should i add valleys mapgen?
05:27 paramat   srifqi yes add it to your PR, the settings does work, it's just not documented
05:29 paramat   although of course it can't be selected in advanced settings because it's not present 'possible values'
05:30 srifqi    add another PR?
05:37 paramat   yes if you want, start a new PR to add mg_name = valleys to .conf and settingtypes
05:38 paramat   if you do that i'll merge it tomorrow or very soon
05:50 sofar     paramat: grep _script..on src/network/serverpackethandler.cpp
05:51 sofar     wrt callback on_flood()
05:51 paramat   ok thanks
05:52 srifqi    I've just update #2561 , has not rebased.
05:52 ShadowBot https://github.com/minetest/minetest/issues/2561 -- Add mapgen settings to create world dialog by srifqi
05:54 sofar     oh nice I've never seen that PR
05:58 paramat   cool
06:01 sofar     paramat: so you'll end up putting a ScriptApiNode::node_on_flood(v3s16 p, MapNode node) in script/cpp_api/s_node.cpp I guess, doesn't look like too much copy+paste code to make that work
06:02 srifqi    thanks, paramat
06:02 paramat   ok
06:02 sofar     maybe think about return values?
06:03 sofar     although void / no return value seems appropriate
06:03 sofar     well hmmmm
06:03 sofar     no handler: core removes node
06:04 sofar     handler present: lua code can drop item, but node is getting removed anyway, so return value doesn't matter?
06:05 sofar     wait no, I think we want this to be like on_blast
06:06 sofar     no no not on_blast, that returns an itemstack
06:07 sofar     on_burn isn't it either
06:09 sofar     I think it can just be a void. core should remove the node from the map before or after the callback, pass the node/pos to the callback if present
06:12 est31     note that I'll -1 every PR to the engine that floods plantlike by default
06:12 est31     I really dont like water messing that much with worlds
06:12 est31     water is safe, that's good
06:12 sofar     torches, though, right?
06:13 sofar     ;^D
06:13 est31     fire can be easily disabled
06:13 est31     but well thats mtgame
06:13 est31     i dont want to start ranting about fire again :)
06:13 sofar     oh, so you'd be ok with farming:* being floodable?
06:14 sofar     easy to mod that in, as well
06:14 est31     well games can do whatever they want
06:14 est31     with the engine
06:15 est31     but default flooding everything away?
06:15 est31     thats griefers wet dream
06:20 paramat   heh literally
06:21 sofar     right, but might be nice for singleplayer focussed games
06:26 paramat   it will be calls to on-flood functions. i will oppose making plants floodable. torches and fire perhaps ok
06:26 sofar     paramat: btw you wouldn't have to do any registration of on_flood() callbacks, since you know the nodedef when you flood it and just check if that nodedef has a callback
06:27 paramat   yeah
06:28 sofar     so relatively cheap and not very complex
06:28 sofar     I'd have a half-assed implementation already but I don't know the water code, at all ;)
06:31 srifqi    Just opened a PR #3815
06:31 ShadowBot https://github.com/minetest/minetest/issues/3815 -- Add forgotten valleys mapgen in mapgen name by srifqi
06:40 paramat   thanks
06:41 paramat   i'll merge it tomorrow (later today, whatever)
06:42 est31     forgotten valleys
06:42 est31     sounds like an idea for a mapgen :)
06:44 srifqi    :D
06:44 srifqi    maybe something like "old" map?
11:42 celeron55 i don't know about this menu redesign anymore
11:43 celeron55 i don't like the current menu, but on the other hand i'm not sure about any other design either
11:51 celeron55 meh, i guess i'll continue enough to have something to show
12:23 srifqi    what menu did you mean, celeron55?
12:24 yang2003  Hmm... does new minetest coming in a week?
12:32 kaeza     >[One approval]
12:32 kaeza     is it really necessary to have tags for everything?
12:33 sfan5     no
12:33 sfan5     but this tag is useful
12:35 kaeza     is it?
12:36 celeron55 yang2003: it looks like maybe at the end of this month
12:37 yang2003  nice
13:46 celeron55 lol... i just did this: https://github.com/celeron55/minetest/commit/48304d1ada77f0c25e4e58ac5a00bacaaf94563b
13:48 celeron55 this is an example menu that just strips out everything and automatically creates a world when you press "play": https://gist.github.com/celeron55/5f6e5d28caf358684d5c
13:50 celeron55 wuzzy will like this, but i wonder what others do
13:51 yang2003  nice work
13:51 yang2003  I like it
13:51 VanessaE  celeron55: screenshot?
13:51 celeron55 (it's hacky as hell; i haven't bothered cleaning stuff up)
13:51 VanessaE  (too lazy to compile it :P )
13:52 celeron55 that whole branch is actually all lua and a few textures but, eh, http://i.imgur.com/Be2tPed.png
13:52 celeron55 did you really need to see this? :P
13:52 VanessaE  heh
13:52 VanessaE  I suppose not :)
13:53 celeron55 the other parts of the menu you're going to have to try yourself
13:53 celeron55 in fact i would like it if you did
13:54 celeron55 because i'm not going to push this forward unless at least some people like it
13:55 VanessaE  I'll pass.
13:55 VanessaE  but that screenshot/mockup you shared a few days ago looked good.
13:57 celeron55 well, take this then http://imgur.com/a/gFWAy
13:59 Fixer     i think current one is better and more readable
13:59 VanessaE  not bad, but the subgame icons being shown on the opening screen at all doesn't work.
14:00 VanessaE  they should be placed behind a menu similar to the others.
14:00 celeron55 too many clicks
14:00 VanessaE  four icons to open - Play Single-player, Host Game, Join Game, Settings.
14:00 VanessaE  behind the Single-player option would be those subgames.
14:00 celeron55 it's a click more
14:01 VanessaE  that's okay.  it's about the path the player follows to get started isn't it?
14:01 VanessaE  putting those games on the opening screen will confuse some people.
14:02 celeron55 aat least you are not suggesting to have the ridiculous subgame bar always visible on the single-player screen?
14:02 celeron55 at*
14:02 VanessaE  nah
14:02 Fixer     white text on near white background is not good
14:02 VanessaE  he just has the menu dialog background turned off, Fixer
14:03 celeron55 it's trivial to darken it like this http://i.imgur.com/eHcD8Wn.png
14:03 celeron55 but a menu background that doesn't fill the window doesn't look good in that style
14:03 Fixer     it still feels somewhat crappy
14:04 celeron55 http://i.imgur.com/ynvB0j0.png
14:04 celeron55 so is this what we want?
14:04 Fixer     https://i.imgur.com/FirHhjD.png wth is this?
14:04 Fixer     no!
14:04 VanessaE  celeron55: not really, no.
14:05 VanessaE  Fixer: that's a subgame + map that's meant to help the player learn to play
14:05 VanessaE  I forget who made it
14:05 yang2003  Hmm...
14:05 celeron55 Fixer: it's this because this is the only subgame that i so far know to want a custom menu: https://forum.minetest.net/viewtopic.php?f=5&t=10312
14:05 Fixer     VanessaE, big black Tutorial and two grey buttons? %)
14:05 VanessaE  Fixer: well talk to Wuzzy about the style of it then :P
14:06 Fixer     please don't do it like that
14:06 celeron55 none of this discussion should be about style
14:06 celeron55 we need to talk about UI logic and functionality
14:06 VanessaE  celeron55: agreed.
14:06 Fixer     ah
14:06 Fixer     i was talking about style, nevermind
14:07 celeron55 given that the icons were on their own screen, what's the next issue then?
14:07 Fixer     Singleplayer | Network | Settings
14:08 yang2003  Hmm..
14:09 VanessaE  not sure what's next, actually.
14:09 Fixer     icons below can be somewhat confusing, need some kind of Subgame -------------
14:09 celeron55 having the client (join game) and the server (host game) functionality on the same screen as tabs is confusing in my opinion
14:09 VanessaE  Fixer: I already said that.
14:09 celeron55 that's why i separated them
14:09 Fixer     also, what to do with tabs?
14:09 Fixer     tabs should be separated?
14:09 celeron55 ?
14:10 Fixer     pic 1 has icons, but in other places it shows tabs again
14:10 Fixer     just thinking
14:10 yang2003  I thinks...
14:10 yang2003  The texture can be square texture
14:10 celeron55 i think the tabs work fine on the sub-screens but don't suit a launcher screen like that
14:10 yang2003  Just suggest
14:12 celeron55 i wonder if we even have a single person who actually knows jack shit about UI design
14:12 yang2003  XD
14:12 Fixer     maybe open an issue on github and start discussion there, maybe we need some kinds of examples from other games
14:12 Fixer     this mocap feels overcomplicated
14:13 celeron55 other games don't really do this thing where there actually are multiple games inside the game but not if you connect to a server, and not necessarily if you select a world first, and then all settings are still global and then there are mods that kind of apply to everything and nothing at the same time and...
14:14 celeron55 it gets complicated because minetest operates in a quite non-traditional way
14:15 est31     Well on one hand the main menu gradually converged to its current state and hasn't much changed in the past. On the other hand, if we don't experiment with new stuff we are a dead project lol
14:15 celeron55 one thing that could be done would be to make the menu focused on worlds
14:16 celeron55 i.e. the home screen options would be something like "select/create world", "join server" and "settings"
14:16 celeron55 then when you select a world, you would choose whether to play singleplayer or host a game
14:16 yang2003  Hmm...
14:16 yang2003  Yes
14:16 yang2003  I like this
14:17 celeron55 but then you don't get to have game-specific singleplayer screen, if that's what's wanted
14:17 yang2003  I agree
14:18 celeron55 and the world list will be very long if you have many subgames
14:18 celeron55 but of course there would be an option in the world selection screen to filter by subgame
14:18 yang2003  yup
14:18 celeron55 so it's not really an issue
14:19 yang2003  Hmm...
14:19 celeron55 est31: i don't think the menu has converged anywhere; it's the same menu as it was in the 0.2 days; just new stuff tacked to it in whatever way it happened to fit
14:19 celeron55 and with massive implementation changes
14:23 huang2003 hi all
14:25 huang2003 wb
14:27 Fixer     i do like current menu, what I don't like is too much tabs, instead make singleplayer | network | settings | credits, mods and texture packs move into settings or in current world config
14:27 Fixer     current minetest one, not mocup
14:27 huang2003 oh i think it was a good idea
14:28 Fixer     also, make public server list on by default
14:28 huang2003 oh,it sounds good
14:28 celeron55 huang2003: please can you just leave, we need to discuss this among more experienced developers
14:28 huang2003 ok,i will
14:33 celeron55 Fixer: so... more tab layers?
14:34 celeron55 i don't know about this
14:35 celeron55 well, i did what i set out to make a few hours ago; now it's up to feedback what might happen to the menu
14:36 celeron55 i don't want to make a PR because this is just an experiment
14:36 celeron55 well i guess i could
14:36 twoelk    uhm - from where do you open the tutorial screnn and will all games have such a screen?
14:37 twoelk    *screen
14:42 celeron55 https://github.com/minetest/minetest/pull/3818
14:46 twoelk    actually I really like the idea that subgames can start with a simple info page
14:48 twoelk    or rather information can be served before a game is started
14:49 est31     I like the fact that its written out "Host Game"
14:49 est31     and not just "Server"
14:49 est31     if you are new to minetest you dont know what it means: host server? join server?
14:50 VanessaE  out of curiosity,
14:50 VanessaE  why even separate "singleplayer" from "host game" in the first place?
14:50 est31     same for join game
14:51 VanessaE  have the engine just set a default port, require the user to enter their name, and not allow anyone to log in remotely with that name (hence no password would be needed)
14:52 VanessaE  if the user wants to just play singleplayer, they should be able to just "start" without so much as typing in their name (in which case "singleplayer" becomes the username)
14:53 est31     there are some conditions here
14:53 est31     if the default port is taken, it should not end the server or crash
14:53 VanessaE  agreed.  just pick a new port automatically.
14:54 est31     and if you do something like that you still need to provide info for how to join
14:54 est31     thats perhaps what the LAN tab is about...
14:54 twoelk    having to do with little kids, their parents and teachers now and then I think to have a clear boundary between closed-local-game and open-to-others-game is important to many people (parents-teachers)
14:54 VanessaE  well sure, but you can just pop up a dialog box on join
14:54 VanessaE  my point is to avoid presenting a separate LAN tab or similar, at all
14:54 est31     idk there sure is some way to do network auto discovery
14:55 VanessaE  and as we all know, running server+client as two separate processes usually results in a more fluid game
14:55 est31     as long as anti virus doesnt think we try to add the distinctiveness of other computers in the lan to our own :p
14:55 VanessaE  heh
14:56 est31     they already run as separate threads btw
14:56 VanessaE  perhaps but for some reason, having them as actual separate processes seems to work better for some people.
14:56 twoelk    can confirm
14:57 Fixer     how about that mocup? https://i.imgur.com/BPYP1dQ.png mods and texture pack will be in Configure (for each world)
14:58 Fixer     first tab can be either Network or Singleplayer
14:58 Obani     Fixer : yes
14:58 Fixer     maybe rename Network to Join server
14:59 Obani     Also Network is clearer than Client but still not a good word IMO
14:59 Obani     Multiplayer ?
14:59 Obani     Online ?
14:59 Fixer     Online
14:59 Fixer     or Internet
14:59 Obani     Internet is false :p
14:59 Obani     Online imo
15:01 twoelk    start-own-game  and  join-running-game  would be more exact but much too long labels
15:02 Fixer     Start | Join | Settings | Credits
15:02 Fixer     ._.
15:03 srifqi    Are you sure about this?
15:03 srifqi    0.4.14 ?
15:03 Obani     Fixer, Solo | Online | Settings | Credits
15:03 twoelk    Online is not always correct
15:04 twoelk    you can connect to a game on the same machine
15:06 sfan5     merging https://patch-diff.githubusercontent.com/raw/minetest/minetest/pull/3815.patch in 5 minutes (trivial)
15:06 est31     sfan5, +1
15:07 twoelk    from my experience many parents or teachers would love to be able to hide the internet but allow lan
15:07 Fixer     hmm
15:07 Fixer     we forgot about LAN
15:08 Fixer     can MT auto discover servers on lan?
15:08 Fixer     probably not
15:08 celeron55 that feature should be added
15:09 celeron55 i guess it's just a matter of making the server send some broadcast UDP packets
15:10 est31     that should do it probably
15:10 srifqi    maybe check for each IP 192.168.x.y ?
15:10 sfan5     flooding the network is always a good idea
15:10 est31     nah thats a bad idea
15:10 sfan5     also do you want to check each port?
15:11 est31     we could ping on a certain port hardcoded into minetest
15:11 twoelk    maybe a set of 5 or 10 hardcoded ports
15:12 twoelk    schools do use differing internal ip sets sometimes though
15:13 sfan5     that wouldn't be a problem
15:13 celeron55 the server can just broadcast a packet every 10 seconds telling its ip and port
15:13 celeron55 it's pretty much the de-facto way of doing this i think
15:13 srifqi    where to broadcast?
15:13 celeron55 everywhere
15:13 sfan5     192.168.x.255
15:14 celeron55 255.255.255.255
15:14 sfan5     that works too
15:14 celeron55 it won't get to the internet, ISPs block them
15:14 est31     or the client could broadcast
15:14 est31     hello i am client i want servers
15:14 est31     server then answers
15:14 est31     then there is no waste
15:14 Fixer     10.x.x.x or so
15:14 est31     and its super fast
15:14 sfan5     are you afraid of wasting local network bandwidth?
15:14 est31     yeah broadcasts wont get to the internet
15:15 nore      there should however be a setting to disable this
15:15 nore      for those on big local networks (students for example)
15:15 sfan5     might be useful to automatically disable it when server_dedicated is set to true
15:16 celeron55 eh
15:16 celeron55 it's a separate option just like publishing to internet
15:16 est31     sfan5, no i mostly care about speed celeron55 suggested having it broadcast every ten seconds, thats slow in my eyes
15:16 est31     but i dont want it to spam the local network with broadcasts
15:16 est31     and there is no need to
15:16 sfan5     every other program does it like that too
15:17 twoelk    maybe only on request - like a button -searchnow-
15:17 est31     twoelk, that's what I am suggesting, the client searching for the server, not the server searching for clients
15:17 nore      twoelk: +1
15:18 srifqi    ^ +1
15:18 est31     and not even a button needed, just send a ping every second or so if the client has the tab open
15:20 nore      est31: a button would be better though
15:20 nore      to avoid spamming the local network
15:20 sfan5     why?
15:21 sfan5     if you have the tab open it makes sense to search for servers
15:21 Fixer     checkbox Lan
15:21 est31     yeah
15:21 VanessaE  how does CUPS do it?
15:21 Fixer     turned off by default %)
15:21 VanessaE  (it has local auto-discovery of printers)
15:22 est31     RFC 6763
15:22 est31     thats used for the auto discovery part
15:23 VanessaE  oof.. overkill
15:25 est31     it seems to work using standard DNS queries
15:26 est31     so client asks, server responds model or something like that
15:31 kahrl     what about good old multicast/IGMP?
15:32 kahrl     server does a setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP, <multicast ip>)
15:32 kahrl     client then sends the discovery packet to the multicast ip
15:34 est31     it requires ipv4, no?
15:34 kahrl     I'm pretty sure ipv6 supports it too, but I might be wrong
15:35 est31     idk though, I guess its not that big of a network compat issue here, we can surely change LAN auto discovery protocols if we want to support ipv6
15:38 kahrl     actually I guess that does the same thing like the broadcast in almost all situations
15:38 kahrl     since multicast makes use of the ethernet broadcast address FF:FF:FF:FF:FF:FF
15:38 kahrl     so in a LAN it would still go to all hosts
15:38 kahrl     the difference would be that it could be routed if someone cared to configure a router for that
15:40 kahrl     ah, no
15:40 kahrl     there are special MAC addresses in ethernet that correspond to ipv4/ipv6 multicast addresses
15:41 kahrl     so decent ethernet switches would deliver these packets only to hosts that want them
15:44 est31     IGMP has leave messages
15:44 est31     thats good
15:48 twoelk    like: your server has left, deal with it?
15:48 est31     yes
15:48 est31     dont click at it thinking it still runs and then get a timeout
15:51 twoelk    the client not telling me that you are disconnected did happen to me quite often on my crappy internet-line - many blocks placed in vain
15:53 est31     it should work like we have it in the minetest protocol:
15:53 est31     there are ping requests, and for a normal leave there is an extra request
15:54 est31     so for the normal leave you get a fast response that the server is shutting down
15:54 est31     no timeout
15:54 est31     and if the internet dies its handled by the ping
17:09 est31     kahrl, btw if you want to review a PR, my LBM PR is ready
17:09 est31     #3677
17:09 ShadowBot https://github.com/minetest/minetest/issues/3677 -- Add minetest.register_lbm() to run code on block load only by est31
17:33 kahrl     est31: ah nice, I will soon
18:11 kahrl     est31: question, what is the motivation behind nameHash?
18:13 est31     kahrl, its so that I dont have to handle that many strings inside the code
18:14 est31     but you are the second reviewer critical of it perhaps I remove it entirely
18:14 kahrl     for performance reasons?
18:14 est31     yes
18:15 kahrl     ah, is it still relevant since m_lbm_lookup is no longer referenced by nameHash but by time?
18:16 kahrl     everything else that uses nameHash seems to be only done on initialization
18:18 est31     yes m_lbm_lookup is referenced by time
18:18 est31     why is it a u64 map
18:18 est31     thats wrog
18:18 est31     wrong*
18:20 kahrl     yeah, I was going to mention that too
18:32 est31     what do you think, keep the hash or remove it?
18:32 kahrl     I'd remove it (unless I missed a spot where it's used in performance critical code)
18:33 kahrl     the extra code complexity and (small) risk of collisions imho isn't worth slightly faster initialization
18:33 hmmmm     i don't think minetest should reinvent its own hash table data structures when we already have the c++ standard library
18:34 kahrl     well, we don't really
18:34 kahrl     we can't use unordered_map
18:34 est31     the pr uses the already existing std::map
18:34 est31     just with different key type
18:35 hmmmm     C++11 support is coming soon
18:35 est31     when was precise end date?
18:35 est31     somewhen in 2017
18:35 hmmmm     if you want something to really be a hash table, then why not use a typedef for the container type and add a //TODO(namehere): change this to unordered_map
18:37 est31     C++11 has auto for iterators right?
18:37 hmmmm     yup
18:37 est31     because std::map<std::string, u32>::const_iterator is long already
18:37 est31     no need for "unordered_"
18:38 hmmmm     doesn't really matter, if you typedefed the container you wouldn't need to retype any of that
18:38 hmmmm     ThingMap::const_iterator
18:39 kahrl     I think I see a use-after-free
18:40 kahrl     use-after-delete, whatever
18:40 kahrl     you have LBMContentMapping as the value type in lbm_lookup_map which is a map
18:40 kahrl     so the map might copy those things around
18:41 kahrl     but when *any* of the copies of the LBMContentMapping is destructed, the contained LBMs are deleted
18:41 kahrl     even when there are still other copies of the LBMContentMapping
18:42 est31     aha
18:43 est31     you say that the map doesnt keep the stuff at one place, but copies it around?
18:43 est31     this made the set-up just a bit harder
18:43 kahrl     yeah, I'm pretty sure it does
18:43 kahrl     well, or uses move constructors, dunno; it doesn't matter here
18:44 est31     once the stuff lands in LBMContentMapping it isnt supposed to be moved around much
18:44 nrzkt     est31: undordered map can't take a std::string as key
18:45 est31     why
18:45 nrzkt     only integers are allowed by this type
18:45 nrzkt     try yourself :)
18:45 kahrl     I think you just have to delete the LBMs from the LBMManager destructor
18:45 kahrl     and remove the LBMContentMapping destructor
18:45 est31     that was the original goal that should happen
18:46 est31     the LBMManager destructor automatically destructs lbm_lookup_map
18:46 est31     which then deletes all the LBMContentMapping and the pointers
18:47 celeron55 unordered_map can take std::string as key
18:47 kahrl     I mean explicitly loop over the mappings and the LBMs in them and delete the LBMs
18:47 est31     yeah
18:47 celeron55 (it's just hash table in that case)
18:47 est31     seems I have to do some loops i wanted to avoid
18:48 est31     idk doesnt become an iterator become invalid
18:48 est31     ah yeah
18:48 est31     will do end != begin check or something
18:48 est31     thats most safe
18:48 est31     then delete them one by one
18:48 kahrl     the other option would be to make the lbmdef refcounted, and maybe add proper copy constructors / operator= to LBMContentMapping
18:48 kahrl     which seems more complicated
18:49 est31     agreed
18:53 nrzkt     celeron55, yes you are right, it's not std::string the problem :) sorry. It's irrlicht types. We will need to overload them with the hashing functions to permit using std::unorered_map on them
18:54 est31     or we convert them to std strings as soon as possible
18:54 celeron55 yes, you can define the hash functions for anything of course
18:55 celeron55 i guess minetest doesn't do it anywhere for any type yet
19:05 nrzkt     VBO was a good patch. On my Radeon HD 7970 with Mesa 11.1 i now have 60 with 215 node limit. Before 85
19:07 est31     60 drawtime?
19:07 nrzkt     fps :)
19:08 nrzkt     and 85 nodes
19:17 est31     ok PR is updated
19:24 est31     kahrl ?
19:29 kahrl     looking
19:31 est31     I haven't tested the latest version, but if I get a +1 I'll do another test to be certain nothing broke
19:31 est31     (i cant imagine that sth broke)
19:31 kahrl     I notice several places where you do a find() on a map, then if that fails you insert an item at the same key
19:31 kahrl     that can be shortened by just doing map[key]
19:31 est31     yeah c++14 or something has a method for that
19:32 est31     or...
19:32 est31     perhaps we are talking about different usecases
19:33 kahrl     C++03 also guarantees that map::operator[] inserts a key/value pair if the key doesn't exist yet
19:33 kahrl     no need for C++14
19:33 est31     ah
19:34 kahrl     (just checked the standard to be sure: 23.3.1.2)
19:35 est31     so its like lua tables in that case
19:35 est31     thats good
19:37 est31     in fact the code is really a bit stupid
19:38 est31     at some places its required though
19:38 est31     in LBMManager::loadIntroductionTimes
19:38 est31     to declare lbms_we_introduce_now
19:39 est31     and lbms_running_always
19:39 est31     aww
19:39 est31     not even required there
19:39 est31     and just found a bug lol
19:39 est31     lbm_lookup_map::iterator jtt = m_lbm_lookup.find(U32_MAX);
19:40 est31     if (jtt == m_lbm_lookup.end()) { LBMContentMapping m; m_lbm_lookup[now] = m; jtt = m_lbm_lookup.find(now); }
19:40 est31     copy paste mistake
19:41 kahrl     oh, right :)
19:43 sofar     est31: ping me if you want me to retest
19:43 est31     oh thats cool, thanks, will do, if the PR seems ready.
19:47 est31     ok PR updated kahrl
19:49 kahrl     commented 2 lines in s_env.cpp for style
19:49 kahrl     otherwise code style looks good
19:50 est31     fixed
19:50 est31     there is one issue perhaps remaining
19:50 est31     dunno if its an issue
19:50 est31     just noticed it
19:51 est31     when I compile the engine, and then checkout another version of minetest e.g. to edit code, and then start the engine
19:51 est31     then I get a pretty unfriendly error message
19:51 kahrl     hmm
19:51 est31     my guess the luaL_checktype in s_env.cpp is failing
19:51 kahrl     what does it say?
19:52 est31     PANIC: unprotected error in call to Lua API (bad argument #4 to '?' (table expected, got nil))
19:52 ShadowBot https://github.com/minetest/minetest/issues/4 -- cppcheck warnings
19:52 est31     hah
19:52 kahrl     oh, yeah, that makes sense
19:53 kahrl     the C++ side is still looking for core.registered_lbms but the lua builtin side is a different version that doesn't provide that
19:53 kahrl     until you rebuild the C++ side
19:53 est31     its copied from ABMs so similar behaviour existed already
19:54 kahrl     I don't think that is really a problem, builtin is tightly coupled to the engine
19:54 est31     dunno if its worth fixing, i have no idea how to do "protected" error propagation
19:54 est31     yeah
19:54 kahrl     and you are expected to recompile the C++ code if you change builtin version
19:56 kahrl     though you can make it more robust by getting rid of the luaL_checktype
19:56 kahrl     the type is checked below anyway by lua_istable
19:57 est31     its sort of an internal engine error
19:57 est31     perhaps FATAL_ERROR_IF?
19:57 est31     or FATAL_ERROR
19:58 kahrl     yeah, I guess it's best to alert the user if builtin and core versions don't match
19:58 kahrl     other things might be out of sync too
19:59 est31     my main concern is that the error message is very confusing
19:59 est31     I'll fix that I think
20:09 est31     hmmm I dont know whether FATAL_ERROR calls lua stuff or not
20:11 est31     I'll keep an empty stack behind just to make sure
20:13 est31     okay kahrl uploaded the new code
20:25 est31     kahrl, do you want some time to review the PR?
20:25 est31     it would be great if it went in during the weekend
20:31 est31     I'll go i think
20:31 est31     sofar, if you want you can test the current state of the PR
20:32 est31     not guaranteed to be the last one though
21:46 paramat   i'll merge games 891 799 892 880 later
23:00 sofar     ShadowNinja: the problem I have with "front" (e.g.) is that usually front is the south side of the node with facedir=0 ;)
23:01 sofar     but yes, both will do
23:10 sofar     yuck, I messed up top/bottom with up/down
23:10 * sofar   fixes
23:52 sofar     I'm making a mess of things