| Time |
Nick |
Message |
| 00:05 |
|
turtleman joined #minetest-dev |
| 00:26 |
|
yang2003 joined #minetest-dev |
| 00:44 |
|
KaadmY joined #minetest-dev |
| 00:55 |
|
yang2003 joined #minetest-dev |
| 00:59 |
|
paramat joined #minetest-dev |
| 01:01 |
|
twoelk|2 joined #minetest-dev |
| 01:05 |
|
Void7 joined #minetest-dev |
| 01:18 |
|
twoelk|2 joined #minetest-dev |
| 01:30 |
|
yang2003 joined #minetest-dev |
| 02:07 |
|
MillersMan_ joined #minetest-dev |
| 02:40 |
|
lisac joined #minetest-dev |
| 02:40 |
|
domtron joined #minetest-dev |
| 03:00 |
|
domtron joined #minetest-dev |
| 03:16 |
|
srifqi joined #minetest-dev |
| 03:45 |
|
sloanonlinux joined #minetest-dev |
| 03:50 |
|
DI3HARD139 joined #minetest-dev |
| 03:56 |
|
paramat joined #minetest-dev |
| 04:19 |
|
Zeno` joined #minetest-dev |
| 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 |
|
domtron joined #minetest-dev |
| 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 |
| 04:58 |
|
srifqi joined #minetest-dev |
| 05:07 |
|
domtron joined #minetest-dev |
| 05:16 |
|
srifqi joined #minetest-dev |
| 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:49 |
|
Hunterz joined #minetest-dev |
| 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:24 |
|
TheReaperKing joined #minetest-dev |
| 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:53 |
|
T4im joined #minetest-dev |
| 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... |
| 07:24 |
|
Krock joined #minetest-dev |
| 07:49 |
|
nrzkt joined #minetest-dev |
| 08:57 |
|
MillersMan joined #minetest-dev |
| 09:23 |
|
sloanonlinux joined #minetest-dev |
| 10:04 |
|
edgrey joined #minetest-dev |
| 10:16 |
|
paramat joined #minetest-dev |
| 10:37 |
|
eeew joined #minetest-dev |
| 10:43 |
|
fling joined #minetest-dev |
| 10:45 |
|
est31 joined #minetest-dev |
| 10:46 |
est31 |
pushing this: https://github.com/est31/minetest/commit/a37f7ac1b0cb394c5c7cc03136a34c15b1d05f7c |
| 10:46 |
est31 |
in 20 minutes |
| 11:08 |
|
Darcidride joined #minetest-dev |
| 11:23 |
|
sloanonlinux joined #minetest-dev |
| 11:34 |
|
damiel_ joined #minetest-dev |
| 11:45 |
|
lisac joined #minetest-dev |
| 12:02 |
|
xunto joined #minetest-dev |
| 12:04 |
|
Krock joined #minetest-dev |
| 12:38 |
|
Fixer joined #minetest-dev |
| 13:14 |
|
xunto left #minetest-dev |
| 13:41 |
|
xunto joined #minetest-dev |
| 13:48 |
|
yang2003 joined #minetest-dev |
| 14:04 |
|
sloanonlinux joined #minetest-dev |
| 14:06 |
|
asl97 joined #minetest-dev |
| 14:51 |
|
Void7 joined #minetest-dev |
| 15:35 |
|
KaadmY joined #minetest-dev |
| 15:36 |
|
nrzkt joined #minetest-dev |
| 15:45 |
|
paramat left #minetest-dev |
| 16:01 |
|
whitephoenix joined #minetest-dev |
| 16:02 |
|
hmmmm joined #minetest-dev |
| 16:11 |
|
Void7 joined #minetest-dev |
| 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:38 |
|
srifqi joined #minetest-dev |
| 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:58 |
|
srifqi joined #minetest-dev |
| 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:06 |
|
Zeno` joined #minetest-dev |
| 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:22 |
|
srifqi joined #minetest-dev |
| 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:43 |
|
whitephoenix joined #minetest-dev |
| 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:07 |
|
Player_2 joined #minetest-dev |
| 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:44 |
|
ElectronLibre joined #minetest-dev |
| 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 |
|
Void7 joined #minetest-dev |
| 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:13 |
|
Fixer joined #minetest-dev |
| 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:28 |
|
proller joined #minetest-dev |
| 19:37 |
|
Krock joined #minetest-dev |
| 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:45 |
|
Void7 joined #minetest-dev |
| 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 |
|
TheGManRocks joined #minetest-dev |
| 19:54 |
est31 |
lol |
| 19:54 |
|
whitephoenix0 joined #minetest-dev |
| 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:02 |
|
TheGManRocks left #minetest-dev |
| 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:00 |
|
MillersMan joined #minetest-dev |
| 21:26 |
|
MillersMan joined #minetest-dev |
| 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 :) |
| 21:45 |
|
troller joined #minetest-dev |
| 22:11 |
|
Miner_48er joined #minetest-dev |
| 22:13 |
|
Miner_48er joined #minetest-dev |
| 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:04 |
|
Miner_48er joined #minetest-dev |
| 23:15 |
|
Tmanyo joined #minetest-dev |
| 23:30 |
|
Ambistic joined #minetest-dev |
| 23:34 |
|
yang2003 joined #minetest-dev |
| 23:34 |
|
yang2003 joined #minetest-dev |
| 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 |
