| Time |
Nick |
Message |
| 00:01 |
RealBadAngel |
if i would make it blend with day night lights, they would become just invisible |
| 00:04 |
RealBadAngel |
ShadowNinja, also they do not change just overall lighting for visibility, separate color channels do cycle too |
| 00:05 |
RealBadAngel |
without that for example on leaves or obsidian you wouldnt notice that |
| 00:15 |
RealBadAngel |
but feel free to code it in a better way than i did |
| 00:15 |
RealBadAngel |
im going to sleep :P |
| 00:17 |
VanessaE |
sleep? |
| 00:17 |
VanessaE |
you can't do that |
| 00:28 |
|
blaise joined #minetest-dev |
| 00:44 |
|
Zeno` joined #minetest-dev |
| 00:54 |
|
blaise joined #minetest-dev |
| 00:54 |
|
blaise joined #minetest-dev |
| 01:17 |
|
Taoki joined #minetest-dev |
| 01:20 |
|
Taoki joined #minetest-dev |
| 01:56 |
|
iio7 joined #minetest-dev |
| 02:09 |
|
Miner_48er joined #minetest-dev |
| 02:11 |
|
iio7 left #minetest-dev |
| 02:15 |
|
iio7 joined #minetest-dev |
| 02:17 |
iio7 |
Hi. I am talking a quick look at the minetest code and I have three questions. Why are so much stuff commented out without any comments explaining why? Why the approx zero comments in general? And does the exist a coding standards for the project? |
| 02:23 |
iio7 |
Found the style guide. |
| 02:24 |
|
NakedFury joined #minetest-dev |
| 02:31 |
|
iio7 left #minetest-dev |
| 04:02 |
|
sol_invictus joined #minetest-dev |
| 04:38 |
|
emrys joined #minetest-dev |
| 05:39 |
|
Hunterz joined #minetest-dev |
| 05:47 |
|
kaeza joined #minetest-dev |
| 06:35 |
|
proller joined #minetest-dev |
| 06:42 |
|
sol_invictus joined #minetest-dev |
| 07:17 |
|
mberends joined #minetest-dev |
| 07:53 |
|
proller joined #minetest-dev |
| 08:42 |
Megaf |
Hi, I got a possible bug for you, and the bug developers like the most! Memory Leaks/Seg faults! https://github.com/minetest/minetest/issues/1619 |
| 08:42 |
|
jin_xi joined #minetest-dev |
| 08:51 |
Zeno` |
Last I ran valgrind I detected no leaks |
| 08:52 |
Megaf |
yep, but valgring doesnt run mods... |
| 08:52 |
Megaf |
nor players |
| 08:52 |
Zeno` |
yes it does |
| 08:52 |
Zeno` |
I run it with mods..... probably not *your* mods though |
| 08:53 |
Megaf |
well, I said possible memory leaks |
| 08:53 |
Megaf |
that doesnt happens all the time |
| 08:54 |
Megaf |
I dont know what trigers that, I had yesterday close to 20 players at the same time and memory use was pretty low |
| 08:54 |
Megaf |
but at the moment of the crash there was less than 8 players |
| 08:59 |
Zeno` |
hmm |
| 09:08 |
Megaf |
and I rebooted the server hourse before the crash |
| 09:21 |
Zeno` |
Why does the debug build use -O1? That's kind of unusual isn't it? |
| 09:53 |
Zeno` |
Megaf, can you add what mods you use to the report? |
| 09:53 |
Megaf |
ep |
| 09:53 |
Megaf |
yep |
| 09:56 |
Megaf |
Zeno`: done |
| 10:07 |
Zeno` |
Ok, I'll try with all your mods and see if I can reproduce |
| 10:09 |
Megaf |
Zeno`: well, I think you will have no luck with that |
| 10:09 |
Megaf |
sometimes my server stays up for a week, till some mod bring it down |
| 10:10 |
Megaf |
and without leaks |
| 10:22 |
|
ImQ009 joined #minetest-dev |
| 10:34 |
RealBadAngel |
ShadowNinja, ok, i made it adapting to light level at position |
| 10:35 |
RealBadAngel |
so it wont depend on day/night cycle just but a real light |
| 10:36 |
Zeno` |
RBA, do you know why the debug build uses -O1? |
| 10:36 |
RealBadAngel |
no idea |
| 10:37 |
RealBadAngel |
bbl, off to work |
| 10:54 |
|
proller joined #minetest-dev |
| 11:19 |
|
john_minetest joined #minetest-dev |
| 11:25 |
|
Taoki joined #minetest-dev |
| 11:31 |
|
Taoki joined #minetest-dev |
| 11:37 |
|
ImQ009 joined #minetest-dev |
| 11:41 |
|
Taoki joined #minetest-dev |
| 11:42 |
|
PenguinDad joined #minetest-dev |
| 12:00 |
Megaf |
#1620 |
| 12:01 |
Megaf |
hm, bot wont show the issue anymore, ok. Request: Progress bar/status/info for /clearobjects. https://github.com/minetest/minetest/issues/1620 |
| 12:13 |
|
NakedFury joined #minetest-dev |
| 12:19 |
|
proller joined #minetest-dev |
| 12:21 |
Megaf |
It depends on the mod john_minetest |
| 12:22 |
Megaf |
I learned that NPC mod will get wiped out by builtin item mod but sheeps from Creatures wont |
| 12:43 |
|
Amaz joined #minetest-dev |
| 12:52 |
Zeno` |
Are you here sfan5? |
| 12:52 |
sfan5 |
yes |
| 12:52 |
Zeno` |
I've pushed an update that I hope your comment refers to, but I'm not sure |
| 12:52 |
sfan5 |
what? where? |
| 12:52 |
Zeno` |
https://github.com/minetest/minetest/pull/1618 |
| 12:53 |
Zeno` |
I wasn't sure if you were referring to the indentation or something else |
| 12:53 |
sfan5 |
Zeno`: taking about the stange indent |
| 12:53 |
Zeno` |
yeah, thanks. I missed it |
| 12:53 |
sfan5 |
my comment was not that clear.. soo |
| 12:56 |
Zeno` |
As to why I want those member functions to be const correct... it's because I'm insane |
| 12:57 |
Zeno` |
I'm not sure it's low priority if somebody happens to want to refactor main() |
| 12:58 |
Zeno` |
although *shrug* |
| 13:41 |
|
ImQ009_ joined #minetest-dev |
| 13:50 |
|
Fury joined #minetest-dev |
| 13:52 |
|
NakedFury joined #minetest-dev |
| 13:59 |
|
NakedFury joined #minetest-dev |
| 14:33 |
|
ImQ009 joined #minetest-dev |
| 14:51 |
|
proller joined #minetest-dev |
| 15:05 |
|
proller joined #minetest-dev |
| 15:16 |
|
zat joined #minetest-dev |
| 15:25 |
|
sol_invictus joined #minetest-dev |
| 15:32 |
|
diemartin joined #minetest-dev |
| 15:38 |
|
blaaaaargh joined #minetest-dev |
| 15:39 |
|
Calinou joined #minetest-dev |
| 15:42 |
|
NakedFury joined #minetest-dev |
| 15:45 |
|
hmmmm joined #minetest-dev |
| 15:48 |
|
diemartin joined #minetest-dev |
| 15:52 |
|
Zeno` joined #minetest-dev |
| 15:52 |
Zeno` |
ShadowNinja, are you saying my pull request doesn't compile for you? |
| 15:53 |
|
kaeza joined #minetest-dev |
| 15:53 |
|
proller joined #minetest-dev |
| 15:53 |
ShadowNinja |
Zeno`: Yes, I haven't pulled your patch, but making the same change locally doesn't work. |
| 15:53 |
ShadowNinja |
Zeno`: What compiler are you using? |
| 15:53 |
Zeno` |
You have to change the mutex to mutable |
| 15:54 |
Zeno` |
I just commented in the pull request |
| 15:55 |
ShadowNinja |
Zeno`: Ah, I didn't think of that keyword. |
| 15:55 |
ShadowNinja |
Zeno`: const references should also be used for parameters though. |
| 15:56 |
|
Megaf_ joined #minetest-dev |
| 15:59 |
Zeno` |
ShadowNinja, correct |
| 16:00 |
Zeno` |
Did I miss something? |
| 16:00 |
|
kaeza joined #minetest-dev |
| 16:00 |
ShadowNinja |
Zeno`: https://github.com/minetest/minetest/pull/1618#issuecomment-55285956 |
| 16:00 |
Zeno` |
The reason for this is because I cannot refactor main to use const paramaters without this change |
| 16:01 |
Zeno` |
And in fact, the parameters should be const in a lot of other places as well, but this is just to "set it up" so to speak |
| 16:02 |
ShadowNinja |
Zeno`: Try to merge it with my commit, but it looks good. You should also make Settings const for the operators, and lock the mutexes after the (this == &other) check. |
| 16:03 |
Zeno` |
Ok, I will check tomorrow |
| 16:03 |
Zeno` |
it's 2AM here and I'm already asleep :) |
| 16:04 |
Zeno` |
Yes I see what your saying now (looking at your commit) |
| 16:04 |
ShadowNinja |
Alright, I might just do it myself and merge it while you're gone. |
| 16:04 |
Zeno` |
Can you merge mine first? |
| 16:05 |
Zeno` |
meh, it doesn't matter so long as it's done |
| 16:05 |
ShadowNinja |
Zeno`: I'll merge them at the same time, but in seperate commits. |
| 16:05 |
Zeno` |
ok |
| 16:05 |
Zeno` |
msg me tomorrow if you need something (I doubt you will) |
| 16:06 |
Zeno` |
night |
| 16:07 |
|
kaeza joined #minetest-dev |
| 16:08 |
Zeno` |
your changes look good but the unnecessary copying is avoided because you're passing by reference rather than them being const (I do agree 110% that the params should be const though) |
| 16:08 |
|
kaeza joined #minetest-dev |
| 16:09 |
Zeno` |
u16 getU16Ask(const std::string &name, const std::string &question, const u16 def) <--- def does not need to be const |
| 16:09 |
Zeno` |
it's not a reference nor a pointer so it's pass by value anyway |
| 16:09 |
Zeno` |
night |
| 16:11 |
|
kaeza joined #minetest-dev |
| 16:11 |
proller |
remove this unused shit |
| 16:35 |
|
rubenwardy joined #minetest-dev |
| 16:36 |
|
khonkhortisan joined #minetest-dev |
| 16:45 |
hmmmm |
why was m_mutex made mutable? |
| 16:45 |
hmmmm |
that's not necessary unless you're inheriting a class that has the member as const |
| 16:47 |
|
Hunterz joined #minetest-dev |
| 17:08 |
|
Krock joined #minetest-dev |
| 17:16 |
|
^v joined #minetest-dev |
| 17:17 |
|
le683 joined #minetest-dev |
| 17:19 |
|
^v joined #minetest-dev |
| 17:29 |
|
^v joined #minetest-dev |
| 17:41 |
ShadowNinja |
hmmmm: It's necessary for const methods to be able to lock and unlock it. |
| 17:46 |
ShadowNinja |
hmmmm: Here's the combined patch: https://github.com/ShadowNinja/minetest/compare/minetest:master...ShadowNinja:settings-cleanup |
| 17:47 |
ShadowNinja |
Comments on that ^? |
| 17:47 |
ShadowNinja |
(anyone) |
| 17:50 |
|
kaeza joined #minetest-dev |
| 17:51 |
|
sapier joined #minetest-dev |
| 17:52 |
hmmmm |
ShadowNinja, what's the point of this: https://github.com/ShadowNinja/minetest/compare/minetest:master...ShadowNinja:settings-cleanup |
| 17:52 |
hmmmm |
you just introduced a memory leak with that btw |
| 17:52 |
ShadowNinja |
hmmmm: It makes less copies and lets you use getters on a const Settings. |
| 17:53 |
ShadowNinja |
hmmmm: Where's the meak? |
| 17:53 |
ShadowNinja |
leak* |
| 17:53 |
hmmmm |
s isn't deleted |
| 17:53 |
hmmmm |
&str[0] is not a copy |
| 17:54 |
sapier |
moving the lock in updateValue is dangerous |
| 17:54 |
ShadowNinja |
Ah. |
| 17:54 |
sapier |
same in update |
| 17:55 |
sapier |
those comparisons are far from atomic |
| 17:55 |
hmmmm |
indeed, you're comparing each and every member in Settings with the other one while one may be getting updated |
| 17:57 |
ShadowNinja |
hmmmm: It's a pointer comparison, not a value comparison. |
| 17:58 |
hmmmm |
&? |
| 17:58 |
hmmmm |
you sure |
| 17:58 |
ShadowNinja |
hmmmm: Yes, & -> Address of. |
| 17:58 |
hmmmm |
it's a reference |
| 17:58 |
ShadowNinja |
Yes. |
| 17:58 |
hmmmm |
that's equivalent to having |
| 17:58 |
hmmmm |
Settings *other |
| 17:58 |
hmmmm |
if *other == |
| 17:58 |
hmmmm |
oh |
| 17:59 |
hmmmm |
I misread that |
| 17:59 |
ShadowNinja |
NP. |
| 17:59 |
sapier |
the difference is still it's not locking in between so you change concurrency behaviour ... well not sure if someone should rely on it |
| 17:59 |
hmmmm |
sorry, I thought that was if (other == *this) for some reason |
| 18:00 |
hmmmm |
sapier, it looks safe to me |
| 18:01 |
ShadowNinja |
sapier: You don't need to lock before the (this == &other), nothing else can change it. And locking before that pretty much defeats the purpose of the optimization. |
| 18:01 |
sapier |
well it looks like something to trigger errors beeing there before ;-) |
| 18:02 |
sapier |
= operator relies on recursive mutex if not it's a deadlock |
| 18:02 |
sapier |
do we have recursive mutexes? usually they're quite slow |
| 18:03 |
hmmmm |
sapier, that operator would only be applied if you were comparing the structs |
| 18:03 |
hmmmm |
he's just comparing pointer values |
| 18:04 |
sapier |
I know that's what I meant with it may cause errors that have ever been in there as it's increasing chance of concurrent access to those members ... but that's not an error in this code |
| 18:04 |
hmmmm |
ShadowNinja, don't allocate and copy str in readFlagString, that helps nothing |
| 18:04 |
ShadowNinja |
sapier: If our mutexes aren't recursive then it was flawed before. |
| 18:04 |
sapier |
still the recursive mutex requirement is strange as this is meant to be an optimization |
| 18:05 |
hmmmm |
change it back to char *s = &str[0]; |
| 18:05 |
sapier |
no |
| 18:05 |
sapier |
= operator didn't call update prior your optimitation |
| 18:05 |
sapier |
hmm + operator did |
| 18:05 |
sapier |
did anyone ever use it? |
| 18:06 |
hmmmm |
afiak nobody uses operator += and =, I am very much against operator overloading, ESPECIALLY =, so I'd say we should get rid of that entirely |
| 18:06 |
sapier |
I'd prefere not to have a recursive mutex requirement, as I said for what I experienced they are extremely slow |
| 18:08 |
ShadowNinja |
sapier: Before it depended on recursive mutexes, operator= called clear() and copy() which create their own AutoLocks. |
| 18:08 |
sapier |
at least jthread doesn't set the PTHREAD_MUTEX_RECURSIVE flag |
| 18:08 |
ShadowNinja |
So it could clear someone elses lock or unlock too soon. |
| 18:08 |
sapier |
that's strange I'd have a very close look at it |
| 18:09 |
ShadowNinja |
Actually, it still uses (or tries to use) recursive mutexes there. |
| 18:09 |
sapier |
possibly those are error which never have been triggered |
| 18:09 |
ShadowNinja |
operator+= is fixed though. |
| 18:10 |
hmmmm |
what if you remove the lock from update, change it into a private method, and then make a public wrapper that locks and then calls the private update |
| 18:11 |
hmmmm |
and you just lock in updatePublic, += and = |
| 18:11 |
hmmmm |
again, though, I'd prefer it if you just removed += and = |
| 18:11 |
sapier |
I'd suggest checking this first because current code seems to be wrong too in worst case those functions are just never used |
| 18:12 |
sapier |
or maybe the mutex itself isn't initialized ;-) wouldn't be first time noone checks for errors ;-) |
| 18:13 |
sapier |
hmm no jthread checks for return values .. at least if assert is not a nop |
| 18:13 |
ShadowNinja |
That could work. This entire thing is inlined though, so I'll have to move things around. Is there a good reason for any but the most often-called functions (get()/set()) to be inlined? |
| 18:14 |
|
^v joined #minetest-dev |
| 18:15 |
sapier |
if those mutexes are recursive it's pure chance we never set it, so if default behaviour changes we'd be stuck ... unless those functions aren't ever used |
| 18:16 |
sapier |
I like the const changes |
| 18:17 |
|
Anchakor_ joined #minetest-dev |
| 18:21 |
ShadowNinja |
I fixed the locking by adding clearNoLock and updateNoLock. Next I'll split it into a .cpp and a .h, unless there's an objection. |
| 18:21 |
sapier |
no I don't know how often I was looking for settings.cpp till I remembered there ain't one |
| 18:22 |
ShadowNinja |
sapier: Is that approval or an objection? |
| 18:22 |
sapier |
that's an approval for your split |
| 18:22 |
ShadowNinja |
Alright. |
| 18:24 |
sapier |
btw is there any progress in the player deletion cause? |
| 18:24 |
sapier |
is anyone working at it at all? ;-) |
| 18:26 |
ShadowNinja |
Hmmm, Settings::remove is thread-unsafe. |
| 18:26 |
ShadowNinja |
sapier: Nope. |
| 18:29 |
sapier |
I know that the other bug is hard to track down, but don't you think trying to find it would be more valuable then doing maintenance? I'm a little bit short on time so I can't do it atm |
| 18:34 |
ShadowNinja |
Um, there are two getS16NoEx's, and one takes an int, which is usually 32 bits. |
| 18:34 |
sapier |
I suggest changing to uint16_t |
| 18:37 |
|
diemartin joined #minetest-dev |
| 18:44 |
|
PilzAdam joined #minetest-dev |
| 18:44 |
ShadowNinja |
sapier: There's a u16 version. If you like I could change it to use standard fixed-width types. |
| 18:45 |
sapier |
as I said I'm a little bit short on time atm ;-) |
| 19:16 |
|
proller joined #minetest-dev |
| 19:17 |
sapier |
how may I help you john_minetest |
| 19:23 |
sapier |
well testing is always welcome, for what I know we've got a hard to reproduce crash right now, if someone managed to find a way to reproduce it we could fix it way more fast. |
| 19:25 |
sapier |
current git master |
| 19:26 |
sapier |
https://github.com/minetest/minetest/issues/1592 |
| 19:26 |
sapier |
one of two bugs |
| 19:29 |
sapier |
a crash with even less information how to reproduce ;-) |
| 19:30 |
|
kaeza joined #minetest-dev |
| 19:30 |
sapier |
maybe broken udp packets are involved in the crash |
| 19:30 |
sapier |
but that's a guess only |
| 19:32 |
sapier |
yes that pne seems to be a bug too ... but the information doesn't help very much ;-) |
| 19:32 |
sapier |
do you have a debug build? |
| 19:32 |
|
john_minetest joined #minetest-dev |
| 19:34 |
sapier |
cmake -DCMAKE_BUILD_TYPE=Debug |
| 19:38 |
|
Calinou joined #minetest-dev |
| 19:38 |
sapier |
well it's us to thank you for testing john_minetest |
| 19:40 |
|
JZTech101 joined #minetest-dev |
| 19:43 |
sfan5 |
malformed udp packets are dropped at the kernel |
| 19:43 |
sfan5 |
this doesn't affect minetest |
| 19:43 |
|
emrys joined #minetest-dev |
| 19:43 |
sapier |
sfan5: ppl tell different ;-) |
| 19:44 |
kahrl |
perhaps the client sends a malformed packet, maybe because of memory corruption |
| 19:44 |
sapier |
hmm thinking about it ... maybe broken udp packets and crash have a common usecase |
| 19:44 |
sapier |
kahrl: same thought ;-) |
| 19:44 |
kahrl |
or because it's a hacked client :P |
| 19:44 |
sapier |
yes but that wouldn't crash server |
| 19:45 |
sfan5 |
the client can't send a malformed udp packet (udp itself, not the data) |
| 19:45 |
sfan5 |
it would need root for that |
| 19:45 |
kahrl |
sfan5: malformed in the application layer sense |
| 19:45 |
sapier |
but broken memory on server could cause udp packet verification failure as well as random minetest crashes |
| 19:45 |
sfan5 |
thats ofc possible |
| 19:45 |
sfan5 |
um |
| 19:45 |
sfan5 |
wat |
| 19:45 |
sfan5 |
oh |
| 19:45 |
sfan5 |
you mean broken RAM |
| 19:45 |
sapier |
yes |
| 19:46 |
sfan5 |
broken RAM will likely have more issues that just minetest crashing and udp packets failing to verify |
| 19:46 |
sapier |
a malformed network packet no matter how malformed shall never crash minetest server, worst thing it's supposed to cause is drop of a client |
| 19:46 |
sapier |
sfan5: yes but lots of strange problems |
| 19:47 |
kahrl |
sfan5: maybe minetestserver is the only thing that eats enough RAM to exercise the broken RAM regions |
| 19:47 |
sapier |
today chances for memory to be used for non exact data are quite big, e.g. jpg files, video streams audio streams |
| 19:47 |
sfan5 |
then run memtest ¯\_(ツ)_/¯ |
| 19:48 |
sfan5 |
also most memory is always used by the kernel for cache |
| 19:48 |
sfan5 |
and video streams throw errors |
| 19:48 |
sapier |
no |
| 19:48 |
sfan5 |
at least H.26[3-5] does |
| 19:48 |
sapier |
most video codes just ignore small errors as they contain redundancy |
| 19:48 |
sfan5 |
it depends on the error |
| 19:48 |
sapier |
of course |
| 19:49 |
sfan5 |
if metadata is corrupted there will likely be an error |
| 19:49 |
sfan5 |
image data not so much |
| 19:49 |
sapier |
I just wanted to illustrate that it's quite likely for memory errors to cause that subtile issues ppl don't detect it for quite some time |
| 19:49 |
sapier |
of course if 1GB of your 4gb is broken you'll notice ;-) |
| 19:50 |
sfan5 |
you'll notice anyway |
| 19:50 |
sfan5 |
the kernel uses it for cache |
| 19:50 |
sfan5 |
but I dunno whether the cache is checksummed |
| 19:50 |
sapier |
well last time I had a faulty memory module it tool me months to pinpoint the issue |
| 19:51 |
sapier |
some big files broken every now and then, a crash here and there |
| 19:51 |
sapier |
but 99% of what I did on that machine was perfectly ok |
| 19:53 |
sapier |
but as you said memtest is quite good at finding things like that ... once you run it ;-) |
| 19:55 |
sapier |
so debug build changes timing to an extent where the bug doesn't happen any longer |
| 19:55 |
sfan5 |
then use RelWithDebugInfo |
| 19:55 |
sfan5 |
"RelWithDebInfo" |
| 19:56 |
sapier |
sfan5: suggestion is worth a try, it's still slower but way more near to release |
| 19:56 |
sfan5 |
why slower? |
| 19:56 |
sapier |
bigger file size |
| 19:56 |
sfan5 |
wat |
| 19:56 |
sfan5 |
how does that matter? |
| 19:57 |
sfan5 |
it's loaded into RAM fully before being executed |
| 19:57 |
sapier |
load time as well as possible page faults |
| 19:57 |
sapier |
no it ain't |
| 19:57 |
sfan5 |
and the debug regions are probably not loaded anyway |
| 19:57 |
sfan5 |
[citation needed] |
| 19:57 |
sapier |
parts of code are usually loaded on first usage only |
| 19:58 |
sapier |
well if they're not loaded they wont harm, yet it's not really relevant it's worth a try in any case |
| 20:00 |
sfan5 |
loading only parts of an executable doesn't improve speed at all |
| 20:01 |
sfan5 |
the .rodata, .data, .text section will be used for sure, it's only slower not to load them at startup |
| 20:26 |
|
harrison joined #minetest-dev |
| 20:47 |
|
kaeza joined #minetest-dev |
| 20:52 |
|
sapier left #minetest-dev |
| 20:55 |
|
kaeza joined #minetest-dev |
| 21:31 |
ShadowNinja |
A lot of files depend on settings.h to include filesys.h, util/serialize.h, log.h, strfnd.h, etc. |
| 21:34 |
ShadowNinja |
I now know why you should add an include for everything a file depends on, I've spent the last 10-20 minutes adding includes. |
| 21:40 |
|
proller joined #minetest-dev |
| 21:53 |
RealBadAngel |
ShadowNinja, thx for yesterday ideas, i made highlighting blend with regular light |
| 21:53 |
RealBadAngel |
it works suprisingly good |
| 21:54 |
ShadowNinja |
RealBadAngel: Good. I'll check it when I have a chance, but I'm busy now. |
| 21:54 |
RealBadAngel |
and doesnt depend on daytime, only the light around |
| 21:54 |
RealBadAngel |
i need to fine tune it, will update the pull later today |
| 22:16 |
|
Taoki joined #minetest-dev |
| 22:16 |
|
john_minetest joined #minetest-dev |
| 22:19 |
ShadowNinja |
Finally done, only had to modify 23 files more than the three it should have been (source, header, make) |
| 22:23 |
ShadowNinja |
https://github.com/ShadowNinja/minetest/compare/settings-cleanup |
| 22:24 |
ShadowNinja |
I did some cleanup in addition to the split. |
| 22:24 |
ShadowNinja |
hmmmm: ^ |
| 22:26 |
VanessaE |
ShadowNinja: RealBadAngel is gonna kick your ass for this... :P |
| 22:26 |
ShadowNinja |
VanessaE: Hmmm? Why? |
| 22:27 |
VanessaE |
still reading the code but I *think* this is very much what he wanted to do. you stole his thunder :) |
| 22:28 |
VanessaE |
hm, nope |
| 22:28 |
VanessaE |
not quite |
| 22:28 |
VanessaE |
you're just reworking all the underlying handlers |
| 22:29 |
VanessaE |
he still gets to play :) |
| 22:32 |
hmmmm |
ShadowNinja: looks good, but why do you add the const qualifier to an integer being passed by value... |
| 22:32 |
ShadowNinja |
hmmmm: Where? Doesn't really matter I guess. |
| 22:33 |
hmmmm |
all over |
| 22:33 |
ShadowNinja |
Just writeFlagString(). |
| 22:38 |
ShadowNinja |
Changed. |
| 22:48 |
ShadowNinja |
Hmmm, I broke something somehow, the world doesn't load and the game won't shut down without tripple-Ctrl-C. |
| 22:49 |
kahrl |
sounds like a deadlock? |
| 22:50 |
ShadowNinja |
Could re-sorting the order that the sources are listed in in CMakeLists.txt break it? |
| 22:50 |
kahrl |
no |
| 22:51 |
ShadowNinja |
Otherwise yes, maybe a lock is failing somehow. But it uses only JMutexAutoLocks, which should be foolproof. |
| 22:51 |
kahrl |
didn't sapier say that they are non-recursive? |
| 22:51 |
ShadowNinja |
Yes. |
| 22:52 |
ShadowNinja |
I'll check if they recurse anywhere, but I don't think that would cause this. |
| 22:55 |
ShadowNinja |
Doesn't look like it. |
| 22:56 |
ShadowNinja |
Hmmm, maybe I have a while (<always true>) {} somewhere. |
| 22:57 |
* ShadowNinja |
looks to see if GDB can help |
| 23:01 |
ShadowNinja |
`thread apply all bt` shows no mention of Settings. There are some threads waiting on locks or sellping though. |
| 23:03 |
kahrl |
yeah same here |
| 23:03 |
kahrl |
there are a few waiting on ClientInterface::m_clients_mutex |
| 23:04 |
ShadowNinja |
Exact same mutex here too. It's either that, nanosleep(), select(), sem_timedwait(), or poll(). |
| 23:07 |
ShadowNinja |
ClientInterface::sendToAll and ClientInterface::getClientIDs are fighting for m_clients_mutex on shutdown. |
| 23:08 |
ShadowNinja |
Both in __lll_lock_wait() (pthread_mutex_lock()). |
| 23:09 |
kahrl |
indeed |
| 23:09 |
ShadowNinja |
kahrl: Any ideas for debuging? I'm not very familiar with GDB. |
| 23:10 |
kahrl |
not really |
| 23:10 |
kahrl |
well, you could do "thread apply all bt full" to see even more info |
| 23:11 |
kahrl |
http://stackoverflow.com/questions/3483094/is-it-possible-to-determine-the-thread-holding-a-mutex |
| 23:11 |
kahrl |
^ might help |
| 23:13 |
kahrl |
another thing to try would be to revert the settings changes and try if the other changes by themselves cause the deadlock too |
| 23:17 |
kahrl |
ok, so ServerThread is in ClientInterface::getClientIDs trying to lock m_clients_mutex which is locked by ServerThread |
| 23:17 |
ShadowNinja |
kahrl: That shows that ServerAsyncRunStep -> Server::SendBlocks -> ClientInterface::getClientIDs owns the mutex. There are really only two commits that could cause this, and one is tiny, but I'll try. |
| 23:19 |
ShadowNinja |
kahrl: There's only one ServerThread, main is waiting on ServerThread. |
| 23:20 |
ShadowNinja |
The latest, big commit is causing it. |
| 23:20 |
ShadowNinja |
(But how!) |
| 23:25 |
ShadowNinja |
Include order? |
| 23:26 |
kahrl |
sounds unlikely but at this point everything is possible |
| 23:29 |
kahrl |
why does server.cpp:3856 return without unlocking m_clients? |
| 23:30 |
kahrl |
(but changing return to continue there doesn't fix the deadlock) |
| 23:31 |
kahrl |
this whole concept of locking m_clients_mutex from outside ClientInterface is really ugly |
| 23:40 |
ShadowNinja |
Indeed, but how is my settings patch affecting it? |
| 23:40 |
kahrl |
dunno |
| 23:40 |
kahrl |
OK, with some printf debugging it seems that the m_clients_mutex was last locked by server.cpp:1171 in StageTwoClientInit |
| 23:40 |
kahrl |
(I only traced locks from server.cpp) |
| 23:47 |
ShadowNinja |
emergePlayer is throwing an exception. |
| 23:49 |
kahrl |
"invalid inventory specifier" |
| 23:51 |
ShadowNinja |
Settings might not be eating the EndArgs or something like that... |
| 23:52 |
kahrl |
or it reads too much |
| 23:53 |
ShadowNinja |
Maybe it's just leaving the '\n'... |
| 23:55 |
kahrl |
does anything initialize end_found to false?? |
| 23:55 |
kahrl |
-? |
| 23:58 |
ShadowNinja |
No... |
| 23:59 |
kahrl |
ok, but fixing that still doesn't fix the emergePlayer exception |
