Times are UTC Toggle Colours
04:20:28 *** Webster has joined #openttd 04:49:52 <DorpsGek> [OpenTTD/OpenTTD] eints-sync[bot] pushed 1 commits to master https://github.com/OpenTTD/OpenTTD/commit/b8f3d0dd68855495f677880087a10f3d7eda08f2 04:49:53 <DorpsGek> - Update: Translations from eints (by translators) 05:09:26 *** keikoz has joined #openttd 06:09:29 *** keikoz has quit IRC 07:02:53 <DorpsGek> [OpenTTD/nml] andythenorth commented on pull request #344: Change: add vehicle 'refit' callback (cb 0x163) https://github.com/OpenTTD/nml/pull/344#issuecomment-2482112288 07:08:18 *** keikoz has joined #openttd 07:17:03 <DorpsGek> [OpenTTD/survey-web] survey-summary[bot] pushed 1 commits to main https://github.com/OpenTTD/survey-web/commit/8fc01de3f82ec95ceeaaf0f041688544f3101ac9 07:17:04 <DorpsGek> - Add: summary for week 46 of 2024 (by OpenTTD Survey) 07:21:30 <DorpsGek> [OpenTTD/OpenTTD] Kuhnovic closed pull request #12275: Codechange: Make the road vehicle path cache behave more like a std::deque https://github.com/OpenTTD/OpenTTD/pull/12275 07:21:33 <DorpsGek> [OpenTTD/OpenTTD] Kuhnovic commented on pull request #12275: Codechange: Make the road vehicle path cache behave more like a std::deque https://github.com/OpenTTD/OpenTTD/pull/12275#issuecomment-2482138937 08:44:35 <peter1138> _glx_: Something like that 08:59:45 <DorpsGek> [OpenTTD/OpenTTD] PeterN merged pull request #12345: Codechange: Replace path cache deques with vectors. https://github.com/OpenTTD/OpenTTD/pull/12345 09:02:56 <Rubidium> was it dinner? ;) 09:05:17 <truebrain> Something that devices a day in smaller sections 09:05:22 <truebrain> like .... "hours"? Does that work 09:05:28 <truebrain> let's make it 1/24th of a day, that feels right 09:05:35 <truebrain> I can count to 12 on each hand, so 24 will be fine 09:05:39 <truebrain> how to call this service .. hmm ... 09:05:41 <truebrain> "a clock"? 09:05:47 <truebrain> sounds like something that would catch on 09:05:50 <truebrain> yeah, let's do that! 09:06:42 <Rubidium> oh, you mean the thing where it's now 17:51? 09:09:03 <LordAro> "I can count to 12 on each hand" 09:09:13 <peter1138> And one value is correct once a day. 09:13:08 <kuhnovic> We need is-it-lunch.openttd.org 09:13:23 <peter1138> Static page that says Yes 09:13:52 <peter1138> andy wants bananas to extra metadata from NewGRFs and export it as a website. 09:14:00 <peter1138> Do you stop at cargo? 09:18:43 <LordAro> peter1138: http://isitlunchtimeyet.com/ 09:19:28 <peter1138> Ok. 09:19:32 <peter1138> Who's reading the API? 09:23:28 <xarick> hi 09:24:18 <Rubidium> peter1138: all the LLMs 09:49:06 <xarick> https://cdn.discordapp.com/attachments/1008473233844097104/1308006061663588402/image.png?ex=673c5f12&is=673b0d92&hm=c908ba2b6098c1dcbf98bd18142b67f86a1683f3347cc15f425384daec364331& 09:49:07 <xarick> peter1138: will you do this? 10:09:54 <xarick> i notice openttd is taking less memory usage on visual studio 10:10:04 <xarick> amazed 10:13:10 <peter1138> What, will I remove that code? 10:13:36 <peter1138> I'm not sure what you're asking tbh 🙂 10:13:39 <xarick> yes, it's slow 10:13:57 <peter1138> As far as I know that code is faster than up to 3 per industry type loops. 10:14:00 <xarick> takes half the time on my tests without it 10:14:22 <peter1138> Hmm, I can try it later. 10:14:47 <peter1138> If anything the threshold is wrong as it's not testing GetNumItems() any more. 10:20:35 <peter1138> It's scanning 29x29 tiles isn't it? 10:21:39 <xarick> yes 10:22:27 <peter1138> It will retest the same industry multiple times if there is more than one industry in each row, though. 10:24:37 <peter1138> Hmm 10:24:51 <peter1138> 2.45s instead of 5.9s 10:25:32 <peter1138> Let's try the big one again. 10:25:47 <peter1138> Time to beat is 3m 48s 10:28:37 <peter1138> 2m 20s 10:28:38 <peter1138> Huh 10:30:16 <peter1138> It felt like it took more time :p 10:30:19 *** keikoz has quit IRC 10:30:49 *** keikoz has joined #openttd 10:31:49 <peter1138> `/* 1. Cheap: Built-in checks on industry level. */` 10:31:53 <peter1138> Heh, not so cheap. 10:32:44 <DorpsGek> [OpenTTD/OpenTTD] felixprigge updated pull request #12683: Fix: Timetable precision https://github.com/OpenTTD/OpenTTD/pull/12683 10:43:07 <xarick> a mix of typelist and kdtree 10:43:24 <xarick> typelist is good for 2 of those functions 10:43:34 <xarick> kdtree for the 2 other 10:43:53 <DorpsGek> [OpenTTD/OpenTTD] PeterN commented on pull request #13094: Codechange: Speed up industry generation using industry-type checks. https://github.com/OpenTTD/OpenTTD/pull/13094#issuecomment-2482655244 10:44:10 <peter1138> Well. 10:44:28 <peter1138> 20.6 to 21s is noise-level margin of error. 10:44:34 <peter1138> But 40% and 60% is not. 10:45:39 <xarick> let me test arctic 10:51:50 <LordAro> unless those two cases are particularly pathological, seems pretty conclusive 10:53:17 <xarick> gonna give up on my town industries pair stuff 10:53:29 <xarick> typelist ends up being better overall 10:54:02 <peter1138> KISS - Keep It Simple, SamuXarick. 10:54:05 <xarick> it only loses in the very specific one on newgrf that wants the layout and town specifics 10:55:04 <peter1138> No, because it's "AI" generated art and therefore inadmissable. 10:55:36 <xarick> oh, it's also winning on FindTownForIndustry 10:55:40 <xarick> but meh... 10:55:51 <xarick> the 2 others counterweight massively 10:55:55 <peter1138> Yeah 10:57:09 <xarick> wait a minute, on second thought... if I were to also use kdtree 10:57:51 <xarick> then ... typelist is not winning 10:57:54 <xarick> hmm funny 10:58:06 <peter1138> Oh yes, I was reviewing this PR. 10:58:30 <peter1138> (Yes, I demand $work contributors use PRs) 10:59:47 <LordAro> of course 11:01:22 <xarick> The 2 functions where `peter's` excels over `townindustries pairs` are `CheckIfFarEnoughFromConflictingIndustry` and `GetClosestIndustry`, but `kdtree` does even better on these. 11:01:54 <xarick> so.... let me get this straight: 11:04:12 <xarick> best for `CountIndustryMatchingTypeAndLayout` - Town Industries pairs 11:04:12 <xarick> best for `CheckIfFarEnoughFromConflictingIndustry` - Kdtree 11:04:12 <xarick> best for `FindTownForIndustry` - Town Industries pairs 11:04:12 <xarick> best for `GetClosestIndustry` - Kdtree 11:12:18 <peter1138> How large are a kdtree structures? 11:12:32 <peter1138> I expected kdtree to be faster, of course. 11:13:16 <xarick> I don't know how to check these 11:13:22 <xarick> how large they are 11:13:49 <xarick> each industry type has a kdtree 11:14:00 <xarick> and there's the global kdtree with all industries 11:14:13 <peter1138> Each kdtree has a vector of nodes, and a vector of indices. 11:14:30 <xarick> 241 trees then 11:14:37 <peter1138> Each node is contains 2 indices, and the element itself 11:15:32 <peter1138> Because it uses size_t, that's going to be 8 + 8 + x bytes, so at least 24 bytes per node. 11:16:07 <peter1138> (I'd be surprised if a kdtree ever gets anywhere close to UINT32_MAX, so those size_ts could probably be changed to uint32_t to save a bit of memory) 11:16:18 <peter1138> (memory and cache) 11:17:01 <xarick> maybe the global kdtree can be eliminated 11:17:31 <xarick> so 240, with a bit of increased overhead on the already massively fast CheckIfFarEnoughFromConflictingIndustry 11:21:55 <peter1138> It's not a huge issue as it's an existing design, not whatever your pair map pair vector pair stuff was 😉 11:22:59 <xarick> [CheckIfFarEnoughFromConflictingIndustryTypeList] 11302556 us [avg: 0.4 us] 11:22:59 <xarick> [CheckIfFarEnoughFromConflictingIndustryKdtree] 162043 us [avg: 0.0 us] 11:23:13 <xarick> like 10 times faster? 11:23:42 <xarick> mine was doing 25 seconds here 11:23:48 <xarick> the vector pair 11:24:14 <exceptik> how long does it take to construct those kdtrees tho? 11:25:00 <xarick> zero idea 😦 11:25:54 <xarick> but I can test the total time for generating industries which accounts everything 11:26:21 <xarick> i think kdtree is only slow on removing 11:27:19 <exceptik> could be a good idea to check if kdtrees are faster in the summary, not a specific function you wanted to speed up, but the whole process from gound up 11:28:11 <peter1138> Okay, using uint32_t in the kdtree reduces nodes in station and town trees from 24 bytes to 12 bytes. 11:29:44 <peter1138> On Wentbourne there are 2263 station nodes, so the kdtree is reduced from 54312 to 27156 bytes. 11:29:54 <peter1138> Not huge but maybe worthwhile. 11:31:07 <peter1138> The viewport sign structure is a bit larger but drops from 32 to 20 bytes. 11:32:54 <peter1138> uint16_t could perhaps overflow, as that's nearer the staiton/town limits. 11:36:01 <peter1138> The main issue is that this is dealing with mostly a map-gen problem. 11:36:12 <peter1138> (Industry creation performance, that is) 11:36:26 <xarick> for stations and towns and industries the x y are the coordinates, they can't go over 4096 11:36:40 <xarick> for viewport however, no idea how that part works 11:36:49 <peter1138> I'm not touching the node itself, only the left/right indices. 11:39:25 <peter1138> exceptik: kdtree has an advantage that it's existing code and it was designed to be faster than scanning things. So it's basically designed for (some of) this use case. 11:45:23 <peter1138> I'm "waiting" for Xarick to discover MakeTownHouse has a ForAllStationsAoundTiles() 11:46:07 <xarick> hmm 😦 11:46:42 <peter1138> (And there's a lot of std::set involved) 11:47:53 <peter1138> Probably the worst part is a house can be removed, which then ends up removing a station from the nearby list, but then the house is replaced immediately in the same processing tick, which ends up adding the station back again. 11:48:21 <peter1138> Anyway, std::sets and map scans galore. 11:52:37 <exceptik> peter1138: yeah, i mean like if it has a drawback of very slow preparation time and then is used frequently then the benefits are higher than drawbacks, but if its only used in a preparation time of something bigger, then the benefits become dubious but still could work 11:53:11 <peter1138> Yeah, this stuff IS used during the game for industry generation as well. 11:53:34 <peter1138> But then you don't really notice it becuase it's doing that a couple of times every game month. 11:53:50 <exceptik> unlike houses, ye 11:53:53 <peter1138> I was concerned about the 20µs time of my industry lists. 11:54:25 <peter1138> And then... when I opened the industry list window, the whole game froze for around 1 second every time few seconds, as it was building the list. 11:54:34 <peter1138> So, uh, perspective here. 11:55:05 <exceptik> can't even really parralelize it :< 11:55:16 <peter1138> So you can speed it up in extreme cases at bit, but realististcally those extreme cases aren't really a usable game anyway. 11:55:30 <peter1138> In which case adding the complexity is not worth it. 11:58:05 <peter1138> The industry list window builds a filtered list of industry IDs, and then sorts it. 11:58:26 <peter1138> When there's 20,000 industries the filtering and sorting is not free. 11:59:16 <_glx_> Hmm for filtered version it could use your new lists 12:00:08 <peter1138> It's filtered by a search string, not by type. 12:00:15 <_glx_> Oh right 12:00:25 <peter1138> Actually you can filter by cargo type too. 12:00:38 <peter1138> But yeah I did look at where else it could be used. That isn't one of them. 12:03:59 *** peter1139 has quit IRC 12:04:31 <peter1138> DrawIndustryNames in smallmap, maybe, but would make it complex. 12:05:40 <xarick> a bit of results 12:05:46 <xarick> https://cdn.discordapp.com/attachments/1008473233844097104/1308040451252817920/image.png?ex=673c7f19&is=673b2d99&hm=5387a7e4fb98308bfb9c0aa56236307e989f2ca52eecb7eb4f0fbfb64dfcbf6f& 12:05:57 <xarick> should I try Kdtree + TypeList? 12:06:54 <peter1138> You didn't include master. 12:07:04 <xarick> oh, seriously? it's gonna slow down 12:07:10 <peter1138> Yes. 12:07:12 <xarick> gonna make me wait 1 hours 12:07:28 <xarick> alright, I'll do master during lunch 12:07:49 <peter1138> Without showing master then there's no indication of how much an improvement each method is. 12:08:53 <peter1138> My longest test was 40 minutes. 12:09:16 <peter1138> It's reasonable to test with "normal" cases as well as ultra extremes. 12:09:51 <peter1138> Those extremes are usually not actually playable games anyway. 12:11:14 *** peter1139 has joined #openttd 12:36:03 <peter1138> Is it lunch? 12:39:58 <peter1138> Clock Badges? 12:41:53 *** keikoz has quit IRC 12:49:36 *** keikoz has joined #openttd 12:55:54 <peter1138> Ketchup is industrial waste, it is banned. 13:23:33 <kuhnovic> Is there a reason why invalid orders aren't automatically removed? 13:24:29 <_glx_> Moving airport 13:24:55 <_glx_> Or moving buoys 13:27:10 <peter1138> Or moving stations. 13:27:53 <peter1138> (And it is easily possible to accidentally remove a whole station) 13:28:25 <kuhnovic> Yeah been there done that 13:28:30 <belajalilija> Yes please don’t delete invalid orders 13:28:35 <peter1138> Also if the invalid orders were removed then you wouldn't know that your orders were maybe not correct. 13:30:10 <kuhnovic> I do see that removed stations get a gray label and stick around for a while until they are fully removed. I guess that's for all the reasons above. 13:30:55 <kuhnovic> I guess it _might_ be safe to delete invalid orders after the station is removed-for-real 13:31:21 <kuhnovic> Pitchforks 13:34:53 <_glx_> once the grey sign is gone it's permanently invalid indeed 13:35:31 <peter1138> What's the purpose if deleting them? 13:35:43 <LordAro> maybe something that differentiates invalid (broken save) vs removed? 13:37:00 <_glx_> though if a station is built on the other side of the map after the grey label is gone, it will "fix" the order 13:42:25 <kuhnovic> I was looking at Wentbourne. It contains lots of vehicles with invalid orders in their order list. Probably from removed stations. 13:42:53 <kuhnovic> (no, I did not remove any buoys yet) 13:45:58 <peter1138> Well, load it in an older version and see if that's due to saveload conversion problem of it is just because that's how it is. 13:46:09 <peter1138> (Or maybe watch the conversion to see) 13:48:33 <kuhnovic> Need to check that 13:58:20 *** Heiki has joined #openttd 14:05:24 <peter1138> 1.7.0, so it's not actually REALLY old. 14:05:42 <peter1138> Oh, there's a 1.7.1 update. 14:07:08 <peter1138> <https://cdn.openttd.org/openttd-releases/1.7.1/> 🙂 14:08:55 <xarick> https://cdn.discordapp.com/attachments/1008473233844097104/1308071445510885396/image.png?ex=673c9bf7&is=673b4a77&hm=a91fd1c9e8e8c73c6ce58facd53f215a1ff9eca8a45b908a6652f1e18ee9b4fb& 14:08:55 <xarick> kek 14:09:14 <peter1138> https://cdn.discordapp.com/attachments/1008473233844097104/1308071523482996806/image.png?ex=673c9c09&is=673b4a89&hm=9d9cb3ebbd9a28c28d2dc7bc5a86731c94c2eda9eec1f165911b7665ea0631f0& 14:10:16 <peter1138> Nothing wrong with savegame conversion then. 14:10:40 <peter1138> Old interface scaling is so janky, I'm ashamed. 14:12:17 <kuhnovic> You shouldn't be, because it's fixed now 🙂 14:12:32 <peter1138> Mostly. 14:14:04 <xarick> I'm bad at peercentages? 14:14:13 <peter1138> Looks like. 14:15:08 <xarick> I followed a guidehttps://math.stackexchange.com/questions/1227389/what-is-the-difference-between-faster-by-factor-and-faster-by-percent/2807461#2807461 14:15:11 <peter1138> For the first one, new takes 12.5% the time old takes. 14:16:58 <peter1138> Anyway 14:17:33 <peter1138> What were the timing with my PR but without kdtree? 14:18:15 <peter1138> Like, it seems logical that you'd compare everywhere so that you can see what bits are improved. 14:18:26 <xarick> do you want with the "fast path" removed or the real PR 14:18:43 <peter1138> Removed I guess, as that'll be pushed later. 14:18:51 <peter1138> I thought you already had the figures. 14:19:19 <xarick> I have, but need to confirm because I'm bad at annotating what I did exactly lol 14:20:13 <xarick> ah, I did with fast path removed 14:20:26 <xarick> let me excel your numbers 14:20:38 <peter1138> Argh, he misunderstood my review and did something else. 14:20:57 <xarick> me? 14:21:20 <peter1138> No 14:26:28 <xarick> need to redo the runs to confirm number of industries generated 14:42:54 <xarick> alright, now I need to know how to calculate the % differences like you do 14:44:47 <peter1138> Your "faster" is the same thing, just 100 - x 14:45:14 <peter1138> `new * 100 / old` 14:52:31 <xarick> https://cdn.discordapp.com/attachments/1008473233844097104/1308082414689587260/image.png?ex=673ca62e&is=673b54ae&hm=717397b5958c7bb3b8eb1c7478bc2849aa20fd052bd9abfb6d901bb0ae68d7e3& 14:54:31 <xarick> need to 100 - x, I think 14:55:45 <xarick> https://cdn.discordapp.com/attachments/1008473233844097104/1308083231647862847/image.png?ex=673ca6f1&is=673b5571&hm=4e7753d4579167a855d905caa37a76618940888232f087a5933ab9f7c016db0e& 15:08:39 *** nielsm has joined #openttd 15:36:25 <peter1138> The "mismatch" is due to the bug in the map scan, right? 15:37:26 <peter1138> It's interesting that none of the NewGRF tests are compelling. 15:39:59 <peter1138> Well, maybe ITI. 15:40:13 <peter1138> 420 -> 103 -> 45 is quite a lot. 15:40:46 <peter1138> 894 -> 110 -> 75 is "ok" 16:07:47 <xarick> https://cdn.discordapp.com/attachments/1008473233844097104/1308101357487788102/image.png?ex=673cb7d2&is=673b6652&hm=db05c9e626bf606c82b9258732ca3bbc1b409e7b2b36230b551cdc7aaf3a465a& 16:07:47 <xarick> another perspective 16:12:44 <kuhnovic> Is typelist the solution that PeterN made? 16:12:54 <xarick> yes 16:13:21 <peter1138> That graph is a good representation. 16:13:21 <xarick> without the AreaSearch 16:13:56 <xarick> Master has the area search, untouched 16:14:25 <peter1138> It really does look like the kdtree approach benefits vanilla industries more than NewGRF industries with extra checks. 16:14:41 <peter1138> But vanilla industries don't take as long to start with. 16:14:50 <kuhnovic> Looks like the TypeList is a very good improvement with little code changes or added complexity 16:16:05 <xarick> hmm but if you then exclude master... you get another perspective 16:16:45 <xarick> https://cdn.discordapp.com/attachments/1008473233844097104/1308103613150789702/image.png?ex=673cb9ec&is=673b686c&hm=326ffa9af3387a702b3b7a5570fda205f6c7eeaa4a350f768c41447fe26a9482& 16:17:35 <peter1138> What's "TownIndustries", keeping a list of IndustryIDs for each town? 16:17:42 <xarick> yes 16:17:47 <xarick> the complex structure 16:18:00 <peter1138> Why is that complex? 16:18:08 <xarick> Because I'm a noob 16:18:44 <peter1138> Have you done something other than including a `std::vector<IndustryID>` in `Town`? 16:19:36 <xarick> https://github.com/OpenTTD/OpenTTD/compare/master...SamuXarick:OpenTTD:industry-type-counts-tracks-towns 16:20:13 <peter1138> Okay, so it's not a list of IndustryIDs for each town. 16:21:25 <kuhnovic> xarick: True, but we already went from "geez this takes forever" to "that's reasonable". There's always a way to make the code faster, but often it's not worth the hassle / complexity / etc 16:21:45 <_glx_> and it's only during map generation 16:22:25 <peter1138> xarick: if you generate the map with all these industries and then open the industry list window, is the game playable? 16:22:43 <peter1138> (Hopefuily you saved one somewhere :)) 16:22:49 <xarick> yes, I didn't touched that 16:23:02 <xarick> so, it should do the same as before 16:23:09 <_glx_> slow industry list window is due to string handling 16:23:50 <_glx_> IIRC 16:23:53 <peter1138> I know why it's slow. 16:24:59 <peter1138> If we shave off an extra 20-30% of map generation time with ITI, does that help make the game playable? 16:25:32 <peter1138> The answer is no, it just means we can more quickly make a game that's going to disappoint. 16:25:43 <_glx_> if it was not playable, it will still be indeed 16:26:23 <_glx_> yeah finding out in 5 minutes only (vs 45 minutes) 16:26:32 <peter1138> That's kinda worthwhile 🙂 16:27:12 <peter1138> It does look like it's the kdtree side that's improving things more than the TownIndustries stuff. 16:27:30 <peter1138> kdtree is at least an existing codepath. 16:27:51 <peter1138> Could be interesting to see memory usage figures with those graphs. 16:28:05 *** Wormnest has joined #openttd 16:28:14 <peter1138> e.g. if it doubles memory usage (it won't!), that's a hard no 16:28:20 <xarick> I have no idea how to check that 16:29:08 <xarick> i tried to make the vectors as large as necessary 16:31:01 <xarick> I'm still looking into kdtree, see if it's possible to tweak it for a bail out on first match kind of thing 16:31:02 <peter1138> I wouldn't worry too much about prospectively allocating enough memory. vector is designed to allocate as needed. 16:31:18 <peter1138> If you always know up front exactly how much you need then sure. 16:31:29 <peter1138> But it's a very minor thing that will likely have no bearing. 16:32:44 <xarick> also it would be cool if i could access the elements in the kdtree 16:32:56 <xarick> but they're x, y only, not really useful 16:33:20 <xarick> or wait... they have to store IndustryIDs somewhere 16:33:25 <peter1138> Isn't not designed for it. 16:33:35 <peter1138> Er 16:33:36 <peter1138> It's not 🙂 16:34:20 <xarick> struct Kdtree_IndustryXYFunc { 16:34:20 <xarick> inline uint16_t operator()(IndustryID iid, int dim) 16:34:25 <xarick> IndustryID 16:34:28 <xarick> it's somewhere 16:36:06 <peter1138> kdtree.hpp:223 suggests it's not. 16:36:25 <peter1138> Well 16:36:29 <peter1138> Actually I dunno 😄 16:37:56 <peter1138> Ah yes, the IndustryID is stored, an the xyfunc gets the coerdinates. 16:38:36 <peter1138> So it does not store X/Y data, that's always resolved by looking up the thing. 16:39:11 <peter1138> Except for viewport signs, they do store it in the element. 16:41:06 <peter1138> Anyway. 16:54:15 <mnhebi> Well, that sounds wildly inefficient. 16:56:44 <peter1138> What, for signs? 16:56:52 <mnhebi> no the not storing the X/Y 16:57:01 <mnhebi> like what functional reason is there not do that? 16:57:13 <peter1138> Well it's stored with the pool item. 16:57:23 <mnhebi> imma store you with the pool cue. 16:59:55 <mnhebi> don't tell me you guys also look up industry production every time instead of caching it 17:00:04 <xarick> kdtree is missing some "this->"'s 17:00:52 <mnhebi> imma expand your cache then deflate it 17:01:29 <peter1138> xarick: Make PR 17:01:54 <xarick> ok 😦 17:05:52 <xarick> question 17:06:01 <xarick> this->INVALID_NODE is this acceptable? 17:06:07 <peter1138> No. 17:06:15 <peter1138> INVALID_NODE is a static constant. 17:06:30 <xarick> but but... uhh., it's part of the class 17:06:32 <xarick> ok 17:06:41 <peter1138> It's not a class member. 17:06:47 <mnhebi> this->segfault sounds like prime move here. 17:11:24 <xarick> statics don't let me this? 17:12:21 <xarick> ah it did 17:17:33 <peter1138> Don't use this-> with static members. 17:17:43 <peter1138> You can do it but you shouldn't. 17:18:19 <peter1138> Is there an emote for regret? 17:18:40 <peter1138> I suggest you don't make a PR after all.. 17:19:02 <mnhebi> :death: 17:19:22 <xarick> static node_distance SelectNearestNodeDistance(const node_distance &a, const node_distance &b) 17:19:22 <xarick> does it quality as a member? 17:19:42 <xarick> line 231 17:20:00 <peter1138> It's a static method, do not use this-> 17:20:07 <xarick> ok 17:20:40 <xarick> line 258 and 268 call that function 17:20:51 <peter1138> Correctly. 17:23:47 <DorpsGek> [OpenTTD/OpenTTD] SamuXarick opened pull request #13095: Codefix: Missing this-> in Kdtree https://github.com/OpenTTD/OpenTTD/pull/13095 17:26:51 <peter1138> With my other PR I think that function can be made static. 17:27:16 <peter1138> Eh, the ManhattanDistance one. 17:27:35 <peter1138> Actually both of them. 17:28:01 <mnhebi> static functions for everyone! 17:29:00 <xarick> no more this? 17:31:19 <xarick> level % 2 vs level & 1 what's faster? 17:31:34 <xarick> or compiler decides what's best? 17:32:39 <mnhebi> can always just asm it and bypass the compiler entirely 17:34:36 <xarick> level is int 17:34:38 <dwfreed> use what you mean 17:34:47 <dwfreed> let the compiler figure out the best way to do it 17:34:48 <xarick> maybe sign issues 17:35:08 *** HerzogDeXtEr has joined #openttd 17:37:23 <xarick> `if (initial_count < 8) return false; // arbitrary value for "not worth rebalancing"` 17:37:23 <xarick> magic number detected 17:43:07 *** gelignite has joined #openttd 17:48:03 *** Wolf01 has joined #openttd 17:51:45 <xarick> interesting, I could pass Industry instead of IndustryID, not sure if that would be faster 17:51:52 <xarick> to the xy functor 17:53:33 <xarick> std::nth_element mysteriously named std 18:13:56 <xarick> I wonder if all those level % 2 could be set in some temporary variable 18:14:08 <xarick> reusable 18:14:23 <DorpsGek> [OpenTTD/OpenTTD] PeterN updated pull request #13094: Codechange: Speed up industry generation using industry-type checks. https://github.com/OpenTTD/OpenTTD/pull/13094 18:14:32 <xarick> wow, we got more updates 18:21:15 <peter1138> Only the removal of the 'fast path' 18:29:25 <xarick> `if (ec < nc) nn.left = newidx; else nn.right = newidx;` incorrect code style! 18:30:39 <DorpsGek> [OpenTTD/OpenTTD] MnHebi commented on pull request #13090: [NewGRF] More options for setting vehicle refit masks https://github.com/OpenTTD/OpenTTD/pull/13090#issuecomment-2483815115 18:42:02 <xarick> is there an advantange in passing size_t as a reference? 18:42:11 <xarick> size_t &next = left_side ? n.left : n.right; 18:43:30 <mnhebi> I mean, passing as reference is always more efficient if you do not need to copy the arguments 18:43:58 *** wallaby2 has joined #openttd 18:44:55 *** wallabra has quit IRC 18:47:36 <_jgr_> It's not always more efficient, for a size_t or similar small POD it likely won't be 18:48:10 <mnhebi> yes there is the 1% which inverts that maxim 18:51:26 <johnfranklin> will snow here 18:57:09 <xarick> DistT ManhattanDistance 18:57:14 <xarick> what is DistT? 18:57:27 <peter1138> * @tparam DistT Type to use for representing distance values. 18:57:32 <mnhebi> damn 18:57:35 <mnhebi> faster than me 18:57:37 <xarick> okay, but what is it? 18:57:43 <xarick> undefined? 18:57:51 <peter1138> It's the type to use for representing distance values. 18:57:51 <xarick> int, uint? 18:59:42 <xarick> abs((DistT)this->xyfunc(element, 0) - (DistT)x), whatever the function returns is then transformed into DistT... but I still don't know if it's signed, unsigned 19:00:02 <xarick> visual studio isn't pointing to anything 19:00:59 <xarick> in a place we have that, in the other we have abs((int)xy[dim] - (int)c)) 19:01:15 <xarick> not sure if a consistency issue... 19:01:42 <xarick> or totally unrelated.. 19:05:41 <mnhebi> I would think in this case DistT is whatever the distance type needs. 19:06:49 <mnhebi> you'd be better served looking at if the manhattan calculation properly clusters and computes distances between the matrixes efficiently using euclidean distance....that is assuming they do all that >_> 19:06:57 <mnhebi> <_< 19:11:13 *** gelignite has quit IRC 19:12:04 *** Wormnest has quit IRC 19:12:51 <peter1138> I think you'd be better off doing that yourself. 19:24:48 <mnhebi> Nah man you don't want me around math 19:25:55 <peter1138> https://cdn.discordapp.com/attachments/1008473233844097104/1308151221755838545/image.png?ex=673ce643&is=673b94c3&hm=bdeda17153f5db87bd8943ece8d60ad578a9c771d5af3df9fa05edd88364bdd1& 19:25:56 <peter1138> I'm not keen on this. 19:26:10 <peter1138> "Disk is OK, one bad sector" 19:26:18 *** Wormnest has joined #openttd 19:28:32 <peter1138> `Read SMART Data failed: scsi error badly formed scsi parameters` 19:29:34 <peter1138> ST3000 sounds like it should be a 3TB Seagate drive. 19:39:02 <xarick> Seagate is bad 19:39:29 <xarick> Seagate of old = good 19:39:40 <LordAro> i'm not sure that's remotely true 19:39:58 <xarick> by old i mean pre-2010 19:42:51 *** Flygon has quit IRC 19:47:54 *** Smedles has quit IRC 19:48:10 *** Smedles has joined #openttd 19:54:22 <peter1138> Well this one is certainly bad. 20:00:43 <xarick> is it faster to pass std::pair by reference? 20:01:05 <xarick> something something in kdtree using std::pair 20:02:13 <_jgr_> A pair is just a struct of two things, so what are the things? 20:02:34 <xarick> using node_distance = std::pair<T, DistT>; 20:02:52 <_jgr_> So what are T and DistT? 20:03:31 <xarick> node::element and a DistT DistanceManhattan most likely 20:04:35 <_jgr_> Look at the code 20:04:44 <peter1138> If you're talking about SelectNearestNodeDistance, it could probably return `const node_distance &`, but it's going to be copied anyway. 20:05:01 <peter1138> I suspect it will end up the same compilation. 20:06:08 <peter1138> Hmm, actually it probalby can't, because one of the parameters, whilst being a reference, is a temporary, so can't be returned as a reference. 20:06:39 <peter1138> So here the copy is needed. 20:07:43 <peter1138> References are good for input parameters, but you need to be careful as return parameters. 20:14:47 <xarick> oh, i found another missing this 20:16:08 <xarick> this->CountValue 20:17:15 <xarick> magic numbers in IsUnbalanced 20:21:23 <DorpsGek> [OpenTTD/OpenTTD] PeterN opened pull request #13096: Codefix: Use correct type for IndustryType in FindTownForIndustry. https://github.com/OpenTTD/OpenTTD/pull/13096 20:23:23 <xarick> a bug? 20:23:41 <xarick> ah, that 20:23:51 <xarick> there's several int's out there that are IndustryType 20:24:13 <xarick> the Check Far Conflict one has int too 20:25:15 <peter1138> https://cdn.discordapp.com/attachments/1008473233844097104/1308166151422869534/GcmfN2ZWUAE-gGD.png?ex=673cf42b&is=673ba2ab&hm=a2092abd62cea5d8051e82b2077f9bb09328473c71360af9f6eb7535cabd1efa& 20:26:53 <peter1138> Hmm, so there is. 20:27:16 <peter1138> It's weird how you saw this, and mentioned it, and I said "make a PR" and... 🦗 20:28:28 <DorpsGek> [OpenTTD/OpenTTD] PeterN updated pull request #13096: Codefix: Use correct type for IndustryType in FindTownForIndustry. https://github.com/OpenTTD/OpenTTD/pull/13096 20:31:20 <xarick> I know nothing about the kdtree but... there is no decreaseunbalanced on Removing an element? 20:31:37 <xarick> they're both increase unbalanced no matter if adding or removing 20:32:09 <peter1138> Removing an element does not balance the tree. 20:39:32 <xarick> this->CheckInvariant 20:39:35 <xarick> this missing 20:43:20 <xarick> people were opposed to my PR's 20:49:13 <xarick> i feel like throwing this code into Copilot and ask him to optimize it lol 20:49:17 <xarick> the kdtree 20:49:23 <peter1138> "him"? 20:49:29 <xarick> him, whatever it 20:52:41 <DorpsGek> [OpenTTD/OpenTTD] PeterN approved pull request #13090: [NewGRF] More options for setting vehicle refit masks https://github.com/OpenTTD/OpenTTD/pull/13090#pullrequestreview-2443710491 20:55:23 *** nielsm has quit IRC 21:04:55 <michi_cc[d]> Oh my, now I have to actually write the docs for it 😎 21:09:58 <xarick> Copilot is looking at Kdtree 21:10:34 <xarick> the entire code fit in the message just shy of the limit 21:10:43 <DorpsGek> [OpenTTD/OpenTTD] michicc merged pull request #13090: [NewGRF] More options for setting vehicle refit masks https://github.com/OpenTTD/OpenTTD/pull/13090 21:14:08 <xarick> well, copilot bugged out, couldn't write his entire response 21:16:29 <xarick> the part that i got he's doing some std::move on the element 21:16:52 <xarick> ` node(T element) : element(std::move(element)), left(INVALID_NODE), right(INVALID_NODE) { }` 21:16:58 <xarick> like that 21:17:10 <peter1138> That is not valid. 21:18:43 *** SigHunter has quit IRC 21:19:28 <_jgr_> How is it not valid? 21:19:50 <_jgr_> It would make sense if T was expensive to copy (which it isn't) 21:20:15 <xarick> maybe for the viewport struct 21:20:23 <xarick> class or somth 21:20:47 <xarick> he's also keen on consting T 21:20:53 <xarick> across the code 21:21:44 *** SigHunter has joined #openttd 21:22:12 <peter1138> Wouldn't it need `T && element`? 21:23:33 <xarick> Copilot replaced this line: 21:23:33 <xarick> `typename std::vector<T>::iterator removed = std::remove(elements.begin(), elements.end(), *exclude_element);` 21:23:33 <xarick> with: 21:23:33 <xarick> `auto removed = std::remove(elements.begin(), elements.end(), *exclude_element);` 21:23:39 <peter1138> Hmm, apparently not. 21:25:33 <michi_cc[d]> Don't forget for NML that the callback now needs a callback flag (flag 9). 21:25:47 <peter1138> I've got to change all my badge properties now. 21:25:50 <xarick> oh wow, copilot found a duplicated variable... 21:26:03 <peter1138> Should've used constants 😄 21:26:34 <peter1138> Property labels? 21:28:32 <_glx_> xarick: both are exactly the same, except with `auto` you don't have to write the full type 21:31:25 <xarick> maybe they're not duplicate variables... hmm unsure, need to ask a real human 🙂 21:32:51 *** HerzogDeXtEr has quit IRC 21:32:59 <xarick> nevermind, the comment says it all 21:33:10 <xarick> copilot failed 21:33:33 <xarick> node &n = this->nodes[node_idx]; 21:33:34 <xarick> node &nn = this->nodes[node_idx]; 21:33:38 <xarick> and he removed one of them 21:34:45 <xarick> so basically, some std::move, some const and an auto 21:37:08 <_glx_> would be better if you actually understood what it did 😉 21:43:00 <kuhnovic> Letting Xarick play around with copilot is like letting a kid play with a sharp knife 😛 21:44:02 <xarick> he said something about inlined functions but I don't see any change about it 21:45:02 <xarick> ah, I see something 21:45:25 <xarick> bool IsUnbalanced() 21:45:25 <xarick> becomes 21:45:25 <xarick> bool IsUnbalanced() const 21:45:49 <_glx_> makes sense, but not vital 21:46:14 <_glx_> of course you need to know the meaning of postfix const 21:46:38 <truebrain> a const that comes after! 21:50:07 <michi_cc[d]> Please let me know if anything on <https://newgrf-specs.tt-wiki.net/wiki/Action0/Vehicles/Trains#Cargo_classes_.2828.2C_29.2C_32.29> or <https://newgrf-specs.tt-wiki.net/wiki/Callback:_Custom_refit> looks iffy. 21:50:57 <_glx_> just never use 0x1D 🙂 21:51:39 <peter1138> Yeah, it's been marked obsolete for years. 21:51:43 <peter1138> *a few days 21:52:52 <peter1138> It's an index into the cargo translation table. 21:53:11 <_glx_> uses less space than labels 21:53:25 <michi_cc[d]> andythenorth: It is so you can have a "reserved for future use" spot 🙂 21:53:54 <peter1138> Just don't have 255 labels listed. 21:54:38 <peter1138> Speaking of which, variable 18 bits 0..7 will have the value FF if the cargo isn't present in the translation table. 21:54:48 <peter1138> (For callback 163) 21:54:54 <michi_cc[d]> Good point, I'll add that. 21:55:29 <_glx_> ignore 1D and replace with 2C/2D 21:56:16 <_glx_> https://cdn.discordapp.com/attachments/1008473233844097104/1308189058018709504/image.png?ex=673d0980&is=673bb800&hm=14aaef3ddafc8ca674220d591bcb2653b4d1f7c499fe0ebae4410bf137d1cc6e& 21:56:52 <_glx_> it's in the 28/29/32 block 21:58:18 <xarick> static_cast<DistT>, does this make sense? 22:03:30 <xarick> another case where Copilot may be failing, in FindNearestRecursive 22:15:07 <xarick> I'm afraid to update the PR, as it was mostly suggested by Copilot 22:16:11 <xarick> I'm creating a temp branch so you can take a look 22:17:51 <xarick> https://github.com/SamuXarick/OpenTTD/compare/codefixing-kdtree-by-copilot 22:25:41 *** keikoz has quit IRC 22:26:57 <xarick> where should I expect gains with those std::moves? 22:27:13 <xarick> massing signs? 22:44:46 <peter1138> As the proposal said, exclude... 23:10:34 <michi_cc[d]> You could still use them as a filter *IF* you can accept that certain waggons might not be available if you use certain industry sets. You can't use them as a filter if you'd consider that worse than the sun going nova or something 🙂 23:14:02 *** Wolf01 has quit IRC