Time  Nick      Message
04:25 Zeno`     hmm
04:25 Zeno`     hmmmm, finished review
04:26 hmmmm     sweet
04:27 hmmmm     Zeno`:  the way I understood the terms when i first saw them somewhere, a shallow copy is exactly that, storing a pointer, whereas a deep copy is copying the memory contents
04:27 Zeno`     only minor stuff
04:27 Zeno`     hmmmm, yeah I understand it differently but *shrug*
04:28 hmmmm     perhaps i could avoid the terminology debate by rephrasing it altogether
04:28 Zeno`     I understand a shallow copy as copying the struct (for example) but not the contents of any pointers in the struct itself (i.e. they will still point to where they point to in the original struct)
04:29 hmmmm     ahh
04:29 Zeno`     In a deep copy it would recursively copy anything pointed to in the struct as well
04:29 hmmmm     i think that's a better use of the term
04:29 hmmmm     you're right i'm just going to change it completely
04:31 Zeno`     :)
04:31 Zeno`     I think all of my review comments are about comments (or just questions)
04:31 Zeno`     it looks good
04:33 hmmmm     what do you prefer
04:33 hmmmm     if (foobar != NULL) or if (foobar) ?
04:34 hmmmm     i've always done the latter but maybe it's best to be as explicit as possible
04:35 Zeno`     yeah explicit is ok
04:36 Zeno`     I prefer the other way but I don't really care when reading code as long as it's all consistent
04:36 Zeno`     I think maybe the ones I picked out are in different files which is why it may not have been noticeable until it was in diff format on github
04:37 hmmmm     aaa
04:37 hmmmm     you didn't point out the possible null dereferences in map.cpp
04:37 hmmmm     that's sort of my no-brown-m&ms canary :)
04:38 hmmmm     was waiting to see if any reviewers would pick up on it
04:38 Zeno`     which possible null deref?
04:38 hmmmm     getMapgenParams()->chunksize
04:38 Zeno`     I left that for you to find
04:38 hmmmm     'left as an exercise to the reader'
04:38 Zeno`     =)
04:39 hmmmm     heh
04:39 hmmmm     yeah, idk what to do about that
04:39 hmmmm     how would i handle it if getMapgenParams() returned NULL in those functions?
04:39 hmmmm     so my thinking was maybe getMapgenParams should be guaranteed to never return NULL
04:40 hmmmm     but mehhhhh.... idunno
04:40 Zeno`     yeah making the guarentee seems the most reasonable way if it can be done
04:40 hmmmm     too many allocations reallocations deallocations for something that doesn't ultimately matter because in practice none of those will ever get called before mapgenparams are made
04:41 Zeno`     maybe put asserts there... would slow down debug builds (very slightly) but at least the guarantee is made explicit even if it's not really yet lol
04:41 Zeno`     I dunno
04:41 hmmmm     from an interface point of view I know that creating a copy of the mapgen_params early makes the most sense
04:42 hmmmm     but it just feels excessive
04:42 hmmmm     still not 100% sure what i wanna do
04:42 Zeno`     yeah I'll let you think on that :)
04:42 hmmmm     in fact, the entire class MapSettingsManager feels excessive to me
04:42 hmmmm     what do you think?  too much stuff for a really simple task?
04:43 Zeno`     well... it didn't jump out at me as too much
04:43 Zeno`     probably because it seems clearer than how it was before anyway
04:43 hmmmm     yeah
04:43 hmmmm     now i've made the part where each source of parameters comes together explicit
04:44 hmmmm     and i made their precedence as obvious as possible
04:47 Zeno`     yeah. I like that
04:48 Zeno`     the only bit I didn't understand was where I said "Is this to stop setting a map setting or overriding meta after mapgen_params has already been initialised?"
04:48 Zeno`     which a comment would fix
04:48 Zeno`     the rest was very readable
04:48 hmmmm     gotcha
04:48 hmmmm     i'll add a comment for that too
05:24 hmmmm     i just caught a fatal bug rofl
05:24 hmmmm     https://github.com/kwolekr/minetest/commit/30b77a79505f4e32df5d00d0c8e2ea50f63900c3#diff-5c9fad38a1e2b7a0227fd3f5282dcc09L1393
05:27 Zeno`     a blank line?
05:27 hmmmm     missing defs for get/set_mapgen_setting_noiseparams
05:27 hmmmm     this is what i get for not testing everything
05:27 Zeno`     oops
05:34 hmmmm     i found a bug with CreateAllDirs
05:34 hmmmm     it needs to be passed a path with no file or else it'll create the filename too lol
05:34 hmmmm     (or is that a bug?)
05:35 Zeno`     i think it should be a bug
05:35 Zeno`     err
05:35 Zeno`     hmm
05:35 Zeno`     it would make sense that it *didn't* create a file
05:36 Zeno`     doesn't match with the function name for a start
05:36 hmmmm     /usr/home/fred/minetest/foobar.txt
05:37 hmmmm     obviously foobar.txt isn't intended to be a directory here
05:37 hmmmm     but if that path were /user/home/fred/minetest/foobar.txt/, not so clear
05:37 hmmmm     ending with a trailing dir delimiter means the path is all dirs imho, whereas ending with no dir delimiter should represent a path ending with a file
05:38 hmmmm     ?
05:40 Zeno`     Yeah I'm not sure
05:40 Zeno`     your definition makes sense though
05:42 Zeno`     I don't get it though
05:42 * Zeno`   checks man mkdir
05:43 hmmmm     well mkdir is obviously going to treat the last path component with no trailing dir delimiter as a directory
05:43 hmmmm     due to the context
05:43 Zeno`     err CreateAllDirs() --> CreateDir() --> mkdir()
05:43 Zeno`     how is it creating the file?
05:43 hmmmm     oops :)
05:44 hmmmm     not creating the file, creating a directory that has the same name as the file
05:44 Zeno`     oh ok
05:44 Zeno`     yeah I think that would be a bug
05:44 Zeno`     I *think*
05:44 hmmmm     i've decided that my expectations for this function is probably wrong
05:44 hmmmm     it's called CreateAllDirs
05:45 hmmmm     not CreatePathUpTo
05:45 hmmmm     s/expectations/expectation/
05:45 hmmmm     I'm gonna add a unit test to make sure this stupidity doesn't happen again
05:46 Zeno`     Actually it's not a bug in CreateAllDirs() though because that should behave the same as mkdir() which it does
05:47 Zeno`     I guess it depends on how it's called :/
05:47 hmmmm     yeah
05:47 hmmmm     i added a RemoveLastPathComponent before the call to CreateAllDirs'
05:48 hmmmm     so it should be ok
05:51 hmmmm     thanks for the review Zeno`
05:51 hmmmm     I'm gonna write up some more unit tests and do a little bit more testing and then i will be satisfied
05:51 Zeno`     that's ok... I didn't build and test though
05:51 hmmmm     would you like to do a re-review after I make my changes?
05:52 Zeno`     if they're significant changes
05:52 hmmmm     well
05:52 hmmmm     here are the non-unit test changes
05:52 Zeno`     I don't want to review unit tests so I'll just quickly look through what you're about to provide :)
05:54 hmmmm     ehehe just compiling
05:57 hmmmm     https://github.com/kwolekr/minetest/commit/f809cbe0888e9f4b8cf15f0afc60a6cefb97f89e
05:59 hmmmm     i've decided on that comment in the mapsettingsmanager constructor: no, leave it the way it is, have the caller check for NULL or have knowledge of the object's state before using it
05:59 Zeno`     yeah
06:01 Zeno`     those changes all look good
06:01 Zeno`     I still think there should be some in-code comment regarding https://github.com/kwolekr/minetest/commit/30b77a79505f4e32df5d00d0c8e2ea50f63900c3#diff-a3f773aebe1ab61ac758488712d8dd8dR75
06:02 Zeno`     but apart from that +1
06:03 hmmmm     oh, in the code?
06:03 hmmmm     i put it in the header file
06:03 Zeno`     oh is it?
06:03 Zeno`     that's ok then
06:03 Zeno`     oh I see it
06:03 Zeno`     yeah that's fine
06:04 hmmmm     sweet
06:04 hmmmm     gonna just code up the rest of the unit tests, do some more manual testing with the lua apis, and then i should be good to go
06:05 Zeno`     fun times :)
06:13 Zeno`     I wonder if v3s16 EmergeManager::getContainingChunk(v3s16 blockpos)  should really be moved as well
06:13 Zeno`     meh
06:14 Zeno`     forget it I was reading something wrong
06:14 hmmmm     yeah
06:15 hmmmm     the reason why those were there to begin with was... my reasoning was that they're like static functions except they only require one piece of context, the mapgen params
06:15 hmmmm     and since emergemanager managed mapgenparams, why not put it there?
06:15 hmmmm     but they don't quite fit tbh
06:15 Zeno`     made sense at the time
06:15 Zeno`     but they're better in ServerMap
06:16 hmmmm     yup
06:16 Zeno`     only other option would be to make them friend functions which is kind of silly
06:16 hmmmm     i've been thinking a lot lately about what the roles are for each class in the heirarchy of minetest
06:16 hmmmm     a lot of my earlier stuff isn't the greatest and i'm trying to fix that
06:17 Zeno`     continual improvement
06:17 hmmmm     (but on that note:  the code that wasn't made by me is downright atrocious :P)
06:17 Zeno`     we'll make you our QMS ISO 9001 (or whatever it is) officer!
06:17 hmmmm     heh ISO9001
06:18 Zeno`     you'll spend 15 hours a day filling out useless forms and other paperwork
06:20 hmmmm     we'll become a CMMI level 5 certified organization in no time
06:27 Zeno`     yay
06:42 Hijiri    so does that stuff about CreateAllDirs earlier change the behavior of minetest.mkdir?
06:42 Hijiri    or was that just something used internally in minetest
06:46 Hijiri    I checked, minetest.mkdir does use CreateAllDirs
06:46 Hijiri    Doesn't this conflict with the API in lua_api.txt?
06:46 Hijiri    oh
06:46 Hijiri    you were using CreateAllDirs in another function
06:46 Hijiri    sorr
06:46 Hijiri    sorry*
06:49 paramat   zeno thanks for reviewing that
06:50 Zeno`     paramat, np
06:50 Zeno`     Hijiri, yeah that's what I meant by my "depends how it's used comment" :)
06:54 Zeno`     and just checked Lua docs... all seems good
06:55 Zeno`     everything behaves the same as POSIX mkdir so no bug
06:55 Zeno`     if a mod wants to create a directory called file.txt/ then that's the mods problem :)
06:56 Zeno`     if it was called CreateAllPaths then there might be room for argument but it's not, so...
10:46 est31     pushing this: https://github.com/est31/minetest/commit/a37f7ac1b0cb394c5c7cc03136a34c15b1d05f7c
10:46 est31     in 20 minutes
16:15 Zeno`     who can merge _game commits without being yelled at?
16:15 Zeno`     https://github.com/minetest/minetest_game/pull/669 should just be merged
16:16 Zeno`     that is the longest discussion I've ever seen over, what, less than 50 lines of code?
16:17 est31     is this issue fixed: https://github.com/minetest/minetest_game/pull/669#issuecomment-139694298
16:18 est31     but the two mtgame devs online right now are nore and sfan5
16:19 Zeno`     if it's not fixed wouldn't it be an existing bug anyway?
16:21 Zeno`     est31, I think that it would be an unrelated bug/issue
16:22 Zeno`     if it's still an issue...
16:22 est31     no the PR adds position rounding
16:22 est31     so it would be a regression
16:22 est31     the xanadu server was maintained by tenplus1
16:22 Zeno`     I'll rebase and check
16:23 Zeno`     you mean the .1f stuff?
16:23 Zeno`     when inserting in the table I mean
16:25 Zeno`     est31, so clip's server when it existed used this PR as well?
16:26 est31     i think so
16:26 Zeno`     ok
16:26 Zeno`     there is potentially a problem with .1f but... hmm
16:26 est31     tenplus1 has submitted mods to that server as well
16:27 Zeno`     v.x etc should probably be multiplied by 10, rounded and divided by 10 if it's an issue but you've confused me now :)
16:28 Zeno`     thanks est31
16:29 Zeno`     but I think .1f rounds anyway
16:29 Zeno`     not really sure though
16:30 Krock     > return string.format("%.1f", 0.987654321)
16:30 Krock     1.0
16:30 Krock     <3 LuaJIT command line
16:31 Zeno`     what about 0.58?
16:31 Krock     > return string.format("%.1f", 0.58)
16:31 Krock     0.6
16:31 Zeno`     ok, so that rounding issue is probably old (and fixed)
16:32 Zeno`     maybe a year ago the PR didn't have %.1f
16:32 KaadmY    does anybody know of a mgv7 problem with mt's PRNG and mapgen decorations?
16:36 Zeno`     KaadmY, hmmmm would know if there was/is
16:37 sfan5     Zeno`: if you looked at the code of game#669 tell me and i'll take your word and merge it
16:37 Zeno`     or maybe paramat. What kind of a problem?
16:37 ShadowBot https://github.com/minetest/minetest_game/issues/669 -- Tidy sethome code, add global functions, round coords to 1 decimal by tenplus1
16:38 Zeno`     sfan5, I've looked at it and to me it seems good. I can't see anything wrong. And based on what Krock just contributed to the discussion I'm even more sure. Krock, what do you think?
16:39 Krock     "local input, err"
16:39 Krock     I wonder why not using "err" to check if it failed
16:39 Krock     but well, that doesn't matter
16:39 sfan5     ok whatever
16:39 sfan5     merging
16:40 Zeno`     thanks sfan5
16:40 Krock     yep, merge dat stuff
16:59 T4im      game#747 (except perhaps the documentation?) and game#1127 are essentially duplicates of 669, and can probably be closed
16:59 ShadowBot https://github.com/minetest/minetest_game/issues/747 -- Sethome: Create global functions set_home, go_home and get_home by LeMagnesium
16:59 ShadowBot https://github.com/minetest/minetest_game/issues/1127 -- Globalize sethome by everamzah
17:19 hmmmm     shoot, just found another bug with my commit
17:19 hmmmm     if the seed isn't present in map_meta.txt and hasn't been explicitly set by a mod, the seed in the global config gets overwritten by a random one
17:19 hmmmm     all hail unit tests
17:38 est31     okay what about this commit : https://github.com/est31/minetest/commit/5ff26df4145307d225d4b588fa547026cd9f6621
17:39 est31     its a better version of #4269
17:39 ShadowBot https://github.com/minetest/minetest/issues/4269 -- Removed minetest version number from top left corner by Plonski
17:39 est31     lol did i really write file management bar
17:40 est31     I've meant window manager bar :)
17:41 est31     https://github.com/est31/minetest/commit/e3fa62f372c62eff56f96018892bb8fa93ae9d08
17:41 est31     Its *not* something I actually do like
17:41 est31     but it seems people dont like the watermark
17:42 est31     so lets remove it
17:42 est31     we can't lose version information though
17:45 est31     hmmmm, Zeno` what do you think
17:45 hmmmm     not right now chief, i'm in the zone
17:46 est31     zone ?
17:46 KaadmY    hmmmm: i have several excavators to *help* get people out of places when they're stuck
17:49 est31     I see its a quote
17:49 everamzah it's an expression
17:49 everamzah a figure of speach
17:49 everamzah autozone
17:57 T4im      est31: https://en.wikipedia.org/wiki/Flow_%28psychology%29
18:04 hmmmm     what is this sfan5-patch-1
18:08 hmmmm     well i think i made a big mistake
18:08 hmmmm     one of the patches i thought i had committed weeks ago turns out to not have been committed
18:12 Zeno`     :-o
18:13 * Zeno`   gets out his ISO9001 non-conformance form
18:13 hmmmm     requesting final approval for merging both
18:13 hmmmm     https://github.com/kwolekr/minetest/commit/92705306bfb4994107a43514f29997cea15d48dc
18:13 hmmmm     and https://github.com/kwolekr/minetest/commit/cbf254e316483ea7ff0559327a24650eb797f22c
18:15 T4im      game#1085 could be ready for merge after a second approval (it's quick to look through and review)
18:15 ShadowBot https://github.com/minetest/minetest_game/issues/1085 -- Disabling TNT by tenplus1
18:15 Zeno`     The first I've already approved so I can't review again
18:15 Zeno`     oh they're together
18:15 hmmmm     i'm paranoid that i'm going to screw something up
18:16 hmmmm     well the first one of those, i already had approval for and i thought i merged it weeks ago
18:16 hmmmm     but it turns out that i forgot to push it to upstream
18:16 Zeno`     I see. Well, everything seems ok to me, but someone else should review as well if you have doubts
18:16 Zeno`     I couldn't see anything wrong though
18:16 Zeno`     apart from what we discussed earlier and you've fixed
18:17 hmmmm     hey guys, what's the lua function to print a table?
18:17 hmmmm     is it dump() or something similar?
18:17 T4im      dump or dump2, yea
18:17 Zeno`     has it been tested with existing maps/worlds?
18:17 hmmmm     yup
18:17 Zeno`     be bold
18:18 Zeno`     =)
18:19 Zeno`     wait
18:20 Zeno`     const char *getName() { return "TestMapSettingsManager"; }
18:20 Zeno`     what's that for?
18:20 hmmmm     that's standard for all unit tests
18:20 Zeno`     but what's it used for?
18:20 hmmmm     so the TestBase::runModules knows what to print out
18:20 Zeno`     just printing?
18:21 Zeno`     ok, it's an existing thing so I'll let my concern pass
18:22 Zeno`     C++ doesn't require that string literals that are equivalent be stored just once though, so that's probably something that should be fixed in all the unit tests
18:23 hmmmm     what do you mean?
18:23 hmmmm     you're saying that returning a string literal from a function is bad?
18:23 Zeno`     i.e. if I have "blah" somewhere and "blah" somewhere else C++ is perfectly within rights to have the pointers to each point to a different place
18:23 hmmmm     sure absolutely
18:24 Zeno`     but yeah, it's not applicable here
18:24 hmmmm     in any case, we should have string pooling enabled in our compiler flags...
18:24 hmmmm     MSVC doesn't enable it by default iirc.
18:25 Zeno`     well comparing pointers to test for equivalence is bad practice anyway, so it's not too much of an issue
18:26 est31     doesnt strcmp early return anyway if the pointers match?
18:26 hmmmm     oh no no nothing is being compared here
18:26 est31     I mean a sane implementation would
18:26 Zeno`     hmmmm, yeah, that's why I asked how it was used
18:26 est31     maybe not all
18:26 hmmmm     just printed to stdout
18:26 hmmmm     <hmmmm> so the TestBase::runModules knows what to print out
18:27 Zeno`     hmmmm, yep
18:27 Zeno`     so it's not an issue
18:28 Zeno`     and if it *is* an issue that needs addressing it has nothing to do with these current changes anyway
18:29 Zeno`     so forget I brought it up hehe
18:32 hmmmm     weird
18:32 hmmmm     hey guys, was minetest.get_mapgen_settings always broken?
18:32 Zeno`     est31, strcmp does do that but there is no guarantee that the two string literals "blah" and "blah" point to the same location
18:32 est31     yeah I get that one
18:32 Zeno`     hmmmm, it's broken?
18:33 hmmmm     it appears so
18:33 hmmmm     get_mapgen_params i mean
18:34 Zeno`     no idea... I've never looked at it
18:34 hmmmm     https://paste.fedoraproject.org/387670/46757084/
18:34 hmmmm     is there something wrong with this code?!
18:34 Zeno`     yeah of course
18:35 Zeno`     err
18:35 hmmmm     it is all getting executed
18:35 hmmmm     i don't...
18:35 hmmmm     minetest is an enigma
18:37 Zeno`     I'd have to look into that further
18:37 Zeno`     I can't tell just from what I can see
18:38 hmmmm     nothing seems to be getting set to that table
18:38 hmmmm     the type returned is a table, a blank one
18:39 hmmmm     we push the table onto the stack, so it's at -1
18:39 hmmmm     now we push each parameter onto the stack, making those -1, before setting it to the thing at -2, i.e. the table
18:39 hmmmm     this has not been changed at all....
18:42 Zeno`     not good
18:48 hmmmm     whatever this is, it's been broken since before my patches
18:48 hmmmm     can anybody else verify this?  paramat?
18:49 hmmmm     do dump(mg_params) right after local mg_params = minetest.get_mapgen_params() in minetest_game/mods/default/mapgen.lua
18:51 T4im      hmm
18:52 T4im      I got some stuff out, but only with a loop
18:52 T4im      flags string, water_level number, seed number, mgname string, chunksize number
18:53 T4im      so this might be rather a dump/dump2 problem
18:53 hmmmm     nope
18:53 hmmmm     i tried for k, v in ipairs(mg_params) do print(k) end as well
18:53 T4im      not ipairs, pairs
18:54 T4im      ipairs is indexed, that doesn't work well on a k-v table
18:54 hmmmm     i'll try pairs
18:55 hmmmm     ahh there it is
18:55 hmmmm     okay, seems like dump and dump2 are broken
18:55 hmmmm     i seriously thought my table wasn't getting populated lopl
18:55 T4im      :D
18:55 hmmmm     "shit... how is this broken?!? all i'm doing is creating a table and pushing some values into it"
18:59 T4im      oh, could be that dump/dump2 is attempting ipairs too
18:59 T4im      ah nvm, does both
19:02 T4im      urgh, no, ok this is embarrassing, it's just print(dump(minetest.get_mapgen_params())), nothing broken
19:03 hmmmm     print(dump())!?
19:03 hmmmm     whatt
19:03 T4im      yea
19:03 hmmmm     that is embarassing
19:03 T4im      hah
19:06 hmmmm     works; couldn't be happier.
19:06 hmmmm     thank god.
19:06 hmmmm     i would be pretty embarassed if get_mapgen_params were really that broken.
19:24 hmmmm     found a huge bug with my latest commit
19:24 hmmmm     thank goodness i tested
19:25 hmmmm     https://github.com/kwolekr/minetest/commit/cbf254e316483ea7ff0559327a24650eb797f22c#diff-5c9fad38a1e2b7a0227fd3f5282dcc09R749
19:37 hmmmm     now i can give this the seal of quality
19:40 hmmmm     testing really is essential.  it's so easy to give code a pass by convincing yourself that it's too simple to get wrong, but then you make a typo like forgetting a single !
19:40 est31     okay
19:41 est31     are you still focused on the change
19:41 est31     or some other change
19:41 hmmmm     ?
19:41 est31     or can you review my commit :(
19:41 hmmmm     I can review your commit
19:41 hmmmm     paste here
19:41 est31     err I've meant :)
19:41 est31     https://github.com/est31/minetest/commit/e3fa62f372c62eff56f96018892bb8fa93ae9d08
19:41 hmmmm     aah
19:41 est31     better version of #4269
19:41 hmmmm     but... have you TESTED it?
19:41 ShadowBot https://github.com/minetest/minetest/issues/4269 -- Removed minetest version number from top left corner by Plonski
19:41 est31     yes
19:41 est31     manually
19:42 hmmmm     good good
19:42 hmmmm     so how many people signed off on the concept of removing the watermark?
19:42 est31     #4209
19:42 ShadowBot https://github.com/minetest/minetest/issues/4209 -- Hide watermark in the top left corner
19:42 hmmmm     it personally doesn't bother me much.
19:42 est31     me neither
19:42 hmmmm     hum
19:42 hmmmm     looks like 8 players
19:43 est31     but it seems to be popular amongst players
19:43 hmmmm     alright then
19:43 hmmmm     if 8 players spoke up about it chances are many more like the change as well
19:43 est31     I would be okay with us saying "no, it stays in"
19:43 hmmmm     my position on this is neutral
19:43 est31     but for the case we want it removed, I have made the commit
19:43 hmmmm     i really don't care tbh
19:43 hmmmm     so the original PR simply removes the text
19:44 est31     in a bad way
19:44 hmmmm     rofl
19:44 hmmmm     yeah i can see that
19:44 hmmmm     it just makes it print a blank string
19:44 est31     e.g. chat would leave a blank for the version string
19:45 est31     and in the main menu, its still there
19:45 hmmmm     why not comment in the PR asking the original author to fix it the right way?
19:45 est31     because thats tiresome
19:45 hmmmm     rather, that'd probably be too much work
19:45 hmmmm     coding it yourself is so much quicker
19:45 est31     yeah
19:45 est31     and I have added the version string to the window caption
19:45 hmmmm     so you set it in the window title now
19:46 hmmmm     and you kept the version hash in the watermark
19:46 hmmmm     or, rather, nevermind, that's the window caption again
19:46 hmmmm     fixed the line height
19:46 hmmmm     yup looks good
19:47 est31     nice.
19:47 hmmmm     your version actually fixes it in all places
19:47 est31     I hope so
19:47 est31     its at least more than the PR :)
19:47 hmmmm     it's surprising how many locations the draw version code is replicated
19:47 est31     yeah, we have multiple draw loops
19:47 hmmmm     +1 i approve of this
19:48 est31     ok do you think we need another pair of eyes
19:48 hmmmm     for this?  nah
19:48 hmmmm     it's relatively simple
19:49 hmmmm     with every commit, minetest slowly but surely becomes better than it was the day before
19:49 est31     thats the idea :)
19:50 hmmmm     it's inspiring
19:50 est31     doesnt have to be the case actually
19:50 est31     regressions do happen
19:50 hmmmm     shhhh
19:51 hmmmm     we don't need any bad vibes in here
19:52 est31     is this a hug circle?
19:54 hmmmm     a safe space for recovering alcoholic developers
19:54 est31     lol
19:55 est31     it annoys me a bit that irrlicht has no api function to set the window icon
19:56 est31     we have this ugly X icon
19:56 est31     its not beautiful
19:56 est31     there is a feature though for windows
19:56 est31     its ridicoulous
19:56 T4im      that sounds about as great as the copy paste features?
19:56 est31     on start, it looks for a file on a pre-defined path
19:57 est31     some .ico file
19:57 est31     if its there, it gets loaded
19:57 est31     and becomes the window icon
19:57 est31     https://github.com/minetest/minetest/commit/05d0eaf5fc4947081ae743a58e8675991e3f2422
20:00 est31     https://github.com/zaki/irrlicht/blob/c279615ab8b4cd9c0c787477e003847cde3d90f8/source/Irrlicht/CIrrDeviceWin32.cpp#L1036
20:00 est31     just rofl
20:00 hmmmm     ugh
20:00 hmmmm     the "correct" way to set an icon on windows is to embed it in the executable resource
20:00 hmmmm     which requires an .rc
20:02 est31     hmm
20:02 est31     we seem to not use that irrlicht feature after all then
20:02 est31     well thats good
20:02 est31     but on x, you need to call an api function
20:03 est31     that sets the icon, and irrlicht controls the x handle
20:04 est31     hmm
20:05 est31     seems the handle can be obtained
20:05 est31     that sounds really good
20:05 hmmmm     yeah
20:05 hmmmm     well i propose we just make our own setwindowicon() and put it in porting.cpp
20:08 est31     yeah
20:11 est31     ohh thats nice thanks to ranting i might have discovered a way to fix an age old bug of minetest
20:12 hmmmm     it always helps to talk things out :)
20:12 hmmmm     that's why i rant in this channel occasionally
20:38 est31     lol part of the api that I've thought wouldnt exist was used by minetest already
20:38 est31     this codebase is already open for suprises
20:38 est31     always*
21:44 est31     oh it seems to be working
21:44 est31     okay, now I have to make it beautiful
21:44 est31     atm there is a giant c header file  which spews out a million warnings :)
22:54 est31     hmmmm, what do you think.
22:54 est31     there is a tool
22:54 est31     https://github.com/ZZYZX/png2argb
22:54 est31     it converts into the xlib compatible format
22:54 est31     the one you can use to set the window icon
22:55 est31     now there are two options:
22:55 est31     1. put the output of the program (its a big C-array) into a header in the source tree
22:55 est31     2. store the array in binary form and read it at program start
22:56 est31     I'm leaning towards 2, because its more compact
22:56 est31     also, it seems the c-array is not liked by the c++ compiler
22:57 est31     generates a *ton* of warnings
23:39 est31     oh fuck it
23:39 est31     I gonna use the text file
23:40 est31     in fact, if I just write the array into a file in binary representation, its actually longer than if its written into a header file in ascii
23:40 est31     thats probably because lots of padding bits
23:40 est31     AND I would have had to modify the deserialisation + serialisation to store in consistent order (network order)
23:40 est31     that would be too much
23:40 est31     so lets use the text file instead
23:55 est31     okay if I want to resort to the older method to read the file manually: its my local commit 3def0e57a5f6f58e77855f4cafc4b8c52a04c5f7
23:56 est31     hehe I am mean, using this channel as my notebook