Log for on 13th September 2013:
Times are UTC Toggle Colours
00:57:47  *** LordAro has quit IRC
07:55:15  *** tycoondemon has quit IRC
08:29:33  *** tycoondemon has joined
09:46:42  *** ntoskrnl has joined
10:23:07  *** LordAro has joined
10:23:07  *** ChanServ sets mode: +v LordAro
12:41:16  *** Zuu has joined
12:41:16  *** ChanServ sets mode: +v Zuu
14:24:24  *** TheIJ has joined
14:30:17  <Zuu> Fix to FS#5617:
14:30:33  <Zuu> The FS task:
14:31:37  <Zuu> Perhaps assert(line_height > 0) at both locations in the patch instead of "return 1" in one case and assert in the other?
15:14:05  *** frosch123 has joined
15:14:05  *** ChanServ sets mode: +v frosch123
15:14:14  *** Alberth has joined
15:14:14  *** ChanServ sets mode: +v Alberth
15:29:29  <frosch123> +				return height / line_height + (rest > 0 ? 1 : 0); <- you can replace that line and the one before by using CeilDiv(height, line_height)
15:30:53  <frosch123> i wonder whehter it still makes sense to scroll in "lines"
15:31:03  <frosch123> or whether it should rather scroll in "pixels"
15:34:25  <Alberth> for text, pixels may be useful. For non-text (eg lists of trains or engines or so), 'line' seems best to me
16:15:45  <frosch123> <- i would just assume that that fixes the musa windows issue
16:25:27  <Zuu> Hmm, I vaguely remember having a similar issue in some other part of musa in windows where opening the file in binary mode solved the issue.
16:33:14  <frosch123> i reviewed all "open"
16:33:56  <frosch123> there are already some 'rb' for md5sum computation
16:51:26  <Zuu> height % line_height > 0 ? line_height - (height % line_height) : 0
16:51:27  <Zuu> or
16:51:28  <Zuu> (line_height - (height % line_height)) % line_height
16:53:17  <Zuu> I find the first one a bit more easy to read altohugh it is longer.
16:53:54  <frosch123> yup, first one
16:54:08  <frosch123> unless you want to do some +1/-1 magic :)
16:58:06  <Zuu> Patch updated. I use CeilDiv as suggested by frosch123. I've also fixed the case when a text uses only the normal font and thus don't need any extra padding.
16:59:10  <Zuu> The title text remain unfixed, but the title text is also used in the drop down which doesn't seem to like big text.
18:26:43  <Zuu> Beyond this fix I agree to change to scroll by pixel instead of by lines. After that I'll look into detecting if content change height (eg a {TOWN} that get renamed) and invalidate/update the scroll.
18:29:50  <Zuu> In a by-pixel solution, the padding of images and text to multiples of line heights will be removed. Each element will only get separated by a space of one line height (normal font). Or possible even 2/3 of a line height.
18:31:07  <Zuu> Hmm 2/3 is a stupid idea, then a Text block with paragrapths made using {}{} will have a different distance than paragraphs made using multiple text elements.
18:51:41  *** Supercheese has joined
19:13:22  *** ntoskrnl has quit IRC
19:36:07  <Zuu> I realized that there is no good reason to first fix the line based height method and then scrap most of the added code when going to pixel based heights. So I've made a patch that switch to pixel based heights and as a consequence fixes the same bug:
19:36:08  <Zuu>
19:40:35  <frosch123> GetPageElementVerticalDistance <- why not just FONT_HEIGHT_NORMAL ?
19:41:18  <Zuu> In 67 or so I will fix that broken goal references don't get drawed at all as it is an unrelated bug that I've spotted while reading the code.
19:41:35  <Zuu> frosch123: Because the font size is not known at compile time.
19:41:54  <frosch123> it looks like a constant, but it is none :p
19:42:03  <Zuu> Oh
19:45:31  <Zuu> So you suggest to replace GetStringHeight(STR_JUST_NOTHING, INT_MAX) with the macro?
19:45:38  <frosch123> yup
19:45:39  <Zuu> Or the whole method?
19:45:59  <frosch123> it's uses for that purpose everywhere afaik
19:46:33  <frosch123> well, i guess the method is not needed either, if it only returns that value
19:46:52  <frosch123> there is also a second occurence of STR_JUST_NOTHING for the date
19:47:30  <Zuu> It is useful as to document that the value reprecents the vertical distance between two page elements. But other than that, the method has no real purpose.
19:47:51  <Zuu> Yes, there is some other uses of STR_JUST_NOTHING in story_gui.cpp.
19:48:23  *** Alberth has left
20:08:10  <Zuu> A short patch to change to FONT_HEIGHT_NORMAL in story_gui.cpp:
20:37:30  <Zuu> I've now updated pixel-scroll-patch so that only uses FONT_HEIGHT_NORMAL:
20:39:26  <Rubidium> s/pixles/pixels/
20:41:14  <Rubidium> around 500, shouldn't line_height be removed?
20:41:43  <Rubidium> especially since you add another 'cache' of (essentially) line_height a few lines later
20:42:09  <Zuu> line_height is used further on in the method.
20:42:29  <Zuu> One could argue that line_height and element_vertical_dist could be combined.
20:42:39  <Rubidium> exactly ;)
20:42:45  <Rubidium> the rest seems okay
20:42:52  <Rubidium> though I'm tired
20:42:53  <Zuu> However they are conceptually different
20:44:18  <Zuu> So if a combined cache is used, the code may need more comments to be as readable as now.
20:44:39  <Rubidium> or just element_vertical_dist = line_height ;)
20:45:23  <Zuu> :-)
20:46:07  <Zuu> When I later add line warping to Goal/Location elements, some usage of line_height will be removed.
20:52:07  <Zuu> Hmm, in the end I decided to remove the element_vertical_dist and use line_height there as the code is already commented what it does.
21:26:09  <Zuu> This patch fixes the bug mentioned above related to drawing of broken goal references:
21:27:23  <Zuu> The bug has two sides to it: 1) Broken goals are not painted at all. Instead they just use one line height. 2) In the page element height computation broken goals are assumed to be drawn, thus the drawn height != calculated height.
21:28:25  <Zuu> This patch fixes both and adds a text string "invalid goal reference" when drawing the invalid goal. (I could have used a dummy string ID instead though)
21:31:39  <frosch123> looks fine
22:23:33  *** frosch123 has quit IRC
22:45:27  *** Zuu has quit IRC
23:46:27  *** LordAro has quit IRC

Powered by YARRSTE version: svn-trunk