Minetest logo

IRC log for #minetest-dev, 2015-04-29

| Channels | #minetest-dev index | Today | | Google Search | Plaintext

All times shown according to UTC.

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/f​iles#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/srplo​gin/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/srplo​gin/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/srplo​gin/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/ima​ges/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/s​rplogin/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/minetes​t/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/comm​it/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/minete​st/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

| Channels | #minetest-dev index | Today | | Google Search | Plaintext