Time Nick Message 08:58 MTDiscord Trying 5.8.0 for the first time on linux, and it crashed with an xcb assertion (part of libX11). Is this known issue, or should I try to debug? 12:28 MTDiscord Update: It turns out libX11 is buggy and may not be internally thread-safe, despite offering the option to not initialize thread safety. 12:28 MTDiscord They gave up and just enabled thread safety by default: https://gitlab.freedesktop.org/xorg/lib/libx11/-/commit/afcdb6fb0045c6186aa83d9298f327a7ec1b2cb9 12:29 MTDiscord I noticed the latest libX11 wasn't crashing, so I did a bisect and traced it to that 12:30 MTDiscord if I take the latest libX11 and disable XInitThreads, it still crashes 12:30 MTDiscord An easy fix would be for minetest to just call XInitThreads itself, so that older broken libX11 don't crash 12:32 MTDiscord I didn't see any evidence that minetest is accessing X11 from multiple threads, so it is most likely something else (could be Mesa) that is interfacing with xcb or x11 from multiple threads and triggering a race 12:51 [MTMatrix] time to switch xlib :) 12:51 [MTMatrix] time to switch xlib ? :) 12:56 MTDiscord is there an alternative? 12:58 MTDiscord curious to know, does the crash also occur with the SDL2 device? (I assume you're using the X11 device which is still the default) 13:01 MTDiscord It appears SDL2 always calls XInitThreads, so I would assume not 13:01 MTDiscord https://github.com/libsdl-org/SDL/blob/main/src/video/x11/SDL_x11video.c#L113C24-L113C24 13:18 sfan5 feel free to send a PR to irrlichtmt 13:19 sfan5 but it's best not to spend too much time on this since the x11 driver is going away anyway 16:47 MTDiscord Can I get a second reviewer for #14054? Seems like a really simple change, should be easy to get merged, right? 16:47 ShadowBot https://github.com/minetest/minetest/issues/14054 -- Manually configurable minimum protocol version by Warr1024 17:21 pgimeno OpenGL in general is not thread-safe, if that matters - not just mesa 17:55 Krock will merge #14010 #14083 and #13093 in 15 minutes 17:55 ShadowBot https://github.com/minetest/minetest/issues/14010 -- GUIFormspecMenu: Fix race condition between quit event and cleanup in Game by SmallJoker 17:55 ShadowBot https://github.com/minetest/minetest/issues/14083 -- Add sound volume when unfocused setting by srifqi 17:55 ShadowBot https://github.com/minetest/minetest/issues/13093 -- Avoid movement jitter while attached by lhofhansl 18:09 Krock merging 18:12 Krock done 18:51 sfan5 #13092 is now ready for reviews 18:51 ShadowBot https://github.com/minetest/minetest/issues/13092 -- Lua on each mapgen thread by sfan5 18:58 Krock cool. I did not think it would become this large of a PR 19:05 pgimeno awesome! 19:05 pgimeno is there any means of communication between threads? 19:05 sfan5 one-way only 19:05 sfan5 (emerge -> main) 19:10 MTDiscord Krock: if you're around ... would Settings be an acceptable place to refactor the protocol version limits into? I've got a test branch I'm working on at https://github.com/minetest/minetest/compare/master...Warr1024:minetest:protomin2?expand=1 19:11 Krock @warr1024: Settings is used by client and server. This here is server-specific, thus I'd rather find a place for it in the ServerEnvironment or Server class 19:11 Krock after all, this can be a static function 19:12 MTDiscord Oh geez, it's times like this that make me wish I knew C++... 19:12 Krock oh 19:13 MTDiscord If it's got a g_settings then I can probably move it there. 19:13 Krock it's easy. you just put the `static` keyword in front of the function (header file) and it'll be accessible without the instance itself 19:14 Krock I see how Settings also kind of makes sense since you're only using functions from that class.... it's a question of concept. perhaps it's best to ask for a second opinion 19:14 MTDiscord If I make it static then that would mean I'd need to pass in the settings then, since settings live in instance-land ... would that be okay, or would that be uglier than if I just didn't use static? 19:14 Krock depends on where you put the function. the server uses `g_settings` for everything anyway 19:15 MTDiscord Personally, settings didn't feel quite "right" to me either, since looking at settings, almost everything in there is very generic and not application-specific like this. 19:15 Krock since that's a globally accessible variable it does not really matter where you put the function 19:15 MTDiscord Ohhh I see, g_settings is like a super global thing, not an instancey kind of thing, so yeah, that'd work. 19:16 Krock FYI: you don't need to update minetest.conf.example. It'll be done automatically 19:16 Krock you still can do that if you want, though. 19:16 MTDiscord So all I need is an opinion about whether Server of ServerEnvironment makes more sense then. 19:17 MTDiscord I'm guessing Server since the word "protocol" seems to appear there but not in serverenv. 19:20 sfan5 `Settings` feels like the wrong class to me because in most ways it's just totally generic 19:20 sfan5 maybe one of the protocol related headers would be good, or server.h 19:22 MTDiscord It feels weird to be elbow-deep in C++ code and opening up a search on "how to call a static method" right in the middle of it 😏 19:22 sfan5 you can also make it a top level static inline u16 getMinProtocolVersion() { ... } 19:23 MTDiscord Hmm, I forgot about inline ... do those really make a significant difference, or do compilers just make their own decisions about inlining anyway? 19:24 Krock the compilers tend to put it more in-line but at the end for this case it does not matter. inline only increases the binary size. 19:27 sfan5 the 'static' here means something different than 'static' for a class function 19:28 Krock (that is: no exported function name) 19:28 MTDiscord Haha, I'm vaguely familiar with C and C-descendant languages having multiple weird meanings for "static" in different contexts, but I'm sure I don't know most of them. 19:30 Desour inline nowadays has a different meaning than "please inline". see . TL;DR: you can define the function in multiple translation units 19:36 MTDiscord error: cannot declare member function β€˜static irr::u16 Server::getProtocolVersionMin()’ to have static linkage 19:36 MTDiscord apparently I used the wrong kind of "static" here? 19:36 Krock no static in the C++ file 19:37 MTDiscord That's nice and confusing 😏 19:37 Krock for example: these functions here are all static: https://github.com/minetest/minetest/blob/master/src/script/lua_api/l_env.cpp#L293 19:39 MTDiscord https://github.com/minetest/minetest/compare/master...Warr1024:minetest:protomin2?expand=1 has been updated 19:40 MTDiscord Should I replace my original PR with a squashed version of this then? 19:47 Krock yes, looks good. By the way. the maximal protocol version is always SERVER_PROTOCOL_VERSION_MAX. That and LATEST_PROTOCOL_VERSION have the same value. I don't know why both exist. 19:47 Krock "strict_protocol_version_checking" makes no difference there, however it could still be desirable to force a version later on? I don't know. 19:48 [MTMatrix] oh noes sounds like deprecation 20:02 MTDiscord I was wondering why proto max and proto latest were separate, myself. I assumed it was for some arcane reason that I just didn't know but would make perfect sense if I ever heard it πŸ˜† 20:07 [MTMatrix] some stupid question (maybe): where i can find expain of version rules protocol 20:10 Krock the what? 20:12 [MTMatrix] the server protocol version 20:12 Krock https://github.com/minetest/minetest/blob/master/src/network/networkprotocol.h#L218 20:15 MTDiscord #14054 rebased 20:15 ShadowBot https://github.com/minetest/minetest/issues/14054 -- Manually configurable minimum protocol version by Warr1024