Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font kerning support for LabelCollection #2549

Closed
wants to merge 9 commits into from
Closed

Font kerning support for LabelCollection #2549

wants to merge 9 commits into from

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Mar 4, 2015

I never thought I would be this excited about text rendering, let alone font kerning.

The recent forum discussion on right-to-left language support and unicode characters led me to create a branch, single-billboard-labels, which switches Entity labels to use a single billboard per-word rather than through LabelCollection with a letter-based atlas. Obviously this is not a viable solution overall, but it allowed me to compare our label rendering against "perfect" labels that would be generated one word at a time. (All of these are smaller text scaled up, I've also sized the actual image the same to avoid GitHub scaling screwed things up, this is what you would see in Cesium with label scale set to 4 on a 2em sans-serif font)

Here's a label rendered in master:
image

And here's the same phrase, rendered with a single-billboard:
image

Notice how the spacing (i.e. kerning) of letters is all wrong, and we take up much more horizontal space than we should. If you look closely, you may also notice that the j looks slightly cut off on the tip of the curve at the bottom.

In order to visually see what's going on, I added a random background color to each item drawn by writeTextToCanvas, here's what the alphabet looks like as a single billboard:
image

And here's what it looks like in master:
image

Notice that there is a ton of empty space in between letters, but that space isn't "empty" all of the billboards are touching and some even slightly overlapping (unintentionally I believe). We can clearly see that in master the lowercase j is being cut off on the left side.

After seeing this, I realized that our goal should be to match the single billboard result by properly positioning the letters in relation to one another (a process called kerning for those of you unfamiliar with the term). It turns out, the dimensions object returned by writeTextToCanvas actually has everything we need to do this, it just took a while to figure out.

Here are the results, drawn using LabelCollection but with proper kerning.

image

image

We now match the single billboard image almost exactly (better than I ever thought possible)!

Eagle-eyed developers, such as @shunter and @emackey, will most certainly have already noticed that something is not quite right with the new code. There seems to be black overlap between some letters, where it looks like one letter is carving out the other. The st in just the ook in look, the work at and the ow in now are the most obvious (Remember these are small fonts scaled up, so it's more obvious here than at larger sizes). This problem has actually been something troubling me for a long time and I always assumed it was a bug in either writeTextToCanvas or LabelCollection, however, once I started looking into the issue, I realized that it's actually a bug in BillboardCollection itself, and one we already know about: #2130. I actually had this realization the moment I started randomly coloring billboard backgrounds and ignoring this bug was one of the keys to solving the kerning problem.

So long story short (too late), this change is the goal I've been chasing for a long time and fixes font rendering in Cesium labels once and for all (RTL and Unicode issues aside). Everything is drastically better. However, I think this bumps up the priority of #2130, because it makes fonts look pretty bad at smaller sizes.

Let me know what you think. We can take our time merging this, because in some cases it's a step backwards without #2130; but we should definitely make improving our labels a priority because they are pretty embarrassing in master (in my opinion).

`writeTextToCanvas` now always draws individual letters more reliably and
keeps them completely in the canvas.

`LabelCollection` now uses the `dimensions` object returned by
`writeTextToCanvas` to perform accurate kerning, producing much more
legible and visually appealing text.
@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 4, 2015

All sounds great. We will not be able to look at #2130 until after we finish vector data on terrain (parts of which may help) so if you want to wait on that fix, it could be a while.

@mramato
Copy link
Contributor Author

mramato commented Mar 4, 2015

All sounds great. We will not be able to look at #2130 until after we finish vector data on terrain (parts of which may help) so if you want to wait on that fix, it could be a while.

No worries, I figured that would be the case. It's up to @shunter and @emackey if they think this is worth bringing in sooner rather than later. I really don't know what the best answer is.

I just noticed that there seems to be an issue with vertical position as well, something I didn't really look at in this PR. It's only noticeable with some fonts and words. That's worth checking out at some point as well, but it shouldn't hold up this PR. If I get to it before this gets merged, then I'll roll those changes into here as well.

@emackey and @shunter, let me know what you think about when the best time to merge this will be.

@emackey
Copy link
Contributor

emackey commented Mar 5, 2015

@mramato Congrats on this. This is a major improvement here.

You can alleviate much of the remaining problem by sharpening the alpha channel on the text. I've been experimenting with this by just hacking it into the BillboardCollectionFS.glsl shader, but really you would want this to apply only to label billboards.

To test it, try replacing lines 20-23:

    if (color.a == 0.0)
    {
        discard;
    }

with this:

    color.a = pow(color.a, 4.0);  // sharpen the alpha channel.
    if (color.a < 0.0078)   // discard any less than 2.0 / 255.0
    {
        discard;
    }

Of course, this should only be applied to text, not all billboards. But take a look at your text samples with this in place. They're not 100% perfect, but they look about 80% better to my eyes.

@mramato
Copy link
Contributor Author

mramato commented Mar 5, 2015

I just realized we reference #2310 in a few places in the above comments, when it should be #2130 everywhere, I updated them to fix that. #2310 has nothing to do with this.

@emackey Thanks for the suggestion. I think the text is slightly more legible, but it has the negative effect of making everything really thin. I know you said it was only an 80% solution but I'm not sure it's worth it if we can get the real fix in by 1.9. Of course this is shader code, so @pjcozzi would make that decision.

Without the shader change
image

With shader change
image

Another solution would just be to change our default fonts to 1) be consistent everywhere (they currently aren't) and 2) looks decent even without #2130 (monospace might actually work here).

For example, here's 14pt monospace at the head of this branch

image

Whatever we decide, I think we should make sure all labels in Cesium, whether from KML, LabelCollection, LabelGraphics, CZML, etc.. all have the same default font.

@mramato
Copy link
Contributor Author

mramato commented Mar 5, 2015

To complete the set, here's what master looks like now:

image

@mramato
Copy link
Contributor Author

mramato commented Mar 5, 2015

Also, I realized the i is missing in some of my alphabet images. This was a typo on my end and not a bug somewhere.

@mramato mramato mentioned this pull request Mar 5, 2015
76 tasks
@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 6, 2015

Did we decide if we want to merge this for 1.8 before fixing #2130? Just curious.

@mramato
Copy link
Contributor Author

mramato commented Mar 7, 2015

While I don't think any kind of official decision has been made, I lean towards just waiting for #2130 to be fixed. The font rendering has been mostly the same since the beginning of Cesium and I'd rather not mess with it or change any defaults only to change them again in the following release. This, of course, assumes #2130 gets fixed in 1.9 (or at the latest, 1.10).

@dotansimha
Copy link

Looks great!!! It is possible to merge this to the upcoming release?

@mramato
Copy link
Contributor Author

mramato commented Jul 14, 2015

Looks great!!! It is possible to merge this to the upcoming release?

Unfortunately there is a billboard transparency issues (#2130) that prevent us from merging this into master at this time. It will definitely happen once that issue is resolved.

@slozier
Copy link
Contributor

slozier commented Nov 13, 2015

I guess I should have looked before I went and implemented kerning myself... any chance of getting this merged?

@pjcozzi
Copy link
Contributor

pjcozzi commented May 24, 2016

Have we tried merging billboard-transparency into single-billboard-labels to verify that it fixes the problem? See #2130 (comment).

@pjcozzi
Copy link
Contributor

pjcozzi commented May 24, 2016

Or merged into whatever branch has this change in the LabelCollection.

@mramato
Copy link
Contributor Author

mramato commented May 24, 2016

billboard-transparency is the right branch. At one point I did a local merge and it fixed the issue (if I recall correctly). I didn't submit it because we didn't think the approach was the best one at the time. The shader management API has changed since billboard-transparency was last updated, so someone need to merge in master and resolve any conflicts.

# Conflicts:
#	Source/Core/writeTextToCanvas.js
@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 23, 2016

Now part of #4675

@pjcozzi pjcozzi closed this Nov 23, 2016
@pjcozzi pjcozzi deleted the font-keming branch November 23, 2016 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants