IRC logs for #openttd.dev on OFTC at 2013-10-20
⏴ go to previous day
00:44:16 *** adf88 has joined #openttd.dev
00:44:16 *** ChanServ sets mode: +v adf88
08:23:08 *** zydeco has joined #openttd.dev
08:26:27 *** Alberth has joined #openttd.dev
08:26:27 *** ChanServ sets mode: +v Alberth
08:34:38 *** Ristovski has joined #openttd.dev
08:53:40 *** LordAro has joined #openttd.dev
08:53:40 *** ChanServ sets mode: +v LordAro
09:05:43 <Alberth> lol, head-2-head freerct? :)
09:06:20 <LordAro> just random ideas from my head :)
09:06:32 <LordAro> (sure you're in the right channel?)
09:07:05 <Alberth> hmm, good point, I am in the right window only :p
09:14:05 *** adf88 has joined #openttd.dev
09:14:05 *** ChanServ sets mode: +v adf88
09:46:15 <fonsinchen> I'd like to do something about FS#5671 today
09:46:31 <fonsinchen> I have two alternative solutions available:
09:47:47 <fonsinchen> Either only up to "use smallstack to allow for multiple next hops"
09:47:51 <fonsinchen> or the whole thing
09:48:00 <fonsinchen> I'm in favour of the simpler solution
09:48:38 <fonsinchen> It uses a pool for storing the elements of the stack and thus is not as nice for locality.
09:49:32 <fonsinchen> However, as the stack is currently only used in a relatively obscure case this shouldn't hurt
09:50:32 <fonsinchen> If someone comes up with more uses for such SmallStacks we still may rethink that decision.
10:04:12 <Alberth> refactor pool to make FindFirstFree and ResizeFor... 585e44e672979a824de9db17700 (not sure how to indicate these patches)
10:04:12 <Alberth> core/pool_type.hpp line 78 +struct Pool : PoolBase { <-- adds a second space before the "{"
10:06:18 <Alberth> I cannot say much about the contents itself; it looks ok to me
10:06:56 *** zooks has joined #openttd.dev
10:07:46 <Alberth> Given that you asked yesterday as well, apparently no dev has anything relevant to say
10:38:55 *** ntoskrnl has joined #openttd.dev
10:48:18 *** frosch123 has joined #openttd.dev
10:48:18 *** ChanServ sets mode: +v frosch123
12:50:12 *** adf88 has joined #openttd.dev
12:50:12 *** ChanServ sets mode: +v adf88
12:50:25 <fonsinchen> pool_func.hpp:197 says that delete'ing NULL PoolItems is valid
12:50:54 <planetmaker> afaik according to c specs
12:51:19 <fonsinchen> However, pool_type.hpp:164ff dereferences pointers passed to operator delete()
12:51:50 <fonsinchen> I wonder why I'm the first one to hit this ...
12:52:22 <planetmaker> maybe you found the reason for one or few of the crashes in our bug tracker?
12:52:23 <fonsinchen> The spec probably assumes the default operator delete()
12:52:39 <frosch123> you usually access a pool item via the Get or GetIfValid methods
12:52:46 <frosch123> the former asserts if the item is invalid
12:52:54 <frosch123> so, you only ever get NULL from GetIfValid
12:53:01 <frosch123> and usually that is checked immediately
12:53:21 <fonsinchen> In my case it crashes in CleanPool
12:53:36 <frosch123> (my "most likely" refers to the "you are the first one to tirgger it")
12:53:36 <fonsinchen> That is, there is a "hole" in the pool, which should be common.
12:54:07 <fonsinchen> I can just add a check for NULL to operator delete
12:54:18 <frosch123> irrc there is some _cleaning_pool variable which skips deleteing of stuff which is deleted by other means
12:55:05 <fonsinchen> It's CleanPool itself which triggers it and the hole has been there before
12:55:35 <fonsinchen> Such a hole is created whenever you delete a PoolItem
12:55:54 <fonsinchen> (except for the last one in the pool)
12:58:05 <frosch123> CleanPool does not use PoolItem::delete
12:58:33 <fonsinchen> It does "delete this->Get(i)"
12:58:35 <frosch123> it uses TItem::delete
12:58:49 <frosch123> hmm, maybe that's the same
12:59:51 <fonsinchen> Titem should always be a PoolItem, shouldn't it?
13:00:02 <frosch123> that should be triggered all the time, indeed
13:01:58 <adf88> @planetmaker there has one thing left to correct in the orders window
13:02:19 <adf88> www.dropbox.com/sh/j0whg8ho4uwm20s/hVzyv4cKOE
13:02:38 <adf88> it was about to unify different goto tools
13:03:29 <adf88> as the "normal" goto works little different then others when selecting from the dropdown
13:08:06 <fonsinchen> Probably CleanPool is usually called when the program terminates and nobody cares if it crashes then. But still, very weird
13:08:28 <frosch123> it's also called when returning to main screen
13:08:30 <frosch123> or when loading a game
13:08:37 <frosch123> in fact when starting ottd, and loading the title game
13:08:45 <planetmaker> indeed, adf88, let's see
13:09:18 <planetmaker> hm, why don't they have a .patch or .diff file extension :D
13:10:01 <adf88> sorry, it is kind of discrimination
13:10:23 <adf88> if you know what I mean
13:10:57 <fonsinchen> Could also be that holes in pools are not as common as we think. I'll do some testing.
13:11:55 <adf88> discrimination of OS'es that are basing file types on their extensions ;)
13:13:51 <Alberth> not to mention web-browsers :)
13:13:52 <planetmaker> oh. dropbox knows the extension and then allows syntax highlighting in the browser :-)
13:14:19 <adf88> I'm working currently on a house picker
13:14:25 <Alberth> indeed, something to remember for the next time
13:14:45 <adf88> to build custom houses at custom locations
13:15:06 <fonsinchen> No, there are tons of holes
13:15:49 <frosch123> fonsinchen: my debug output says it does not call the delete operator when it is NULL
13:15:51 <michi_cc> Especially the proposed (and probably also adopted) resolution for section 5.3.5: "Otherwise, it is unspecified whether the deallocation function will be called."
13:16:07 <michi_cc> unspecified == bad :)
13:16:37 <michi_cc> Just add a "if (v == NULL) return;", I can'
13:17:03 <michi_cc> t imagine any reason what would do any harm.
13:17:13 <fonsinchen> That will be done anyway.
13:17:43 <fonsinchen> I'd like to get to the core of this, though. Even if it's only my compiler, I should have hit this problem about 1000 times before ...
13:18:34 <adf88> it's everything OK with handling house placement, new CmdBuildHouse command works like a charm
13:18:34 <adf88> but I found another thing problematic - house drawing a house in the GUI
13:18:49 <adf88> I'm curius, do you think that letting to some houses
13:19:01 <adf88> to be drawn with gliches is an acceptable solution
13:19:23 <planetmaker> why should they glitch?
13:19:27 <adf88> I mean until they get updated to new changes (if that ever happens)
13:19:32 <fonsinchen> Maybe my compiler decides to do this only for PooledSmallStack, not for other types. The reason could be that PooledSmallStack doesn't have its own destructor.
13:20:14 <adf88> most likely because GRF protocol is not fully prepared
13:20:20 <adf88> to drawinf houses in the gui
13:20:39 <adf88> i think I will be able to fix this on the OTTD internals level
13:20:40 <frosch123> my compile does not call operator delete for NULL
13:20:48 <adf88> keeping full compatibility
13:21:13 <adf88> but who knows what crazy assumptions some could make when creating GRFS
13:21:20 <frosch123> adf88: planetmaker: you will obviously run into trouble if the look of the house depends on the town zone and stuff
13:21:24 <frosch123> you have no "location" the gui
13:21:44 <frosch123> you can only return some default values for those
13:21:52 <adf88> i'm chenking which house zones are available for a certain tile
13:21:57 <adf88> and choosing first available
13:22:16 <adf88> sorry - for a certain house not tile
13:23:04 <frosch123> adf88: i mean HouseScopeResolver::GetVariable needs a tile for most variables
13:23:16 <adf88> basically the main idea is to provide "dummy" vales
13:23:37 <frosch123> yup, that's also what we do for vehicles in the purchase list
13:23:39 <adf88> it's partially done, but not for variables
13:24:09 <adf88> there is not_yet_constructed bool
13:24:32 <adf88> to tell that the house does not exist
13:24:34 <frosch123> yes, but it still needs a "tile"
13:24:53 <frosch123> not_yet_constructed only makes sure that it does not try to read random bits from the map array
13:25:04 <frosch123> but tile is still used to get nearest town/distance and stuff
13:25:05 <adf88> i'm trying to add new category
13:25:14 <adf88> similar to not_yet_constructed
13:25:29 <frosch123> i think you can also just add a new Reolver these days
13:25:39 <adf88> but this new category will not require
13:25:45 <adf88> to point a tile or a town
13:25:48 <frosch123> these "bool variables" are the old way to do stuff :p
13:26:57 <planetmaker> adf88, I think I'll commit 40 and 41 later today unless s/o objects. 40 would need some alignment of the doxygen in the last hunk, that's no issue, though. Just got a call. Will be back later.
13:27:49 *** zooks has joined #openttd.dev
13:30:17 <adf88> this is a work in progress, you can take a look if you wish to know what I mean
13:30:54 <adf88> it's to allow resolving houses in the GUI (tile-less, town-less)
13:32:32 <frosch123> adf88: this adds switch-cases to all methods of HouseScopeResolver though. instead there could be multiple derived classes from ScopeResolver
13:32:51 <frosch123> i.e. PreviewHouseScopeResolver, and ConstructionHouseScopeResolver or so
13:33:15 <fonsinchen> In fact it depends on the class having a destructor. I'll just add the check for NULL and be done with it now.
13:36:28 <frosch123> i am not sure how much they would share
13:37:04 <frosch123> i.e. whether one of them could be derived from another one
13:37:26 <adf88> but for now i'll stick on switch'es and see what happens
13:37:57 <frosch123> true :) first checking whether it works at all can pay off :)
13:40:51 <fonsinchen> I think we should do CeilDiv in vehicle.cpp:1332
13:41:16 <fonsinchen> Usually for very small amounts of cargo you rather want 1% than 0%, so that conditions based on that work correctly
13:41:57 <frosch123> you can say the same about full load :p
13:42:04 <frosch123> but well, i never used conditional orders
13:42:48 <frosch123> you could use CeilDiv for < 50% :p
13:44:48 <fonsinchen> Probably percentage is not the right measure there
13:53:29 <fonsinchen> LordAro: that's a lot. Any preference about what to look at first?
13:53:45 <LordAro> the ones with the small numbers :p
13:53:59 <LordAro> 007 and 009 got skipped because of concerns, iirc
13:54:30 <LordAro> have fun discussing :)
14:08:43 <frosch123> hmm, pm fell for the same issue with randomRange some days ago
14:08:52 <frosch123> maybe the @param is misleading in that function
14:12:12 <fonsinchen> I can fix that, too, while I'm at it
14:13:03 <fonsinchen> Actually RandomRange doesn't have any documentation
14:13:31 <frosch123> even worse :) the param says "max" only
14:13:42 <frosch123> maybe "count" would be more appropiate
14:17:06 <fonsinchen> We're not actually counting anything there
14:17:19 <frosch123> it's the number of cases
14:19:23 <fonsinchen> I like limit more. It's the limit of the range to be drawn from
14:19:36 <fonsinchen> We're not usually talking about cases in that context.
14:23:28 <fonsinchen> Otherwise the patch is OK I assume?
14:29:43 <fonsinchen> My take on documenting RandomRange and friends
14:35:04 <Alberth> you can sprinkle some \a and/or \c in them
14:35:38 <Alberth> \a limit makes it clear you mean the argument 'limit'
14:36:32 <Alberth> \c <expression without spaces> switches to a fixed-width font
14:39:49 <fonsinchen> Some \a but no \c as I find the argument identification more appealing
14:40:36 <fonsinchen> Should I fix FS#5677 ot FS#5684 next?
14:44:06 <Alberth> patch looks fine to me
14:46:46 <fonsinchen> I'm actually not sure how many people read the generated doxygen as opposed to the comments in the code
14:47:13 <fonsinchen> When you read the comments those primitives are not so helpful.
14:47:25 <fonsinchen> They make the text harder to read.
14:47:39 <Alberth> yeah, they are quite subtle
14:49:21 <Alberth> While I have a local copy of generated docs that I use whenever I need to find some item, I also agree that having the documentation right there in the source code is more important than the ability to generate html
14:50:28 <Alberth> on the other hand, the number of primitives that's used is quite small.
14:51:26 <Alberth> that table also contains the @param/@return etc etc stuff, which not used in-line.
14:51:50 <fonsinchen> Those are totally helpful, even when reading the code
14:52:10 <fonsinchen> We just shouldn't increase usage of inline primitives too much.
14:52:24 <fonsinchen> \me will get some food before deciding on what to do next
15:45:59 *** Supercheese has joined #openttd.dev
16:09:13 *** Ristovski has joined #openttd.dev
16:11:12 *** Ristovski has joined #openttd.dev
16:13:54 *** Ristovski has joined #openttd.dev
16:21:14 <fonsinchen> I'd like to revert r25495, close FS#5684 and reopen and subsequently close as "wontfix" FS#5553.
16:21:40 <fonsinchen> Changing the station while the train is arriving is probably a case we don't really have to support.
16:22:01 <fonsinchen> However, a train stopping twice in a row at the same station without moving is clearly a bug.
16:22:58 <fonsinchen> It's hard to fix both cases as there is no easy way to tell if the train has left the station after stopping last time.
16:23:48 <fonsinchen> So if we allow a train to stop at a different position than originally intended we can't easily prevent the "stopping twice" behaviour.
16:25:47 <fonsinchen> wait, we can check for VS_TRAIN_SLOWING ...
16:28:28 <fonsinchen> It could slow because of line end, though
16:29:20 <Rubidium> check last station visited?
16:32:41 <fonsinchen> That may be the same
16:33:10 <fonsinchen> The vehicle may intentionally stop twice at the same station, but it should not do so without moving.
16:57:40 <fonsinchen> However, you're sort of right: if it's passed the point where it should have stopped and has the same station as last_visited we're not in the realm of FS#5553 anymore, except if it's actually stopping twice at the same station on purpose.
16:57:50 <fonsinchen> The thing is not getting less complicated, I fear.
18:56:06 *** Alberth has left #openttd.dev
18:59:34 * Rubidium wonders what the use case is for stopping twice at the same station without an order in between
19:08:35 *** Lord_Aro has joined #openttd.dev
19:10:21 *** LordAro is now known as Guest2949
19:10:21 *** Lord_Aro is now known as LordAro
19:11:40 *** ChanServ sets mode: +v LordAro
19:18:54 <fonsinchen> You make one order and have the vehicle go a huge tour before returning to the same station
19:19:33 <fonsinchen> As implicit orders are truncated if you reach the station after the current block of implicit orders this becomes a problem.
19:33:42 <fonsinchen> Also constructs like unload->implicitly go to depop->load don't work anymore like that
19:50:38 <frosch123> doesn't that only apply to OPOS_GOTO anyway?
19:50:53 <frosch123> i think canceling is fine
19:51:32 <frosch123> but do you tell me, that if i click share orders and select another vehicle, that share order stays active?
19:51:51 <frosch123> what would be the point of that?
19:52:33 <planetmaker> it applies to goto and to shared orders
19:52:57 <frosch123> well, but does "shared orderes" stays lowered after sharing?
19:53:21 <planetmaker> but not staying active after you used it. But after you re-considered your choice of what order type to give
19:53:36 <frosch123> ok, then canceling is the right thing
19:53:50 <planetmaker> shared orders staying active after being used makes little sense to me
19:54:04 <frosch123> exactly :) that's why i was asking
19:54:06 <planetmaker> goto staying active after being used depends on our adv. setting
19:54:29 <planetmaker> this is only about whether the order-giving-mode stays active when re-selecting the same action or not
19:54:47 <planetmaker> I actually prefer to NOT apply the patch I linked :D
19:55:01 <frosch123> i think it makes sense
19:55:35 <frosch123> but might be a rare case anyway
19:55:38 <planetmaker> the goto-tools become inactive when I clicke the button. But when I specifically select one of the items in the drop-down I don't want it to become inactive. Even when I select the same as I did before
19:55:59 <planetmaker> (and that's what the patch is about only. Yeah, very rare case :-) )
19:56:18 <frosch123> hmm, yeah, one would probably just click the button to deactivate
19:56:23 <frosch123> instead of using the dropdown
19:56:39 <planetmaker> adf88 likes it. I don't so much, but... I won't bitch, if most prefer and would commit it then :-)
19:56:44 <planetmaker> I could ask forums :D
19:57:10 <frosch123> would need screenshots, else noone will likely get it from just textual description
19:57:27 <planetmaker> yes. much work for... very little :-)
19:58:04 <planetmaker> I guess I'll just skip it then :-)
20:04:11 <planetmaker> LordAro, do more patches from that queue exist? I've the feeling some only really make sense in some larger context. They're a code change one can do, yes. But... need to?
20:06:57 *** tycoondemon has joined #openttd.dev
20:11:49 <frosch123> planetmaker: ofc there are more patches
20:12:08 <planetmaker> I know. But not exactly where
20:12:19 <frosch123> well, they won't help you to judge a single diff
20:12:44 <frosch123> you can only really use those which make sense on their own
20:13:01 <planetmaker> :-) I planned to do just that
20:13:03 <frosch123> i think code deuplications and comments make most sense to commit
20:13:20 <frosch123> but "optimisations" may not
20:13:28 <planetmaker> there are lots sof ^
20:13:36 <planetmaker> which I'm not sure about
20:14:26 <frosch123> originally i thought lordaro wanted to filter them out
20:16:55 <planetmaker> I think the idea originally was to go through all patches. But yes, filtering out the non-obvious optimizations for now might be good. Or putting them separate
20:24:15 <LordAro> i could go through all of them, but since the whole queue is ~ 550 patches, i'd rather not if there was no chance of a merge ;)
20:24:32 <LordAro> i'll add the full patch (single file with patches contained) to the folder
20:25:44 <LordAro> wait, it's already there, under raw.patch
20:25:51 <planetmaker> well... would be nice, if you could do some kind of pre-filtering for the possibly useful stuff :-)
20:25:56 <planetmaker> raw.patch is one big patch, no?
20:26:33 <LordAro> raw-241.patch is the first 241 patches in the queue, i determined that after that the difference between codechanges and new features gets 'muddy'
20:26:41 <LordAro> raw.patch has everything in it
20:27:10 <LordAro> nothing wrong with refactoring to make it look better :)
20:32:07 <frosch123> yeah, refinery should have the same, but it's not in that patch
20:32:37 <frosch123> hmm, ... i guess refinery is not direction dependent though
20:32:55 <frosch123> yeah, quite unlikely then to be needed a second time
20:34:39 <planetmaker> though I never encountered a global 'error' variable so far :-)
20:34:50 <LordAro> planetmaker: it gets used again in 019 and umm, 289, 347 and some other later patches
20:34:59 <frosch123> it would make sense if there was an actual assert in that diff
20:35:24 <planetmaker> which? 013 or 017?
20:36:42 <LordAro> lots of detailed text about what it's doing
20:37:20 <planetmaker> yes, the comment actually is nicely elaborate. I quite appreciate that
20:38:45 <planetmaker> but still it means I first have to understand the PF code to actually judge it :-)
20:47:55 <planetmaker> omg, you also only have this one huge file which is a list of concatenated patches, LordAro ? :-(
20:49:00 <LordAro> that was how cirdan presented his patch queue, yes :/
20:51:35 <LordAro> i probably could split them with a script though :L
20:51:45 <LordAro> not actually review them though
20:53:42 <planetmaker> well. would be nice, if you could sort them by some categories :-) But yes
20:54:11 <planetmaker> so basically cirdan bitches about 'no-one reviews' but makes it a hell to actually look at his stuff? nice
20:54:30 <LordAro> he did originally post a 'proper' patch queue, iirc
20:54:39 <LordAro> but he removed it when he updated the patch
20:55:05 <LordAro> and to up fair, the 'update airports' patch has been sitting in flyspray for years without a proper review
21:01:30 <planetmaker> one can always find a reason ;-)
21:02:14 <LordAro> well, if one of my patches had gone unreviewed in ~5 years, i'd be a bit annoyed too :p
21:05:15 <frosch123> it's fine to blame that on us, and to fork. but it's no reason to not give a queue to others, which he has anyway
21:05:33 <frosch123> you do not need to respect the devs, but you should respect the project
21:05:41 <frosch123> that's the point of open source
21:05:51 <frosch123> it's about the software, not about the people doing it
21:06:16 <LordAro> :3 that's very deep :p
21:20:44 <LordAro> planetmaker: so, what's your assessment of the things? :L
21:46:16 *** helpmeplease has joined #openttd.dev
21:48:51 <planetmaker> well, as said, some interesting stuff. Others I'm unsure of. And yet again others I'd not do. Some saveload refactoring might be interesting as seen later
21:49:13 <planetmaker> though I'd also like to know more on the purpose of some of the patches... alas.
21:49:42 <LordAro> and you could always ask cirdan, he is mostly active ;)
continue to next day ⏵