--- Log opened Sun May 27 00:00:10 2018 20180527 00:01:15-!- gallaecio [~quassel@188.79.96.255] has quit [Ping timeout: 268 seconds] 20180527 01:01:44< irker765> wesnoth/wesnoth:1.14.2 Charles Dang 2b7f95164c Pre-release version bump AppVeyor: All builds passed 20180527 01:16:02-!- ToBeFree [uid51591@wikimedia/ToBeFree] has joined #wesnoth-dev 20180527 01:16:06<+discordbot1> @shadowm it's a two-part scenario 20180527 01:16:15<+discordbot1> I took the convention from you... 20180527 01:16:23<+discordbot1> and yes, I am in favor of removing the players changelog 20180527 01:16:26<+discordbot1> and lastly, I remembered last night that there was a piece of code in the loading screen that definitely had the potential to be a race condition under extremely unlikely circumstances. Now, I'm not sure that would manifest itself with the code that's in 1.14 (possibly not), but depending on how struct tsan is it's possible that's the cause of a lot of the warnings it presented on start. Really, could you please just check whether 20180527 01:16:27<+discordbot1> those still happen on master? :/ 20180527 01:34:12-!- gfgtdf [~chatzilla@x4e32b596.dyn.telefonica.de] has joined #wesnoth-dev 20180527 01:34:15< gfgtdf> which wanrings is this about ? 20180527 01:36:10<+discordbot1> I don't have a paste 20180527 01:36:22<+discordbot1> she posted warnings from a different area 20180527 01:36:30<+discordbot1> but she also said there were a ton just starting the game 20180527 01:36:31<+discordbot1> with tsan 20180527 01:37:41< gfgtdf> i see 20180527 01:37:52< gfgtdf> so this is the dialog in initial [lua] issue ? 20180527 01:38:01<+discordbot1> the ones she posted had to do with a dialog being created during the loading screen from lua 20180527 01:38:03<+discordbot1> https://pastebin.com/DuYhZVXa 20180527 01:38:39< gfgtdf> what you ahev to do ehwn ding gui stuff is calling events::call_in_main_thread othwerise you get race conditions or worse 20180527 01:38:51< gfgtdf> liek it's done here https://github.com/wesnoth/wesnoth/blob/1.14/src/game_config_manager.cpp#L196 20180527 01:38:55< gfgtdf> like* 20180527 01:39:01<+discordbot1> always? 20180527 01:39:18< gfgtdf> during the loading screen 20180527 01:39:28< gfgtdf> if you are in the worker tread that is, 20180527 01:40:09< gfgtdf> i don't know for 100% sure whether it still works with those lua callbacks etc, but for no i see no reason why it shodul not work. 20180527 01:40:28<+discordbot1> ok... 20180527 01:40:39< gfgtdf> (by lua callbacks i mean wesnoth.set_dialog_callback) 20180527 01:40:42<+discordbot1> but that doesn't solve all the warnings she reported happen on start 20180527 01:41:01<+discordbot1> set_dialog_callback needs t happen in the main thread too? 20180527 01:41:43< gfgtdf> i don't think it does 20180527 01:42:04<+discordbot1> ok 20180527 01:42:07<+discordbot1> so only show_dialog? 20180527 01:42:20< gfgtdf> that migth be enough 20180527 01:42:43< gfgtdf> do e need to support using dialog from lau in the first place ? we could just disable it there and force users to do suff like create a prestart event that shows those dialogs 20180527 01:42:55< gfgtdf> i feel like i had this dicussion before. 20180527 01:43:15<+discordbot1> dialogs from global lua you mean? 20180527 01:43:50< gfgtdf> ye from initial [lua] tags (that run during the loadingscreen). 20180527 01:44:03<+discordbot1> how would we disallow that? 20180527 01:44:35< gfgtdf> add a check in show_dialog, like if(is_in_loadingscreen) the lua_erro("cannot do this in loadingscreen") 20180527 01:44:58< gfgtdf> then* 20180527 01:45:06<+discordbot1> hmmm 20180527 01:45:19< gfgtdf> actually {, this is c++ 20180527 01:45:23<+discordbot1> I think the reason shadowm uses it 20180527 01:45:29<+discordbot1> is so you can early-exit 20180527 01:45:34<+discordbot1> before seeing the storyscreen 20180527 01:46:11< gfgtdf> i thought prestart if before storyscrteen ? not sure on that part. 20180527 01:46:38< gfgtdf> hmm no seems liek it after 20180527 01:47:12< gfgtdf> like it happens* 20180527 01:47:33<+discordbot1> but she needs to actually hook into prestart or preload to exit 20180527 01:47:52<+discordbot1> without the entire campaign being considered complete 20180527 01:50:11< gfgtdf> hmm ok actually i don't have a strng opinion on this, if simply adding events::call_in_main_thread in the show_dialog implementation fixes this completeley, that's good aswell. 20180527 01:53:03<+discordbot1> Around the entire thing? 20180527 01:53:08-!- mkdroid [~null@unaffiliated/matthiaskrgr] has joined #wesnoth-dev 20180527 01:54:28-!- gfgtdf_ [~chatzilla@x4e32b6c7.dyn.telefonica.de] has joined #wesnoth-dev 20180527 01:55:55< gfgtdf_> hmm ye 20180527 01:56:54< gfgtdf_> i also don't know how events::call_in_main_thread works wrt exceptions though i don't rememebr implementing an exception rethrow mechnism for that. 20180527 01:57:48-!- gfgtdf [~chatzilla@x4e32b596.dyn.telefonica.de] has quit [Ping timeout: 268 seconds] 20180527 01:57:53-!- gfgtdf_ is now known as gfgtdf 20180527 01:58:14< gfgtdf> whether events::call_in_main_thread works also depends on whether lua can works with it's functions suddenly beeing called form differnt threads 20180527 01:58:40<+discordbot1> What thread would we want the exceptions handled in? 20180527 01:59:31< gfgtdf> we want that events::call_in_main_thread basically retrows all ecxeptions that are thrown in the function you passed to it. 20180527 02:00:04< gfgtdf> since the function is usually called in a differnth thread than the caller of call_in_main_thread, impleneting this woudl need std::exception_ptr 20180527 02:00:10<+discordbot1> Yes 20180527 02:00:16<+discordbot1> Should be pretty simple 20180527 02:00:48-!- mkdr0id [~null@unaffiliated/matthiaskrgr] has joined #wesnoth-dev 20180527 02:01:18-!- mkdroid [~null@unaffiliated/matthiaskrgr] has quit [Remote host closed the connection] 20180527 02:10:42<+discordbot1> I can't prevent the storyscreen from showing up, actually. 20180527 02:11:06<+discordbot1> It just goes and does its own thing before the engine runs the preload event. 20180527 02:12:36<+discordbot1> seems there are two issues here 20180527 02:12:55<+discordbot1> the first is th lua dialog called from the worker thread 20180527 02:13:06<+discordbot1> second is the game taking a minute to load when launched with tsan 20180527 02:13:50<+discordbot1> Yeah, I'm not sure why is that. It might be because tsan needs to do its magic on every thread that's started during initialization. 20180527 02:14:11<+discordbot1> I thought you meant it spit out a million warnings 😐 20180527 02:14:13<+discordbot1> (IIRC SDL needs at least one thread for audio and one thread for window management.) 20180527 02:14:29<+discordbot1> What. 20180527 02:14:40<+discordbot1> Yes it does? 20180527 02:14:55<+discordbot1> are these data race warnings? 20180527 02:15:02<+discordbot1> The pastebins? Yes. 20180527 02:15:11<+discordbot1> Wait what are we talking about 20180527 02:15:16<+discordbot1> no the ones on game launch 😐 20180527 02:15:28<+discordbot1> "Let's put it this way, I launched it a minute ago and the game window has only just come up on the screen." 20180527 02:15:40<+discordbot1> Oh it wasn't printing warnings then. 20180527 02:15:56<+discordbot1> I will say though, I didn't include a lot of other warnings. 20180527 02:16:22-!- mkdr0id [~null@unaffiliated/matthiaskrgr] has quit [Ping timeout: 252 seconds] 20180527 02:16:59<+discordbot1> oh 20180527 02:17:11<+discordbot1> ok, then my master changes won't have affected this 20180527 02:17:17<+discordbot1> I excluded warnings involving SDL timers in the main menu (???), occasional warnings involving PulseAudio, and a number of warnings I forgot that take place between the game window being created and execution reaching the main menu. 20180527 02:19:18<+discordbot1> gfgtdf: would it work to have the exception_ptr in invoked_function_data and wrap f() in call() in try { } catch(...) { ptr = std::current_exception(); 20180527 02:19:43<+discordbot1> and thenin call_in_main_thread check whether the invoked_function_data's ptr is storing anything 20180527 02:21:13< gfgtdf> ye that should work. 20180527 02:24:54< gfgtdf> it might make sense to let though some specific exceptions, like game exit exception (CVideo::quit()) 20180527 02:26:57<+discordbot1> so this seems to work but I don't know if tsan likes it 20180527 02:27:10<+discordbot1> @shadowm would you be willing to check this patch under tsan (1.14) 20180527 02:28:12<+discordbot1> gfgtdf: the exception stuff is unrelated to the lua dialog data race right 20180527 02:28:20<+discordbot1> or is it a prerequisite to have? 20180527 02:28:31< gfgtdf> it's unrelated 20180527 02:28:47<+discordbot1> ok 20180527 02:32:22<+discordbot1> @shadowm this one: https://pastebin.com/hmugy1tY 20180527 02:39:21-!- janebot [~Gambot@unaffiliated/gambit/bot/gambot] has quit [Remote host closed the connection] 20180527 02:39:28-!- janebot [~Gambot@unaffiliated/gambit/bot/gambot] has joined #wesnoth-dev 20180527 02:46:04-!- fabi_ [~fabi@200116b82ba72f00b4c62b225e348176.dip.versatel-1u1.de] has joined #wesnoth-dev 20180527 02:46:38-!- fabi [~fabi@200116b82b157800b9a75ed7fd6aae0b.dip.versatel-1u1.de] has quit [Ping timeout: 260 seconds] 20180527 02:52:23-!- gfgtdf [~chatzilla@x4e32b6c7.dyn.telefonica.de] has quit [Quit: ChatZilla 0.9.93 [Firefox 52.8.0/20180430140610]] 20180527 03:10:46< irker765> wesnoth/wesnoth:1.14 Nils Kneuper 78ba4582a1 updated Italian translation AppVeyor: All builds passed 20180527 03:23:19-!- ToBeFree [uid51591@wikimedia/ToBeFree] has quit [Quit: Connection closed for inactivity] 20180527 03:28:23<+discordbot1> Your lambda is so long that I think it would be best to make a named function. 20180527 04:01:39<+discordbot1> I didn't want to add another function just as a wrapper 20180527 04:05:16-!- Appleman1234 [~quassel@124x38x163x22.ap124.ftth.ucom.ne.jp] has quit [Ping timeout: 252 seconds] 20180527 04:53:22<+discordbot1> @Vultraz With your patch applied, this is what things look like leading up to the main menu: https://pastebin.com/SX6jqBta 20180527 04:53:41<+discordbot1> Ignore anything involving some libpulse*. 20180527 04:54:39<+discordbot1> PA is the source of additional noise while playing UI sounds to get to the campaign launch option. 20180527 04:54:44<+discordbot1> No pun intended. 20180527 04:55:36<+discordbot1> Difficulty menu until the IftU Lua prompt: https://pastebin.com/hQcD7d6T 20180527 04:56:12<+discordbot1> sense of this I cannot make. 20180527 04:56:13<+discordbot1> 😦 20180527 04:56:25<+discordbot1> looks like timer problems.. 20180527 04:56:27<+discordbot1> IftU Lua prompt until the story screen: https://pastebin.com/G7RkgXQM 20180527 04:56:57<+discordbot1> which is another reason it would be useful to check master too, since the timer is removed there 20180527 04:57:30<+discordbot1> There's silence between the story screen and the start event until I press Escape after the first [message]. 20180527 04:57:31<+discordbot1> https://pastebin.com/iXnqESG0 20180527 04:57:52<+discordbot1> After dismissing the Objectives dialog I'm getting a constant flood of reports.a 20180527 04:58:11<+discordbot1> what... 20180527 04:58:55<+discordbot1> I had to kill Wesnoth to close it. 20180527 04:59:53<+discordbot1> It seems to be repeating this over and over for some reason: https://pastebin.com/5JbBKkQc 20180527 05:00:35<+discordbot1> very odd.. 20180527 05:01:36<+discordbot1> It's probably best to ignore it for now. 20180527 05:01:44<+discordbot1> It might be induced by some library interaction. 20180527 05:02:20<+discordbot1> there still seem to be data races in the UI 20180527 05:11:07< irker765> wesnoth/wesnoth:master Nils Kneuper 183bc272ea updated Italian translation AppVeyor: All builds passed 20180527 05:14:34-!- stikonas [~gentoo@wesnoth/translator/stikonas] has joined #wesnoth-dev 20180527 05:30:02-!- stikonas [~gentoo@wesnoth/translator/stikonas] has quit [Remote host closed the connection] 20180527 05:33:37<+discordbot1> not sure if it's relevant, but: https://github.com/wesnoth/wesnoth/issues/2698 20180527 06:32:16<+discordbot1> huh 20180527 06:32:33<+discordbot1> apparently when using aggregate initialization you don't need to specify a value for every member 20180527 06:32:34<+discordbot1> 🤔 20180527 06:35:40< irker765> wesnoth: Charles Dang wesnoth:1.14 b06855e60404 / src/events.cpp: Handle exceptions thrown by call_in_main_thread in the caller thread https://github.com/wesnoth/wesnoth/commit/b06855e60404cd64f5489fe518113eb9932c3c0a 20180527 06:36:25< celticminstrel> That's only if there are no constructors defined. 20180527 06:36:41< irker765> wesnoth: Charles Dang wesnoth:master 86792656aed3 / src/events.cpp: Handle exceptions thrown by call_in_main_thread in the caller thread https://github.com/wesnoth/wesnoth/commit/86792656aed349b6ddc67e66b98abdee5fb310e2 20180527 06:37:16<+discordbot1> well, I removed the ctor 20180527 06:37:25<+discordbot1> and used inline initialization for the bool 20180527 06:38:00< celticminstrel> Probably also only works if you have no private member variables. 20180527 06:45:13< irker765> wesnoth/wesnoth:1.14 Charles Dang b06855e604 Handle exceptions thrown by call_in_main AppVeyor: vs2015/Release Failed 20180527 06:45:14< irker765> Details: https://ci.appveyor.com/project/wesnoth/wesnoth-7lnpw/build/Wesnoth-VS2015-1.14-3618 20180527 06:49:30-!- travis-ci [~travis-ci@ec2-54-196-139-215.compute-1.amazonaws.com] has joined #wesnoth-dev 20180527 06:49:31< travis-ci> wesnoth/wesnoth#18392 (1.14 - b06855e : Charles Dang): The build was broken. 20180527 06:49:31< travis-ci> Build details : https://travis-ci.org/wesnoth/wesnoth/builds/384289232 20180527 06:49:31-!- travis-ci [~travis-ci@ec2-54-196-139-215.compute-1.amazonaws.com] has left #wesnoth-dev [] 20180527 06:52:07-!- celticminstrel is now known as celmin|sleep 20180527 06:53:13<+discordbot1> /home/wesnoth-travis/src/events.cpp:46:35: warning: non-static reference 'const std::function& {anonymous}::invoked_function_data::f' in class without a constructor [-Wuninitialized] const std::function& f; ^ /home/wesnoth-travis/src/events.cpp: In function 'void events::call_in_main_thread(const std::function&)': /home/wesnoth-travis/src/events.cpp:758:31: error: no 20180527 06:53:14<+discordbot1> matching function for call to '{anonymous}::invoked_function_data::invoked_function_data()' invoked_function_data fdata{f}; ^ /home/wesnoth-travis/src/events.cpp:43:8: note: candidate: {anonymous}::invoked_function_data::invoked_function_data(const {anonymous}::invoked_function_data&) struct invoked_function_data ^ /home/wesnoth-travis/src/events.cpp:43:8: note: no 20180527 06:53:14<+discordbot1> known conversion for argument 1 from 'const std::function' to 'const {anonymous}::invoked_function_data&' /home/wesnoth-travis/src/events.cpp:43:8: note: candidate: {anonymous}::invoked_function_data::invoked_function_data({anonymous}::invoked_function_data&&) /home/wesnoth-travis/src/events.cpp:43:8: note: no known conversion for argument 1 from 'const std::function' to '{anonymous}::invoked_function_data&&' 20180527 06:53:27<+discordbot1> Looks like Clang doesn't allow you to do it. 20180527 06:59:30<+discordbot1> damn clang... 20180527 06:59:44<+discordbot1> damn MSVC* 20180527 07:00:02<+discordbot1> Clang is ++ compliant. the problem is that Microsoft has their own extensions to the language. 20180527 07:00:08<+discordbot1> *C++ 20180527 07:02:21<+discordbot1> even with their recent standard compliance? 20180527 07:02:45-!- travis-ci [~travis-ci@ec2-54-82-119-166.compute-1.amazonaws.com] has joined #wesnoth-dev 20180527 07:02:46< travis-ci> wesnoth/wesnoth#18393 (master - 8679265 : Charles Dang): The build was broken. 20180527 07:02:46< travis-ci> Build details : https://travis-ci.org/wesnoth/wesnoth/builds/384289388 20180527 07:02:46-!- travis-ci [~travis-ci@ec2-54-82-119-166.compute-1.amazonaws.com] has left #wesnoth-dev [] 20180527 07:02:50<+discordbot1> Yes. C++17 compliance simply means that MSVC can compile every valid C++ program. 20180527 07:03:05<+discordbot1> It doesn't disallow MSVC from also compiling invalid C++ programs. 20180527 07:03:20<+discordbot1> I see 20180527 07:03:33<+discordbot1> still, this extension is convenient. oh well. 20180527 07:03:46< celmin|sleep> Pretty sure clang supports Microsoft extensions though. 20180527 07:04:06< celmin|sleep> Maybe not that specicif one. 20180527 07:04:07<+discordbot1> be that as it may 20180527 07:04:09<+discordbot1> Only some of them. They don't aim for 100% MSVC compliance. 20180527 07:04:09< celmin|sleep> ^specific 20180527 07:10:20<+discordbot1> I wonder why this specific feature was not made part of the standard 20180527 07:10:29<+discordbot1> it seems a pretty logical thing one would want to do 20180527 07:11:04<+discordbot1> Failing to specify a value for every member means that theiy will be uninitialized. 20180527 07:11:35<+discordbot1> It's an easy way to shoot yourself in the foot, as the value will be effectively random. A nightmare to debug. 20180527 07:12:00<+discordbot1> I would be strongly opposed to adding that extension to the standard. 20180527 07:13:27<+discordbot1> except c++11 also allowed you to specify values in the declaration. and as far as I know nothing prevents you from excluding members from the initialization list. 20180527 07:14:10<+discordbot1> and I was under the impression only primitive types (bools, ints, ptrs, etc) really needed an initial value, since other objects (strings, etc) just get default-initialized. 20180527 07:14:37<+discordbot1> The initialization list you always remember to update when you add new members to a class. 20180527 07:15:08<+discordbot1> But if aggregate initialization is used all over the place in a huge codebase, good luck trying to find every single place where objects of that class are initialized. 20180527 07:15:31<+discordbot1> You're correct that objects get default-initialized. 20180527 07:17:04<+discordbot1> so what I was assuming with this case is since I had these... cpp /** The actual function to call. */ const std::function& f; /** Whether execution in the main thread is complete. */ bool finished = false; /** Stores any exception thrown during the execution of @ref f. */ std::exception_ptr thrown_exception; I would use aggregate initialization for member 1, default-declare member 2, and 20180527 07:17:05<+discordbot1> default-initialize member 3 20180527 07:17:05<+discordbot1> though 20180527 07:17:09<+discordbot1> I can also see hw this is messy 20180527 07:17:11<+discordbot1> 🤔 20180527 07:18:34<+discordbot1> looks like c++20 adds designated initializers to aggregate initialization that allow the exclusion of some members. 20180527 07:18:39<+discordbot1> "For a non-union aggregate, element for which a designated initializer is not provided are initialized the same as described above for when the number of initializer clauses is less than the number of members (default member initializers where provided, empty list-initialization otherwise):" 20180527 07:18:45<+discordbot1> so i guess it's kinda coming to the standard? 20180527 07:19:25<+discordbot1> anyway 20180527 07:19:27<+discordbot1> Sounds like it. 20180527 07:19:28<+discordbot1> I'll re-add the ctor 20180527 07:21:14<+discordbot1> (the difference in c++20 would be that instead of invoked_function_data fdata{f}; it'd be invoked_function_data fdata{ .f = f }; which is a bit nicer) 20180527 07:44:43-!- gallaecio [~quassel@188.79.96.255] has joined #wesnoth-dev 20180527 07:50:01-!- zookeeper [~lmsnie@wesnoth/developer/zookeeper] has joined #wesnoth-dev 20180527 07:55:22-!- boucman [~rosen@wesnoth/developer/boucman] has joined #wesnoth-dev 20180527 08:27:44< irker765> wesnoth/wesnoth:master Charles Dang 86792656ae Handle exceptions thrown by call_in_main AppVeyor: vs2015/Release Failed 20180527 08:27:45< irker765> Details: https://ci.appveyor.com/project/wesnoth/wesnoth-7lnpw/build/Wesnoth-VS2015-master-3619 20180527 09:01:45-!- janebot [~Gambot@unaffiliated/gambit/bot/gambot] has quit [Remote host closed the connection] 20180527 09:01:51-!- janebot [~Gambot@unaffiliated/gambit/bot/gambot] has joined #wesnoth-dev 20180527 09:32:08< matthiaskrgr> looks like there's a new compiler warning on master with clang++ https://pastebin.com/YupMhGkL 20180527 10:41:16-!- Appleman1234 [~quassel@124x38x163x22.ap124.ftth.ucom.ne.jp] has joined #wesnoth-dev 20180527 11:03:11< irker765> wesnoth: Charles Dang wesnoth:master 5588588b55d7 / src/events.cpp: Fixup 86792656aed (re-add ctor) https://github.com/wesnoth/wesnoth/commit/5588588b55d7733dd912c887221670927443a9c3 20180527 11:04:12< irker765> wesnoth: Charles Dang wesnoth:1.14 aebf9be79dbc / src/events.cpp: Fixup b06855e6040 (re-add ctor) https://github.com/wesnoth/wesnoth/commit/aebf9be79dbc85229513e8c9bfc6d9553d010ab0 20180527 11:08:25-!- gfgtdf [~chatzilla@x4e32b6c7.dyn.telefonica.de] has joined #wesnoth-dev 20180527 11:09:18< gfgtdf> vultraz: currently cour code swallows CVideo::quit you need to rethrow it. 20180527 11:09:33<+discordbot1> oh 20180527 11:09:42<+discordbot1> just throw;? 20180527 11:19:22< gfgtdf> ye 20180527 11:22:29< irker765> wesnoth: Charles Dang wesnoth:1.14 031f856f3146 / src/events.cpp: Fixed CVIdeo::quit being swallowed in invoked_function_data::call https://github.com/wesnoth/wesnoth/commit/031f856f314695152f1dc5a32270ad40d92d994b 20180527 11:22:37-!- vn971 [~vasya@94.158.103.15] has left #wesnoth-dev ["Leaving."] 20180527 11:22:54-!- travis-ci [~travis-ci@ec2-54-159-180-202.compute-1.amazonaws.com] has joined #wesnoth-dev 20180527 11:22:55< travis-ci> wesnoth/wesnoth#18394 (master - 5588588 : Charles Dang): The build was fixed. 20180527 11:22:55< travis-ci> Build details : https://travis-ci.org/wesnoth/wesnoth/builds/384337537 20180527 11:22:55-!- travis-ci [~travis-ci@ec2-54-159-180-202.compute-1.amazonaws.com] has left #wesnoth-dev [] 20180527 11:23:02< irker765> wesnoth: Charles Dang wesnoth:master 8ecbed020504 / src/events.cpp: Fixed CVIdeo::quit being swallowed in invoked_function_data::call https://github.com/wesnoth/wesnoth/commit/8ecbed0205040b566e898f64eb15103162728ec2 20180527 11:24:57<+discordbot1> ty 20180527 11:40:52-!- travis-ci [~travis-ci@ec2-54-92-152-130.compute-1.amazonaws.com] has joined #wesnoth-dev 20180527 11:40:53< travis-ci> wesnoth/wesnoth#18395 (1.14 - aebf9be : Charles Dang): The build was fixed. 20180527 11:40:53< travis-ci> Build details : https://travis-ci.org/wesnoth/wesnoth/builds/384337926 20180527 11:40:53-!- travis-ci [~travis-ci@ec2-54-92-152-130.compute-1.amazonaws.com] has left #wesnoth-dev [] 20180527 11:45:13< irker765> wesnoth/wesnoth:1.14 Charles Dang b06855e604 Handle exceptions thrown by call_in_main AppVeyor: 1/4 builds failed 20180527 11:45:14< irker765> Details vs2015/Release: https://ci.appveyor.com/project/wesnoth/wesnoth-7lnpw/build/Wesnoth-VS2015-1.14-3618 20180527 11:55:01<+discordbot1> @loonycyborg are the windows and linux packages and steam updates up? 20180527 11:55:40<+discordbot1> yes 20180527 11:56:05<+discordbot1> ok, good 20180527 11:56:07<+discordbot1> thanks 20180527 12:00:46-!- travis-ci [~travis-ci@ec2-54-159-180-202.compute-1.amazonaws.com] has joined #wesnoth-dev 20180527 12:00:47< travis-ci> wesnoth/wesnoth#18396 (1.14 - 031f856 : Charles Dang): The build was fixed. 20180527 12:00:47< travis-ci> Build details : https://travis-ci.org/wesnoth/wesnoth/builds/384341559 20180527 12:00:47-!- travis-ci [~travis-ci@ec2-54-159-180-202.compute-1.amazonaws.com] has left #wesnoth-dev [] 20180527 12:07:09-!- janebot [~Gambot@unaffiliated/gambit/bot/gambot] has quit [Remote host closed the connection] 20180527 12:07:15-!- janebot_ [~Gambot@unaffiliated/gambit/bot/gambot] has joined #wesnoth-dev 20180527 12:07:15-!- janebot_ is now known as janebot 20180527 13:27:44< irker765> wesnoth/wesnoth:master Charles Dang 86792656ae Handle exceptions thrown by call_in_main AppVeyor: 1/4 builds failed 20180527 13:27:45< irker765> Details vs2015/Release: https://ci.appveyor.com/project/wesnoth/wesnoth-7lnpw/build/Wesnoth-VS2015-master-3619 20180527 13:56:32-!- fabi_ [~fabi@200116b82ba72f00b4c62b225e348176.dip.versatel-1u1.de] has quit [Quit: Konversation terminated!] 20180527 14:32:44< celmin|sleep> Huh, this allows attacks to become 0x0... weord. 20180527 14:32:46< celmin|sleep> ^weird 20180527 14:32:51-!- celmin|sleep is now known as celticminstrel 20180527 14:33:47< irker765> wesnoth: Celtic Minstrel wesnoth:master 1413dfd4f387 / src/units/attack_type.cpp: Fix effects being unable to decrease weapon parry/accuracy https://github.com/wesnoth/wesnoth/commit/1413dfd4f387f589d59545f0a4cde665d0d1aac2 20180527 14:34:04< celticminstrel> (The bit just below the first diff in that commit ^ ) 20180527 14:34:12< celticminstrel> (I decided not to change it right now.) 20180527 14:34:35< celticminstrel> (The weird thing is that increase_attacks always ensures it's at least 1, but set_attacks allows you to set to 0.) 20180527 14:34:49< celticminstrel> (increase_damage and set_damage both allow a result of 0.) 20180527 14:36:27< celticminstrel> Basically we should decide whether attacks should be allowed to be 0 and whether damage should be allowed to be 0, and make sure it's enforced not just in modifications but also on attack construction and Lua assignment and any other relevant places. 20180527 14:47:54-!- janebot [~Gambot@unaffiliated/gambit/bot/gambot] has quit [Remote host closed the connection] 20180527 14:48:00-!- janebot_ [~Gambot@unaffiliated/gambit/bot/gambot] has joined #wesnoth-dev 20180527 14:48:00-!- janebot_ is now known as janebot 20180527 14:51:49<+discordbot1> would you happen to know why a unit that acquired a permanent movement effect in a previous scenario would start the next short of max moves but all subsequent scenarios with full? 20180527 14:55:43-!- gfgtdf [~chatzilla@x4e32b6c7.dyn.telefonica.de] has quit [Remote host closed the connection] 20180527 14:57:32-!- APic [apic@apic.name] has joined #wesnoth-dev 20180527 14:57:53< celticminstrel> I'm guessing it starts with its former max moves? 20180527 14:58:11< celticminstrel> I'm not sure why that would happen... 20180527 14:58:35< celticminstrel> We're talking solely about an effect that changes max moves, right? Not one that changes movement costs or anything. 20180527 15:17:57< Ravana_> celticminstrel: https://github.com/wesnoth/wesnoth/commit/1413dfd4f387f589d59545f0a4cde665d0d1aac2 changes increase_damage, increase_parry, and increase_movement; but shouldn't accuracy effect need change at line 327? If I understand apply_modifier() third parameter correctly, then increase_attacks also needs change 20180527 15:20:24-!- gfgtdf [~chatzilla@x4e32b6c7.dyn.telefonica.de] has joined #wesnoth-dev 20180527 15:21:02< celticminstrel> Ravana_: Well, the commit was about accuracy and parry; that said, my understanding of the modifier leads me to believe that increase_attacks doesn't need a change. 20180527 15:21:16< celticminstrel> The function is defined in src/serialization/string_utils.cpp (for some reason). 20180527 15:21:45< celticminstrel> And it looks like the third parameter is the minimum allowed value after applying the modifier, not the minimum value for the modifier itself. 20180527 15:22:09< celticminstrel> But it's ignored if 0, which is why attacks and damage separately check for < 0 20180527 15:22:29< celticminstrel> (Would probably be better to use a boost::optional actually. Oh well.) 20180527 15:23:05< Ravana_> I see 20180527 15:23:54< Ravana_> but how this change fixes accuracy? 20180527 15:23:59< celticminstrel> The change to increase_damage in that commit is therefore a no-op (I only did it for consistency). 20180527 15:24:22< celticminstrel> Well, accuracy/parry was passed 1 as the minimum. 20180527 15:24:52< celticminstrel> So even if the weapon had a -5 parry, if you applied increase_parry=-5, you'd actually end up with a parry of 1 rather than -10. 20180527 15:25:07< Ravana_> seems that it still is, https://github.com/wesnoth/wesnoth/commit/1413dfd4f387f589d59545f0a4cde665d0d1aac2#diff-a2e76bb492cc0fe5d8b7228b6bd0ac69R327 20180527 15:25:18< celticminstrel> On the other hand if the weapon had a parry of 10, IIUC the increase_parry=-5 would have worked as expected. 20180527 15:28:00< irker765> wesnoth: Celtic Minstrel wesnoth:master d03829dfd097 / src/units/attack_type.cpp: Fixup previous commit https://github.com/wesnoth/wesnoth/commit/d03829dfd097848efaa8a6be3359b481524ebf97 20180527 15:29:19< celticminstrel> Ravana_: Thanks for noticing my error. 20180527 16:20:58<+discordbot1> Can the goblin description PR be merged into master and 1.14, or is 1.14 still in a string freeze? 20180527 16:22:34< celticminstrel> AFAIK it's no longer in a string freeze since 1.14.2 has been tagged finally, but I'm not 100% sure. 20180527 16:25:46<+discordbot1> It’s. It in a string feeeze 20180527 16:25:54<+discordbot1> It’s not in a string freeze 20180527 16:26:14<+discordbot1> err 20180527 16:26:27<+discordbot1> so it is, or it isn't? 20180527 16:26:43< celticminstrel> BTW, whoever uploaded 1.14.2 to sf.net once again forgot to mark it as the latest version. 20180527 16:28:28< celticminstrel> The link on wesnoth.org is also not updated yet. 20180527 16:29:22-!- gfgtdf [~chatzilla@x4e32b6c7.dyn.telefonica.de] has quit [Quit: ChatZilla 0.9.93 [Firefox 52.8.0/20180430140610]] 20180527 16:30:58< Ravana_> olik: I added that to https://github.com/inferno8/wesnoth-Era_of_Magic/issues/16 20180527 16:31:21<+discordbot1> celticminstrel: Yes, the effect only alters max moves. 20180527 16:32:24<+discordbot1> It has immediate effect in the scenario (max moves are increased by 1 from 6 to 7) but at the start of the next one the unit in question (who is the player's leader unit) starts with their previous max moves left (6/7). 20180527 16:32:44< celticminstrel> I'm not sure why this would happen, sorry. 20180527 16:33:50<+discordbot1> Well, expect to see a bug report soon. 20180527 16:34:01<+discordbot1> The annoying part is that I need to write a small test campaign for this. 20180527 16:35:55< celticminstrel> ...Cold Steel changed to name? Seriously? 20180527 16:36:04<+discordbot1> Yes. 20180527 16:36:21<+discordbot1> (Not sure why that needs to be a subject of discussion in the dev channel though.) 20180527 16:36:30< celticminstrel> I mean I guess it's his choice, so whatever. But it's a bit weird. 20180527 16:36:44< celticminstrel> It doesn't need to be a subject of discussion, I guess. 20180527 17:01:23<+discordbot1> Was upkeep=free removed in 1.13.x? 20180527 17:04:03<+discordbot1> Hm, no it wasn't. 20180527 17:04:13<+discordbot1> What the hell? 20180527 17:05:37<+discordbot1> I think my unit is losing upkeep="loyal" after leveling up? 20180527 17:06:42<+discordbot1> She is. 20180527 17:07:28< celticminstrel> I'm guessing you're not adding it as a modification? 20180527 17:07:41<+discordbot1> No, I'm not. 20180527 17:09:05<+discordbot1> Someone once told me to never use the upkeep= key, but instead use the loyal trait. Not sure who, but since then I always have that in mind. 20180527 17:09:17<+discordbot1> Whoever told you that needs to get slapped in the face. 20180527 17:09:40<+discordbot1> It's always worked fine, this clearly was broken at some point in 1.13.x. 20180527 17:14:35< celticminstrel> A lot of changes made directly get reverted when the unit levels up. 20180527 17:14:43<+discordbot1> Look. 20180527 17:14:48< celticminstrel> There are some exceptions coded in for things like profile. 20180527 17:14:49<+discordbot1> I've been developing campaigns since 2007. 20180527 17:15:00< celticminstrel> So an exception could be added for upkeep. 20180527 17:15:15< celticminstrel> IMO it's better to just us an [object] instead, but... 20180527 17:15:41<+discordbot1> It's harder to remove an object than change an attribute when the need arises. 20180527 17:15:56< celticminstrel> I guess this is technically a regressions so I wouldn't mind adding an exception for upkeep. 20180527 17:16:05< celticminstrel> Well, removing an object is a lot easier than it used to be though. 20180527 17:16:24< celticminstrel> ^-s 20180527 17:16:29<+discordbot1> That's not an excuse for breaking behaviour that's been known and used for over 10 years tbh. 20180527 17:16:29< Ravana_> upkeep like any other attribute modification is lost when unit is rebuilt 20180527 17:17:54<+discordbot1> Also I need to remind you that UMC creators are not engine developers. They don't want to hear a technical expanation of why something was changed/broken, theyu want to see it fixed. 20180527 17:18:11< celticminstrel> I did say I'd be fine with adding an exception for upkeep. 20180527 17:18:37<+discordbot1> There's nothing more frustrating than basic things like this being swept from beneath your feet without prior notice. 20180527 17:19:03<+discordbot1> Basically now I need to wait for a fix in 1.14.3 or add an [object] hack anyway. 20180527 17:19:26< celticminstrel> Adding an exception should be pretty easy. 20180527 17:19:33<+discordbot1> (A fix that I reckon could result in OOS between a 1.14.2 and a 1.14.3 client.) 20180527 17:19:40< celticminstrel> True. 20180527 17:20:07< Ravana_> how would exception work? level 1 unit with upkeep of 1 advances to level 2, and keeps upkeep of 1? 20180527 17:22:03<+discordbot1> Sure. 20180527 17:22:09< celticminstrel> Uhh. 20180527 17:22:25< celticminstrel> I think it'd be best to only have the exception for upkeep=loyal, then. 20180527 17:22:34< celticminstrel> Or maybe upkeep != level 20180527 17:22:42<+discordbot1> You do realize that normall yunits are upkeep="full", right? 20180527 17:22:51<+discordbot1> That value magically means "whatever this unit's level is". 20180527 17:23:44< celticminstrel> Oh right. 20180527 17:23:50< Ravana_> upkeep != level would have to ensure https://github.com/wesnoth/wesnoth/issues/1779 does not happen again 20180527 17:25:32<+discordbot1> https://github.com/project-ethea/Invasion_from_the_Unknown/issues/27#issuecomment-392349086 20180527 17:25:36<+discordbot1> That's the original bug report for the record. 20180527 17:26:55-!- travis-ci [~travis-ci@ec2-54-196-139-215.compute-1.amazonaws.com] has joined #wesnoth-dev 20180527 17:26:56< travis-ci> Pentarctagon/wesnoth#46 (dunefolk-units-fixup - 78a7e38 : Pentarctagon): The build passed. 20180527 17:26:56< travis-ci> Build details : https://travis-ci.org/Pentarctagon/wesnoth/builds/384417409 20180527 17:26:56-!- travis-ci [~travis-ci@ec2-54-196-139-215.compute-1.amazonaws.com] has left #wesnoth-dev [] 20180527 17:35:37<+discordbot1> Do I see that correctly, that for units.wesnoth.org, unit lines which contain at least one unit that is not part of a faction, are completely considered factionless? In my case, I have lvl0 units which can be created via plague, but can't be recruited. Similar to the peasant, most of the lvl 1 units which can be recruited are advancements of this lvl 0 unit. Looking at the database, i see that all of them are considered 20180527 17:35:37<+discordbot1> factionless, even though the lvl 1 units definitely are recruits of a faction. 20180527 17:36:02< zookeeper> celticminstrel, you're aware that you have some validation-related commits (not sure if there are others) you haven't forward-ported in months? 20180527 17:36:21< celticminstrel> Hmm? 20180527 17:36:46< celticminstrel> There's at least one validation commit that should not be forward-ported (I handled it differently in master) 20180527 17:37:31< zookeeper> at least a bunch of stuff that touched SoF (the campaign) 20180527 17:38:29< celticminstrel> If you point out which commits, I could probably tell you whether they need to be forward-ported. (If it's a string of consecutive commits, just the last commit would be fine and let me know how many in the string.) 20180527 17:39:15< celticminstrel> (Last as in most recent) 20180527 17:40:06< zookeeper> you sure you're in no position to just click open the commit log for SoF and take a look? 20180527 17:40:18< celticminstrel> Are they all SoF commits? 20180527 17:40:53< zookeeper> all SoF commits are SoF commits, i don't know how i can make myself more clear. 20180527 17:42:47< celticminstrel> ... 20180527 17:43:14< celticminstrel> I mean, you said I have some commits that might need forward-porting. Are all of those commits SoF commits? 20180527 17:43:23< celticminstrel> I see only four commits that touched SoF on 1.14. 20180527 17:43:28< zookeeper> i don't know 20180527 17:43:52< celticminstrel> Ohhh, I just realized what's up here. 20180527 17:44:02<+discordbot1> Is 1.14.2 up? I didn't see the notice on Steam 20180527 17:44:04< celticminstrel> The forward-ports of all the validation commits are still on a branch. 20180527 17:44:18< celticminstrel> They'll be merged eventually, I just haven't finished work on the branch yet. 20180527 17:44:47< zookeeper> okay 20180527 17:45:05<+discordbot1> @Macrowolf Yes, 1.14.2 has been released. 20180527 17:45:09< celticminstrel> (And in the hypothetical "hit by a bus" situation, they could easily be cherry-picked from the schema branch.) 20180527 17:46:33<+discordbot1> Strangely there's not a notification https://store.steampowered.com/news/?appids=599390 20180527 17:46:33< celticminstrel> I don't know when I'll finish the schema branch. It's hard to get any work done when it's hot (my air-conditioning is generally not turned on and in any case doesn't work properly even when it is on, at least for my room). 20180527 17:47:18< celticminstrel> Still, I'd be very shocked if it's not done by the end of the year. Probably far sooner. 20180527 17:47:31<+discordbot1> @Macrowolf Releasing a new version doesn't implicitly create a news item. Someone writes one if they feel like it. 20180527 17:47:36< celticminstrel> I mean, I'd be surprised if it wasn't done by October TBH. 20180527 17:48:40<+discordbot1> But there's nothing on the official forum either. How do I know what has been changed? https://forums.wesnoth.org/viewforum.php?f=62&sid=aa1bc9b4b325d84cc9c5ca0738d93b63 20180527 17:48:48< zookeeper> celticminstrel, the sooner the better, since it can and will cause conflicts when porting other commits. 20180527 17:49:40<+discordbot1> @Macrowolf See https://github.com/wesnoth/wesnoth/blob/1.14.2/players_changelog.md , or if you want more details, https://github.com/wesnoth/wesnoth/blob/1.14.2/changelog.md 20180527 17:51:20< celticminstrel> Actually @Pentarctagon, I don't think even that temporal marker is sufficient to prove whether an image is under CC or not - if we add images from addons (such as the relatively recent rat fangs attack icon), then those images will still be under GPL unless we contact the creator to relicense them. 20180527 17:51:40< celticminstrel> zookeeper: Yeah, true. There are already a few conflicts which will need to be resolved. 20180527 17:51:56<+discordbot1> Ok thanks. But I still suggest you to post a notice on Steam and on the forum, so the people can see the changelogs in a more accessible way 🙂 20180527 17:52:21< celticminstrel> @Macrowolf The forum announcement is already written, so it should appear soon. 20180527 17:52:36< celticminstrel> I don't know if there's going to be a policy of announcing every update on Steam though. 20180527 17:52:43<+discordbot1> celticminstrel: temporal marker? 20180527 17:52:49<+discordbot1> I think there should be one 20180527 17:52:58<+discordbot1> Just a suggestion though 20180527 17:52:59< celticminstrel> @Pentarctagon Sorry, this is about our conversation on that commit about the units. 20180527 17:53:23<+discordbot1> I figured, I'm just not sure what you're referring to by that 20180527 17:53:25<+discordbot1> It's just a copy-and-paste btw 20180527 17:57:45< celticminstrel> @Pentarctagon I meant the date listed on the wiki after which all new content is supposed to be CC. I don't think that can be guaranteed. 20180527 17:59:01<+discordbot1> I mean, the PR added new GPL images, so that seems pretty indisputable. 20180527 17:59:20< celticminstrel> True, true. 20180527 17:59:25< celticminstrel> Anyway I opened an issue about this. 20180527 18:04:17< irker765> wesnoth/wesnoth:1.14 Charles Dang 031f856f31 Fixed CVIdeo::quit being swallowed in in AppVeyor: All builds passed 20180527 18:08:33<+discordbot1> celticminstrel: Every update on Steam is announced, we're just having timing issues. 20180527 18:08:52< celticminstrel> Okay. 20180527 18:08:55<+discordbot1> Vultraz went to bed right when we hit the 24 hours period after the release was tagged. 20180527 18:09:28< celticminstrel> Is this also why wesnoth.org and sf.net weren't updated yet to point to 1.14.2 as the latest? 20180527 18:29:25< loonycyborg> I updated sf.net 20180527 18:47:21<+discordbot1> Yeah, someone has to do SF.net by hand. 20180527 18:48:02<+discordbot1> loonycyborg missed macOS, fixed. 20180527 18:48:55< loonycyborg> I updated only those files that I uploaded :P 20180527 18:49:29<+discordbot1> Yikes https://forums.wesnoth.org/viewtopic.php?f=4&t=48263 20180527 18:50:03<+discordbot1> loonycyborg, you're a release manager even if vultraz is officially the release manager so it'd be great if you took care of all of them in case everyone else forgets (which will keep happening). 20180527 18:52:15< loonycyborg> release technician :P 20180527 18:52:36< loonycyborg> but yeah, I guess I should be checking this too 20180527 18:54:38< loonycyborg> sf doesn't have too convenient interface for this 20180527 18:55:04< loonycyborg> like I don't see where it lists what files are current default downloads for each platform 20180527 19:01:47<+discordbot1> Yeah, you have to just assume. 20180527 19:01:59<+discordbot1> Assume and make sure the files for the latest stable release are the default ones. 20180527 19:03:04<+discordbot1> I should be remembering to do this as well since I look at the file list when writing the announcement draft tbh. 20180527 21:04:28-!- irker765 [~irker@uruz.ai0867.net] has quit [Quit: transmission timeout] 20180527 21:17:12-!- janebot [~Gambot@unaffiliated/gambit/bot/gambot] has quit [Remote host closed the connection] 20180527 21:17:18-!- janebot [~Gambot@unaffiliated/gambit/bot/gambot] has joined #wesnoth-dev 20180527 21:28:22-!- irker141 [~irker@uruz.ai0867.net] has joined #wesnoth-dev 20180527 21:28:22< irker141> wesnoth/wesnoth:master Celtic Minstrel d03829dfd0 Fixup previous commit AppVeyor: All builds passed 20180527 21:39:45-!- boucman [~rosen@wesnoth/developer/boucman] has quit [Remote host closed the connection] 20180527 21:46:55-!- gfgtdf [~gfgtdf@134.76.63.8] has joined #wesnoth-dev 20180527 21:47:05< gfgtdf> zookeeper: online ? 20180527 21:47:14< zookeeper> yes 20180527 21:48:33< gfgtdf> ah i got the answer already. 20180527 21:50:23< zookeeper> uh, okay 20180527 22:00:39< gfgtdf> https://forums.wesnoth.org/viewtopic.php?f=4&p=628988#p628988 loosk really bad, and i have no idea whther/how many other addons are effected. 20180527 22:07:07< zookeeper> ah, must be that they chose to not take lady outlaw in S02, so she got stored with negative hp and now gets unstored... 20180527 22:10:32< gfgtdf> the probalby lost the lady that so it got stored with negative hp (during die event) 20180527 22:11:06< zookeeper> oh right, that's possible too 20180527 22:12:00< zookeeper> or, rather, the only possibility, since she gets fullhealed in S02 before storing. 20180527 22:15:12< zookeeper> it's easy enough to fix, but i'm more interested in how/why was this not caught when there was such an engine-side change (since presumably it must have happened in 1.13). 20180527 22:15:43<+discordbot1> The engine-side change happened in 1.14.2 20180527 22:16:43< zookeeper> uh. what/why/how/who? i didn't see anything obvious in the commit logs i just checked. 20180527 22:17:14< gfgtdf> https://github.com/wesnoth/wesnoth/issues/3042 20180527 22:23:26< zookeeper> oh my. 20180527 22:24:01< zookeeper> granted, cases where that would lead to breakage are surely rare, but it's not like you can trivially _find_ them. 20180527 22:24:25< zookeeper> also, why do we still skip the rest of the event when an ActionWML lua error happens. 20180527 22:24:48< zookeeper> fix plz, it's a massive bug in its own right. 20180527 22:26:02< Ravana_> could be fixed, though the functionality to drop current execution could be added as new lua function 20180527 22:27:31< Ravana_> no idea if it could be useful anywhere, just provides access to current behavior 20180527 22:27:31-!- gallaecio [~quassel@188.79.96.255] has quit [Remote host closed the connection] 20180527 22:27:45<+discordbot1> I guess the reason is to avoid errors based on the previous error 20180527 22:28:17< zookeeper> @sevu, that doesn't make sense, though, that's the problem. 20180527 22:29:58< zookeeper> that behavior lets the gamestate become a completely arbitrary mess while letting the player seemingly continue playing 20180527 22:31:30< celticminstrel> Actually, fixing that shouldn't be difficult, I think? 20180527 22:32:03< celticminstrel> Just need to add some pcall()s in handle_event_commans() - I think that's the function name - defined in wml-utils.lua. 20180527 22:32:53< celticminstrel> Forgot the D in commands, wow. 20180527 22:35:41-!- mattsc [~mattsc@wesnoth/developer/mattsc] has joined #wesnoth-dev 20180527 22:38:26< zookeeper> funnily enough, in that TRoW savefile, lady outlaw's hitpoints aren't even negative, they're zero :p 20180527 22:42:50< celticminstrel> Maybe it the HP are corrected on save or something? 20180527 22:42:55< celticminstrel> I dunno. 20180527 22:42:57< celticminstrel> ^-it 20180527 22:53:54< mattsc> celticminstrel: Hi - no, combining these steps (referring to your forum post) is not always faster. 20180527 22:54:17< mattsc> First, as I say here: https://github.com/wesnoth/wesnoth/blob/master/data/ai/lua/ai_helper.lua#L911 20180527 22:54:53< mattsc> using formula= with moves>0 is very slow in itself. Or at least it was back a couple years ago when I did those tests. 20180527 22:55:32< celticminstrel> I can't deny the possibility that formula may be slow, particularly in SLF, as the formula does need to be compiled, and if it's being compiled for every single unit insteadof just once... 20180527 22:55:47< celticminstrel> I think gfgtdf's optimization may account for that though, allowing it to be compiled only once. 20180527 22:56:04< celticminstrel> But only SUF has that optimization, not SLF, nor SSF or SWF. 20180527 22:56:27< mattsc> I know for sure that back in the day that use as shown in the comment I linked to was a lot slower than the alternative that is used in that function. 20180527 22:56:32< celticminstrel> Though I don't think it would actually make a difference in SSF or SWF, because they operate on already-tiny sets. 20180527 22:56:36< mattsc> That is the whole reason why I coded that. 20180527 22:57:04< celticminstrel> Could still help for SLF, I think gfgtdf planned to eventually add the optimization there. 20180527 22:57:29< mattsc> Sure. 20180527 22:57:30< celticminstrel> Still, what about the other, non-formula option? 20180527 22:57:40< mattsc> Which are? 20180527 22:57:40< celticminstrel> Though that one won't work on 1.14 obviously. 20180527 22:57:48< celticminstrel> Adding moves as a key in SUF. 20180527 22:57:49< mattsc> [filter_wml] is even slower, IIRC. 20180527 22:58:11< mattsc> Well, sure, that would probably be faster. 20180527 22:58:16< mattsc> Is that what you meant? 20180527 22:58:31< mattsc> I completely misunderstood what you said then. 20180527 22:58:44< celticminstrel> My previous post (before the one that prompted this conversation) suggested two options. 20180527 22:58:55< celticminstrel> One was formula="moves>0", the other was adding moves= to SUF. 20180527 23:00:00< mattsc> Right. I missed that you meant adding that as a key to the filter that is allowed by the engine. 20180527 23:00:22< mattsc> Rereading it, it’s pretty clear. 20180527 23:00:35-!- zookeeper [~lmsnie@wesnoth/developer/zookeeper] has quit [Ping timeout: 240 seconds] 20180527 23:01:09< mattsc> But that will still not always be faster, unless you can somehow guarantee that moves>0 will be evaluated before anything else in the filter. 20180527 23:01:34< celticminstrel> Hmm. 20180527 23:01:44< mattsc> Are the keys evaluated in the order in which they appear in the WML? 20180527 23:01:57< mattsc> *keys or tags 20180527 23:01:59< celticminstrel> Not sure how much that guarantee really matters thougH? But I can see how it might make a small difference for complicated filters, at least. 20180527 23:02:01< gfgtdf> 1.14 has unit filter better optimized 20180527 23:02:20< celticminstrel> Tags are evaluated in order IIRC, keys are evaluated in arbitrary order. 20180527 23:02:30< celticminstrel> Which means you could use [and] to force moves to be evaluated first. 20180527 23:02:44< celticminstrel> (Because all keys are evaluated before any tags IIRC.) 20180527 23:02:47< mattsc> Okay, that would probably work then. 20180527 23:03:47< gfgtdf> if you for examle use get_unit, you could do you benchmkr for formula= again 20180527 23:04:00< mattsc> Side question, so the $this_unit” is not needed in formula= any more (in an SUF)? 20180527 23:04:18< mattsc> gfgtdf: Yeah, I can do that. 20180527 23:04:21< gfgtdf> it never was? 20180527 23:04:33< gfgtdf> a least foe i can remeber 20180527 23:04:36< gfgtdf> for* 20180527 23:04:36< celticminstrel> ??? 20180527 23:04:38< mattsc> Hmm. 20180527 23:04:41< celticminstrel> What's this about $this_unit? 20180527 23:04:45< celticminstrel> Ohh. 20180527 23:05:03< celticminstrel> In formula, yeah, it's probably generally not needed anymore. It used to be for some cases, I would think. 20180527 23:05:03< mattsc> Look at the comment I link to up there. 20180527 23:05:29< mattsc> I’m pretty sure that it did not work without back … don’t know. Might have been 1.9, who knows. 20180527 23:06:10< celticminstrel> Some things were exposed to WFL under odd names. 20180527 23:06:18< celticminstrel> And WML variables definitely weren't exposed. 20180527 23:06:22< gfgtdf> ye using $this_unit i still slow. 20180527 23:06:35< mattsc> Okay. 20180527 23:06:41< mattsc> I’ll do some more timing tests sometime in the next few days. 20180527 23:06:43< celticminstrel> What, gfgtdf? 20180527 23:06:44< gfgtdf> if you want to us formula ="moves > 0" is proably faster 20180527 23:06:55< gfgtdf> want to use formula+ 20180527 23:07:11< celticminstrel> Does it somehoe detect if the formula uses $this_unit and only optimize if it doesn't? 20180527 23:07:17< gfgtdf> than "$this_uni.moves > 0" 20180527 23:07:17< celticminstrel> ^somehow 20180527 23:07:20< gfgtdf> yes 20180527 23:07:25< celticminstrel> Nice! 20180527 23:07:48< mattsc> Well, I learned something today! :) 20180527 23:07:49< celticminstrel> Does it optimize if you use variable subst other than $this_unit? 20180527 23:07:55< gfgtdf> the poin is it cannot compile it it it does not know the final string at during iniltailisatin 20180527 23:08:14< celticminstrel> Because variables other than $this_unit won't change mid-filter. 20180527 23:08:16< gfgtdf> no it doesn't it basicially checks for '$' 20180527 23:08:37< celticminstrel> Only problem with optimizing in that case would be that there's a chance that the variable substitution evaluates to a string which then contains $this_unit. 20180527 23:09:41< gfgtdf> this is the code https://github.com/wesnoth/wesnoth/blob/1.14/src/units/filter.cpp#L285 20180527 23:23:25< mattsc> celticminstrel, gfgtdf: Really quick test just checking for units with moves > 0: 20180527 23:23:53< mattsc> using “formula = moves > 0” takes ~60% more time than the method in the function I link to above. 20180527 23:24:15< mattsc> Using “formula = $this_unit.moves > 0” takes about 100 times longer. 20180527 23:28:10< gfgtdf> hmm do you remember hw fas “formula = moves > 0” was back in the day ? 20180527 23:28:15< mattsc> So … while slower, the second method (“formula = moves > 0”) is probably perfectly acceptable. 20180527 23:28:37< mattsc> I don’t think that back in the day that syntax worked. 20180527 23:28:51< celticminstrel> I think it did work. 20180527 23:28:55< mattsc> Or if it dud, I was not aware that it did. 20180527 23:29:01< celticminstrel> However the variable may have been called something other than "moves". 20180527 23:29:04< mattsc> Point is, I never tested it. 20180527 23:29:40< mattsc> Anyway, I think I can use the method without the $this_unit — as long as I can guarantee that the moves get evaluated first. 20180527 23:29:42< celticminstrel> I know I ran into that with max moves, which was actually called something like "max_movement" or something instead of the obvious (I don't remember exactly), which led to me using Lua instead, 20180527 23:29:46< mattsc> So I’ll verify that next. 20180527 23:29:52< mattsc> (not right now though) 20180527 23:30:34< mattsc> Hmm, okay. Well, as I said, I am sure that I did not know about that, otherwise I would have either used that, or not written that comment the way it is written. 20180527 23:30:38< gfgtdf> if the current method is faster then there i sno reason to change it i'd say. 20180527 23:31:07< celticminstrel> gfgtdf: Is the WFL formula itself compiled only once for the entire filter, or is it compiled once per unit? 20180527 23:31:27< mattsc> gfgtdf: It is not necessarily faster when there is also a filter to be evaluated. 20180527 23:31:43< mattsc> a filter that contains things other than moves>0, I mean 20180527 23:31:45< gfgtdf> dpends whether the fomrula contains '$' 20180527 23:33:16< mattsc> And for reference, I ran 1000 loops on a map with 102 units, and that took ~250 ms for the current method, and 400 ms for the ‘moves > 0’ method. 20180527 23:33:18< celticminstrel> gfgtdf: If it doesn't cotain $, is the formula itself compiled only once? 20180527 23:33:24< gfgtdf> yes 20180527 23:33:40< celticminstrel> Hmm, so basically, evaluating the formula is itself a bit slow, then? 20180527 23:34:15< mattsc> So that sort of additional evaluation time needed is perfectly acceptable, IMO, if it reduces complexity otherwise. 20180527 23:34:20< irker141> wesnoth/wesnoth:master Pentarctagon 007671fc56 Add a couple missing things from the PR AppVeyor: All builds passed 20180527 23:34:57< gfgtdf> ye (or cpmiling time is just to slow that it still noticable if devided by number_of_units) 20180527 23:38:34-!- gfgtdf [~gfgtdf@134.76.63.8] has quit [Quit: Leaving] 20180527 23:39:24< mattsc> Ooo, I just noticed that for testing the order of evaluation, I can use both ‘formula = moves > 0’ and ‘formula = $this_unit.moves > 0’ in the same filter. :P 20180527 23:47:37< celticminstrel> XD Why would you do that! 20180527 23:48:46< celticminstrel> BTW, if you put them in the same tag, the second one will win because they're both the same key. 20180527 23:48:52< celticminstrel> IIUC 20180527 23:52:32< Ravana_> I found suspicious code https://github.com/wesnoth/wesnoth/blob/1.14/src/formula/callable_objects.cpp#L235 20180527 23:52:38< Ravana_> it causes wesnoth.get_units({formula="side=1"})[1].side==2 20180527 23:53:43< celticminstrel> ??? 20180527 23:53:49< celticminstrel> Is that the wrong link? 20180527 23:53:53< celticminstrel> That's cpp not lua 20180527 23:54:08< Ravana_> correct link 20180527 23:55:04< Ravana_> it is same function where wml_vars exists, so it seems to be responsible for unit filter formula 20180527 23:56:34< celticminstrel> Oh wait. 20180527 23:56:38< celticminstrel> ... 20180527 23:57:19< celticminstrel> (It's not "responsible for unit filter formula", but it's the object that said formula is called on.) 20180527 23:57:40< celticminstrel> I think that's probably left like that for compatibility because for some absurd reason it has been like that forever? 20180527 23:59:25< Ravana_> its commit message says nothing about changing how side formula works 20180527 23:59:40< celticminstrel> ? --- Log closed Mon May 28 00:00:12 2018