We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3679

Summary: TTF_RenderUTF8_Blended_Wrapped ignores lineSpace
Product: SDL_ttf Reporter: Tim <tim.montague>
Component: miscAssignee: Sam Lantinga <slouken>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: normal    
Priority: P2 CC: sylvain.becker
Version: 2.0.13   
Hardware: x86_64   
OS: Linux   
Attachments: TTF_RenderUTF8_Blended_Wrapped lineSpace patch
Test program showing the problem
Font used by the test program
patch
patch v2

Description Tim 2017-06-23 00:21:27 UTC 
Created attachment 2768 [details]
TTF_RenderUTF8_Blended_Wrapped lineSpace patch

The TTF_RenderUTF8_Blended_Wrapped function sets lineSpace to 2 and uses it to compute the height of the textbuf SDL_Surface, but lineSpace is ignored when rendering each character.  This means that there is no spacing between lines, which causes the characters of some fonts to touch.

Here is where lineSpace is used to compute the height of textbuf:

/* Create the target surface */
textbuf = SDL_CreateRGBSurface(SDL_SWSURFACE,
        (numLines > 1) ? wrapLength : width,
        height * numLines + (lineSpace * (numLines - 1)),
        32, 0x00FF0000, 0x0000FF00, 0x000000FF, 0xFF000000);

And later, rowSize is computed without taking lineSpace into account:

rowSize = textbuf->pitch/4 * height;

I attached a patch with a suggested fix.

This was originally reported here:
https://stackoverflow.com/questions/44701892/sdl-ttf-line-wrapping-changing-height-of-wrapped-lines
Comment 1 Sam Lantinga 2017-09-10 07:34:44 UTC 
Created attachment 2919 [details]
Test program showing the problem
Comment 2 Sam Lantinga 2017-09-10 07:35:11 UTC 
Created attachment 2920 [details]
Font used by the test program
Comment 3 Sam Lantinga 2017-09-10 07:43:54 UTC 
I committed what I think the actual fix should be, but I don't want to enable it until the next release that can change font rendering size, so I don't break anyone who is relying on this behavior for their UI.

https://hg.libsdl.org/SDL_ttf/rev/b3c9090d1120

Feel free to grab this change and define TTF_USE_LINESKIP for your build though!
Comment 4 Tim 2017-09-11 19:37:49 UTC 
Shouldn't it be:
numLines * TTF_FontLineSkip(font)
instead of:
height * TTF_FontLineSkip(font)
?
Comment 5 Sam Lantinga 2017-09-11 19:55:36 UTC 
Yes indeed, thanks!
https://hg.libsdl.org/SDL_ttf/rev/3528ff8cb609
Comment 6 Sylvain 2018-11-03 13:01:40 UTC 
Created attachment 3438 [details]
patch

I have attached a patch to clean-up the define because we've made already lots of changes.

also:

- lineskip is still not really used as it just defined as the font height, so that doesn't change in the end.

- the current font height definition (ascender - descender + 1) is sometimes bigger than lineskip. -> just use font height.


If we want to propose an api to set the lineskip, I think  it will be easier to propose a "linegap". currently the height between to line is not exactly "font->height", but "font->height + a potential ystart" for strings that contain a glyph that goes higher than ascender.
Comment 7 Sylvain 2018-11-03 21:57:39 UTC 
Created attachment 3441 [details]
patch v2

I would go for this version of patch: font->height is defined as 

font->height   = FT_CEIL(FT_MulFix(face->ascender - face->descender, scale));

Since the freetype definition in FT_FaceRec gives:

/*    height              :: This value is the vertical distance         */
/*                           between two consecutive baselines,          */
/*                           expressed in font units.  It is always      */
/*                           positive.  Only relevant for scalable       */
/*                           formats.                                    */
/*                                                                       */
/*                           If you want the global glyph height, use    */
/*                           `ascender - descender'.                     */

That doesn't make lots of differences.
Comment 8 Sam Lantinga 2018-11-06 14:39:59 UTC 
Okay, I've accepted this patch. This will definitely change layout for some applications, so I'm thinking with all the changes in this release maybe we should mark it as ABI changing.

https://hg.libsdl.org/SDL_ttf/rev/b9d25816d9dc