| Time |
Nick |
Message |
| 00:03 |
|
prozacgod joined #minetest-dev |
| 00:17 |
|
Fritigern joined #minetest-dev |
| 00:21 |
|
Wayward_Tab joined #minetest-dev |
| 00:28 |
|
OldCoder joined #minetest-dev |
| 00:54 |
|
kahrl joined #minetest-dev |
| 01:00 |
|
kahrl joined #minetest-dev |
| 01:02 |
|
kahrl joined #minetest-dev |
| 01:10 |
|
est31 joined #minetest-dev |
| 01:26 |
est31 |
hmmmm, time for the review? |
| 01:27 |
hmmmm |
yeah sure |
| 01:30 |
hmmmm |
anybody else want to join in? |
| 01:39 |
hmmmm |
est31: you did test this on android right? |
| 01:39 |
est31 |
should I? |
| 01:40 |
hmmmm |
there is a lot of android build code |
| 01:40 |
hmmmm |
i have no idea if any of it is correct or not |
| 01:40 |
est31 |
ah yes I tested the building part |
| 01:40 |
hmmmm |
ok |
| 01:40 |
est31 |
(one of nrzkt's bots also tests it) |
| 01:46 |
|
_X_C_V_B_ joined #minetest-dev |
| 01:48 |
|
Zeno` joined #minetest-dev |
| 01:57 |
hmmmm |
hrmm |
| 01:57 |
hmmmm |
did you ever consider changing AUTH_MECHANISM_FIRST_SRP to, say, AUTH_MECHANISM_CREATE_ACCOUNT or something clearer? |
| 01:57 |
hmmmm |
AUTH_SRP_CREATE_ACCOUNT |
| 01:58 |
hmmmm |
AUTH_SRP_LOGIN |
| 01:58 |
hmmmm |
AUTH_UPGRADE_LEGACY_TO_SRP |
| 01:58 |
hmmmm |
idk |
| 01:58 |
hmmmm |
i think that adding the word MECHANISM to those enum values might be extra-verbose |
| 01:58 |
hmmmm |
and the space could be used to provide more/better description |
| 01:59 |
est31 |
ok |
| 01:59 |
hmmmm |
another thing, pointer values = NULL |
| 01:59 |
hmmmm |
not 0! |
| 01:59 |
est31 |
can you comment that in gituhub? |
| 02:00 |
est31 |
about FIRST_SRP vs CREATE_ACCOUNT: its also used for changing the password |
| 02:00 |
hmmmm |
sure |
| 02:00 |
hmmmm |
ahhh okay |
| 02:00 |
hmmmm |
makes more sense then |
| 02:00 |
hmmmm |
it'd probably be a good idea to add a comment explaining exactly which each of these mechanisms is then |
| 02:00 |
hmmmm |
s/is/are/ |
| 02:00 |
hmmmm |
also you can have multiple mechanisms per request??? |
| 02:01 |
est31 |
no |
| 02:01 |
hmmmm |
i'm confused about get_auth_mech |
| 02:01 |
est31 |
changing passwords is done otherwise |
| 02:01 |
est31 |
ah |
| 02:01 |
hmmmm |
I need to actually check out your branch hold on |
| 02:01 |
est31 |
first about the coments: there are comments |
| 02:01 |
hmmmm |
so then i can goto definitions of things |
| 02:01 |
est31 |
https://github.com/minetest/minetest/pull/2620/files#diff-d6c71fb6c2c5e2ad979f39a0422f7786R909 |
| 02:08 |
hmmmm |
est, about AuthMechanism |
| 02:08 |
hmmmm |
I see you're trying to use it like a set of flags |
| 02:09 |
est31 |
yes |
| 02:09 |
hmmmm |
but the second you combine at least two enum values it becomes an invalid value |
| 02:09 |
hmmmm |
AUTH_MECHANISM_SRP | AUTH_MECHANISM_LEGACY_PASSWORD is not in the enum |
| 02:10 |
est31 |
perhaps I should store the result in a u32 where I do that |
| 02:11 |
hmmmm |
why the free((char *) salt)? |
| 02:12 |
hmmmm |
first the style is to not have a space between unary operators and their operands |
| 02:12 |
est31 |
because itget_auth_mech |
| 02:12 |
hmmmm |
second, you don't need to cast it at all |
| 02:12 |
est31 |
oh sorry |
| 02:12 |
hmmmm |
then m_authstate = 1; |
| 02:12 |
est31 |
really? |
| 02:12 |
hmmmm |
yeah |
| 02:13 |
hmmmm |
in any case all these are minorish problems that can be addressed as we go along |
| 02:14 |
est31 |
casts are really without space? |
| 02:14 |
hmmmm |
yeah |
| 02:14 |
est31 |
I like them more with space |
| 02:14 |
hmmmm |
well |
| 02:14 |
hmmmm |
it's not a dealbreaker |
| 02:14 |
est31 |
yes |
| 02:14 |
hmmmm |
i'm just saying what the rest of the code uses |
| 02:14 |
est31 |
ok |
| 02:15 |
hmmmm |
(char *)foo is an expression consisting of the identifier 'foo' as the operand of the unary operator (char *) |
| 02:15 |
hmmmm |
much like ! or ~ |
| 02:15 |
hmmmm |
~foo |
| 02:15 |
hmmmm |
you don't write ~ foo, now do you :)? |
| 02:15 |
est31 |
yes |
| 02:15 |
Zeno` |
haha |
| 02:15 |
est31 |
nor do I write ++ i |
| 02:15 |
hmmmm |
I know some people who do |
| 02:16 |
hmmmm |
:( |
| 02:16 |
hmmmm |
at work we have code reviews and I have a particularily large lump of code that happened because our buildbot was broken for the longest period of time on the repo i needed to commit to |
| 02:16 |
hmmmm |
and now it's being reviewed too and there are like 100+ comments |
| 02:16 |
hmmmm |
the more you look at it, the more delay |
| 02:17 |
hmmmm |
because there's always SOMETHING |
| 02:17 |
hmmmm |
i think i would advocate pushing it as-is, as long as we don't find like horrible deadly bugs that cause bluescreens or likewise |
| 02:18 |
hmmmm |
and then things get fixed as we go along |
| 02:18 |
hmmmm |
what do the rest of you guys think |
| 02:19 |
hmmmm |
right now I'm just trying to understand all the paths of this code and the overall structure |
| 02:21 |
Zeno` |
I think the basic style issues should be fixed before merging, though |
| 02:21 |
Zeno` |
e.g. https://github.com/minetest/minetest/pull/2620/files#diff-34f48ad91ac6c202ac60b0348ae90e30R1019 |
| 02:21 |
hmmmm |
if I sat here and enumerated the style issues there would be like 50000 |
| 02:21 |
hmmmm |
eww yea that's gross |
| 02:21 |
Zeno` |
I don't know about enclosing case's within {} (a compound statement) |
| 02:22 |
est31 |
yes I did it because I declared variables inside the case |
| 02:22 |
hmmmm |
doing /that/ is fine, but putting the next case on the same line as the previous closing bracket is not |
| 02:22 |
hmmmm |
there are a lot of levels of review |
| 02:22 |
Zeno` |
I'm just unsure about the break being inside said compound statement |
| 02:23 |
hmmmm |
review for the code style, review of language idioms used, review of the code structure and approach, review for potential bugs and exploits, etc. |
| 02:23 |
Zeno` |
to me something subtle could go awry that would be difficult to find |
| 02:23 |
hmmmm |
sure |
| 02:23 |
hmmmm |
as you encounter things that are weird let's at least mark them down in the pull request |
| 02:23 |
Zeno` |
e.g. see Duff's Device on the intricacies of C's switch |
| 02:24 |
hmmmm |
so it's not forgotten about |
| 02:24 |
hmmmm |
duff's device? |
| 02:24 |
Zeno` |
http://en.wikipedia.org/wiki/Duff%27s_device |
| 02:24 |
hmmmm |
is that the one where it effectively pulls off a loop inside ofa switch statement |
| 02:24 |
Zeno` |
yeah |
| 02:24 |
hmmmm |
yeah hahaha |
| 02:24 |
hmmmm |
oh man that's crazy |
| 02:24 |
Zeno` |
but it's not *obvious* how the switch works |
| 02:24 |
hmmmm |
as long as it's well commented I'd accept that, considering the performance benefit :-) |
| 02:25 |
Zeno` |
well, yeah :) |
| 02:25 |
Zeno` |
I'm using it as an example of how switch/case is not always obvious hehe |
| 02:25 |
hmmmm |
that guy shouldve s/% 8/& 7/ |
| 02:25 |
hmmmm |
s/ \/ 8/ >> 8/ |
| 02:25 |
hmmmm |
erm, >> 3 |
| 02:26 |
est31 |
yes |
| 02:26 |
Zeno` |
haha |
| 02:26 |
sofar |
*** Error in `bin/minetest': double free or corruption (!prev): 0x0882b8c8 *** |
| 02:26 |
sofar |
just... starting it up |
| 02:26 |
sofar |
crashed immediately |
| 02:26 |
hmmmm |
sofar, can you reliably reproduce that? |
| 02:26 |
est31 |
thats natural for minetest |
| 02:26 |
sofar |
$ git describe --tags |
| 02:26 |
sofar |
0.4.11-489-g6ec3163 |
| 02:26 |
Zeno` |
sofar, you're not using valgrind or something are you? |
| 02:26 |
est31 |
the problem starts when its reproducable |
| 02:26 |
sofar |
Zeno`: not right now :) |
| 02:26 |
Zeno` |
ok |
| 02:27 |
sofar |
I'll do some startup cycling and see if it comes again |
| 02:27 |
hmmmm |
sofar, what platform is that? |
| 02:28 |
sofar |
linux 32bit |
| 02:28 |
sofar |
self-compiled, like most of my software stack :) |
| 02:28 |
hmmmm |
hmm |
| 02:29 |
sofar |
I've done at least 50+ starts with this build already since I use that to debug / work on my mods |
| 02:29 |
sofar |
so it's a rare thing |
| 02:29 |
est31 |
I'm having it too, every now and than |
| 02:29 |
est31 |
then* |
| 02:29 |
est31 |
issue is, how do you debug when you can't reproduce? |
| 02:31 |
_X_C_V_B_ |
is minetest web scale |
| 02:33 |
Zeno` |
est31, tbh I'd make those two switch cases a function each :) |
| 02:33 |
Zeno` |
thanks Mr ShadowBot |
| 02:35 |
est31 |
Zeno`, the problem with functions would be that you have to add them in the header too |
| 02:35 |
est31 |
and thats bad:( |
| 02:35 |
Zeno` |
why? |
| 02:35 |
est31 |
because they are nonstatic |
| 02:35 |
est31 |
they use the client's instance |
| 02:35 |
est31 |
ofc I could give the client as pointer |
| 02:36 |
est31 |
but dunno of that would still be cool |
| 02:36 |
Zeno` |
having them private or protected is bad? |
| 02:36 |
est31 |
if* |
| 02:36 |
est31 |
no |
| 02:37 |
Zeno` |
or yeah you could have them as a static function (not member function) in the C++ file an pass 'this' |
| 02:37 |
Zeno` |
I don't see much that's awful about that |
| 02:37 |
_X_C_V_B_ |
mongodb is webscale |
| 02:37 |
_X_C_V_B_ |
so, is minetest web scale???? |
| 02:37 |
sofar |
it's not |
| 02:38 |
est31 |
Zeno`, yea perhaps |
| 02:38 |
est31 |
can you add a github line note? |
| 02:38 |
est31 |
(I won't fix this right now too tired) |
| 02:38 |
Zeno` |
ok |
| 02:39 |
_X_C_V_B_ |
I'm going to make my own open source version of minecraft |
| 02:39 |
sofar |
cheers, please talk somewhere else |
| 02:40 |
_X_C_V_B_ |
o |
| 02:40 |
_X_C_V_B_ |
ok |
| 02:40 |
|
_X_C_V_B_ left #minetest-dev |
| 02:42 |
sofar |
o_O that... worked |
| 02:42 |
Zeno` |
est31, m_password == "" |
| 02:43 |
hmmmm |
hrmm |
| 02:43 |
Zeno` |
I know it *might* seem hacky but what would be wrong with simply (u8)(m_password == "") ? |
| 02:43 |
Zeno` |
i.e. true *is* 1 already |
| 02:44 |
hmmmm |
okay |
| 02:44 |
est31 |
yea |
| 02:44 |
est31 |
is false automatically 0? |
| 02:44 |
Zeno` |
yes, but see my line comment as well |
| 02:44 |
hmmmm |
so m_auth_data is an SRPUser * in the case of auth mechanism == *_SRP |
| 02:45 |
hmmmm |
and then it *can* be something different? |
| 02:46 |
est31 |
yes |
| 02:46 |
hmmmm |
if I were coding this I would probably use some C++ features and make AuthMethod or something into a base class |
| 02:46 |
hmmmm |
and then have AuthMethodSRP, AuthMethodLegacy, etc. |
| 02:46 |
hmmmm |
so a fundamentally different approach |
| 02:46 |
hmmmm |
you have things more flattened out rather than structured by class |
| 02:46 |
est31 |
yea I've thought of that but then I remembered of how you liked c++ |
| 02:46 |
hmmmm |
well |
| 02:46 |
hmmmm |
I like things that aren't overly complicated unless they need to be |
| 02:47 |
hmmmm |
you should do it however you want to have it and then unless it's absolutely horrible, we change it |
| 02:47 |
hmmmm |
errm, we keep it * |
| 02:47 |
hmmmm |
but why isn't m_chosen_auth_mech an AuthMechanism? |
| 02:49 |
est31 |
because I didn't want to include the header just for that |
| 02:49 |
est31 |
perhaps its included anyways |
| 02:49 |
est31 |
add a line note, I'll change it if it still compiles |
| 02:49 |
hmmmm |
well I'm adding line notes for everything |
| 02:49 |
|
prozacgod joined #minetest-dev |
| 02:49 |
hmmmm |
just so i don't forget it later |
| 02:50 |
hmmmm |
but i'm most concerned about 1). no bugs, and 2). good structure |
| 02:50 |
Zeno` |
oh man |
| 02:50 |
Zeno` |
here I am adding pedantic comments and NOW you say that |
| 02:50 |
hmmmm |
no |
| 02:50 |
hmmmm |
you can add pedantic comments too |
| 02:51 |
hmmmm |
it's just that they're not as high in terms of priority |
| 02:51 |
Zeno` |
of course not :) |
| 02:51 |
Zeno` |
heh |
| 02:51 |
hmmmm |
I have to deal with this one guy at work who is the most stubborn person in the universe |
| 02:51 |
est31 |
about the separate classes: currently the handlers are at one single place |
| 02:51 |
hmmmm |
and one missing space is like the end of the world |
| 02:52 |
hmmmm |
and refuses to review anything else unless all trivial style "issues" are done |
| 02:52 |
est31 |
so it would perhaps be confusing to separate it into other files/classes |
| 02:52 |
hmmmm |
it's so annoying so I try to not replicate that |
| 02:52 |
Zeno` |
Finally somebody uses 1 << 0, 1 << 1 etc instead of 0x1, 0x2, 0x4, 0x8, 0x10, 0x20 |
| 02:52 |
hmmmm |
at the end of the day, i have literally never seen him review code |
| 02:53 |
hmmmm |
yeah but having flags in an enum defeats the purpose |
| 02:53 |
est31 |
SN's idea. |
| 02:53 |
Zeno` |
I'm not allowed to add positive comments though |
| 02:53 |
est31 |
lol |
| 02:53 |
Zeno` |
;D |
| 02:53 |
est31 |
you can add them here :p |
| 02:53 |
Zeno` |
already have |
| 02:53 |
hmmmm |
there should be a Flags data type in some language |
| 02:54 |
est31 |
the first implementation of this used strings with every auth mechanism being a byte |
| 02:54 |
hmmmm |
what why :/ |
| 02:54 |
est31 |
when I got into issues to get it actually working I dumped it |
| 02:55 |
est31 |
strings might have unlimited length (so unlimited number of auth mechs) |
| 02:55 |
est31 |
but they are clunky |
| 02:55 |
hmmmm |
rolls eyes |
| 02:55 |
hmmmm |
yeah like we're going to have more than 42795494396 auth mechanisms |
| 02:55 |
est31 |
and I had those issues, so I used the c way |
| 02:56 |
est31 |
right now 32 are possible |
| 02:56 |
est31 |
thats enough |
| 02:59 |
Zeno` |
https://github.com/est31/minetest/blob/srplogin/src/network/serverpackethandler.cpp#L184 |
| 02:59 |
Zeno` |
what's that for? |
| 02:59 |
Zeno` |
A synonym for playerName ? |
| 02:59 |
est31 |
SN's casing changes |
| 03:00 |
est31 |
makes the protocol work for userHello, when they first registered as UsErHellO |
| 03:00 |
est31 |
so right now both client and server are ignoring it. |
| 03:01 |
Zeno` |
I don't like that... |
| 03:01 |
est31 |
but when we want to add it later, its there |
| 03:01 |
hmmmm |
lots of game networks have that case insensitivity |
| 03:01 |
Zeno` |
it looks too much like implementation defined behavior |
| 03:01 |
hmmmm |
are minetest player names currently case insensitive?? |
| 03:01 |
est31 |
no |
| 03:01 |
est31 |
there is a PR for that somewhere |
| 03:01 |
Zeno` |
(storing the pointer to the c_str() and then changing playerName I mean) |
| 03:02 |
hmmmm |
hrmm |
| 03:02 |
hmmmm |
yeah I think that may be undefined behavior |
| 03:02 |
|
Wayward_Tab joined #minetest-dev |
| 03:02 |
hmmmm |
i wouldn't know without consulting the standard |
| 03:02 |
est31 |
where do I do that? |
| 03:03 |
Zeno` |
hmm |
| 03:03 |
Zeno` |
line 165 and line 184 are what I'm wondering about |
| 03:04 |
Zeno` |
it just seems odd |
| 03:04 |
est31 |
isn't the assignment the other way round? |
| 03:06 |
Zeno` |
Maybe. I'm only glancing, but if the meaning isn't clear at a glance I don't like it :) i.e. if something needs to be copied to be changed then it should be copied and changed in the lines immediately following |
| 03:07 |
Zeno` |
and on line 165 I don't know why you're storing the pointer returned by .c_str() |
| 03:07 |
Zeno` |
maybe it's for convenience? |
| 03:07 |
est31 |
yes |
| 03:07 |
Zeno` |
(I could look through of course, but I'm lazy) |
| 03:07 |
est31 |
nono only convenience |
| 03:08 |
est31 |
also to keep diff small |
| 03:08 |
Zeno` |
I guess maybe the comment immediately above it is misleading in that case |
| 03:09 |
Zeno` |
Ok, what if somebody *does* change playerName after you store that pointer? |
| 03:09 |
est31 |
perhaps I should swap lines 182 and 183? |
| 03:10 |
Zeno` |
Not sure what the best way to approach it is. What I'm saying is that it could lead to bugs... |
| 03:10 |
Zeno` |
even if there are not currently any |
| 03:10 |
est31 |
yea perhaps I should add //for convenience to 164 |
| 03:10 |
est31 |
yes bug prevention is one of the things style tries to achieve |
| 03:11 |
Zeno` |
perhaps... or that *and* move it closer to where it's being used for convenience |
| 03:13 |
est31 |
add it as nore |
| 03:13 |
est31 |
note* |
| 03:13 |
est31 |
sorry nore :D |
| 03:15 |
Zeno` |
that whole function probably needs tidying up (and you probably added it in haste because it looks different to the rest of your changes heh) |
| 03:16 |
hmmmm |
gosh now more than ever I realize how much I hate networkpacket |
| 03:17 |
est31 |
? |
| 03:17 |
hmmmm |
is it actually necessary to overload <<? what's wrong with having explicit .insertU8() or .insertU16() calls |
| 03:17 |
hmmmm |
why did we agree to this |
| 03:17 |
Zeno` |
https://github.com/est31/minetest/blob/srplogin/src/network/serverpackethandler.cpp#L501 |
| 03:17 |
Zeno` |
and line 504 |
| 03:17 |
est31 |
its sugar |
| 03:17 |
est31 |
hmmmm ^ |
| 03:17 |
Zeno` |
wtf is that supposed to do? |
| 03:17 |
est31 |
yes thats the legacy password |
| 03:17 |
hmmmm |
it doesn't make things sweet, it makes them error prone and nonsensical |
| 03:18 |
est31 |
thats why we DO need new packets |
| 03:18 |
Zeno` |
that's just wrong |
| 03:18 |
Zeno` |
(not you, est, the link I pasted) |
| 03:18 |
est31 |
why wrong? |
| 03:18 |
Zeno` |
because it's dumb |
| 03:19 |
est31 |
perhaps it should use memcpy |
| 03:19 |
Zeno` |
no, the given_password array can never be filled |
| 03:19 |
hmmmm |
is that your code or nerzhuls? |
| 03:20 |
Zeno` |
nerzhuls |
| 03:20 |
est31 |
not mine |
| 03:20 |
hmmmm |
pfooooo |
| 03:20 |
Zeno` |
ah I see |
| 03:20 |
Zeno` |
yuck, yuck, yuck though |
| 03:20 |
hmmmm |
well what did we honestly expect |
| 03:20 |
est31 |
what do you mean with can never be filled? |
| 03:20 |
hmmmm |
let's fix it when we get a chance |
| 03:20 |
hmmmm |
I think a lot of the network code needs a refactor |
| 03:21 |
Zeno` |
est31, what is i at the end of the loop? |
| 03:21 |
Zeno` |
(the value of i... assuming it didn't go out of scope) |
| 03:21 |
est31 |
password_size - 2 |
| 03:21 |
est31 |
and? |
| 03:21 |
est31 |
then we set the last char to 0 |
| 03:21 |
est31 |
at password_size-1 |
| 03:22 |
est31 |
then we have password_size chars, terminated by 0 |
| 03:22 |
est31 |
regardless of what game the client wants to play with us (minetest or hack-you) |
| 03:23 |
Zeno` |
but... that's not PASSWORD size |
| 03:23 |
Zeno` |
so if I say PASSWORD_SIZE is 8, I expect the user to be able to use 8 characters for their password |
| 03:23 |
Zeno` |
now they can only use 8 |
| 03:23 |
Zeno` |
7* |
| 03:24 |
est31 |
so, instead of 20 you can now use 19? |
| 03:24 |
est31 |
hm you might be right |
| 03:24 |
est31 |
(that its bad) |
| 03:24 |
Zeno` |
this is probably why there is the weird hack in main.cpp |
| 03:25 |
Zeno` |
which I can't remember the line # of, but it basically truncates playernames and passwords IIRC |
| 03:25 |
est31 |
ok |
| 03:26 |
Zeno` |
don't fix it in your PR though (of course) |
| 03:27 |
est31 |
yes |
| 03:27 |
hmmmm |
what is SudoSuccess and SudoMode? |
| 03:27 |
est31 |
I dont touch that packet |
| 03:27 |
Zeno` |
it shouldn't be 0 either... should be '\0' |
| 03:27 |
est31 |
hmmmm, its a short name for passwordchangemode |
| 03:27 |
est31 |
I've tried to explain it everywhere where used |
| 03:28 |
hmmmm |
isn't that an indication that the naming sucks? |
| 03:28 |
hmmmm |
:/ |
| 03:28 |
Zeno` |
line 444 is the bigger problem |
| 03:28 |
est31 |
passwordchangemode is longer to write than sudo |
| 03:28 |
est31 |
also can be used later on for other "about my account" changes |
| 03:29 |
est31 |
link Zeno`? |
| 03:29 |
Zeno` |
https://github.com/est31/minetest/blob/srplogin/src/network/serverpackethandler.cpp#L444 |
| 03:30 |
Zeno` |
meh |
| 03:30 |
Zeno` |
I can't look at this |
| 03:30 |
est31 |
Zeno`, you are inside the wrong packet |
| 03:30 |
est31 |
its legacy |
| 03:30 |
hmmmm |
I can't fap to this |
| 03:30 |
Zeno` |
It's too inconsistent and my mind refuses to accept what it sees |
| 03:30 |
hmmmm |
http://i2.kym-cdn.com/photos/images/facebook/000/556/128/3b9.jpg |
| 03:31 |
hmmmm |
WTF |
| 03:31 |
hmmmm |
we should've caught this kind of crap before it got committed |
| 03:31 |
hmmmm |
well, no worries as long as it's eventually fixed |
| 03:33 |
est31 |
when its removed, its fixed. |
| 03:35 |
hmmmm |
gah |
| 03:35 |
hmmmm |
okay I made enough comments for now |
| 03:36 |
hmmmm |
I really really don't like NetworkPacket now |
| 03:36 |
hmmmm |
I think what's being inserted needs to be explicit and obvious |
| 03:36 |
hmmmm |
not... pkt >> blah >> foobar >> asdf; |
| 03:36 |
est31 |
thats what c++ has static typing for |
| 03:37 |
hmmmm |
this is painful to look at |
| 03:37 |
est31 |
its much shorter |
| 03:37 |
hmmmm |
shorter IS NOT NECESSARILY BETTER |
| 03:38 |
hmmmm |
we could also write our code like |
| 03:38 |
hmmmm |
if(a+b>34){ |
| 03:38 |
hmmmm |
d.crnlz(); |
| 03:38 |
hmmmm |
but i'm pretty sure that would make people want to stab things |
| 03:38 |
Zeno` |
est31, you run this on your server? |
| 03:39 |
est31 |
no Zeno` that server runs stable 0.4.12 :D |
| 03:39 |
Zeno` |
ok, just thought you did |
| 03:39 |
hmmmm |
what is a C String used for in the protocol again?? |
| 03:40 |
est31 |
? |
| 03:40 |
hmmmm |
putCString() |
| 03:40 |
est31 |
ah |
| 03:40 |
est31 |
its the same as << std::string(ptr, len) |
| 03:40 |
est31 |
spares a copy |
| 03:40 |
hmmmm |
yeah but why |
| 03:40 |
hmmmm |
what's wrong with the pascal-style strings |
| 03:41 |
est31 |
? |
| 03:41 |
est31 |
I've learned pascal once for uni, then forgot it again |
| 03:42 |
hmmmm |
pascal style strings are how variable length strings in packets are currently serialize |
| 03:42 |
hmmmm |
d |
| 03:42 |
est31 |
yes the fun part is putCString() does really exactly the same |
| 03:42 |
hmmmm |
(u16 string length) (u8 characters[]) |
| 03:42 |
est31 |
so it also adds the length |
| 03:42 |
est31 |
thats why I said its the same as |
| 03:42 |
hmmmm |
oh |
| 03:43 |
est31 |
https://github.com/est31/minetest/blob/srplogin/src/network/networkpacket.h#L49 |
| 03:43 |
hmmmm |
then that's not a C string.. |
| 03:43 |
Zeno` |
pascal strings are not nul terminated though |
| 03:43 |
hmmmm |
in fact you specify src and len |
| 03:44 |
hmmmm |
you just want to do a zero-copy string insertion |
| 03:44 |
Zeno` |
(well, they might be but the standard wouldn't say they have to be) |
| 03:44 |
Zeno` |
that is if there is a pascal standard lol |
| 03:44 |
|
Hunterz joined #minetest-dev |
| 03:44 |
hmmmm |
do you really have to define a new function for this? |
| 03:44 |
hmmmm |
i mean it's such a microoptimization.. |
| 03:44 |
est31 |
I've thought its useful at other points too |
| 03:45 |
est31 |
but yea that code isn't really performance relevant |
| 03:45 |
hmmmm |
i don't know this is so fubar |
| 03:45 |
hmmmm |
I wouldn't be using this operator overloaded interface to begin with |
| 03:45 |
hmmmm |
does anybody aside from you and nerzhul like it? |
| 03:46 |
est31 |
don't know |
| 03:46 |
est31 |
Zeno`, do you like it? |
| 03:46 |
hmmmm |
NetworkPacket::insertString(const std::string &); could be overloaded by NetworkPacket::insertString(void *, size_t) |
| 03:47 |
hmmmm |
but moreover, we need to ask the more fundamental question what makes network packets so different from other types of serialization to justify having NetworkPacket to begin with |
| 03:48 |
hmmmm |
how is any of this better than the istringstream-based interface we had before... :( |
| 03:48 |
Zeno` |
like what? |
| 03:48 |
Zeno` |
operator overloading used like that? |
| 03:48 |
Zeno` |
nah, I don't |
| 03:48 |
hmmmm |
you're lagging |
| 03:49 |
Zeno` |
yeah, because I'm not watching hehe |
| 03:49 |
est31 |
its easier this way than touching the raw bits and bytes |
| 03:49 |
Zeno` |
have to go to Dr shortly and I'm doing about 8 things at once |
| 03:49 |
est31 |
and less error probe |
| 03:49 |
est31 |
prone* |
| 03:49 |
hmmmm |
how is |
| 03:49 |
est31 |
protobuf for the poor |
| 03:49 |
hmmmm |
packet.data.insertU32(some_value); more error prone than pkt << some_value; |
| 03:49 |
hmmmm |
what happens if we have |
| 03:49 |
hmmmm |
int some_value;? |
| 03:50 |
est31 |
istringstream has insertu32? |
| 03:50 |
Zeno` |
the thing I don't like about pkt << some_value is what happens if some_value is not u32 and I forget to cast it? |
| 03:50 |
hmmmm |
now you don't know which variation of NetworkPacket::operator << gets called unless you know what the size of int is on that architecture |
| 03:50 |
hmmmm |
est31: no it doesn't, but that's how it was used before |
| 03:50 |
hmmmm |
write32(is, foobar); |
| 03:51 |
Zeno` |
if it's packet.data.insertU32() then it's always cast (implicitly) |
| 03:51 |
hmmmm |
well this could just be modified to have a more OO-style interface and bam |
| 03:51 |
est31 |
yes but you will have to make every single of those operations its own line |
| 03:51 |
Zeno` |
so I don't have to worry about data types (well sizes really) at all |
| 03:51 |
hmmmm |
so? |
| 03:51 |
hmmmm |
that's a good thing |
| 03:51 |
est31 |
which needlessly bloats the code |
| 03:51 |
hmmmm |
no way |
| 03:51 |
est31 |
I mean you already have all the type information you need |
| 03:52 |
hmmmm |
I want things being sent on the wire to be very explicit |
| 03:52 |
hmmmm |
I don't want it to be like |
| 03:52 |
Zeno` |
you mean the compiler has it :) |
| 03:52 |
hmmmm |
pkt<<a<<b<<c; |
| 03:52 |
hmmmm |
pkt<<d; some lines down |
| 03:52 |
hmmmm |
we should hold a vote |
| 03:52 |
Zeno` |
lol, the last vote didn't go very well |
| 03:52 |
hmmmm |
what happened to it |
| 03:53 |
Zeno` |
nothing... I think the "meh, I don't care" option won and both got merged hehe |
| 03:54 |
Zeno` |
lol, I can't find it now |
| 03:54 |
hmmmm |
I though it was like 4/3 |
| 03:54 |
Zeno` |
http://strawpoll.me/4215136/r |
| 03:54 |
hmmmm |
and then I ended up saying that I agree with it anyway |
| 03:54 |
hmmmm |
so I changed my vote |
| 03:55 |
hmmmm |
that's a lot of votes for not caring, who did that |
| 03:55 |
Zeno` |
the people who don't even have a say in the vote probably lol |
| 03:55 |
hmmmm |
that was so much less consequential though |
| 03:55 |
est31 |
yea |
| 03:55 |
hmmmm |
i think the clarity of what gets sent over the wire is much more important |
| 03:55 |
est31 |
so votes for big things should span a longer time |
| 03:56 |
hmmmm |
smushing it up "so the code gets bloated" is VERY debatable |
| 03:56 |
hmmmm |
and I completely 100% disagree |
| 03:56 |
est31 |
for me its important that humans can understand the code easily, and what gets sent in each packet |
| 03:56 |
Zeno` |
if it came down to a vote I do not like the current overloading of << method because I think it's more error prone than having the example that hmmmm typed above |
| 03:57 |
hmmmm |
so that's 2/2 |
| 03:57 |
hmmmm |
yeah I don't know |
| 03:57 |
hmmmm |
I need to take a break from reviewing |
| 03:57 |
Zeno` |
i.e. if somehow something innocently gets changed from u8 to u32 and the << does not have a (u8) cast then aren't the wrong number of bytes going to be sent? |
| 03:58 |
est31 |
yes but you will notice that fast wont you? |
| 03:58 |
Zeno` |
possibly |
| 03:58 |
est31 |
I mean the main thing you have to remember is that not only the order and what is sent matters but also the type |
| 03:59 |
Zeno` |
but if the u8 cast *is* there you won't notice it fast |
| 03:59 |
hmmmm |
well let's put it this way |
| 03:59 |
hmmmm |
the benefits of having it this way are what again? |
| 03:59 |
Zeno` |
and nor will you with the function approach, so we're back where we were |
| 03:59 |
hmmmm |
making the code shorter for something that's kind of critical anyway? |
| 03:59 |
est31 |
not just shorter but also easier to give a overview |
| 04:00 |
est31 |
I know fast *what* gets sent and recievec |
| 04:00 |
est31 |
d* |
| 04:00 |
hmmmm |
packets written with one field per line will all fit on a single screen of code |
| 04:00 |
hmmmm |
if you have it as a line of pkt.insertblah(); you can tell it mimics the documentation for the code |
| 04:00 |
hmmmm |
[U8] Command |
| 04:00 |
hmmmm |
[U16] Blah code |
| 04:01 |
hmmmm |
etc.. |
| 04:01 |
hmmmm |
pkt.insertU8(command); |
| 04:01 |
Zeno` |
to me insertU32(a) (or whatever it happens to be called) is more human readable because I don't need to know what type 'a' is |
| 04:01 |
Zeno` |
a<<b<<c<<d I need to know the type (and size!!!, which is a problem) of a, b, c, and d to know how many bytes there are |
| 04:01 |
est31 |
so perhaps it would be a good style thing to require everything declared outside the current method (global stuff and m_ stuff) to be casted |
| 04:02 |
est31 |
Zeno`, why is it important to know how many bytes there are? |
| 04:02 |
est31 |
touching raw bytes is bad |
| 04:02 |
Zeno` |
insert8(a); insert16(b); insert8(c); insert32(d); I can immediately see how many bytes are sent |
| 04:02 |
est31 |
its like using memcpy all over the place because "you know how many bytes you copy" |
| 04:03 |
Zeno` |
not when you're constructing something that *has* to be a certain number of bytes |
| 04:03 |
Zeno` |
what's wrong with memcpy()? |
| 04:04 |
est31 |
why should networkpackets have to have a certain size? |
| 04:04 |
Zeno` |
this is low level stuff so, surely, you're working with... bytes |
| 04:04 |
est31 |
yes, networkpacket gives you an abstraction |
| 04:04 |
Zeno` |
outside of serverpackethandler? |
| 04:04 |
est31 |
you shouldnt care about bytes |
| 04:04 |
Zeno` |
you don't outside of that |
| 04:05 |
est31 |
only about things passed to and from the packet |
| 04:05 |
est31 |
and their type |
| 04:06 |
est31 |
so what is easier, $TYPE a; pkt->put$TYPE(a); is easier or $TYPE a; pkt << a; |
| 04:09 |
Zeno` |
I'm doing too many things at once... need to go in 5 minutes. I think maybe I've been at "the wrong level of abstraction" if such a phrase exists |
| 04:09 |
est31 |
thanks Zeno` and hmmmm for the review :) |
| 04:09 |
Zeno` |
I was looking at things from the "inside" point of view where bytes count |
| 04:09 |
hmmmm |
wait it's not done yet |
| 04:09 |
Zeno` |
if we're outside the abstraction then everything I said above is incorrect |
| 04:10 |
hmmmm |
[12:04 AM] <est31> why should networkpackets have to have a certain size? |
| 04:10 |
hmmmm |
because reverse compatibility |
| 04:10 |
hmmmm |
we have a very specific format that is to be followed, and if you do, we promise you that you'll be able to talk with minetest |
| 04:11 |
est31 |
yes |
| 04:11 |
est31 |
it has to remain stable I agree |
| 04:12 |
est31 |
but whats better pkt >> a >> b >> c or a = pkt[1]; b = pkt[2]; c = pkt[3];? |
| 04:12 |
est31 |
as long as we are doing the same as that code everything is well |
| 04:12 |
est31 |
people should only then change things when they know whats happening |
| 04:24 |
|
Miner_48er joined #minetest-dev |
| 04:24 |
Zeno` |
anyway, back to the PR before I leave |
| 04:25 |
Zeno` |
The only thing I strongly disagree with is the casts where casts are not needed |
| 04:26 |
Zeno` |
e.g. https://github.com/minetest/minetest/pull/2620/files#diff-2d67c2347848dbd1d1d74e4b8d608694R93 |
| 04:26 |
Zeno` |
bbl |
| 04:27 |
est31 |
thats nothing to argue about, its only me not knowing c good enough :) |
| 04:35 |
|
cib0 joined #minetest-dev |
| 04:58 |
hmmmm |
so on a scale from 1 to 10, what would you rate your C? |
| 05:00 |
est31 |
tough question |
| 05:00 |
hmmmm |
and how many other languages do you know? would you call yourself experienced? |
| 05:01 |
hmmmm |
like what's your background |
| 05:01 |
hmmmm |
it's difficult to judge you. on one hand it seems like you're very knowledgable, you know a lot about cryptography and such, but your C sucks :p |
| 05:01 |
hmmmm |
so I don't consider you a novice |
| 05:01 |
est31 |
I'm studying math thats perhaps why I know cryptography |
| 05:02 |
est31 |
but I dont have that coding backgroung |
| 05:02 |
est31 |
d* |
| 05:02 |
hmmmm |
really? hm |
| 05:02 |
hmmmm |
for no coding background you're pretty good |
| 05:02 |
hmmmm |
keep up with it |
| 05:02 |
hmmmm |
I was a math person but the career prospects for pure math frankly sucks, and applied math is pretty boring |
| 05:02 |
hmmmm |
i wouldn't want to be an analyst |
| 05:02 |
hmmmm |
or actuary |
| 05:03 |
est31 |
yeah banking and insurance or so sucks |
| 05:03 |
hmmmm |
that's the only place where jobs are |
| 05:03 |
hmmmm |
I am so happy I ended up getting a software job in the end |
| 05:04 |
est31 |
I'm aiming for software too |
| 05:05 |
hmmmm |
i forgot a lot of math and i wish i didn't |
| 05:06 |
hmmmm |
trying to write a unit test for PcgRandom::randNormalDist(), so obviously I'd need to do some kind of chisq goodness of fit, but to do that i need to know the variance, but i forgot how to figure out the variance of multiple independent trials :/ |
| 05:07 |
est31 |
I've yet to hear this \sigma X = 1 stuff |
| 05:08 |
|
jin_xi joined #minetest-dev |
| 05:28 |
|
Wayward_Tab joined #minetest-dev |
| 05:30 |
|
Wayward_Tab joined #minetest-dev |
| 05:47 |
|
Hunterz joined #minetest-dev |
| 06:08 |
est31 |
many things you point out hmmmm I know, I just missed them |
| 06:08 |
est31 |
like identation |
| 06:09 |
est31 |
or that pointers should be NULL |
| 06:09 |
est31 |
that one I didn't deem too important |
| 06:59 |
|
nore joined #minetest-dev |
| 07:02 |
|
kilbith joined #minetest-dev |
| 07:05 |
|
Darcidride joined #minetest-dev |
| 07:12 |
|
leat2 joined #minetest-dev |
| 07:23 |
|
leat2 joined #minetest-dev |
| 07:39 |
|
FR^2 joined #minetest-dev |
| 07:48 |
|
leat2 joined #minetest-dev |
| 07:56 |
|
leat2 joined #minetest-dev |
| 08:04 |
|
Yepoleb joined #minetest-dev |
| 08:04 |
|
cib0 joined #minetest-dev |
| 08:28 |
|
selat joined #minetest-dev |
| 08:34 |
|
Hijiri joined #minetest-dev |
| 09:22 |
|
Megaf joined #minetest-dev |
| 10:03 |
|
deezl joined #minetest-dev |
| 10:05 |
|
VargaD joined #minetest-dev |
| 10:14 |
|
AnotherBrick joined #minetest-dev |
| 10:35 |
|
selat joined #minetest-dev |
| 10:50 |
|
proller joined #minetest-dev |
| 11:47 |
|
proller joined #minetest-dev |
| 11:54 |
|
err404 joined #minetest-dev |
| 11:57 |
|
leat2 joined #minetest-dev |
| 12:01 |
sfan5 |
https://github.com/minetest/minetest/issues/2656#issuecomment-96998742 |
| 12:01 |
sfan5 |
how relevant |
| 12:02 |
|
err404 joined #minetest-dev |
| 12:03 |
deezl |
lol |
| 12:04 |
deezl |
it does seem to defy logic, but there are many little things that stray a bit from reality...people seem to forget it is a game |
| 12:07 |
|
leat2 joined #minetest-dev |
| 12:17 |
|
leat2 joined #minetest-dev |
| 12:28 |
|
leat2 joined #minetest-dev |
| 12:28 |
|
prozacgod joined #minetest-dev |
| 12:29 |
|
neoascetic joined #minetest-dev |
| 12:38 |
|
Darcidride_ joined #minetest-dev |
| 12:41 |
|
prozacgod joined #minetest-dev |
| 12:58 |
|
Taoki joined #minetest-dev |
| 12:59 |
|
cib0 joined #minetest-dev |
| 13:03 |
|
AnotherBrick joined #minetest-dev |
| 13:21 |
|
cib0 joined #minetest-dev |
| 13:27 |
|
Darcidride__ joined #minetest-dev |
| 13:40 |
|
proller joined #minetest-dev |
| 14:20 |
|
est31 joined #minetest-dev |
| 14:28 |
|
cheapie_ joined #minetest-dev |
| 14:35 |
|
hmmmm joined #minetest-dev |
| 14:37 |
|
ElectronLibre joined #minetest-dev |
| 14:43 |
|
Player_2 joined #minetest-dev |
| 14:49 |
|
est31 joined #minetest-dev |
| 15:28 |
|
leat2 joined #minetest-dev |
| 15:30 |
|
AnotherBrick joined #minetest-dev |
| 15:38 |
|
leat2 joined #minetest-dev |
| 15:39 |
|
Hunterz joined #minetest-dev |
| 15:55 |
|
cib0 joined #minetest-dev |
| 16:02 |
|
AnotherBrick joined #minetest-dev |
| 16:16 |
|
Player_2 joined #minetest-dev |
| 16:17 |
|
Krock joined #minetest-dev |
| 16:45 |
|
ElectronLibre joined #minetest-dev |
| 17:00 |
|
Robert_Zenz joined #minetest-dev |
| 17:02 |
Krock |
hmmmm, https://github.com/minetest/minetest/blob/master/src/unittest/test_random.cpp#L182 do you seriously mean 'i' or shouldn't there be an other variable? |
| 17:02 |
Krock |
It is two times initialized |
| 17:02 |
hmmmm |
shoot |
| 17:03 |
hmmmm |
that is not good |
| 17:03 |
|
ElectronLibre joined #minetest-dev |
| 17:03 |
Krock |
Hold on, I'm doing a pull to fix all mistakes |
| 17:03 |
hmmmm |
but i'm surprised clang did not give a warning for shadowed variables |
| 17:03 |
hmmmm |
Krock, don't bother....... |
| 17:03 |
Krock |
well I do because there are round() issues |
| 17:03 |
Krock |
in the same file |
| 17:03 |
hmmmm |
what do you mean |
| 17:04 |
Krock |
MSVC doesn't have round() use myround() instead |
| 17:04 |
hmmmm |
nice |
| 17:04 |
hmmmm |
I should fix all those at the same time |
| 17:05 |
hmmmm |
i don't know if we should use myround() - does this work correctly for negative numbers? |
| 17:07 |
Krock |
floor works with negaive, so yes. |
| 17:08 |
Krock |
here some other tiny changes.. not worthy to create a commit for each one #2657 |
| 17:08 |
ShadowBot |
https://github.com/minetest/minetest/issues/2657 -- Fix several MSVC issues by SmallJoker |
| 17:10 |
hmmmm |
floor doesn't work for -.5 |
| 17:10 |
hmmmm |
it rounds up |
| 17:11 |
Krock |
I see |
| 17:11 |
hmmmm |
oh didn't I remove that constructor? |
| 17:11 |
Krock |
apparently you didn't |
| 17:12 |
hmmmm |
by the way, what version of msvc are you using |
| 17:14 |
Krock |
The CXX compiler identification is MSVC 16.0.30319.1 |
| 17:14 |
|
est31 joined #minetest-dev |
| 17:17 |
|
rubenwardy joined #minetest-dev |
| 17:20 |
|
proller joined #minetest-dev |
| 17:20 |
Krock |
so, the round problem is solved now |
| 17:22 |
hmmmm |
it would be nice to have a small script that checks modifications before committing |
| 17:23 |
|
Calinou joined #minetest-dev |
| 17:23 |
hmmmm |
looks for accidental usages of uint32_t instead of u32, or round instead of myround, for example |
| 17:23 |
est31 |
? |
| 17:23 |
Krock |
plsno |
| 17:23 |
hmmmm |
things that could easily break msvc builds that people don't notice |
| 17:23 |
Krock |
I wasn't ready with the CmakeLists.txt change |
| 17:23 |
hmmmm |
huh?? |
| 17:23 |
Krock |
see comments |
| 17:24 |
* Krock |
headdesks |
| 17:24 |
Krock |
nvm. |
| 17:25 |
hmmmm |
was that causing a problem?? |
| 17:25 |
est31 |
https://github.com/SmallJoker/minetest/commit/417a38c61e04d9f6e32defdb568892c7d8289c16 |
| 17:25 |
est31 |
look at cmakelists.txt |
| 17:25 |
hmmmm |
can see that |
| 17:26 |
Krock |
est31, he pushed an other commit |
| 17:26 |
est31 |
yes |
| 17:26 |
est31 |
but without cmakelists, no? |
| 17:26 |
hmmmm |
erm |
| 17:27 |
hmmmm |
doesn't that strip out all optimizations for jsoncpp |
| 17:27 |
|
proller joined #minetest-dev |
| 17:27 |
Krock |
dunno but without that line I can't compile minetest |
| 17:27 |
hmmmm |
it should be |
| 17:29 |
hmmmm |
wtf did somebody remove the flags for msvc in src/CMakeLists.txt?? |
| 17:30 |
hmmmm |
oh guess not |
| 17:30 |
Krock |
No, it's still there at line 564 |
| 17:30 |
Krock |
*562 |
| 17:31 |
hmmmm |
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /MT" CACHE STRING "" FORCE) |
| 17:31 |
hmmmm |
are you sure that it works for you without the CACHE STRING "" FORCE) ? |
| 17:32 |
hmmmm |
for some reason CMake ignores my custom set CMAKE_CXX_FLAGS_RELEASE otherwise |
| 17:32 |
Krock |
returns in " /MD /O2 /Ob2 /D NDEBUG /MT" |
| 17:32 |
hmmmm |
MD and MT? |
| 17:32 |
hmmmm |
isn't MD the debug one |
| 17:33 |
Krock |
they eliminate ech other |
| 17:33 |
Krock |
looks like MD must be removed |
| 17:33 |
hmmmm |
yes |
| 17:34 |
hmmmm |
we should be using the static version everywhere |
| 17:34 |
hmmmm |
don't get why it's not the case... |
| 17:34 |
Krock |
is it because json is included before changing the variable? |
| 17:35 |
hmmmm |
there's no way |
| 17:35 |
hmmmm |
this is supposed to be run first |
| 17:36 |
hmmmm |
and it's not because all the add_subdirectory() calls are before the variables get set |
| 17:36 |
hmmmm |
is it supposed to work like this??? |
| 17:38 |
|
proller joined #minetest-dev |
| 17:42 |
Krock |
Looking at http://www.cplusplus.com/reference/cmath/round/ is the change in myround() required to make it work exactly like the round() function |
| 17:42 |
hmmmm |
i tested it and what myround() had at first worked good enough i figured |
| 17:43 |
|
OldCoder joined #minetest-dev |
| 17:45 |
sfan5 |
Krock: passing random strings ("/MT") to gcc is guaranteed to break |
| 17:45 |
sfan5 |
(in response to https://github.com/minetest/minetest/pull/2657#discussion_r29358189) |
| 17:46 |
Krock |
Oh sure, haven't thought this could be a MSVC-only param :3 |
| 17:46 |
sfan5 |
of couse it is |
| 17:46 |
sfan5 |
"/MT" can be a valid path in linux |
| 17:46 |
sfan5 |
it can't be a compiler flag |
| 17:47 |
Krock |
Ah, that's the reason for '-' and '--' command line args |
| 17:47 |
|
Miner_48er joined #minetest-dev |
| 17:47 |
sfan5 |
(what if you want to compile a file called MT at the root /) |
| 17:47 |
sfan5 |
additionally /FLAG looks stupid, but thats just a matter of person preference |
| 17:48 |
est31 |
mine too btw |
| 17:49 |
hmmmm |
?? |
| 17:49 |
sfan5 |
??? |
| 17:49 |
hmmmm |
why add that stuff to DecoSchematic()? |
| 17:50 |
sfan5 |
add? you mean remove? |
| 17:50 |
sfan5 |
also isn't the parent class constructor automatically called? |
| 17:50 |
hmmmm |
yes |
| 17:50 |
hmmmm |
so you shouldn't need to explicitly set the same variables set the parent constructor |
| 17:51 |
sfan5 |
oh wait,nevermind |
| 17:52 |
sfan5 |
umm |
| 17:52 |
sfan5 |
Krock: does floor() not work for negative ints in msvc? |
| 17:53 |
Krock |
sfan5, it does but what if you get negative numbers to round, as example -5.5? |
| 17:53 |
sfan5 |
the old implementation rounds that to -5 |
| 17:53 |
sfan5 |
why change it |
| 17:53 |
hmmmm |
because it's incorrect. |
| 17:53 |
Krock |
the orignal round() function rounds it to -6 |
| 17:54 |
Krock |
and that's already the only reason for that change ^ |
| 17:54 |
sfan5 |
i know that it's incorrect, I was asking because the PR didn't mention fixing myround anywhere |
| 17:55 |
Krock |
the commit's description is correct. |
| 17:55 |
sfan5 |
"numeric.h -> Use - 0.5f for negative numbers" |
| 17:55 |
|
cib0 joined #minetest-dev |
| 17:55 |
sfan5 |
that doesn't really describe what you changed |
| 17:56 |
sfan5 |
imagine reading "numeric.h -> Use - 0.5f for negative numbers" in a commit msg, what do you think the person changed? |
| 17:56 |
sfan5 |
you certainly won't come up with the idea that myround was fixed to round correctly when n < 0 |
| 18:01 |
Krock |
I'm unsure. Is the "parent" initialization function called with the extended DecoSchematic() function? |
| 18:02 |
|
Hijiri joined #minetest-dev |
| 18:02 |
hmmmm |
test it yourself |
| 18:02 |
Krock |
yes, that might be the safiest way |
| 18:02 |
hmmmm |
hell, even use ideone |
| 18:03 |
Krock |
duh. what a great tool! |
| 18:29 |
|
jin_xi joined #minetest-dev |
| 18:34 |
|
blaze joined #minetest-dev |
| 19:00 |
|
MinetestForFun joined #minetest-dev |
| 19:26 |
|
Hijiri joined #minetest-dev |
| 19:54 |
|
MinetestForFun joined #minetest-dev |
| 19:59 |
|
leat2 joined #minetest-dev |
| 20:48 |
|
err404 joined #minetest-dev |
| 21:18 |
|
err404 joined #minetest-dev |
| 21:32 |
|
ElectronLibre left #minetest-dev |
| 22:32 |
hmmmm |
hrmm krock isn't around |
| 22:33 |
hmmmm |
I wonder how Krock was having trouble with his MSVC build when it seems fine on Travis |
| 22:51 |
|
Wayward_Tab joined #minetest-dev |
| 23:55 |
|
est31 joined #minetest-dev |
| 23:55 |
est31 |
hmmmm, travis builds for windows yes but not with visual studio |
| 23:55 |
est31 |
but doing a cross build I think |
| 23:55 |
est31 |
using clang |
| 23:56 |
hmmmm |
oh... well that's not as useful as i thought |
| 23:58 |
est31 |
I think what sfan5 did here was quite cool |
| 23:58 |
est31 |
or others |
| 23:58 |
est31 |
dont know the original author or whose idea it was |
