Time Nick Message 12:11 proller https://github.com/minetest/minetest/pull/11843 - simple type rename! 12:15 MTDiscord it is also woefully untested! 12:29 proller its okay. 12:31 MTDiscord does it still segfault when reaching 31000 in any given direction? 12:32 erlehmann uh 12:32 erlehmann that should not happen at all 12:33 proller nothing changed by default. 12:33 MTDiscord I was able to go beyond in devtest although in MTG it segfaults 12:33 erlehmann prove it with unit tests 12:34 proller but if you compile with USE_POS32=1 - you can walk to any coords up to 2^31 12:34 nrz_ it's just a typedef replacing types, it's pretty simple, you just can break the game mayby by changing the map coordinates 12:34 erlehmann no one cares about that 12:34 erlehmann AFAIK minetest has *never* crashed when reaching 31000 12:34 erlehmann and also, again, it is ridiculously stupid to stop any behaviour at 31000 12:35 nrz_ proller i suggest on your PR to just comment out or add a cmake warning for people configuring USE_POS32 that it's very experimental 12:35 nrz_ and also not retrocompatible with old clients 12:35 erlehmann i suggest proller you do actually add in proper boundary checks 12:35 nrz_ or regular clients ? 12:35 erlehmann yeah lol 12:35 nrz_ proller what happen if you connect a MT client with USE_POS32=0 on a server with USE_POS32=1 ? 12:36 nrz_ i think we should have a crash, same for inverted flags i think 12:36 proller actually you will stop at some 1000000 positions because float precision - increment on small values gives old value, but can walk fast or use teleport 12:36 erlehmann proller that has to be fixed anyway 12:37 proller nrz_, option(USE_POS32 "32 bit positions (experimental, not compatible with 16 bit)" FALSE) 12:37 erlehmann even at 32767 the delta on floats is too much for some mods 12:37 nrz_ i think if we only have type rename and we discourage people to increase for a moment it can be nice yeha 12:37 proller nrz_, cant connect because protocol packets have bigger length 12:37 nrz_ at a moment maybe POS instead of a typedef should be a normal structure with backward compat 12:37 nrz_ proller: what i thought 12:38 nrz_ tbh proller i like your PR but we should merge it after 5.5 release 12:38 nrz_ and after that maybe we can worker harder on the engine in order to manage both compat modes 12:38 proller erlehmann, i making option to switch floats to doubles by same way, its already works except some bugs 12:40 erlehmann proller ok but PLEASE add in proper boundary checks. the problem is that minetest devs never add in the correct checks afterwards 12:40 proller but good solution is use int base coord + float exact coord lime m_camera_offset - but for all positions calculations 12:40 nrz_ it's not really the purpose of its PR, it's just a rename, it doesn't reduce or increase code risks 12:40 erlehmann and by never, i mean “the general attitude is that putting some arbitrary limit somewhere in the stack will suffice” 12:40 erlehmann nrz_ of course it does increase risks if a lot of the conversion code implicitly assumes 16 bit 12:41 proller erlehmann, there is still LOT OF todo after this pr, it just makes all further work possible 12:41 nrz_ it's why i think we should just keep this PR for the 5.6 merge window (and sorry proller, it's a good PR but not the good moment to merge it) 12:41 erlehmann proller, what i am telling you is if you do not get it the right time it takes a lot of effort to fix tihs 12:41 erlehmann proller, what do you think of my idea of handling old clients by fake teleporting them? 12:42 erlehmann that would give you backwards compat, but break map saving 12:42 nrz_ erlehmann, i suggest to just stop talking about this PR as it's not the good moment to work with it, as all you request is more improvement we must add after reviewing & merge it 12:42 proller maybe ok, need to try, simplest is teleport them to spawn 12:44 proller maybe break completely network code for 6 release? 12:44 proller 1. switch to usrsctp 12:44 proller 2. stole network packets from freeminer OR use protobuf 12:45 erlehmann nrz_ you just need to look at https://github.com/minetest/minetest/pull/11858 to see that the simplest bugfix ever has no chance of being merged if there is a somewhat workable (but buggy) solution 12:45 erlehmann therefore proller should try to do everything right the first time IMO 12:46 erlehmann nothing lasts as long as a provisorium 12:48 erlehmann proller breaking backwards compatibility is often the solution if ppl are not motivated or able to figure out better ideas. users don't really love it. 12:49 proller users still suffers from slowest networking code 12:49 erlehmann proller, sctp snice though 12:50 erlehmann devs keep saying that thing, but minetest is surprisingly playable on a 15 year old laptop with slow internet, once the cache is filled with textures and models and so on 12:50 proller not true sctp, it not completely supported, just sctp over udp with all sctp benefits 12:50 erlehmann i know i know 12:50 erlehmann you need to do that bc windows can't do it right? 12:51 proller not only windows, it can be removed from android kernel, can be banned by network providers 12:52 erlehmann oof 12:52 erlehmann well 13:21 celeron55 enet was another option that was tested to give better transfer rates, i think the problem back then was no official ipv6 support and questionable project maintenance, would have had to make a fork 13:23 celeron55 usrsctp seems pretty good 13:24 celeron55 i would like to see performance numbers comparing USE_POS32 enabled and disabled 14:04 erlehmann celeron55 btw regarding your reservations regarding the performance impact of bounds checks, would you accept the output of the --speedtests option as evidence if something does or does not impact performance? 14:05 erlehmann because of course you are right that more checks impact performance 14:06 erlehmann some people actually think that -fwrapv is an easy cop-out for making algorithms bounds-safe 14:06 erlehmann but it inserts bounds checks EVERYWHERE lol 14:07 erlehmann so the queston i have is just what benchmark would satisfy you 14:07 erlehmann ig --speedtests is ok i'll go do my research 17:01 MTDiscord I don't get how https://github.com/minetest/minetest/pull/10329 ever got merged. TL;DR: - We're not sticking to math conventions (scalar multiplication, vector addition), as scalar addition/subtraction stayed accepted - Dubious claims of the Schur product being useless were taken for granted, no "vector.schur_product" / "vector.scale" or the like were added - Upvotes / downvotes appear to have been ignored Now that I need Schur product 17:01 MTDiscord / Schur quotient, I consider it slightly frustrating that it got marked as deprecated. Please revert. 17:12 celeron55 erlehmann: --speedtests doesn't do anything useful for this 17:13 celeron55 a test for this should run the mapgen on the server, with lighting generation, and then the meshgen on the client - i think those are some of the heaviest users of node coordinates 17:15 celeron55 networking doesn't particularly concern me altough it could (maybe should) concern someone 17:20 erlehmann tbh networking wise the very impactful change “do not send lua entity or player properties if they are set to a value that they already have and emit a warning, so mods can fix their the ridiculous globalstep loops that do this kind of nonsense” is taken about as seriously as all my other proposals, i.e. not at all, despite mineclonia proving that this naive approach can reduce traffic by several orders of magnitude. 17:21 erlehmann i.e. cora has managed to make “idling in a singlenode world” produce about 100 bytes per player per second instead of 10kb per player per second 17:21 erlehmann (both playerplus and the engine are at fault here, since bones seem to move without playerplus, but playerplus also spams bone position setting) 17:23 erlehmann fixing that is a) easier b) more impactful … than changing some network protocol in an incompatible way 17:44 celeron55 that change sounds reasonable 17:48 MTDiscord I myself made that change but didn't do the warning. The API is instead intended to just accept naive set calls and accepts responsibility for making it so. 17:49 MTDiscord Specifically this is regarding set_properties, I don't think I touched bone stuff. 17:49 erlehmann the problem of doing it without the warning is that there is a lot of code that becomes expensive no-ops 17:49 MTDiscord unless that's in there. 17:49 erlehmann technically it is already expensive no-ops 17:50 erlehmann almost every setting of a property that already has the value to the same value that i debugged happened in some kind of logic that could be entirely avoided if you sth is checked before 17:50 MTDiscord In my experience much of what I need to do to determine whether the properties need to be changed or not is what I need to do to determine what to change them to. That would mean that I'd just basically add another layer of the same logic to fix the warnings. 17:51 erlehmann well for stuff like field of view you are not really forced to check it before 17:51 MTDiscord I'm basically already doing the check, and that's what my patch IS. I moved the check into set_properties because it made no sense to leave it outside and thus risk not having all cases covered. 17:52 erlehmann Warr1024 are you rounding floaty values though? 17:52 erlehmann cora had to round the bone positions to 2 digits after the decimal point 17:52 erlehmann bc the engine does something fucky with some float values (not normal rounding) 17:52 MTDiscord I'd be fine with making the warning optional, e.g. by sending a allow_same=true property or something, but if it's mandatory then I'll end up just patching around the patch with a de facto copy of the patch anyway. 17:52 MTDiscord the engine really shouldnt be holding peoples hands. it should do as instructed. also if a game wants this they can override the api and do the check before passing the data onto the api 17:54 MTDiscord The way I handle floats is I compare the ratio between them and allow them to be off 1 part in 100k 17:55 erlehmann ah 17:55 MTDiscord If doing sane things by default is "holding peoples' hands" then yes, the engine absolutely should be doing that. 17:56 erlehmann i agree. i have seen the entity/player thing so often 17:56 erlehmann most devs do not even know this can lead to their mod being the difference between a player lagging with 5 players or wih 50 players 17:56 erlehmann a server lagging i mean 17:57 MTDiscord If it were object:send_properties_packet_to_client() then it would make sense to blindly send the packet every time, but it's set_properties() which implies that it sets the state of a thing, and the engine should be responsible for determining how to replicate that state to the client. 17:57 erlehmann yeah also it should be idempotent in the first place 17:58 MTDiscord set_* methods should probably all by implicitly idempotent. 17:59 erlehmann well, my experience is that working around such stuff in lua is safer than trying to get it fixed in the engine 17:59 MTDiscord If we really want to expose things like sending packets or writing to disk to the modders then it seems like those should be their own APIs. 17:59 erlehmann so i am kinda ok with no one doing stuff about it 18:00 erlehmann bc if it goes as well as the map border stuff the chance is higher it would break sth than fix it 18:00 erlehmann (i would bet money that someone who approaches the network code finds an opportunity to insert “clever” further optimizations) 18:00 MTDiscord I mean it should be the "engine's" responsibility in the sense that it should be fixed centrally instead of requiring everyone to install the same hack. The "engine" to me includes builtin, and I think that may be the sanest place to implement this, and certainly the cheapest in terms of code maintenance costs. 18:01 erlehmann IIRC luatic and me have made this point in the past, but core devs disagree 18:01 MTDiscord I don't like getting too clever, but set_properties seems close enough to no-brainer... 18:01 erlehmann the problem is not you 18:01 MTDiscord Heh, I think I opened an issue about it myself at one point. What other issues or PRs are there for it? 18:01 erlehmann the problem is the person who thinks: this is simple and there i can totally improve on it by 5% with this one simple trick 18:02 erlehmann which then is neither simple nor improves on it 18:02 celeron55 i think making mods simpler by allowing them to do stupid stuff by cutting their calls to no-ops on the C++ side is fine 18:02 MTDiscord I've done that shit myself, I just have no qualms about throwing that work out if my hypothesis is disproven. 18:02 erlehmann celeron55 cool can you convince someone else of that too? 18:03 erlehmann i want to reiterate that having a warning is absolutely necessary for improving server performance 18:03 MTDiscord I'm convinced. 18:03 celeron55 i don't agree with the warnings 18:03 MTDiscord I want to reiterate that having a warning should be a decision that we should not force upon our users. 18:03 celeron55 requiring lua code to always check to not update to the existing value makes lua code look ugly 18:04 erlehmann that is an aesthetic argument though 18:05 MTDiscord The engine's job is to enable game creators to create games. We should not critique the quality of their work, especially in cases where we don't actually know whether the thing we're measuring relates to quality or not. 18:06 erlehmann i have yet to see a case where it was not an error to set something to the same value every server tick 18:07 MTDiscord Basically all of NodeCore is a counterexample. 18:07 erlehmann oh tell 18:07 erlehmann i am curious 18:07 MTDiscord just did 18:07 erlehmann i thought you had wrapped the method and that was it? 18:07 erlehmann mineclonia wraps stuff too 18:07 erlehmann with the wrapper, you should not get any warning then 18:07 MTDiscord I thought we were making an argument about why the wrapping should be in core? 18:08 MTDiscord If my having wrapped it in my game is going to still be necessary then putting it in core doesn't really help. 18:08 erlehmann i see 18:08 erlehmann well then not a warning, a DEBUG whatever 18:08 erlehmann just a way for devs to figure out if their game is running NOP loops 18:08 erlehmann idc about the loglevel 18:08 erlehmann but currently it is next to impossible to figure all that stuff out easily 18:09 MTDiscord I'd be okay with a debug/verbose, i.e. anything below ACTION ... but I also don't want log spamming if it's happening constantly, so either only the first one, like "hey some mod is doing bad stuff" or summaries, like a "happened X times in the past T" 18:09 MTDiscord The problem with WARNINGS is that some server owners take those quite seriously, and mods shouldn't have a bad mark for doing what they're supposed to. 18:10 MTDiscord Ultimately it's the experience with using a mod is the mod author's responsibility and we can't do much to prevent most forms of dumb/naive, and this one is a case where there are significant use-cases that are NOT naive. 18:10 erlehmann "happened X times in the past T" would be perfectly fine 18:11 MTDiscord Basically I don't want it as a warning because you're making a big assumption that letting engine/builtin do the deduping work is not ALREADY the most optimal it can be. If you want to demand that an author improve something you need to be sure it CAN be. 18:11 erlehmann but warning on the first occurance would also help 18:11 erlehmann i understand 18:12 erlehmann i should add that all cases i have seen are cases that happened in player or entity steps. 18:12 erlehmann that probably narrows my perspective a lot 18:12 erlehmann because only those cases are likely to create ridiculous performance problems. 18:12 erlehmann i guess nodecore will not do such a folly 18:13 MTDiscord Heh, NodeCore is tricky. I have 39 registered_playersteps, which are all run by 1 globalstep, which actually has something like 2 layers of automatic idempotence. 18:13 erlehmann celeron Warr1024 i am convinced a warning is too buch and debug/verbose is good. thx for explaining your positions. 18:14 MTDiscord I'm sure it could be performance-optimized more, in theory, but performance isn't the only thing I have to worry about optimizing for. 18:14 erlehmann Warr1024 you are more careful and more talented than a lot of mod devs i guess, from all i have heard so far. 18:14 erlehmann Warr1024 if you ever do a writeup about all the cool optimizatons in nodecore, i'd love to know about it. especially regarding unexpected engine behaviour. 18:15 MTDiscord The idea that I'm more careful than many is another reason why I think it makes a lot of sense to make the engine do the sanest things by default. The fact that I may be more careful is also one reason why I'm skeptical about the value of warnings, i.e. the people who actually pay attention to them are already more likely to not need them. 18:15 erlehmann from my experience that last thing is not generally true 18:15 erlehmann ppl who had no idea about textures suddenly got into alpha shenanigans bc of the warnings 18:16 erlehmann and understood their own mods beter that way 18:18 MTDiscord I suppose that could be. I do take warnings seriously myself. I just want to make sure that nobody feels compelled to chase down a warning that's actually counterproductive. 18:18 celeron55 a more pedantic engine might have a separate method for when you intend to set a new value vs. when you intend for the engine to check the value and determine whether anything has to be done, but it's not really a design MT uses and it seems bloated to me 18:18 MTDiscord I ran into one with set_player_velocity being deprecated in 5.4+, but mandatory in 5.3-. I had to put in a hack to make set_velocity work on players in 5.3- without breaking 5.4+ 18:19 MTDiscord At least in that case, though it really CAN'T be fixed in the engine/builtin 18:20 erlehmann celeron55 what you specify as a more pedantic engine to me looks like a cop-out that tries to outsource optimization to mod code, with predictably horrible results 18:22 celeron55 i mean it would have both options with different method names 18:22 erlehmann and i stand by my assertion 18:22 celeron55 but given lazy modders wouldn't care what method they'd use, you'd end up with the exact same problem 18:22 MTDiscord Warr, what's your opinion on the GCC warnings about memset or memcpy with a non-trivial type? Minetest has at least 2 IIRC. 18:22 erlehmann yeah 18:22 celeron55 as they'd use the wrong one 18:23 MTDiscord I hate warnings and want to fix them all. The question is really just (1) are we sure the warnings are valid for our situation, and (2) what cost would it be to fix them properly? 18:24 erlehmann josiah_wi do you mean the null pointer thing or something else? 18:25 erlehmann sfan5 could you maybe try to use the “possible close” label either less or add another tag to it more often? i have seen you use it for at least three purposes: when you could not reproduce a bug, when you were convinced something might not be a bug, when you consider an alternative proposal to be better. 18:26 erlehmann i am not sure what the solution here is, it just seems a bit confusing 18:26 MTDiscord I appreciate your sense of humor and your opinions on programming, Warr. I think I could learn a lot from you. 18:26 erlehmann Warr1024 can you write an AOSA chapter on nodecore? 18:29 erlehmann celeron55 how do you propose to move forward with the property setting idempotence performance idea so that it does not suffer the same fate as every map border discussion i have started? 18:29 erlehmann like what is the next step to get it into minetest 18:32 celeron55 someone has to make a PR that matches this discussion 1:1 18:32 celeron55 so as to not require a new discussion 18:34 erlehmann Warr1024 would you do that? while you have no need for this thing, i trust you more to start with something good than myself, since you have the wrappers already. 19:06 sfan5 erlehmann: dunno, do you think the label isn't accurate? 19:09 erlehmann sfan5 i think it is not helpful in grouping tasks by what needs to be done next. in particular, i would be happy if you would stop applying it as the sole tag to stuff you can not reproduce like he i386 floating point thing. 19:14 sfan5 I could use it more sparingly on unconfirmed bugs but I usually only apply it when I'm reasonably sure that it's not an actual bug 19:25 sfan5 merging #11859, #11867, #11868 in 10m 19:25 ShadowBot https://github.com/minetest/minetest/issues/11859 -- Restructure devtest's unittests and run them in CI by sfan5 19:25 ShadowBot https://github.com/minetest/minetest/issues/11867 -- [no squash] Mod security improvements by sfan5 19:25 ShadowBot https://github.com/minetest/minetest/issues/11868 -- Protect font initialization with mutex by sfan5 19:35 erlehmann sfan5 maybe just stop using it when you try to reproduce with a different setup than the submitter or sth dunno 19:35 erlehmann sfan5, it's not a big deal, i just thought maybe you immediately had a better idea how to handle it 19:37 MTDiscord erlehmann, I don't know whether you saw, but I've PRd some of Irrlicht's unit tests back to IrrlichtMT now. 19:37 erlehmann josiah_wi i did not know. link? 19:37 erlehmann thx for the notice 19:47 rubenwardy Ironically, logging a warning would make it slower than the redundant call 19:55 erlehmann well maybe just do it in a debug build or sth 19:56 erlehmann if dispatching the log message is a problem in itself 20:40 MTDiscord erlehmann, irr#84 20:40 ShadowBot https://github.com/minetest/irrlicht/issues/84 -- Restore some unit tests by JosiahWI 20:56 erlehmann looks nice, but i do not have the energy to look into it much rn