Time Nick Message 07:49 wrrrzr Do I need to extract classes from files so that there is only one class? 07:49 wrrrzr Or it useless? 09:45 wrrrzr Anyone in chat? 09:47 wrrrzr Hello 09:47 MTDiscord hello 09:47 wrrrzr Do I need to extract classes from files so that there is only one class? 09:47 MTDiscord well this isn't java so you are free to do your own judgement on that I believe 09:48 MTDiscord though keeping the size of individual source files small is also to be preferred 09:48 wrrrzr I talk aboutb code quality, Not about compiler warrings 09:49 wrrrzr I split chat.cpp into small files 09:49 wrrrzr Can i send pull request, or it not impactful 09:49 wrrrzr ? 11:10 sfan5 I quickly looked at chat.cpp and in my opinion it doesn't need to be split 11:49 wrrrzr So i dont send PR? 12:17 wrrrzr Do i need to exctract little classes like MediaInfo in server.h ? 12:31 celeron55 server.h and server.cpp might be reasonable subjects for splitting, but MedaInfo won't get you anywhere, it's so small. it calls for heavier rework 12:32 celeron55 if you can propose getting something out from the actual Server class, that'd be awesome 12:32 celeron55 i mean, having those little classes there don't really add complexity 12:38 MTDiscord the code style guidelines for files mentions that source files should generally stay under 1500 lines. so if you check the line counts of all source files in the main engine codebase you can probably find some older monolithic source files that are good picks to try to split up 12:49 wrrrzr Server is very big file 12:50 wrrrzr I remove now server playing sound 12:55 sfan5 you want to move 43 lines to its own file? 12:57 wrrrzr sfan5: Yes 12:57 sfan5 that's not worth it 12:57 celeron55 while a move of a couple of tens of lines may seem to the right direction, it probably won't suffice for a PR because it will only confuse people more 12:58 wrrrzr celeron55: So i cancel PR and try to make more lines moved? 13:01 wrrrzr Maybe extract ServerThread in own file? 13:01 celeron55 you need to do something which coherently and substantially improves readability without affecting functionality. that's all i can specify. if i had something more specific in mind, i'd already have told you to do something specific 13:02 wrrrzr Ok 13:20 wrrrzr Can please check my pull request https://github.com/minetest/minetest/pull/15517 14:25 MTDiscord wrrrzr, I'm looking at server.cpp, and I see that almost the entirety of the file consists of methods for communicating various events to clients. 14:25 MTDiscord Moving out small helper classes to other files will have almost no impact on the size of the file. 14:26 MTDiscord It also makes sense that all those server methods go in the same place. There's simply a very large number of them. 14:31 celeron55 yeah it might not be trivial to shorten it. of course one could make a generalized event handling system which cuts down almost all the individual methods, but that probably requires a lot of finesse in order to avoid affecting functionality 14:32 celeron55 (i didn't look at it more than a few seconds) 14:34 MTDiscord Yep, "not trivial" is a good way to put it. Having looked at it for a bit, I think splitting it up in a way that is still cohesive and easy to understand would require rearchitecting some things maybe. 14:35 MTDiscord And it would be very easy to make it worse than it is. 14:37 MTDiscord It might be possible to just do general cleanup on the existing implementation, and that might generate ideas for further improvement. 14:38 MTDiscord It doesn't look too difficult to do something like add a new server method, though, so I'm not sure it'd be that beneficial to spend time on? 14:40 MTDiscord There's a very noticeable pattern: each event has a method to kick off the event and update data, and a method to send the correct packet to clients. 14:41 celeron55 very possible. well, a better indication of complexity compared to line count is the number of member variables. a dumb list is not complex 14:41 MTDiscord I guess my two cents then after looking at it is: we could try moving the information on how to send each type of packets into plain data instead of source code. 14:42 MTDiscord Then you can have one (new) definition file for all the packets, and reduce the number of send... methods from 10s of methods to just 1. 14:44 MTDiscord Yeah, some things just require a lot of code. In this case there are quite a few member variables - oh wow more than quite a few, I didn't finish scrolling. 14:45 celeron55 i'm scrolling through the member variables and seems to me like even those aren't particularly bad. the list is long, but this class basically contains all of the state that a running luanti server needs. given that it's literally everything, it's not bad 14:45 celeron55 however some of it could be organized into subclasses or structs to make it more structured 14:46 celeron55 there are hints for doing that in the comments surrounding the variables 14:46 celeron55 and then possibly some of the Server methods could be moved to those subclasses also 14:46 celeron55 if they depend only on those variables 14:46 MTDiscord Agreed, that's a good idea for wrrrzr if he wants to keep looking at it. 14:48 celeron55 yeah, it's not a particularly complex task and you don't need that much knowledge about the codebase if you're ready to just try and see and roll back a couple of times if some portion wasn't as independent as it originally looked like 14:53 MTDiscord agreed about the subclasses. just add more hierarchy. don't extract singular small items while leaving the rest there, that's counterproductive. 14:53 MTDiscord Might also look at 699d42efc and see why CodeScene thinks it halved the complexity. It might be possible to keep doing what someone else already did, and that helps keep people on the same page as well. 14:53 MTDiscord from a very quick glance it seems for example you could group HUD-related stuff together and move it elsewhere. same for sound-related stuff. 14:53 MTDiscord and media-related stuff. 14:54 MTDiscord now if you just move mediainfo, that won't be very helpful: the rest of the media-related stuff is still in there and now it's spread over different files. 14:54 MTDiscord I was thinking of making a ShutdownServer subclass and finding a way to dynamically swap out the implementation of the server, but then I thought it might not be worth the complexity for just one subclass lol. 14:56 MTDiscord basically, what we've got here is a large system consisting of largely orthogonal subsystems which all live "at the same level", in the same file. these large subsystems ought to be extracted. 14:56 MTDiscord The other place that could definitely use some work is game.cpp, and I think there's already a PR open to make a big improvement there if it hasn't already been merged. 14:58 Desour it looks to me like a lot of Server functionality is modding related. maybe it'd make sense to put all that into a separate subsystem, which then uses server 15:30 Desour will merge #15669, #15657, #15653 in 10 min 15:30 ShadowBot https://github.com/minetest/minetest/issues/15669 -- Fix CFileSystem::FileSystemType related UB by cosin15 15:30 ShadowBot https://github.com/minetest/minetest/issues/15657 -- VoxelArea: Fix missing cacheExtent calls in helpers by Desour 15:30 ShadowBot https://github.com/minetest/minetest/issues/15653 -- [no sq] Refactor Meshgen, and fix black plantlike_rooted by Desour 15:38 [MatrxMT] can we put #15661 in 5.11? #15652 is messing with our server logs and it's not possible for us to tell what's the real reason of a crash, as it gets overridden by a globalstep crash. Luatic and I talked about this in private yesterday 15:38 ShadowBot https://github.com/minetest/minetest/issues/15661 -- [no sq] Misc fixes by sfan5 15:38 ShadowBot https://github.com/minetest/minetest/issues/15652 -- Globalsteps continue executing well after an error is thrown 15:39 [MatrxMT] we had 4 crashes yesterday and the log gives us no clue, as the log is obviously the wrong one 16:35 sfan5 it will be merged before 5.11 anway 16:38 [MatrxMT] perfect, ty 16:53 MTDiscord Mizokuiam is weird / spammy. Commented the exact same thing ("The implementation is clean. Consider adding more error handling...") on 3 different PRs, and another vacuous statement on another ("Great observation! Let me add some thoughts..."). Hiding as spam. 16:53 sfan5 already reported the account to github 17:12 MTDiscord did we ban the account from the org to prevent more? 18:48 wrrrzr Maybe replace MutexedVariable to std::atomic ? 18:49 sfan5 I don't think std::atomic exists 18:50 wrrrzr Ah, yes 18:50 wrrrzr Sorry 18:52 sfan5 but if there's a stdlib helper for what we're doing, sure 19:37 MTDiscord https://cdn.discordapp.com/attachments/747163566800633906/1327723044155949131/image0.png?ex=678419f1&is=6782c871&hm=31562002cb17551367d64828a96ad82b4093ab67028e80a4cca364698931f450& 19:40 MTDiscord wrrrzr: since you want to refactor things, have you seen #15350 yet? you could refactor some core::array away by replacing it with std::vector. 19:40 ShadowBot https://github.com/minetest/minetest/issues/15350 -- Irrlicht refactoring checklist 19:40 MTDiscord if we do enough of that we can eventually get rid of core::array. 19:46 MTDiscord How bad is core::array? 19:46 MTDiscord How many times a custom allocator for std::vector may be needed? 19:49 MTDiscord The answer to the latter question is practically never, though it is something to explore more in the future. But note that this isn't equivalent to the first question. 19:53 MTDiscord core::array is annoying because it lacks many basic modern C++ vector features and overall integration with the C++ standard library. 19:54 MTDiscord it also has some differing annoying behavior if we look e.g. at clear() or reallocate() vs resize() and friends. (and it is "more" than just an array: it is bloated with a "sorted" bit.) 20:13 sfan5 @herowl core::array is already an std::vector internally. this was never benchmarked when we ripped it out