| Time |
Nick |
Message |
| 00:05 |
|
Eragon joined #minetest-dev |
| 03:52 |
|
TheCoffeMaker joined #minetest-dev |
| 03:54 |
|
TheCoffeMaker joined #minetest-dev |
| 03:57 |
|
TheCoffeMaker joined #minetest-dev |
| 04:14 |
|
TheCoffeMaker joined #minetest-dev |
| 05:00 |
|
MTDiscord joined #minetest-dev |
| 05:43 |
|
SFENCE joined #minetest-dev |
| 07:48 |
|
wrrrzr joined #minetest-dev |
| 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:46 |
|
Warr1024 joined #minetest-dev |
| 09:47 |
wrrrzr |
Hello |
| 09:47 |
MTDiscord |
<rollerozxa> hello |
| 09:47 |
wrrrzr |
Do I need to extract classes from files so that there is only one class? |
| 09:47 |
MTDiscord |
<rollerozxa> well this isn't java so you are free to do your own judgement on that I believe |
| 09:48 |
MTDiscord |
<rollerozxa> 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 |
? |
| 10:21 |
|
ivanbu joined #minetest-dev |
| 10:46 |
|
wrrrzr left #minetest-dev |
| 10:53 |
|
hwpplayer1 joined #minetest-dev |
| 11:05 |
|
wrrrzr joined #minetest-dev |
| 11:07 |
|
wrrrzr left #minetest-dev |
| 11:10 |
sfan5 |
I quickly looked at chat.cpp and in my opinion it doesn't need to be split |
| 11:49 |
|
wrrrzr joined #minetest-dev |
| 11:49 |
wrrrzr |
So i dont send PR? |
| 11:50 |
|
wrrrzr left #minetest-dev |
| 12:06 |
|
hwpplayer1 joined #minetest-dev |
| 12:17 |
|
wrrrzr joined #minetest-dev |
| 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 |
<rollerozxa> 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 |
| 13:23 |
|
wrrrzr left #minetest-dev |
| 13:26 |
|
panwolfram joined #minetest-dev |
| 13:54 |
|
panwolfram joined #minetest-dev |
| 14:25 |
MTDiscord |
<josiah_wi> 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 |
<josiah_wi> Moving out small helper classes to other files will have almost no impact on the size of the file. |
| 14:26 |
MTDiscord |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> And it would be very easy to make it worse than it is. |
| 14:37 |
MTDiscord |
<josiah_wi> It might be possible to just do general cleanup on the existing implementation, and that might generate ideas for further improvement. |
| 14:38 |
|
Desour joined #minetest-dev |
| 14:38 |
MTDiscord |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> 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 |
<josiah_wi> 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 |
<luatic> 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 |
<josiah_wi> 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 |
<luatic> 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 |
<luatic> and media-related stuff. |
| 14:54 |
MTDiscord |
<luatic> 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 |
<josiah_wi> 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 |
<luatic> 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 |
<josiah_wi> 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] |
<Zughy> 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] |
<Zughy> 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] |
<Zughy> perfect, ty |
| 16:53 |
MTDiscord |
<luatic> 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:09 |
|
hwpplayer1 joined #minetest-dev |
| 17:12 |
MTDiscord |
<wsor4035> did we ban the account from the org to prevent more? |
| 18:47 |
|
wrrrzr joined #minetest-dev |
| 18:48 |
wrrrzr |
Maybe replace MutexedVariable to std::atomic ? |
| 18:49 |
sfan5 |
I don't think std::atomic<std::string> 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:00 |
|
wrrrzr left #minetest-dev |
| 19:37 |
MTDiscord |
<herowl> https://cdn.discordapp.com/attachments/747163566800633906/1327723044155949131/image0.png?ex=678419f1&is=6782c871&hm=31562002cb17551367d64828a96ad82b4093ab67028e80a4cca364698931f450& |
| 19:40 |
MTDiscord |
<luatic> 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 |
<luatic> if we do enough of that we can eventually get rid of core::array. |
| 19:46 |
MTDiscord |
<herowl> How bad is core::array? |
| 19:46 |
MTDiscord |
<herowl> How many times a custom allocator for std::vector may be needed? |
| 19:49 |
MTDiscord |
<luatic> 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 |
<luatic> core::array is annoying because it lacks many basic modern C++ vector features and overall integration with the C++ standard library. |
| 19:54 |
MTDiscord |
<luatic> 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 |
| 21:04 |
|
panwolfram joined #minetest-dev |
| 23:33 |
|
panwolfram joined #minetest-dev |
