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 2779 - TTF_GetFontKerningSize err code
Summary: TTF_GetFontKerningSize err code
Status: ASSIGNED
Alias: None
Product: SDL_ttf
Classification: Unclassified
Component: misc (show other bugs)
Version: unspecified
Hardware: x86 All
: P2 normal
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-10 11:39 UTC by Jeffrey Carpenter
Modified: 2017-09-10 06:09 UTC (History)
2 users (show)

See Also:


Attachments
TTF_GetFontKerningSize err code fix (1.37 KB, patch)
2014-11-10 11:39 UTC, Jeffrey Carpenter
Details | Diff
proposed function prototype for SDL_GetFontKerningSize (1.84 KB, patch)
2014-12-19 18:52 UTC, Jeffrey Carpenter
Details | Diff
additional documentation notes for SDL_GetFontKerningSize (SDL_ttf.h) (1.05 KB, patch)
2014-12-21 02:08 UTC, Jeffrey Carpenter
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeffrey Carpenter 2014-11-10 11:39:02 UTC
Created attachment 1929 [details]
TTF_GetFontKerningSize err code fix

Hello,

I believe I've stumbled upon a bug in TTF_GetFontKerningSize -- the recently patched one at 275:86d0c63699f4 (from bug 2572) -- while writing unit tests in my text rendering class. The function returns -1 when an error occurs, but this error code conflicts with the valid range of possible kerning pair offsets.

I discovered the problem while using OpenSans-Regular.ttf at a point size of 72, with the text "Hello, World" (how terribly ironic, I thought!). The mismatch occurs between the glyph character 'W' and 'o'.

I've attached a patch that resolves the problem for me.

Thanks,
Jeffrey Carpenter <i8degrees@gmail.com>
Comment 1 Yohann Ferreira 2014-12-19 08:55:24 UTC
Hi there,

I confirm this patch is useful as certain glyphs pairs can return -1, which is a perfectly valid return value.

Can someone push that?
Comment 2 Sylvain 2014-12-19 13:14:44 UTC
Yes, it looks like a bug.

But, maybe the prototype of the function should be changed :

0/-1 on success/error

a pointer to get the value.
Comment 3 Jeffrey Carpenter 2014-12-19 13:33:51 UTC
Sylvain,

I seem to recall considering the same prototype change to a pointer. I feel like outputting the value as a pointer would be result in a less error prone API and additional consistency with other SDL functions.

The only reason I had at the time for leaving it as-is was ... perhaps backwards compatibility with the previous function prototypes, but I think that's a moot point; the original function prototype did not work for me at all before bug #2572 was resolved (not sure if it ever functioned for anybody?), and none of these changes have actually been released yet.

I'm in favor of changing the prototype function signature. I'll try to get around to submitting a proposed patch diff with that change here before the end of the year.

Cheers,
Jeffrey Carpenter <i8degrees@gmail.com>
Comment 4 Yohann Ferreira 2014-12-19 14:27:20 UTC
Hi :)

> not sure if it ever functioned for anybody?
As pointed out by the patch author in bug #2572, IIRC the previous function was asking glyph indeces parameters you simply couldn't get from the SDL API. So, indeed I can't see how this function could have been used, except maybe within SDL_ttf.

I'm also quite fine with getting the return value using a reference parameter/pointer to get the value. This should be hopefully rather trivial to fix.

Yohann
Comment 5 Jeffrey Carpenter 2014-12-19 17:58:24 UTC
Hi Yohann!

Excellent point, thanks for checking that. I haven't been back through the history logs in some time :-)

The change is indeed quite trivial. I would like to also look at possibly replacing the internal calls that get kerning pairs directly with TTF_GetFontKerningSize instead (in a separate diff patch) so that it would be easier for future maintenance; there are no internal calls being made to the function at the moment (forcing one to pull in external tests).

Cheers,
Jeffrey Carpenter <i8degrees@gmail.com>
Comment 6 Jeffrey Carpenter 2014-12-19 18:52:37 UTC
Created attachment 1976 [details]
proposed function prototype for SDL_GetFontKerningSize
Comment 7 Jeffrey Carpenter 2014-12-19 18:53:35 UTC
(In reply to Jeffrey Carpenter from comment #6)
> Created attachment 1976 [details]
> proposed function prototype for SDL_GetFontKerningSize

Oops, I got trigger happy wee bit soon :-P

...So I got bored and found a bit of time to make the proposed change to return the kerning pair size by passed in pointer. I tested this patch diff using this bit of
temporary test code inside showfont.c -- right before the call to draw_scene():

int kpair = 0;
if( TTF_GetFontKerningSize(font, 87, 111, &kpair) != 0 ) {
    fprintf(stderr, "Couldn't obtain kerning pair size: %s\n", TTF_GetError() );
    TTF_CloseFont(font);
    cleanup(2);
}

SDL_assert(kpair == -1);
// printf("kerning pair size: %d\n", kpair);

$ DYLD_LIBRARY_PATH=/Users/jeff/opt/SDL2/lib ./showfont /Library/Fonts/Arial.ttf 72 "Hello, World"

Works as intended on my system. Built against SDL2 hg rev 9278. I haven't done any external testing yet, but I'll get around to that sooner or later! :-) Let me know if the attached patch diff gives you any problem, or if you spot anything else!

Cheers,
Jeffrey Carpenter <i8degrees@gmail.com>
Comment 8 Yohann Ferreira 2014-12-19 21:54:06 UTC
Looks and works fine here. :D

I'd simply add this in the function doc:
(Based on: http://www.freetype.org/freetype2/docs/glyphs/glyphs-4.html)

Kerning distances are expressed in pixels. They are usually oriented in the X axis, which means that a negative value indicates that two glyphs must be set closer in a horizontal layout.
Comment 9 Jeffrey Carpenter 2014-12-21 02:08:32 UTC
Created attachment 1979 [details]
additional documentation notes for SDL_GetFontKerningSize (SDL_ttf.h)

(In reply to Yohann Ferreira from comment #8)
> Looks and works fine here. :D
> 
> I'd simply add this in the function doc:
> (Based on: http://www.freetype.org/freetype2/docs/glyphs/glyphs-4.html)
> 
> Kerning distances are expressed in pixels. They are usually oriented in the
> X axis, which means that a negative value indicates that two glyphs must be
> set closer in a horizontal layout.

I thank you for your suggestion! I do agree, and thus have attached another proposed patch diff with the changes made to the documentation notes in SDL_ttf.h.

To whomever is applying these patch diffs, they will probably have to be applied sequentially, in the order that they were created, for a clean merge.

Thanks!
Jeffrey Carpenter <i8degrees@gmail.com>
Comment 10 Sam Lantinga 2017-09-10 06:09:11 UTC
These patches look good for the next release that updates the ABI, thanks!