| Time |
Nick |
Message |
| 00:01 |
|
grrk-bzzt joined #minetest-dev |
| 00:43 |
|
cheapie joined #minetest-dev |
| 01:25 |
|
us`0gb joined #minetest-dev |
| 01:43 |
|
us`0gb joined #minetest-dev |
| 02:03 |
|
MichaelRpdx joined #minetest-dev |
| 02:19 |
us`0gb |
I disabled the fog in the Android game and the graphics weirded out. I'm seeing sky-colored discolorations on every node. |
| 02:20 |
us`0gb |
Most of these blemishes are in the form of stripes that fade as they move from the center. |
| 02:21 |
us`0gb |
I'd take a screenshot, but I don't knowhow on this thing. |
| 02:46 |
|
OldCoder joined #minetest-dev |
| 03:13 |
|
OldCoder joined #minetest-dev |
| 03:22 |
ShadowNinja |
us`0gb: If it's a semi-recent device (4.0+ or so) power & volume down should do it. |
| 03:31 |
us`0gb |
ShadowNinja: 4.2, no effect. |
| 03:32 |
|
Miner_48er joined #minetest-dev |
| 05:44 |
|
werwerwer_ joined #minetest-dev |
| 06:38 |
|
darkrose joined #minetest-dev |
| 06:38 |
|
darkrose joined #minetest-dev |
| 07:17 |
|
PenguinDad joined #minetest-dev |
| 07:29 |
|
werwerwer joined #minetest-dev |
| 07:33 |
|
proller joined #minetest-dev |
| 07:44 |
|
kaeza joined #minetest-dev |
| 07:50 |
|
proller joined #minetest-dev |
| 09:10 |
|
jin_xi joined #minetest-dev |
| 09:38 |
|
iqualfragile joined #minetest-dev |
| 09:44 |
|
Taoki[laptop] joined #minetest-dev |
| 09:50 |
|
Anchakor_ joined #minetest-dev |
| 10:04 |
|
Jordach joined #minetest-dev |
| 10:06 |
|
Anchakor_ joined #minetest-dev |
| 10:17 |
|
Anchakor_ joined #minetest-dev |
| 10:21 |
|
proller joined #minetest-dev |
| 10:33 |
|
proller joined #minetest-dev |
| 11:01 |
|
proller joined #minetest-dev |
| 11:37 |
|
proller joined #minetest-dev |
| 11:44 |
|
proller joined #minetest-dev |
| 11:45 |
|
ImQ009 joined #minetest-dev |
| 11:49 |
|
kaeza joined #minetest-dev |
| 12:05 |
|
PilzAdam joined #minetest-dev |
| 12:50 |
|
OldCoder joined #minetest-dev |
| 12:51 |
|
hmmmm joined #minetest-dev |
| 13:08 |
|
VargaD_ joined #minetest-dev |
| 13:44 |
|
Anchakor_ joined #minetest-dev |
| 13:45 |
|
rubenwardy joined #minetest-dev |
| 14:12 |
|
rubenwardy joined #minetest-dev |
| 14:17 |
rubenwardy |
What is a "user-agent" in terms of Minetest? |
| 14:35 |
|
NakedFury joined #minetest-dev |
| 15:08 |
|
ShadowNinja joined #minetest-dev |
| 15:11 |
|
ShadowNinja joined #minetest-dev |
| 15:12 |
|
ShadowNinja joined #minetest-dev |
| 15:34 |
ShadowNinja |
celeron55: Is Minetest a registered trademark? |
| 15:37 |
celeron55 |
it's not registered, but it is a trademark |
| 15:43 |
|
zat joined #minetest-dev |
| 15:44 |
|
Calinou joined #minetest-dev |
| 15:47 |
|
jin_xi joined #minetest-dev |
| 15:55 |
|
restcoser joined #minetest-dev |
| 16:01 |
us`0gb |
We should start spelling it as Minetestâ„¢ then. |
| 16:08 |
rubenwardy |
ofc |
| 16:09 |
|
sapier joined #minetest-dev |
| 16:09 |
ShadowNinja |
Either that's the box character, or my font doesn't support it. |
| 16:10 |
ShadowNinja |
sapier: #1226 |
| 16:10 |
ShadowBot |
https://github.com/minetest/minetest/issues/1226 -- [WIP] Remove dependency on marshal and push the Lua error handler only once by ShadowNinja |
| 16:12 |
ShadowNinja |
sapier: Also, what's the rationalle for UpperCamelCase/lowerCamelCase in the async classes? lowerCamelCase or underscore_style is normally used for things that aren't types. |
| 16:12 |
sapier |
already looking at it ... haven checked much but you're wrolng about lua -> cpp move cpp is part of scriptapi providing interface to access FROM cpp ... I don't know of any way to use async from core |
| 16:12 |
|
Robby joined #minetest-dev |
| 16:14 |
sapier |
In any case this move should be done on a separate commit as it's impossible to review your changes in this files |
| 16:15 |
sapier |
why do you add "minetest" to mainmenu? |
| 16:15 |
ShadowNinja |
sapier: Lua doesn't call anything in s_async. |
| 16:15 |
sapier |
async is a pure lua feature not part of scriptings core interface |
| 16:15 |
ShadowNinja |
For serialize.lua. I'm not sure why a seperate engine table was made in the first place. |
| 16:16 |
ShadowNinja |
Otherwise all common files need local tbl = minetest or engine or similar. |
| 16:16 |
sapier |
because of starting transition from "minetest" name to some generic |
| 16:16 |
|
Garmine joined #minetest-dev |
| 16:16 |
sapier |
minetest core is supposed to be a engine for arbitrary games why does api have to contain "minetest"? |
| 16:16 |
ShadowNinja |
Ah, then change it for the game API too and add minetest = engine to builtin.lua |
| 16:17 |
sapier |
I don't change anythin prior checking all of it |
| 16:18 |
sapier |
and later it's gonna be your task to fix it |
| 16:18 |
us`0gb |
sapier: The engine itself is called Minetest maybe? |
| 16:18 |
ShadowNinja |
us`0gb: But there's freeminer. |
| 16:18 |
sapier |
STOP |
| 16:18 |
us`0gb |
ShadowNinja: Isn't Freeminer a fork of the engine? |
| 16:18 |
sapier |
don't argue about names we've got enough tecnical things to check first |
| 16:19 |
* us`0gb |
sets +q on 0gb.us@* |
| 16:21 |
ShadowNinja |
sapier: So, I agree with the minetest -> engine change, but think it should be done everywhere. But why are you using UpperCamelCase for code and data? |
| 16:22 |
sapier |
most likely I did work on code using that style right before writing it ... I hate working on different code I'm allways critizized for using style of oposit code in each one |
| 16:23 |
ShadowNinja |
Ah. I'll fix that then, since this is already a pretty big change. |
| 16:24 |
sapier |
please first split the file move from your other changes |
| 16:24 |
sapier |
noone can check changes in those two files |
| 16:25 |
sapier |
did you do any performance check comparing marshal to dump? |
| 16:26 |
ShadowNinja |
No, but as long as they both finish within a few miliseconds it doesn't really matter. |
| 16:26 |
ShadowNinja |
And can you check if the broken LuaJIT version checking is still relevant? |
| 16:27 |
sapier |
first of all I'm gonna check if dump works at all as I decided not to use it because of failing to serialize a full class of functions ... if this didn't improve since that time this fix is broken by definition |
| 16:29 |
sapier |
btw how do you wanna fix the issue loadstring not beeing possible on any security relevant lua engine? |
| 16:29 |
sapier |
e.g. client side lua |
| 16:30 |
ShadowNinja |
sapier: Hmmm... |
| 16:33 |
ShadowNinja |
sapier: You'll have to trust builtin and add setfenv(func, engine.restricted_environment) or similat to the caller. |
| 16:34 |
sapier |
it's not relevant for async but I won't allow loadstring on client side lua as long as I contribute to minetest |
| 16:36 |
|
Zeitgeist_ joined #minetest-dev |
| 16:36 |
|
Zeitgeist_ joined #minetest-dev |
| 16:37 |
ShadowNinja |
sapier: It can't be allowed outside the local builtin install. |
| 16:47 |
celeron55 |
sapier: the engine's name is minetest; i don't see what the problem is with the minetest namespace |
| 16:47 |
celeron55 |
more so, it's bad practice to have a generic namespace like that |
| 16:48 |
celeron55 |
maybe some lua library has something called "engine" in it if they have named things badly |
| 16:48 |
celeron55 |
but such definitely won't have anything called "minetest" - that is the point of having a name |
| 16:54 |
sapier |
I wonder why this comes into mind about half a year after it was introduced ... I don't care about names as long as I don't have work to change them back and forward so if everyone wants to change it I suggest the one changeing it everywhere |
| 17:01 |
sapier |
ok performance of string.dump seems to be even better then marshal (using luajit) ... guess it'd be different on lua but if someone needs performance that badly he's gonna need luajit anyway |
| 17:01 |
sapier |
ShadowNinja: we don't "need" the check any longer for our builtin features but luajit still is broken |
| 17:02 |
sapier |
btw using serialize for data isn't compatible to current way of doing it you'd need to use dump for them too |
| 17:03 |
sapier |
this is relevant if someone uses some sort of "object" containing functions in lua |
| 17:07 |
sapier |
s_item.cpp changes are not related to async at all, nor are they in a file changed for it make a different commit ... why do people always believe to mix up things ... thats so proller |
| 17:07 |
sapier |
s_inventory.cpp too |
| 17:08 |
sapier |
s_entity.cpp too |
| 17:08 |
ShadowNinja |
sapier: string.dump dumps only functions. We already have minetest.(de)serialize for data, and an option to serialize functions could be added (if t == "function" then s = s.."loadstring("..serialize_string(string.dump(x))..")" end |
| 17:08 |
sapier |
minetest.deserialize doesn't dump functions |
| 17:08 |
sapier |
then add it and use it for everything |
| 17:08 |
ShadowNinja |
sapier: Yes, I know. Does it need to? |
| 17:09 |
sapier |
if you want this to be a 100% replacement and not 90% only ... yes |
| 17:09 |
ShadowNinja |
I can try to seperate the changes. And 90% seems close enough, as long as functions aren't being passed already. |
| 17:10 |
sapier |
no 90% is crap |
| 17:11 |
sapier |
either 100% or nothing ... to often people never add the missing 10% once they're beta code is merged ... and async is meant to be made available for minetest api too |
| 17:12 |
sapier |
and I don't have any interest in (re) adding function passing again once I need it for mobf ... the whole api is originaly meant for performance hungry things like mobs ... mainmenu was just a simple (mostly) uncritical testcase |
| 17:13 |
sapier |
I guess separating the inventory entity and item things is just commiting them in a second step instead of all at once |
| 17:14 |
sapier |
none of our code uses space after if why do you change this all around? |
| 17:16 |
|
Robby joined #minetest-dev |
| 17:16 |
celeron55 |
i guess it's the official coding style these days |
| 17:17 |
sapier |
I consider this to be code style fetishism ;-) |
| 17:17 |
celeron55 |
i will always hate spaces there though |
| 17:18 |
sapier |
That's one of the sideeffects defining project codestyle to be "kernel" + something ... if kernel changes in a year we've got to change anything again ;-) |
| 17:19 |
sapier |
Another point local variables in for loops. Althogh I hate iterators defined there bloating code (as long as we don't have cx11), I don't think those variables should be declared prior for loop polluting namespace of whole function |
| 17:20 |
ShadowNinja |
sapier: Actually a lot of code does. |
| 17:20 |
sapier |
C code does have to do so because of ansi standard but that's not required in c++ and as I said IMHO that's bad style |
| 17:21 |
celeron55 |
it's definitely bad style in C++ |
| 17:21 |
sapier |
variable scope should be as minimal as possible |
| 17:21 |
ShadowNinja |
sapier: We stay at the version of the kernel code style that was added at that time unless we explictly chose to use the new style. |
| 17:22 |
sapier |
come one ShadowNinja in about a year you/someone're gonna claim "this isn't new but has ever been this way" |
| 17:22 |
ShadowNinja |
sapier: Hmmm, good point on the scope. |
| 17:22 |
sapier |
but no style discussion again that's not gonna give any benefit |
| 17:23 |
ShadowNinja |
Maybe we should have a discussion about code style. But this is one of those thinks where it's unlikely that developers will budge much. |
| 17:23 |
sapier |
wow I'm disappointed ... s_server.cpp L84 ... you missed a space |
| 17:23 |
sapier |
;-) |
| 17:23 |
ShadowNinja |
sapier: I only changed ones that were nearby my edits. |
| 17:24 |
sapier |
for() loop thing isn't code style but coding style |
| 17:24 |
sapier |
you changed L84 ;-P |
| 17:24 |
ShadowNinja |
(To prevent breaking pulls) |
| 17:24 |
ShadowNinja |
Alright, I'll check it. |
| 17:24 |
sapier |
you even added a space there but missed a second one ;-) |
| 17:25 |
ShadowNinja |
Oh, after. |
| 17:26 |
sapier |
MAINMENU_ASYNC_THREADS I'd not remove this to be the number ... or is the define meant to be the threads semselfs? |
| 17:26 |
sapier |
-s+th |
| 17:30 |
ShadowNinja |
The fact that it's a number seems fairly obvious. That could be changed though. |
| 17:30 |
sapier |
scripting_mainmenu.cpp L88++ you're a changeing fetishist aren't you ;-) |
| 17:31 |
sapier |
it's only obvious at the define (of course) but not where it's used |
| 17:33 |
sapier |
ok so imho three things are left 1) split that pull to reasonable parts, especially the moved files are very very hard to check if done this way (and of course the unrelated changes shouldn't be mixed up) ... 2) add the missing features (primary passing lua tables containing functions themselfs as parameters) |
| 17:33 |
sapier |
and 3rd .. the cpp_api lua_api thing ... the difference between those two was meant to be cpp_api things provide interface to be called by core while lua_api is code providing interface to lua |
| 17:34 |
sapier |
async is not anything provided to core but to lua only |
| 17:35 |
ShadowNinja |
sapier: On the contrary, C++ calls things in s_async, but it does not provide a Lua API, l_mainmenu does that. |
| 17:35 |
sapier |
no core doesn't call s_async things but just starts it |
| 17:35 |
ShadowNinja |
It's also more similar since it's derived (or should be derived) from ScriptApiBase, not ModApiBase. |
| 17:36 |
sapier |
still it's not a core interface |
| 17:36 |
ShadowNinja |
sapier: It also calls Step(). |
| 17:36 |
ShadowNinja |
It's meant to be used by Lua, but it doesn't provide a lua API, just like s_item and the like. |
| 17:36 |
sapier |
what exactly is "it" right now? |
| 17:37 |
ShadowNinja |
it = [ls]_async.* |
| 17:37 |
sapier |
please define it to be a single thing not everything |
| 17:38 |
sapier |
to me cpp_api contains functions to access lua data from core |
| 17:38 |
ShadowNinja |
BTW, why do lua_api files have a l_ prefix, and cpp_api files have a s_ prefix? |
| 17:38 |
sapier |
while lua_api contains functions provided to lua |
| 17:38 |
ShadowNinja |
^ |
| 17:39 |
sapier |
because the folder was called scriptapi before and I didn't change everything |
| 17:39 |
sapier |
the change was requested 30 minutes prior merge ... as always |
| 17:39 |
ShadowNinja |
Lua API functions are provided by lua_api/l_mainmenu.cpp. The core classes for managing lua_State's for the async API are provided by ccp_api/s_async.cpp. |
| 17:40 |
ShadowNinja |
I'll remove the l_ and s_ prefixes in a later commit then. |
| 17:40 |
sapier |
no you can't |
| 17:40 |
sapier |
you're gonna mess up build |
| 17:41 |
sapier |
you need those prefixes because our build system just ignores folders |
| 17:41 |
sapier |
at least the way it's configured right now ... you can clean it if you want |
| 17:43 |
ShadowNinja |
Commit split. |
| 17:43 |
ShadowNinja |
It ignores folders? O_O |
| 17:43 |
sapier |
at least for headers ... so if you've got any naming conflict e.g. entity.h you're gonna have a lot of trouble |
| 17:44 |
sapier |
of course you can use abosolute paths everywhere .. and make sure cmake uses right one at right location |
| 17:45 |
sapier |
guess you're gonna find out how to fix it I didn't wanna spend hours on fixing this |
| 17:48 |
ShadowNinja |
I'll do that after my async changes are finished to minimize conflicts. |
| 17:50 |
ShadowNinja |
sapier: Is killing all threads without waiting safe? A long proccess can stop the menu from exiting. |
| 17:51 |
sapier |
no it isn't |
| 17:51 |
sfan5 |
https://github.com/minetest/minetest/pull/1227 |
| 17:51 |
sapier |
killing a thread might cause arbitrary damage |
| 17:51 |
sfan5 |
^ I am going to merge this soon(tm) |
| 17:52 |
sfan5 |
~title |
| 17:52 |
ShadowBot |
sfan5: Fix all warnings reported by clang by sfan5 · Pull Request #1227 · minetest/minetest · GitHub |
| 17:52 |
sapier |
:-) you're third one to do thins in half a year sfan5 |
| 17:52 |
sfan5 |
yeah |
| 17:52 |
sfan5 |
but this time I'm not going to let it wait 2 months until it's outdated |
| 17:52 |
sapier |
I closed my pull 2 or three days ago becaus of beeing outdated |
| 17:53 |
celeron55 |
sfan5: what is this collisionbox change |
| 17:53 |
celeron55 |
64 |
| 17:53 |
celeron55 |
-virtual bool getCollisionBox(aabb3f *toset) = 0; |
| 17:53 |
celeron55 |
64 |
| 17:53 |
celeron55 |
+virtual core::aabbox3d<f32>* getCollisionBox() = 0; |
| 17:53 |
celeron55 |
and the implementations |
| 17:53 |
sapier |
collision box is messing up virtual declarations |
| 17:53 |
sapier |
interface implemented by derived class has same name but different signature |
| 17:54 |
sfan5 |
it now uses aabb3f* which makes more sense than bool foobar(aabf *pointer) |
| 17:55 |
celeron55 |
i think it's very wrong |
| 17:55 |
sfan5 |
why? |
| 17:55 |
celeron55 |
the interface requires that the caller deallocates the result |
| 17:56 |
celeron55 |
return it by value or as std::unqiue_ptr or some other sane way |
| 17:56 |
sfan5 |
how would I return "no collisionbox" if returned by value |
| 17:57 |
celeron55 |
well, for example the way you're changing it away from |
| 17:57 |
sfan5 |
so I should make it bool foobar(aabf *pointer) again? |
| 17:58 |
sapier |
no matter how it's done the interface should be used everywhere :-) |
| 17:58 |
sapier |
not one derived class using this the other one another |
| 17:58 |
celeron55 |
or alternatively require that the object stores the collisionbox by itself so that the caller will not have to deallocate anything but instead has to just keep the object existing as long as it uses the collisionbox |
| 17:58 |
celeron55 |
i think some other code does it that way but i am not sure (or it could have been changed already) |
| 18:00 |
|
Exio4 joined #minetest-dev |
| 18:00 |
celeron55 |
if you insist on doing it that way, then it should be called "createCollisionBox" |
| 18:01 |
|
nore joined #minetest-dev |
| 18:01 |
celeron55 |
which implies that it allocates a new object that the caller must manage |
| 18:01 |
sfan5 |
*changes it back* |
| 18:02 |
sapier |
hmm german easter holidays ... now I know why development speeds up right now :-) |
| 18:02 |
sfan5 |
lel |
| 18:02 |
celeron55 |
lol |
| 18:02 |
sfan5 |
you got it |
| 18:02 |
sfan5 |
even when I'm on vacation and only have about 1 hours in the morning and 4 in the evening, I still manage to do stuff |
| 18:03 |
sapier |
hope people think about cleaning up stuff for merging needs to be done too |
| 18:04 |
sapier |
talking about merge ... any objections for #1130? |
| 18:04 |
ShadowBot |
https://github.com/minetest/minetest/issues/1130 -- Jthread: remove locks that arent absolutely required by sapier |
| 18:10 |
celeron55 |
that will cause errors on thread error detectors like helgrind |
| 18:10 |
sapier |
why? |
| 18:11 |
celeron55 |
is it valid code in x86 and ARM? is setting and reading a boolean an atomic operation? |
| 18:11 |
sapier |
hmm I didn't check on arm by now true |
| 18:11 |
celeron55 |
they check that memory isn't accessed from multiple threads without locking |
| 18:11 |
celeron55 |
if you haven't tried helgrind, try it |
| 18:11 |
celeron55 |
it's one of the tools in valgrind |
| 18:12 |
|
restcoser joined #minetest-dev |
| 18:12 |
celeron55 |
(valgrind doesn't work with luajit so that needs to be left out from the build first with -DDISABLE_LUAJIT=1) |
| 18:13 |
sapier |
I know that's why I added the locks when I did this (and have been punched for using that much locks) ;-) ... but I don't se any way how this might fail ... even if there was a concurrent read the desired function would still remain working |
| 18:13 |
celeron55 |
oh well |
| 18:14 |
celeron55 |
maybe you should add comments to both of those functions that it may cause warnings but is safe |
| 18:14 |
sapier |
even if a invalid value is read the desired behaviour would only be delayed by short time |
| 18:14 |
celeron55 |
so that if someone uses helgrind, they can correctly set up suppressions |
| 18:14 |
celeron55 |
and don't need to try to guess whether it's intended or not |
| 18:15 |
celeron55 |
it's fine to me after you do that |
| 18:16 |
|
OldCoder joined #minetest-dev |
| 18:21 |
sfan5 |
celeron55: fixed it |
| 18:21 |
sapier |
ok I'm gonna check on arm too (as I now have a arm device) and add those comments |
| 18:23 |
celeron55 |
sfan5: seems fine |
| 18:23 |
sfan5 |
meging in 10 mingw |
| 18:23 |
sfan5 |
merging* mins* |
| 18:23 |
sfan5 |
I can't english today |
| 18:24 |
sapier |
seems to be quite less changes ... I think I had quite some more ;-) hope you didn't miss a lot ;-) |
| 18:25 |
sapier |
assert(state < sizeof(statenames)); why did you remove this? |
| 18:26 |
sapier |
assert(m_list_size <= SEQNUM_MAX+1); and this onw? |
| 18:27 |
celeron55 |
i don't see a reason to remove those either |
| 18:28 |
sapier |
if interface is used correct they can't happen ok ... but if not they're valid assert cases |
| 18:28 |
sfan5 |
1) enum ClientState seems to be guaranteed to be in range (clang said this) [also sizeof(statenames) is incorrect anyway] |
| 18:28 |
sapier |
as long as noone calles the function casting int to enum |
| 18:28 |
sfan5 |
2) m_list_size is u16, it would overflow and never go above 65536 |
| 18:28 |
ShadowNinja |
sapier: http://ix.io/bHx/diff |
| 18:29 |
sapier |
SEQNUM_MAX doesn't always have to be defined as 65536 |
| 18:29 |
sfan5 |
but then I can't remove the warning :-( |
| 18:29 |
sfan5 |
should I make m_list_size u32? |
| 18:29 |
sapier |
values above 65536 aren't possible due to protocol format |
| 18:30 |
sfan5 |
IIRC m_list_size does not come from a network packet |
| 18:30 |
sapier |
but it'd be better then removing the check |
| 18:30 |
sfan5 |
(not directly) |
| 18:30 |
sapier |
no but the SEQNUM_MAX is a network parameter |
| 18:31 |
sapier |
can we rename ShadowNinja to RenameNinja? ;-) I'm quite sure he'd be glad about beeing renamed :-) |
| 18:32 |
|
kaeza joined #minetest-dev |
| 18:33 |
sapier |
can we please once and forever define if class MEMBERS shall be named m_ or not ... imho this is crucial if you wanna use code without full blown ide support ... what do others think? |
| 18:33 |
ShadowNinja |
sapier: Nope, Charybdis and -seven don't support /sanick or /svsnick or /onick or any similar commend other than RSFNC, which is services-server-only. |
| 18:33 |
sapier |
sad renameNinja would match quite good ;-) |
| 18:34 |
ShadowNinja |
Use of Hungarian notation is very limited, to g_ for globals (rare in the first place) and m_ for members. The latter is discouraged for newer code (since one can simply notice there is no variable by that name declared in that method's scope), but needed when adding a member to older code for consistency. |
| 18:34 |
sfan5 |
sapier: you did not read the code style guidelines |
| 18:34 |
sfan5 |
it says members should only be m_ if writing additional code for classes that already use this |
| 18:34 |
sapier |
it's discouraged because of you added it and this is one of the major faults in our coding guidelines |
| 18:35 |
sfan5 |
>because of you added it |
| 18:35 |
sfan5 |
what? |
| 18:35 |
sapier |
it's adding quite a lot of code readability while spending ony 2 characters |
| 18:35 |
ShadowNinja |
sapier: How did this work on Linux? https://github.com/minetest/minetest/commit/960d731587f58036bd4957f4a77db41e145c2d04 |
| 18:35 |
sapier |
sorry meant ShadowNinja ;-) |
| 18:35 |
sfan5 |
ShadowNinja: it worked with #ifdef _WIN32 |
| 18:36 |
sapier |
old version never did work |
| 18:36 |
sapier |
IMHO removing m_ g_ and things like that are plain wrong and reason of avoidable misstakes |
| 18:37 |
sapier |
you always have to check twice if a variable is a member or a local one ... you see it at once if it's prefixed with m: |
| 18:37 |
ShadowNinja |
sfan5: Ah. |
| 18:38 |
ShadowNinja |
sfan5: Why are the extra () required around core::stringc(x)? |
| 18:38 |
sfan5 |
ShadowNinja: ask the clang devs |
| 18:38 |
sapier |
g_ shouldn't be required to remove as g_ are always evil |
| 18:39 |
ShadowNinja |
Global variables are bad, so a g_ prefix seems fine. |
| 18:39 |
sapier |
http://ix.io/bHx/diff IMHO is mostly bad and wrong (it's almost only removing m_) |
| 18:40 |
sapier |
why do you add a deque ? vector is already used there what's wrong with it? |
| 18:41 |
sapier |
can you please stop changeing things for sake of changeing? |
| 18:41 |
ShadowNinja |
sapier: Because it's a queue. |
| 18:41 |
|
proller joined #minetest-dev |
| 18:41 |
sapier |
AND? |
| 18:42 |
ShadowNinja |
queue uses a linked list I imagine, making pushing and poping constant-time. |
| 18:43 |
ShadowNinja |
Complexity is defined as constant. |
| 18:43 |
sapier |
if there was int_liter and int_kg and int_apples we'd use those for counting apples and int_kg for kilogramms because int wouldn't fit any longer? |
| 18:43 |
|
OldCoder joined #minetest-dev |
| 18:43 |
ShadowNinja |
What? |
| 18:44 |
sapier |
ok lets first check but for what I remember pushing and popping vector is same complexity |
| 18:44 |
ShadowNinja |
Nope, it's O(n). |
| 18:46 |
ShadowNinja |
Pops are O(n), pushes are O(1) if no reallocation is needed. |
| 18:46 |
ShadowNinja |
deque is O(1) for pops and pushes. |
| 18:46 |
celeron55 |
yes those pop_fronts are slow there |
| 18:47 |
sapier |
that's wrong |
| 18:47 |
celeron55 |
altough, there are probably only like 1 task so they're not |
| 18:47 |
sapier |
dequeue is only faster for random inserts |
| 18:47 |
sapier |
and for push front |
| 18:48 |
celeron55 |
"they provide a functionality similar to vectors, but with efficient insertion and deletion of elements also at the beginning of the sequence, and not only at its end. But, unlike vectors, deques are not guaranteed to store all its elements in contiguous storage locations: accessing elements in a deque by offsetting a pointer to another element causes undefined behavior." |
| 18:48 |
celeron55 |
http://www.cplusplus.com/reference/deque/deque/ |
| 18:48 |
celeron55 |
i.e. it's approximately the right thing for the job, unlike vector |
| 18:49 |
sapier |
but we don't push to front or insert |
| 18:49 |
celeron55 |
a vector will always keep the first element at [0] |
| 18:50 |
celeron55 |
hmm |
| 18:50 |
ShadowNinja |
deque is approximately equivalent to std::list. |
| 18:50 |
|
EvergreenTree joined #minetest-dev |
| 18:50 |
celeron55 |
i wonder if it's optimized for pop_front to not immediately reallocate, actually |
| 18:50 |
celeron55 |
vector i mean |
| 18:50 |
sfan5 |
I did some more changed |
| 18:50 |
celeron55 |
it could, but it would waste memory |
| 18:50 |
sfan5 |
can I merge #1227 now? |
| 18:50 |
ShadowBot |
https://github.com/minetest/minetest/issues/1227 -- Fix all warnings reported by clang by sfan5 |
| 18:51 |
sapier |
http://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html |
| 18:52 |
celeron55 |
there's no pop_front there |
| 18:52 |
celeron55 |
also we can't know what every standard library implementation does; for example g++'s std::list's size() is O(n) while MSVC's is O(1) |
| 18:54 |
sapier |
sadly none of the benchmarks tells about pop front ... and in push back vector is slightly faster |
| 18:54 |
celeron55 |
the standard clearly says that deque does both pop_front and push_back fast, while it doesn't say that vector would do pop_front fast |
| 18:54 |
celeron55 |
to me it seems likely that vector will copy all elements every pop_front |
| 18:57 |
sapier |
did anyone benchmark it? ... I've seen to often "assumptions" beeing wrong because of compilers already optimizing way better |
| 18:57 |
celeron55 |
how many jobs do you expect there to be at maximum? |
| 18:58 |
celeron55 |
if the answer is under 20, the choice of container doesn't matter at all |
| 18:58 |
ShadowNinja |
It probably doesn't matter much performance-wise, but a deque is the right container to use. |
| 18:58 |
sapier |
if it's no performance result why don't we stop discussing and just keep it the way it's already in? |
| 18:58 |
celeron55 |
because correct is better 8) |
| 18:58 |
sapier |
it's not correct it's just using something different |
| 18:59 |
ShadowNinja |
We use a queue for a queue, rather than an array. |
| 18:59 |
sapier |
and no, on reading modstore count gets way beyond 20 |
| 19:00 |
sapier |
a vector ain't an array |
| 19:00 |
ShadowNinja |
A vector is a dynamically realocated array. |
| 19:00 |
sapier |
for gods sake change it but it's gonna be you looking for issues in that code |
| 19:01 |
|
nore joined #minetest-dev |
| 19:01 |
sapier |
and I'm gonna stress it to the end once using it for mobf believe me |
| 19:02 |
ShadowNinja |
http://pastebin.ubuntu.com/7256975/ O_O |
| 19:04 |
sapier |
your problem ;-P |
| 19:06 |
sapier |
ShadowNinja: did you do java programming before? |
| 19:07 |
ShadowNinja |
Hmmm, It might be because it's empty... |
| 19:07 |
sapier |
it can't be empty |
| 19:08 |
sapier |
m_JobQueueCounter.Post(); one token per job, there can't be more tokens then jobs queued |
| 19:08 |
sfan5 |
something something #1227 something something |
| 19:08 |
ShadowBot |
https://github.com/minetest/minetest/issues/1227 -- Fix all warnings reported by clang by sfan5 |
| 19:08 |
ShadowNinja |
Oh, derp. I was poping from the wrong queue. |
| 19:09 |
sapier |
that's happening if you change working code ;-) |
| 19:09 |
sapier |
especially if you change lots of things same time |
| 19:10 |
ShadowNinja |
Well, it works now. |
| 19:10 |
ShadowNinja |
http://ix.io/bHA/diff |
| 19:11 |
sapier |
no need to check withing that much changes I don't see the relevant things any longer maybe others are capable of doing it I'm not |
| 19:11 |
sapier |
well to be more exact I don't wanna spend hours for looking for s <-> S |
| 19:12 |
sapier |
finding out which variable is global member or local and things like that |
| 19:12 |
sapier |
especially not in diffs ... where it's impossible to decide about a variable |
| 19:13 |
sapier |
who the hell did discourage m_? proller? |
| 19:13 |
sapier |
xyz? |
| 19:13 |
sapier |
celeron? |
| 19:13 |
ShadowNinja |
sfan5: That seems fine if you fix (or have fixed) the other mbstowcs return value ignore and fix the commit message. |
| 19:13 |
sfan5 |
ShadowNinja: gcc still complains about the return value |
| 19:14 |
sfan5 |
I don't know how to make it shut up |
| 19:14 |
ShadowNinja |
sapier: hmmm. |
| 19:14 |
ShadowNinja |
sfan5: I mean, there was annother one that has to be ignored too. |
| 19:14 |
sapier |
can you remove mbtowc completely in there? that's a function beeing very os specific and shouldn't be used outside of porting |
| 19:14 |
proller |
m_ is stupidest thing |
| 19:15 |
|
tomreyn joined #minetest-dev |
| 19:15 |
sapier |
of course proller you're able to remember thousands of variables on using kwrite to edit code |
| 19:16 |
sfan5 |
sapier: it is in an #ifdef linux |
| 19:16 |
sapier |
that's even worse |
| 19:16 |
sapier |
ok change it another time |
| 19:16 |
sfan5 |
ShadowNinja: https://github.com/sfan5/minetest/commit/7614d0cab23ab67a7b72b7fb3e02a37d0ee82b76#diff-420ef4c204f7735e35d574866057a20bR560 |
| 19:16 |
sfan5 |
gcc still complains on the same line |
| 19:18 |
|
kaeza joined #minetest-dev |
| 19:18 |
sapier |
ok seems I'm the only one who want's code to be easyly readable so I wont fix any bugs in code that was changed from readable to non readable ... |
| 19:19 |
ShadowNinja |
sfan5: There is a second mbtowc with an ignored return value. |
| 19:19 |
sfan5 |
where? |
| 19:21 |
|
nore joined #minetest-dev |
| 19:21 |
ShadowNinja |
intlGUIEditBox.cpp |
| 19:23 |
sfan5 |
line? |
| 19:23 |
ShadowNinja |
sfan5: And this is related: https://github.com/minetest/minetest/issues/1158 |
| 19:23 |
sfan5 |
I tried to fix that |
| 19:23 |
sfan5 |
but gcc still complains |
| 19:29 |
ShadowNinja |
Can #808 be closed? |
| 19:29 |
ShadowBot |
https://github.com/minetest/minetest/issues/808 -- Static Analysis Warnings |
| 19:30 |
ShadowNinja |
sfan5: grep mbtowcs src/intlGUIEditBox.cpp |
| 19:30 |
sfan5 |
probably |
| 19:30 |
ShadowNinja |
Around 276. |
| 19:31 |
ShadowNinja |
You can probably add a -Wno-X if casting to void doesn't fix it. |
| 19:32 |
sfan5 |
no, those warnings can be useful |
| 19:32 |
sfan5 |
let's just leave them |
| 19:32 |
sfan5 |
gcc sucks anyway |
| 19:32 |
sfan5 |
clang does not report any warnings |
| 19:35 |
sfan5 |
ShadowNinja: care to agree so I can push it? |
| 19:39 |
sapier |
have those warnings been there before? |
| 19:40 |
sfan5 |
dunno |
| 19:40 |
sapier |
if they have been there ignore them if they havent fix them ... it's quite simple |
| 19:40 |
sfan5 |
well |
| 19:40 |
sfan5 |
I did definitely not add them |
| 19:41 |
sapier |
it's useless to fix warnings by adding new ones so if your changes would be source of warning that'd be a problem if not it's obviously not your problem |
| 19:42 |
sapier |
but by now gcc is still mainstream compiler and clang will take some time to get mainstream (if ever) |
| 19:42 |
sfan5 |
gcc only gives 3 warnings (client: 2, server: 1) when compiling |
| 19:42 |
sfan5 |
(atleast on travis-ci) |
| 19:42 |
sapier |
how did we configure gcc? |
| 19:43 |
|
BlockMen joined #minetest-dev |
| 19:43 |
sapier |
no doubt clang is way mor strict ... but you can get a lot of warnings from gcc too |
| 19:57 |
ShadowNinja |
sfan5: https://github.com/sfan5/minetest/commit/d6f193434b7c31d4b9771a0cbcc1c0e59cf125d3#diff-a8c2ccea72474c4e178b267a03b8610eR212 Why? Default is 0, right? |
| 19:58 |
sfan5 |
I need the value for 'invalid' to be > 0 because it's signed |
| 19:58 |
sfan5 |
unsigned* |
| 19:59 |
ShadowNinja |
OK. You could also add DECO_INVAILID somewhere in there and use that. |
| 20:00 |
sfan5 |
>could |
| 20:00 |
ShadowNinja |
Otherwise it seems good (with squashing). |
| 20:00 |
sfan5 |
thank you! |
| 20:00 |
sfan5 |
\o/ |
| 20:00 |
sfan5 |
pushing! |
| 20:00 |
sfan5 |
finally we have this sorted out |
| 20:01 |
ShadowNinja |
sfan5: Now can you check this? :-) https://github.com/minetest/minetest/pull/1226 |
| 20:02 |
sfan5 |
I'll try, I'm not that good with lua stuff |
| 20:02 |
ShadowNinja |
sfan5: And this, mich simpler: http://sprunge.us/NRgQ |
| 20:02 |
ShadowNinja |
+?diff |
| 20:03 |
sfan5 |
why are you adding {'s |
| 20:03 |
ShadowNinja |
You mean if (x) y(); -> if (x) { y(); } ? |
| 20:03 |
sfan5 |
yes |
| 20:04 |
ShadowNinja |
It reduces confusion and prevents errors when adding something new to the block. Eg, if (x) y(); z(); Won't work as if (x) { y(); z(); } |
| 20:05 |
sfan5 |
it increases confusion |
| 20:05 |
sfan5 |
rest seems fine |
| 20:06 |
|
PenguinDad joined #minetest-dev |
| 20:07 |
ShadowNinja |
sfan5: And the DB tweak? |
| 20:07 |
sfan5 |
I meant that one with <sfan5> rest seems fine |
| 20:07 |
ShadowNinja |
OK. |
| 20:13 |
sapier |
return (u64) pos.Z * 0x1000000 come on guys can you pleas use shift where you do shift? |
| 20:13 |
sapier |
how much other things like that are burried in there? |
| 20:14 |
ShadowNinja |
sapier: I had some issues with a bit shift there. |
| 20:14 |
sapier |
what issues? |
| 20:16 |
ShadowNinja |
sapier: Try it and load a map, then look around at 0, 10, 0. :-) |
| 20:16 |
ShadowNinja |
The map itself shouldn't be corrupted as long as you don't touch any nodes. |
| 20:17 |
sapier |
that doesn't make sense ShadowNinja |
| 20:18 |
sapier |
shift and multiplication by pot2 values are identical operations ... despite of insane compiler may really use multiplication in later case |
| 20:18 |
sapier |
none I know does |
| 20:18 |
ShadowNinja |
sapier: The map is loaded into memory incorrectly, but it won't be written unless you touch something. |
| 20:18 |
ShadowNinja |
sapier: Negative numbers cause issues. |
| 20:18 |
sapier |
did you debug it? |
| 20:18 |
sfan5 |
how about using abs() ? |
| 20:19 |
sapier |
there aren't negative numbers for u64 |
| 20:19 |
sapier |
or is map format broken as of beginning and we abuse a overflow to keep compatibility? |
| 20:19 |
* ShadowNinja |
tries again. |
| 20:20 |
ShadowNinja |
sapier: There are apperently negative indices, I don't know how, considering that we don't use the last bit. |
| 20:20 |
sapier |
the whole function doesn't make sense it's casted to unsigned for calculation and returned as signed |
| 20:24 |
ShadowNinja |
I got it to work. I had order-of-operations wrong. I don't know what is happening with the signedness. |
| 20:25 |
ShadowNinja |
http://ix.io/bHG/diff Pushing in a minute |
| 20:25 |
sapier |
wait a second |
| 20:26 |
sapier |
0xfffffffffc000001 is composed of z=0xfffc y=0x0000 and x=0x001 |
| 20:26 |
ShadowNinja |
Yes. |
| 20:26 |
ShadowNinja |
Er, ffc rather. |
| 20:27 |
sapier |
I'm missing some bytes |
| 20:27 |
sapier |
if there was a 16 bit shift ... I'd have expected something like 0xFFFFFFFc00000001 |
| 20:27 |
ShadowNinja |
Only part of the 64-bit number is used. |
| 20:28 |
ShadowNinja |
There's only a 12 bit shift because it's the BLOCK position, not the NODE position. |
| 20:28 |
sapier |
ok in this case it's gonna make more sense |
| 20:28 |
sapier |
and that's reason why I prefere << number for * somevalue |
| 20:29 |
ShadowNinja |
So, seems fine? |
| 20:30 |
sapier |
I guess you tested it, I don't see a obvious error |
| 20:31 |
sapier |
I don't even wanna think about how much it's gonna be to rebase android port :-( |
| 20:33 |
|
proller joined #minetest-dev |
| 20:45 |
|
EvergreenTree joined #minetest-dev |
| 20:52 |
|
Jordach joined #minetest-dev |
| 21:09 |
PilzAdam |
sfan5, https://github.com/minetest/minetest/issues/1158#issuecomment-40527349 |
| 21:21 |
|
BrandonReese joined #minetest-dev |
| 21:22 |
|
jin_xi joined #minetest-dev |
| 21:22 |
zat |
Guys please consider making the names of the players case-insensitive while still displaying in the case the user originally registered with. |
| 22:01 |
|
harrison__ joined #minetest-dev |
| 22:15 |
|
harrison__ joined #minetest-dev |
| 22:20 |
|
Zeitgeist_ joined #minetest-dev |
| 22:22 |
|
harrison__ joined #minetest-dev |
| 22:29 |
|
harrison__ joined #minetest-dev |
| 22:31 |
|
kahrl joined #minetest-dev |
| 22:42 |
|
sapier left #minetest-dev |
| 22:45 |
|
cheapie joined #minetest-dev |
| 23:51 |
ShadowNinja |
sfan5: Some changes to the mapper: http://ix.io/bHU/diff Performance difference: http://pastebin.ubuntu.com/7258368/ |
| 23:52 |
ShadowNinja |
This makes both DBs faster, and SQLite3 slower. The reason is because each block is fetched individually rather than a whole Z-slice at a time. |
| 23:59 |
|
iqualfragile joined #minetest-dev |
