Time Nick Message 12:31 erlehmann the response to #11989 really irritates me. has anyone ever proposed a rule to *not* do breaking API changes if at least two games or mods can be pointed out that would break? 12:31 ShadowBot https://github.com/minetest/minetest/issues/11989 -- Incompatible API change in get_player_control() causes server crashes 12:32 erlehmann it would shortcut all these ridiculous debates about “oh this is not really a breaking change because while game devs and mod devs know it, we haven't ever written it down in 10 years, so they are totally wrong in assuming stuff works – THOSE FOOLS!” 14:34 sfan5 having it return a string if not called on a player is literally a copy-paste mistake from whoever implemented this 14:34 sfan5 changing the API is already hard enough as is, that we have to seriously discuss the implications of a change like this says a lot 14:35 erlehmann IMO there is literally no cost with reverting the breakage and maybe outputting a warning about it 14:36 sfan5 and in case it isn't obvious I do not support the "Microsoft" approach to compatibility 14:36 erlehmann the problem is changing it in a minor release, with no warning 14:36 erlehmann and no deprecation period 14:36 sfan5 we only have minor releases 14:36 sfan5 >deprecation period 14:37 erlehmann then stop breaking stuff 14:37 sfan5 please don't make me laugh 14:37 erlehmann hey deprecation worked fine for the alpha texture spam thing 14:37 erlehmann mod devs were like how do i fix this and fixed it 14:37 sfan5 the minetest.deserialize semi-vulnerability is just as much of a "changed in a minor release with no warning" like this 14:37 MTDiscord erlehmann: how should such a warning work? 14:37 sfan5 yet you don't seem to care about that one 14:38 celeron55 if someone's wondering how that code actually came to be, i think it was written by RBA in 2012-11, tested by Taoki and merged by myself. apparently i didn't catch the mistake 14:38 sfan5 @luatic you put a log_deprecated(L, "stupid"); in the right part of the if, easy 14:38 MTDiscord sfan5: not easy 14:39 erlehmann sfan5 a) i did not notice b) the only thing you can deduce from that is that i may be a hypocrite, not that what i say is wrong c) security is a good reason to break an API, random musings about API return values over 8 years after the API was first implemented, is probably one of the worst reasons 14:39 MTDiscord because it has come to be expected that player methods can be called for entities without spamming logs 14:39 erlehmann luatic may be right 14:39 erlehmann i have seen code in node on punch handlers 14:39 erlehmann obviously anything can punch a node right? 14:39 sfan5 "it has come to be expected" that sounds like the actual problem 14:39 erlehmann so people ask for puncher:get_player_control().sneak orwhatever 14:40 MTDiscord sfan5: the header literally says it 14:40 MTDiscord https://github.com/minetest/minetest/blob/master/doc/lua_api.txt#L6646 14:40 MTDiscord "no-op for other objects" 14:40 erlehmann which is a lie 14:40 MTDiscord now logging a warning isn't exactly no operation, is it? 14:40 erlehmann the documentation lied about other stuff too 14:40 sfan5 is returning an empty string a "no-op"? 14:40 erlehmann luatic you are right, we can't do the warning probably 14:41 erlehmann the empty string wins on terms of not breaking a bunch of unrelated shit that the person who introduced this change is probably not going to fix by themselves 14:41 erlehmann this would be different if someone would go around and fix all those mods that go crashy and ensure they caught all of them 14:41 MTDiscord I have an idea for the warning, but it's not clean either. If we returned an empty table, we could set a metatable on it that logs the warning if the table is indexed. 14:41 sfan5 minetest development is hell, no wonder nobody wants to do it 14:42 sfan5 you change literally anything and you get yelled at by devs and server owners 14:42 rubenwardy the fix for mods is really simple 14:42 sfan5 AND you are expected to either fix their code for free or revert the change 14:42 erlehmann hey i submitted a patch 14:42 erlehmann for free 14:42 erlehmann and i spent time helping users to fix their worlds 14:42 sfan5 submit a patch to your broken game instead 14:42 erlehmann that were going crashy 14:42 erlehmann i will *also* do that 14:43 erlehmann but come on, you can't casually break 3 games and claim it's someone elses fault for not subscribing to your idea of API aesthetics 14:43 erlehmann i am pretty sure there are more cases of that, in particular node punch handlers 14:43 erlehmann and for server admins it is really ugly 14:45 erlehmann from my POV (game/mod dev), minetest development has been hell whenever someone changed stuff that previously worked. literally all of the issues i had that frustrated me were of the “someone thinks this should be different for aesthetic reasons, code goes crashy or works different” type 14:45 erlehmann it is perfectly possible to move forward without breaking things 14:46 sfan5 puncher:real_get_player_control_v2_fixed() 14:46 erlehmann it's just that it is ugly and apparently enough people think looking good is better than not creating a ton of work for mod devs 14:46 erlehmann yeah, puncher:get_player_control_2() 14:46 erlehmann exactly that, then remove the old function in some major release down the road 14:47 erlehmann at least then the bugs make sense. no one will get bug reports with “i updated minetest and your game crashes at startup” 14:47 erlehmann (it was a wolf that was a boat passenger, btw) 14:49 erlehmann the worst thing about this is that everyone of the devs is perfectly able to not casually break stuff. it is not hard work, no one is too stupid for it. but time and time again, aesthetics trump practical choices, creating more bugs. 14:52 MTDiscord PlayerRefs and ObjRefs should be seperated entirely for a clean API. PlayerRefs should extend ObjRefs. Same for object properties; players should have player properties instead. 14:52 MTDiscord it's legal to change api between major releases 14:53 MTDiscord the entire player:get_player_something() is ugly already 14:53 MTDiscord next major release is 6.0, just saying 14:53 MTDiscord nah, 5.5 is also major 14:53 sfan5 it's not 14:53 MTDiscord ^ 14:53 MTDiscord semver doesn't work that way, @savilli 14:53 MTDiscord I mean, technically it might be 14:53 erlehmann but minetest uses sentimental versioning, not semantic versioning! 14:54 MTDiscord But according to semver it may not be 14:54 erlehmann increase the numbers how it feels right 14:54 sfan5 but applying semver by-the-letter would eternally stall progress 14:54 MTDiscord it's clearly major bc it has new features 14:54 sfan5 so we don't do it 14:54 MTDiscord not bc some numbers 14:54 MTDiscord you don't understand semver 14:54 MTDiscord read https://semver.org/ 14:54 erlehmann sfan5 applying semver by the letter does not stall progress. it just means that it is immediately obvious when devs can't be bothered to make code backwards compatible. 14:55 MTDiscord new features allow increasing the minor version number 14:55 erlehmann IMO the x.y.z numbers are best interpreted in way of risk. x is a big risk when upgrading, y is a small risk, z is next to none. 14:55 sfan5 erlehmann: in reality applying semver by the latter would mean you only ever do major bumps 14:55 sfan5 which helps nobody 14:56 sfan5 letter* 14:56 MTDiscord changing the discussed api looks like a small risk to me 14:56 MTDiscord you don't need to rewrite everything 14:57 MTDiscord If it can be shown that a technically breaking change is highly unlikely to cause mod breakage and yields a considerable improvement, it's fine to just increment the minor version I believe. 14:57 erlehmann sfan5, i know that argument and yeah, everyone who either can't write or can't be bothered to write future-proof code thinks this is 100% the truth. the reality is that breaking changes are tested and batched. 14:58 erlehmann and then applied in the next major release 14:58 MTDiscord savilli: it will lead to immediate crashes for all MCL users with outdated or MCL2 14:58 erlehmann i am not sure if the fixed mcl mods are even on contentdb lol 14:58 MTDiscord well, should have read release notes 14:58 MTDiscord Command sent from Discord by Luatic: 14:58 MTDiscord !? 14:58 erlehmann yeah the users should have read the release notes lol 14:58 MTDiscord MCL2 has 160k downloads 14:59 MTDiscord then MCL2 doesn't work with 5.5, and users shouldn't update until it will be fixed 14:59 MTDiscord that's a good contract 15:00 erlehmann you know what, i am not saying this lightly, but in my opinion, every single one of you who thinks normal users should suffer to satisfy your sense of aesthetics has a bit of an empathy problem. it's the same as with the maps: there are people out there who have no idea what lua or c++ is who happily play minetest and they have no idea what to do if they upgrade and it fails. 15:00 MTDiscord It isn't!? 15:00 MTDiscord Side note, MT 5.5 aggressively upgrading maps is still frustrating 15:00 MTDiscord It has lead to me having both a 5.5 and 5.4 test world for my mods now 15:01 erlehmann luatic i meant the tga maps actually. but yes lol. it is frustrating. it means you can not even go back to 5.4 if minetest changes make your world crashy! 15:01 erlehmann that makes it worse lol 15:01 MTDiscord didn't even think of that, poor users 15:02 erlehmann as a game dev, i am willing to update the check for get_player_controls(), of course. but think of users please. 15:02 erlehmann get_player_control() sorry 15:02 sfan5 I am pretty sure MCL contains a bunch of engine-version specific workaround that rely on undocumented or internal things 15:03 sfan5 supposed those break suddenly, would it means you also have an empathy problem? 15:03 sfan5 -s 15:04 erlehmann well in this case it costs literally nothing to not upset users. also i am pretty sure mcl2 is a bunch of bad code held together with duct tape and chewing gum, because most of mineclonia development consists of cleaning that up. 15:06 erlehmann sfan5 if other mods or games broke with 5.5 in a way that users can not easily fix it or downgrade to 5.4 (because the world gets converted) i would say, that, yes: whoever broke it evidently does not care about other people's problems enough and does not value their time. i would be surprised if that is a controversial statement, even. 15:07 erlehmann i'd prefer to not go into it deeper, because i do not want to turn into kilbith 15:08 sfan5 does "easily fix" include updating the game? 15:09 erlehmann not really, since for that the game would have to be fixed first. 15:09 erlehmann and you can't expect users to figure out which mod has this behaviour. 15:09 erlehmann the mcl* mods are probably not the only ones that break, just the only ones i know about. 15:10 erlehmann a user having mod soup of like 15 mods is the case i envision 15:10 sfan5 they don't have to, open the content tab and click all green squares until none are left 15:10 sfan5 or I think there's an "Update all" button actually 15:10 celeron55 the mod name is probably on the error message shown in the dialog, right? 15:11 erlehmann AsyncErr: Lua: Runtime error from mod '??' in callback luaentity_Step(): ...netest/games/mineclonia/mods/ENTITIES/mcl_boats/init.lua:263: attempt to index local 'ctrl' (a nil value) 15:11 erlehmann stack traceback: 15:11 erlehmann ...netest/games/mineclonia/mods/ENTITIES/mcl_boats/init.lua:263: in function 'on_step_old' 15:11 erlehmann ...mes/mineclonia/mods/ENVIRONMENT/mcl_void_damage/init.lua:17: in function <...mes/mineclonia/mods/ENVIRONMENT/mcl_void_damage/init.lua:16> 15:11 celeron55 personally i would go with "ok whatever, return a string then and write to the docs 'sorry this API was designed in 2012'" 15:11 erlehmann i agree with celeron55, it's the simplest thing that works 15:12 erlehmann having assisted server owners in fixing crashes, i can say that whatever is obvious to us now is not obvious to someone who just set up a server in their spare time for their friends or kids. 15:12 celeron55 this isn't something that prevents making good design choices in the future 15:13 erlehmann also, there are kids out there who have no idea about all of this stuff. maybe the error message will make some of them into programmers! or primitivists, if they conclude software was a mistake. 15:13 MTDiscord erlehmann: BTW, I think the proper fix for mods is to check for ref:is_player() instead of ref:get_player_controls() or {}, ref:get_player_controls() and ref:get_player_controls().something or the like. 15:14 erlehmann luatic how to fix mods is not the issue here. 15:15 erlehmann i mean, i appreciate the input, but i know perfectly well what the boat code does wrong. 15:15 sfan5 anyway it's not like there isn't a good way forward for this issue, but it would have likely turned out different if the text wasn't the same "noo you can't change this undocumented and never intended behaviour, now my mod broke, go revert it!!!" we are very tired of hearing 15:15 sfan5 @luatic it just crossed my mind that you can return a table which throws a warning when it's indexed 15:16 sfan5 (not a serious suggestion) 15:16 erlehmann sfan5 i provided literally every bit of information and wrote a patch which i tested. what more do you want me to do? 15:16 erlehmann if it is “fix your game”, yes, i will 15:16 erlehmann regardless of what minetest does 15:17 MTDiscord sfan5: lol I suggested that 35 min ago "I have an idea for the warning, but it's not clean either. If we returned an empty table, we could set a metatable on it that logs the warning if the table is indexed." 15:17 erlehmann yeah and then you have to scan contentdb to ensure that no one had the admittedly stupid idea to check for the empty string 15:17 MTDiscord or for the type 15:17 erlehmann indeed 15:17 MTDiscord unfortunately such scans are practically impossible due to Lua's dynamic nature 15:18 sfan5 I'd say mineclone is the only game/mod where this can reasonably cause problems in practice 15:18 erlehmann hahahahaha 15:18 MTDiscord I did in fact grep around a bit, and driver:get_player_control().something seems to be a common idiom in mods which provide "driveables". 15:18 erlehmann sfan5, based on what? gut feeling? 15:18 erlehmann i greped too, this is how i found the puncher:get_player_control() pattern 15:18 MTDiscord OFC I don't have the time to analyze the content to determine whether driver can be an object and whether driver:is_player() is checked earlier on. 15:18 erlehmann in nodecore i think 15:19 sfan5 erlehmann: how many games do you know where entities can "ride" other objects? 15:19 sfan5 don't answer "three" 15:19 MTDiscord I know of a fourth ;) but it's 0.4.x-only 15:19 MTDiscord the most frequent "idiom" seems to be placer:get_player_control().sneak 15:20 erlehmann sfan5 uh no idea about games, but you are focusing too much on my example. see the thing that luatic showed. 15:20 erlehmann like 4 or 5, but does it matter? 15:22 sfan5 of course it does 15:22 sfan5 nothing changed about calling get_player_control() on players 15:23 MTDiscord if you wanted to fight fire with fire, continue returning empty strings but set a metatable on the string table such that attempting to index a string with any of the control names will result in a warning 15:23 erlehmann look, people call this not only for boats 15:25 erlehmann lib_mount has 15:25 erlehmann local ctrl = entity.driver:get_player_control() 15:25 erlehmann if ctrl.aux1 then 15:25 erlehmann painting has 15:25 erlehmann if puncher:get_player_control().sneak 15:25 MTDiscord the question is just whether entity.driver can even be a non-player objref 15:25 MTDiscord I believe this showcases something else though 15:25 MTDiscord Modders relying on playerrefs not changing during the session 15:26 MTDiscord Hence storing the player instead of the player name 15:26 MTDiscord Some modders even compare players by reference xD 15:26 sfan5 if you have one ref it cannot reasonably expire 15:26 MTDiscord Therefore, I believe the docs should guarantee this, as it also makes code a lot neater. 15:26 sfan5 (because why would it?) 15:26 MTDiscord Theoretically, the engine might swap it out 15:27 sfan5 no? 15:27 sfan5 the userdata object itself cannot be changed 15:27 sfan5 only it's contents / what it points to 15:27 MTDiscord I know 15:27 MTDiscord That's the point, what if the engine decided to recreate it's internal player object, invalidating the obj ref mods hold? 15:28 sfan5 erlehmann: how do you know that those refs can be non-players? is it "gut feeling?" 15:28 MTDiscord Listen, I just want a guarantee that player refs stay valid for the session duration xD 15:30 erlehmann sfan5 for the puncher case, i am pretty sure that non-player entities can punch others, right? 15:30 erlehmann firing an arorw on a travelnet box or something would punch it, no? 15:30 erlehmann arrow 15:30 sfan5 depends on the implementation 15:31 erlehmann yeah, depends. we have two ways forward: choose something incompatible and make sure it is not a problem in practice … or revert to the compatible API and discuss it whenever 6.0 is a thing 15:31 erlehmann i prefer the thing where we can stop worrying and just apply my patch 15:31 erlehmann and discuss other problems 15:32 MTDiscord I hope that we will properly distinguish players by 6.0 15:32 erlehmann i actually hope not! 15:32 erlehmann i find it pretty useful to be able to have one code path for players and mobs 15:32 MTDiscord not by, with* 15:32 erlehmann but then again i am the person who wrote code so that boats can enter minecarts, so … 15:33 erlehmann (the trick is to punch the minecart in the right direction when the boat goes forward) 15:33 MTDiscord By distinguishing I mean inheritance 15:33 MTDiscord Players could still be used as ObjRefs, but ObjRefs not as players 15:34 MTDiscord So if you want MCL boats to be rideable by any nearby ObjRef, that still works and requires just one code path 15:34 erlehmann above everything else, i wish for minetest to never ruin existing setups except for security/crash/hang reasons – and then only as much as ncessary. 15:34 MTDiscord In this one case, actually a Lua "quirk" is to blame (the overly permissive string metatable) 15:35 MTDiscord for those who want strictness in a dev environment: you can do setmetatable(string, {__index = function() error"attempt to index a string value" end}) 15:35 Pexin 0.4.x "we're not even 1.0, dont expect stability" 5.0 "WELP nevermind!" haha 15:41 MTDiscord Seems like erlehmann is determined to piss off/drive away the main dev behind minetest 15:42 MTDiscord As if there wasnt already a lack of them 15:42 MTDiscord Agreed. 15:49 MTDiscord sfan5: re 11976 Here's what the profiler gives me: https://pastebin.com/UK6eDrQV 15:49 MTDiscord About 18 players results in 2022-01-25 15:58:29: INFO[Main]: Server: map saving (sum) [ms] . . . . . . . 1x 5860 15:52 erlehmann I just encountered a crash where a paleotest carnotaurus got punched by a non-player. 15:52 erlehmann The new API is so *fun*! 15:53 erlehmann and yes, it is because it assumes get_player_control() on the puncher 16:01 erlehmann jonathon, i am not determined to piss off anyone. which is why i made it as easy as possible this time to fix this bug. 16:04 rubenwardy could rename it to get_player_controls() and deprecate the singular form, which is gramatically invalid anyway 16:04 rubenwardy totally not confusing 16:09 erlehmann just so you know, the paleotest crash also makes worlds unusable. as soon as i rejoin, the dinos fight again. 16:13 MTDiscord @Greens Sleep Paralysis Demon you're dinos are being broken 16:27 sfan5 @Bastabun and it freezes for about 5 seconds regularly right? 16:28 MTDiscord @Bastrabun ^ 16:30 MTDiscord It freezes in the interval of whatever I set server_map_save_interval to. If I leave it the default 5.3, we get freezes every 5.3 seconds, currently it's on 30 and we get freezes every 30 seconds. 16:46 sfan5 how long is the freeze itself? 16:51 MTDiscord That depends on the number of players and the value of server_map_save_interval 16:52 MTDiscord Roughly 30 players and server_map_save_interval 5.3 gives me a 2 to 3 second lag, while server_map_save_interval 30 gives me a 3 to 4 second lag. 16:59 v-rob Sheesh, maybe we should invite Microsoft over to do our compatibility for us. Having to support undocumented behaviour that you're obviously not supposed to do... 16:59 MTDiscord This is what the log says over the past two hours: https://pastebin.com/0H5u1CU5 17:28 celeron55 v-rob: i think people are probably overreacting to this 17:29 celeron55 modders probably tried out if the result works for mobs when used like a table with true-ish and false-ish items, and it does, so they assume it returns such a table for mobs also 17:29 celeron55 turns out it's actually a string, but a string just happens to behave like one 17:29 celeron55 thus, changing it into actually being an empty table will work fine 17:32 erlehmann “will probably work out fine” is what got us into this mess 17:33 celeron55 not really 17:33 erlehmann well, the API was changed without even greping contentdb for its usage (which would have revealed that problem) 17:33 celeron55 you're all overreacting, basically 17:33 MTDiscord When is he not? 17:34 rubenwardy yeah, I think modders will assume that this just returns {} 17:35 Krock get_player_by_name returns nil when there's no player 17:35 Krock the player control and information should do the same 17:35 MTDiscord Minetest: consistently inconsistent 17:37 celeron55 Krock: arguably it can be different because it needs to return an object (of sorts) 17:38 celeron55 the rest are just values 17:38 MTDiscord I consider {} and 0 a decent solution 17:38 celeron55 anyway, this is the wrong time to design this api 17:38 celeron55 this api was already designed. 8 years ago 17:40 celeron55 any energy you have for api design should be focused on those apis currently in waiting to be released as stable 17:41 Krock what was the previous behaviour? did it provide garbage values, or none at all? 17:41 celeron55 the empty string 17:42 celeron55 then sfan5 changed it to nil 17:42 celeron55 and it broke stuff 17:42 celeron55 2 weeks ago 17:42 erlehmann curiously, no one ever complained about that empty string. i mean, it's obviously garbage design. but it works. 17:42 celeron55 that's because the empty string looks like an empty table 17:42 Krock ? 17:43 Krock "" ~= {} 17:43 MTDiscord not exactly, but due to the string metatable, as long as you don't try indexing it with "sub", "gmatch" etc it does 17:43 MTDiscord {} ~= {} though 17:43 celeron55 yes not exactly. but you can even do # on it to count the items. and it says 0 like for an empty table 17:43 Krock fair point 17:43 celeron55 actually 17:43 erlehmann Krock so people are doing puncher:get_player_control().sneak in mobs that turn enemy when they are punched or node on_punch handlers 17:43 MTDiscord I don't see why anyone would be calling # on a dict 17:43 celeron55 i think it would say 0 for the player table also as that's used as a dictionary 17:44 Krock well then {} would be a saner option, provided that mods do not explicitly rely on this string bs 17:44 Krock erlehmann: yes, within callbacks the player object must be valid and contain this data 17:45 erlehmann Krock the thing is, non-players can also punch 17:45 MTDiscord ^ 17:45 erlehmann or be passengers in a boat, which is how the mcl thing crashed 17:45 Krock get_player_control() is undefined behaviour for non-players 17:45 MTDiscord wrong 17:45 erlehmann it has been perfectly well defined in the code for a long time and was changed 17:46 MTDiscord the docs say it's a "no-op" 17:46 MTDiscord so yes, returning nothing (which is actually not the same as returning nil) is reasonable for a "no-op" 17:46 celeron55 i think basically everyone is agreeing on {} now 17:46 MTDiscord yes 17:46 celeron55 so there's no need to discuss this more 17:46 celeron55 someone just needs to commit the patch 17:46 Krock oh right. no-op would then indicate return 0; (i.e. no arguments, no action) ? 17:47 erlehmann i do not agree, because i am not in the mood to file another bug as 5.5.0 is released 17:47 erlehmann it will probably work out, but some mad person will have relied on this being a string 17:47 Krock celeron55: as a hacky solution, yes. 17:47 MTDiscord then that's on them honestly 17:47 erlehmann does not matter who is at fault 17:47 celeron55 the api didn't say it's a string. you are supposed to file a bug at that point 17:47 erlehmann it literally does not matter who is guilty of shitty code, be it engine or mod devs. the users have crashes. 17:49 erlehmann will make sure that mineclonia does not crash with whatever 5.5 does anyway 17:49 celeron55 erlehmann: you're doing more bad than good to MT by draining the time from devs that have more productive use for the time 17:49 MTDiscord ^this 17:49 erlehmann celeron55 i provided a solution and then everyone else had an opinion on a thing that does not hinder them. i will be silent. 17:49 erlehmann until the next breakage! 17:50 celeron55 it's good that you brought the bug up 17:50 celeron55 but as the core team is responsible for the solution, you have to give the core team the freedom to make the solution 17:51 erlehmann and sorry that i posted so much about how it is bad. i just really thing it has to be solved *somehow*, even if it is not my preferred solution. it seems you have found one. 17:52 celeron55 sorry for the shitty code made in 2012, i'm trying to not let it happen anymore 8) 17:53 celeron55 whether that's succesful, well, we'll see in 2020 17:53 celeron55 eh 17:53 celeron55 i mean 2030 17:54 erlehmann well … i doubt you can anticipate what someone else will find ugly years later. but if you ever come up with an API everyone likes, maybe ask zughy about it ;P 18:11 jordan4ibanez Hmm, perhaps you could preemptively break it to avoid future breakage? 20:40 sfan5 I'll import weblate commits later and stuff so translators have a chance to translate before release 20:41 sfan5 @luatic how's the "send gui scale to server" PR going 20:52 MTDiscord sorry, not going at all currently, 5.5 will probably have to do without it 20:58 sfan5 then so be it 22:20 sfan5 https://github.com/minetest/minetest/commits/ci translation commits, will push after build finishes