Time Nick Message 14:59 erle hey what's the place to add a new unit test to irrlichtmt? 15:32 MTDiscord I guess I closed my PR to re-add the unit tests. 15:33 MTDiscord Or is that still open? 15:33 MTDiscord Yes I closed it. 15:33 erle well i do need test cases for proper fixing of input handling bugs, because step 1 is *always* to add a failing test case, then make it pass. 15:35 MTDiscord I guess you can be the first to add a test. That'll make it harder to land the PR no doubt. 15:36 erle i guess i should first contribute a test upstream because they have tests (hopefully) 15:36 MTDiscord Right, we just deleted them all because of the amount of effort it would've taken to fix them. 15:37 MTDiscord I believe I did fix a lot of the relevant ones, but it was low priority and there was nobody to review it. 15:40 erle to quote CuteAlien (irrlicht maintainer) on the test removal … “bad idea” 15:40 erle :P 15:40 erle anyway, i'll figure it out 15:40 erle thanks for confirming that there are no tests though 15:40 MTDiscord I have a recollection of sfan adding some kind of Catch2 tests to Irrlicht recently, but I can't find them for some reason. 15:40 erle bc i vaguely remembered your work and was confused i could not find an obvious place to put them 15:41 MTDiscord irr#200 has a Catch2 test suite 15:41 ShadowBot https://github.com/minetest/irrlicht/issues/200 -- Static glTF mesh loading by JosiahWI 15:41 celeron55 regardless of what happened to old tests or why it happened, there surely should be a place to add new tests where needed 15:42 MTDiscord Part of the issue here is that we don't have a test runner in Irrlicht, because the original tests used a custom runner which we deleted. 15:43 erle “we“ 15:43 erle but yeah 15:43 MTDiscord Sorry, maybe it's not appropriate for me to use "we". 15:44 erle nah 15:45 erle it was just my way too sarcastic way to say “it is not your fault” 15:45 celeron55 feel free to, there exactly aren't rules for that 15:45 erle josiah_wi that gltf thing … a) did you contribute it upstream too b) did you fuzz it? 15:46 MTDiscord Because I thought upstream was dead it never occurred to me to contribute it to upstream, and I have not fuzzed it. There is an issue I am currently aware of that is not fixed and will definitely be caught by a fuzzer. 15:47 MTDiscord I need to work on that and finish cleaning up the last todo items, but I've been really busy with school and am now part of another open source project that's much higher priority. 15:48 MTDiscord Ah, yeah, shoot, there was also the issue there of that CI run I added that uses CMake 3.5. I used a third party action which we shouldn't necessarily trust. 16:41 nrz merging #13828 16:41 ShadowBot https://github.com/minetest/minetest/issues/13828 -- Improve readability and infos in verbose log by sfan5 16:41 nrz and #13827 16:41 ShadowBot https://github.com/minetest/minetest/issues/13827 -- doc: `animaition` -> `animation` by Panquesito7 16:45 erle josiah_wi as far as i can tell, irrlicht upstream has gltf on the todo list for a while, so maybe you can just say hi there and even get some assistance! 19:11 grorp Merging #13819 in 10 min 19:11 sfan5 merging #13773, #13819 in 5m 19:11 ShadowBot https://github.com/minetest/minetest/issues/13819 -- Escape package description in content tab by rollerozxa 19:11 ShadowBot https://github.com/minetest/minetest/issues/13773 -- Don't trigger a key event if a key with the same associated char was pressed by savilli 19:11 ShadowBot https://github.com/minetest/minetest/issues/13819 -- Escape package description in content tab by rollerozxa 19:11 grorp lol 19:11 sfan5 heh 19:11 appguru :o a merge race condition 19:13 erle ROllerozxa what evil can be done with unescaped package descriptions? 19:13 appguru sfan5: 19:13 appguru ~minetest/src/network/networkpacket.cpp:556:8: runtime error: null pointer passed as argument 2, which is declared to never be null 19:13 appguru ~/minetest/src/network/networkpacket.cpp:66:8: runtime error: null pointer passed as argument 1, which is declared to never be null 19:13 appguru should we do something about these? 19:13 ShadowBot appguru: Error: You must be registered to use this command. If you are already registered, you must either identify (using the identify command) or add a hostmask matching your current hostmask (using the "hostmask add" command). 19:13 ShadowBot appguru: Error: You must be registered to use this command. If you are already registered, you must either identify (using the identify command) or add a hostmask matching your current hostmask (using the "hostmask add" command). 19:15 Desour appguru: that's the memcpy thing 19:15 erle appguru, is this this? https://github.com/minetest/minetest/issues/11798 19:15 erle wdym ”memcpy thing” 19:15 appguru erle: yep it's that 19:15 appguru i'm currently fiddling with a segfault a b3d model causes, but i can't make sense of the backtrace 19:15 sfan5 we can fix the "issue" but it's kind of dumb that memcpy(nullptr, ..., 0) is UB 19:16 erle doesn't matter much that it is dumb, a compiler will use UB for optimization 19:17 Desour yep, it's kinda dumb, wich is why I wanted to introduce wrappers that do a nullptr check: #13656 19:17 ShadowBot https://github.com/minetest/minetest/issues/13656 -- Add wrappers for memcpy and memset by Desour 19:17 erle uh 19:18 appguru here's the backtrace of the segfault, if anyone wants to take a look: https://pastebin.com/VDg2G0kh 19:18 appguru i don't see how getNormal should be able to return an invalid reference 19:19 sfan5 why not? if i is out-of-bounds 19:20 appguru i is 0 19:20 appguru i don't think the buffers can or should ever be empty 19:20 sfan5 0 is out-of-bounds for an empty array 19:21 Desour btw that invalid bool load at src/client/game.cpp:1954, reported earlier in #minetest would be a one-liner to init m_game_focused, if someone wants to do a quick fix 19:22 appguru sfan5: that might actually be it 19:23 sfan5 that the mesh buffer is empty is probably an indicator that something is up, however Minetest shouldn't explode in that case 19:23 appguru i had not expected my code to produce empty buffers 19:23 appguru i wonder whether this is a minetest or a b3d reader responsibility 19:23 erle Desour given that one could fix the issues at the places where they exist, what's the thought process behind the wrapper? that it's easier to wrap everything and not ever think about it later? 19:23 appguru empty vrts or tris chunk don't really make sense, the b3d reader should probably reject them 19:24 Desour erle: that question was asked at the PR, read the answer there 19:25 appguru here's the b3d btw: https://paste.c-net.org/SwerveSpree 19:25 erle Desour i re-read the comments and i think it is not, but don't bother, it is not important anyway. 19:25 appguru haha this is hilarious 19:25 appguru i finally see it 19:25 erle what? 19:25 appguru if nobody minds a quick story i'd tell it 19:26 appguru hmm, let me just tell it in #minetest 19:31 appguru I'll file a bug report 19:31 sfan5 don't bother 19:32 erle why not? it crashes minetest? 19:38 appguru sfan5: what about minetest.get_dir_list not erroring when fed an invalid symlink? 19:38 sfan5 there is no mechanism defined for returning errors 19:38 appguru huh wdym? couldn't it raise a lua error? 19:39 appguru nil would be fine as well, just not an empty table as if an invalid symlink was an empty directory.. because it is not 19:39 sfan5 sure it could but it didn't do so before and it also doesn't match what other FS functions do 19:39 sfan5 (they return a bool for success, no errors thrown) 19:40 appguru I'm fine with a bool for success; I can `assert' that 19:40 appguru I'd be fine with returning nothing here 19:41 appguru As for backwards compatibility: I really hope no one is relying on get_dir_list returning an empty table for an invalid path. 19:41 appguru But it would be good if we could get a zipgrep of usages. rubenwardy? 19:41 sfan5 I didn't mean that in the "we can't do this" way, just as an argument why it's not a simple fix 19:42 appguru i'll make the PR if we can do this 19:42 appguru anyways, off for dinner 19:42 sfan5 this late? 19:43 appguru this late 19:45 pgimeno it's not that hard to check for empty table: t = minetest.get_dir_list(); assert(next(t)) 19:46 sfan5 (but empty tables are a valid result) 19:47 pgimeno ah so the problem is distinguishing nonexisting from empty, ok 20:12 MTDiscord erle, thank you very much for letting me know! What would be the best way for me to get in contact with the upstream dev team? 20:19 erle josiah_wi no idea what the best way is, but sfan5 and me both posted on their forum 20:34 appguru pgimeno: also small side note: assert(next(t)) doesn't check for empty table if false is a valid key (not the case here, but in general it may be important to be aware of this) 20:36 appguru I should have had assertions in my code; I didn't because I just quickly threw it together for a quick demo. Still minetest.get_dir_list shouldn't treat an invalid path like an empty directory. 20:39 appguru okay so get_dir_list uses our own fs::GetDirListing which returns an empty vector on error; because fs::GetDirListing is used in a couple places, this makes it awkward to just change that behavior e.g. by throwing an exception 20:40 appguru I could introduce another method which throws an error and implement GetDirListing as a wrapper which catches that and returns an empty vector, or I could deal with the error at all callsites. What do you suggest? 20:46 erle appguru what is actually the scenario that you want to optimize for? 20:48 sfan5 latter 20:48 appguru I just want to fix get_dir_list not erroring on invalid paths. One solution is pretty easy and unlikely to break something, but incurs a small technical debt. The other has higher potential to break something, but should also address lack of proper error handling at callsites and would probably reduce technical debt. 20:49 appguru sfan5: ok 20:49 appguru I think I'll have to compile Irrlicht with asan too to catch the memory funkiness my bad apple b3d triggers. 20:53 appguru whatever it is that is happening here, it seems asan isn't catching it 20:55 erle appguru why should it *not* error on an invalid path? i mean, taking a step back, can you even recover gracefully from it? 20:56 appguru erle: the internal function (fs::GetDirListing) or the lua api function? 20:56 erle the lua api function, of course the whole program should not crash 20:56 appguru the lua api function should error 20:56 erle and the recovery would be … ? 20:57 pgimeno pcall? 20:57 appguru that is up to the user, they can use pcall 20:57 erle i mean in terms of “you write a mod, it calls this function, what is it trying to do” 20:57 erle on a technical level i know 20:57 appguru erle: I'm hoping for a grep of CDB for that 20:57 erle but i wonder in which scenario you encounter this stuff 20:57 erle ah ok 20:57 erle i see! 20:59 appguru About the only usecase I recall is filesystem persistence where you have some directory structures (Epidermis has something like this). The recovery could be creating/initializing the directories. 21:02 appguru oh here's a smol font-related memleak asan catched when I exitted: https://pastebin.com/WqpMuB21 21:10 erle appguru how about you figure out if this was reported and create an issue if it was not? 21:13 appguru erle: is this memleak worth it? 21:14 erle what do i know 21:14 erle i just think on the bug tracker it will not scroll away like here 21:14 appguru I think we should probably just have one big issue to collect such things. 21:14 erle i think that's a really bad idea, but you know 21:15 appguru technically it might even be considered not a memory leak considering that memory is freed on process exit hehe 21:17 erle just to spell it out: if you make a single “all things i found with sanitizer X” issue, that issue will probably either never or erroneously be closed 21:17 erle unless it's like 3 things 21:19 appguru anyways, #13832 21:19 ShadowBot https://github.com/minetest/minetest/issues/13832 -- Small memory leak detected by ASan 21:21 erle the steps to reproduce are missing the compiler options 21:22 appguru erle: added 21:22 erle this is funny because it probably should be part of the form you hate 21:22 erle or the version info, no idea 21:23 appguru anyways, the dirt block thing seems like it was a false positive and devtest is doing that? 21:23 appguru (it would be nice if that was documented) 21:24 erle wdym devtest is doing it 21:25 appguru the punch node unittest does that it seems 21:28 appguru tho it should also remvoe the node.. 21:44 appguru oh hey, i just realized we can use c++ 17, so i can use an std::optional instead of an exception 23:25 erle question to the people who think one big irrlichtmt commit is preferable to git subtree: what is the downside of subtree? please don't answer if you are pro-submodule or think subtree is the best ever or have never used git subtree at all. 23:25 erle (i.e. i only want answers from people who tried it and think it sucks.)