Time  Nick       Message
12:03 Wuzzy      Can someone help us out on #9339, please?
12:03 ShadowBot  https://github.com/minetest/minetest/issues/9339 -- Fix pathfinder bugs: returning nil frequently, broken A*, jump through solid nodes by Wuzzy2
12:03 Wuzzy      It's about the pathfinder fixes...
12:04 Wuzzy      sfan5 wants to add a new field to minetest.features ("pathfinder_works_correctly"), but I'm not sure if I should. Asking for a 2nd core dev opinion
12:05 p_gimeno   oh my... bad name
12:05 Wuzzy      <sfan5> this is only possible if it can reliably detect if the pathfinder has been fixed yet.
12:06 Wuzzy      sfan5: I have to admit, that's a quite strong argument. But I still feel uneasy about forcing modders to basically do an if statement before every minetest.find_path. It's not good if code blows up ?
12:06 p_gimeno   reminds me of the LSL function llXorBase64Strings in second life, which was broken and they released a new function llXorBase64StringsCorrect, but it had bugs too and they ended up releasing llXorBase64, which had bugs but were caught early on and fixed
12:06 sfan5      nobody needs to do an if statement
12:07 sfan5      mods which don't "care" about the result or don't have an alternative to the pathfinder can just call find_path as usual
12:07 Wuzzy      true dat
12:08 Wuzzy      i think there needs to be a rule at which point a field like "<feature>_works_correctly" is justified
12:08 sfan5      fyi the "add a feature to signal that something works" has been done before https://github.com/minetest/minetest/blob/master/builtin/game/features.lua#L6 (and on line 15)
12:08 p_gimeno   well, currently the pathfinder "works"
12:08 Wuzzy      clearly it's a bad idea to extend minetest.features for every bugfix. there needs to be a limit
12:10 Wuzzy      anyway, I guess I just wait for a 2nd core dev opinion for now
12:10 sfan5      well firstly, the bugfix needs to be relevant to the script api and there must be a reasonable alternative action modders could take if they could detect that <feature> is broken
12:10 sfan5      I think that excludes most
12:10 Wuzzy      granted, minetest.find_path was pretty broken
12:11 sfan5      oh and if the absence/brokenness can be reliably determined by other ways minetest.features is not necessary at all
12:11 Wuzzy      obviously
12:12 Wuzzy      anyway, did you perform any tests so far?
12:12 sfan5      I am doing that right now
12:12 Wuzzy      great! ?
12:12 Wuzzy      ping me in chat if you need to know something
12:12 sfan5      it appears find_path requires you to specify an algorithm despite A*_noprefetch supposedly being the default
12:13 Wuzzy      huh?
12:13 sfan5      > bad argument #6 to 'find_path' (string expected, got no value)
12:13 ShadowBot  https://github.com/minetest/minetest/issues/6 -- Apples on the trees can not be eaten
12:13 Wuzzy      before or after the PR?
12:13 sfan5      after, but I don't think that makes a difference
12:14 Wuzzy      oh, that's definitely a bug. yes the algorithm argument must be optional accordig to lua_api
12:15 p_gimeno   <sfan5> oh and if the absence/brokenness can be reliably determined by other ways minetest.features is not necessary at all  <-- I suggested to provide a code-friendly version number some years ago
12:15 sfan5      version numbers don't work for forks
12:15 Wuzzy      ^^
12:16 sfan5      https://github.com/minetest/minetest/blob/master/src/script/lua_api/l_env.cpp#L1201 hmmm
12:16 sfan5      I guess absence of a value is not the same as nil?
12:16 Wuzzy      i dont think code-friendly version numbers are a good method to verify if a feature exists. this would only work if we also had giant feature tables that you manually need to look up somehow. not fun
12:17 Wuzzy      sfan5: yes. iirc Lua makes indeed a difference, at least in C
12:17 Wuzzy      but in hte programming language Lua, absense needs to be treateted equally as nil
12:18 Wuzzy      if i understood correctly, its the responsibility of the API provider (=us) to make sure that absense is treated correctly. but i'm not sure about that
12:19 Wuzzy      anyway, this is fixable
12:19 Wuzzy      i wonder if we have other API functions in which absense is not treated correctly...
12:21 Wuzzy      by the way, the function that is used to check absense (in C) is "lua_isnone"
12:21 sfan5      http://sprunge.us/87l9PJ?diff
12:21 Wuzzy      oh, i just saw there's also lua_isnoneornil. how nice of them ?
12:22 sfan5      just use that one then
12:22 Wuzzy      yep
12:26 sfan5      the pathfinder assumes that the object walking the path is one node tall, right?
12:27 Wuzzy      yes
12:28 Wuzzy      adding height clearance is a much needed feature (but for a later PR)
12:30 sfan5      well I'd say it's working then
12:31 Wuzzy      yay
12:34 rubenwardy I support a feature flag
12:34 rubenwardy This isn't just a bug fix, this is actually making code function
12:36 Wuzzy      sfan5: rubenwardy: ok but i suggest to name it "pathfinder_works" instead of "pathfinder_works_correctly" because it's more consistent with similar names and shorter
12:36 rubenwardy Yes lol
12:42 Wuzzy      done
12:43 Wuzzy      rubenwardy: did you do a full review again or is the code still to horrifying to you? ? I have cleaned up the code (a little) and sprinkled a few comments. I hope it helps
12:52 rubenwardy Some time soon
12:52 rubenwardy Request a review to remind me
12:53 Wuzzy      I just did?!
13:32 Unarelith  rubenwardy, about formspec refactor, I don't really know what to say.. yes I think it's needed, and yes it should be decoupled into multiple classes which have their own role.
13:32 Unarelith  imo, you shouldn't ditch Irrlicht right away, but build a new formspec implementation on top of it which wouldn't be dependent on Irrlicht
13:33 Unarelith  then, write MT's own GUI system that would replace Irrlicht
13:33 Unarelith  but this is a big amount of work and I don't see how this will be possible given the current state of the project
13:33 Unarelith  I went for a completely different way of handling GUI in OpenMiner
13:35 Unarelith  well, not that different, but still. this is an example of GUI definition: https://github.com/Unarelith/OpenMiner/blob/master/mods/default/workbench.lua#L37-L94
13:36 Unarelith  the GUI created is stored in a struct, which is serialized and sent to the client. then, the client will interpret that data to build the GUI elements from it
13:37 Unarelith  so basically, for each GUI element there is: a struct (GUI def) in the server, and a class (GUI element) in the client. one class in the client is responsible for handling deserialization and GUI element creation (will move in to the element class), and one other class in the server is reponsible for Lua parsing and serialization
13:38 Unarelith  (the lua parsing will move in the struct)
13:38 Unarelith  I can pack what I said here into a comment on #9358 if you want, but idk if this can be of any help
13:38 ShadowBot  https://github.com/minetest/minetest/issues/9358 -- Formspec Refactor
13:40 Unarelith  imo, each GUI element should have two classes/structs (one for client with rendering info + deserialization, one for server with GUI data, lua parsing and serialization)
13:41 Unarelith  plus two classes for the management of these classes, one in the client to store and render the complete GUI, one in the server to store and send the GUI data
13:42 Unarelith  that's how it works in OpenMiner, I don't know if this is applicable to Minetest tho
13:43 Unarelith  but if you go with this implementation and then want to get rid of Irrlicht for that later, only the rendering part of the client classes will change
15:27 p_gimeno   rubenwardy on #9358 on ImGUI: "This is not appropriate for production games - it says this at the top of the readme"
15:27 ShadowBot  https://github.com/minetest/minetest/issues/9358 -- Formspec Refactor
15:28 p_gimeno   neither is Irrlicht
15:28 rubenwardy Irrlicht is intended for production games
15:28 rubenwardy It's outdated and terrible
15:28 p_gimeno   not the GUI
15:28 rubenwardy No, it's not
15:28 rubenwardy Hence why we want to replace it
15:29 p_gimeno   I'm not defending imGUI, just noting that what we have now is in the same condition and yet it has stuck for a while
15:30 p_gimeno   and if you're able to get rid of Irrlicht's scene tree, I might be able to solve the ludicrous hack of the "camera offset" and do things properly
15:31 rubenwardy Changing GUI systems doesn't mean we'll change from irrlicht
15:31 rubenwardy That's infeasible at this point
15:32 p_gimeno   shame
15:32 rubenwardy Most of the problems we have that blamed on irrlicht are actually our problems
15:33 rubenwardy One thing that could be worth doing is isolating irrlicht code from the rest of the code
15:33 rubenwardy Making it so that it's not used at all on the server
15:33 rubenwardy Although idk
15:34 rubenwardy It's currently used on the server for vectors at least, iirc
15:36 p_gimeno   https://git.unarelith.net/Unarelith/OpenMiner/pulls/9 - OpenMiner is safe against precision problems. That solution is unfeasible in Minetest due to the scene graph handling single precision positions.
15:40 p_gimeno   And also because with Irrlicht you don't have any control over how the position is applied to the transformation for rendering.
15:52 rubenwardy Nice
20:35 Krock      will merge game#2611 in 5 minutes
20:35 ShadowBot  https://github.com/minetest/minetest_game/issues/2611 -- Creative: Skip redundant refreshes, fix reset button by SmallJoker
20:41 Krock      <<<<
20:44 sfan5      that reminds me...
20:45 sfan5      merging game#2596 in a few mins
20:45 ShadowBot  https://github.com/minetest/minetest_game/issues/2596 -- Update Chinese Translation - 2 by IFRFSX
20:45 Krock      of this week's meeting points? https://dev.minetest.net/Meetings
20:45 Krock      ah, no.
20:46 sfan5      well those are interesting to read too