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

Separation of glyphs and characters for BitmapFontCache #2955

Merged
merged 10 commits into from Mar 24, 2015

Conversation

Projects
None yet
5 participants
@NathanSweet
Copy link
Member

NathanSweet commented Mar 19, 2015

This PR refactors how BitmapFontCache works. Previously it was assumed each character in a string mapped to a glyph for display. As discussed in #1589, this is not the case for some types of font rendering. This PR has two main goals: 1) to separate characters and glyphs, 2) to make parsing the string and preparing for display cleaner. The new approach works like this:

  1. GlyphLayout, a new class, is given a string of characters. It first breaks the strings into "runs". A run is a string of characters all the same color and without newlines. The reason to break the text into runs is that each run can be "shaped" individually, which means to convert the characters into glyphs for display. When breaking the string into runs, GlyphLayout parses color markup tags and stores the color for each run.
  2. Next, the runs are shaped using a new BitmapFontData method, getGlyphs. This takes the characters for the run and generates both a list of glyphs and a list of positions for each glyph. The default implementation just looks up the glyph for the character as has always been done. A more complex implementation could use HarfBuzz or another lib to do the shaping.
  3. Next, the shaped runs are laid out. If wrapping is enabled and a run's glyphs exceed the wrap width, then glyphs are split for line wrapping using another new BitmapFontData method, getWrapIndex. The run is ended, the remaining glyphs are moved to a new run on the next line, and the layout process repeats. I'm not sure if line wrapping glyphs is the best solution, but at least where the wrap happens can be customized. Once all runs are laid out, if center or right alignment is requested then run positions are shifted appropriately.
  4. At this point GlyphLayout is done. It stores the Array<GlyphRun> and provides a width and height which is the bounds of all the runs. Previously getting text bounds required all the work of laying out the glyphs, which then had to be repeated for drawing. With GlyphLayout, the work is only done once. Each GlyphRun has Array<Glyph> and a FloatArray for the positions.
  5. Next comes BitmapFontCache. This class now has an Array<GlyphLayout> and its job is to store the vertex data for rendering the glyphs in those GlyphLayouts. Most of BitmapFontCache was left intact, eg it still can render from multiple texture regions.

There are a number of relatively small API breaking changes:

  1. The BitmapFontCache setText, setMultilineText, setWrappedText, and similar addXxx methods are replaced by setText and addText. The number of overloads was kept reasonable. The same happened to the BitmapFont draw methods, which delegate to BitmapFontCache. These methods all return GlyphLayout instead of TextBounds.
  2. BitmapFont.TextBounds is deprecated. I should have never added it and just used Vector2. Now, GlyphLayout has width and height and replaces TextBounds.
  3. BitmapFont.HAlignment is gone. Align is used instead.
  4. markupEnabled was moved to BitmapFontData.
  5. BitmapFont getBounds and related methods were mostly removed, with the single most commonly used method deprecated. GlyphLayout should be used directly to avoid computing glyphs multiple times.
  6. BitmapFont computeGlyphAdvancesAndPositions and computeVisibleGlyphs are deprecated and should be removed because they assume a 1:1 mapping of characters to glyphs. GlyphLayout can be used instead, which provides each glyph position. I haven't made this change since it affects TextField and friends, but I will once this gets merged. Now done.

Feedback on these changes is welcome. I've used the gdx-tests to verify things are still working. I also have updated Spine to use this, which is relatively complex real world usage. The font APIs are widely used so it's important we get this right. Now is a good time to break the API further if we need to.

NathanSweet added some commits Mar 18, 2015

Added GlyphLayout, rewrote BitmapFontCache.
This has API changes that break code throughout libgdx, I'm committing that separately so it's easier to review the core changes.
@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 19, 2015

The more interesting files to look at:
GlyphLayout
BitmapFontCache
BitmapFont

@@ -515,7 +362,7 @@ public int computeVisibleGlyphs (CharSequence str, int start, int end, float ava

for (; index < end; index++) {
char ch = str.charAt(index);
if (ch == '[' && markupEnabled) {
if (ch == '[' && data.markupEnabled) {
index++;

This comment has been minimized.

@davebaol

davebaol Mar 19, 2015

Member

This is the issue mentioned by @MobiDevelop in #2886 (comment)

Looks like this code has not changed drastically. So, if this PR is not merged soon I can fix and test the issue using the snapshot, then I can paste the fix here. Objections?

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 19, 2015

@NathanSweet
Hmmm.. your last commit says "Removed computeVisibleGlyphs".
Does this mean that we can consider issue #2886 fixed by this PR?

EDIT:
Haven't looked into the code yet.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 19, 2015

I've now removed computeVisibleGlyphs and computeGlyphAdvancesAndPositions, replaced by GlyphLayout. Wasn't as bad as I thought it would be! This doesn't fix #2886 though, I will add a test and fix now.

You haven't looked into the code yet!?

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 19, 2015

You haven't looked into the code yet!?

Nope, I'm in office right now 😄

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 19, 2015

Offices typically have computers, yes? Get paid to libgdx! 😉

import com.badlogic.gdx.graphics.g2d.BitmapFont.HAlignment;
import com.badlogic.gdx.graphics.g2d.BitmapFont.TextBounds;
import com.badlogic.gdx.graphics.g2d.GlyphLayout.GlyphRun;
import com.badlogic.gdx.scenes.scene2d.utils.Align;

This comment has been minimized.

@MobiDevelop

MobiDevelop Mar 19, 2015

Member

Perhaps Align has outgrown its place in the scene2d package. Though I guess we can't really move it now.

This comment has been minimized.

@davebaol

davebaol Mar 19, 2015

Member

Perhaps for backward compatibility you can leave it there as an empty class that simply extends the real Align class living out of the scene2d package.

This comment has been minimized.

@NathanSweet

NathanSweet Mar 19, 2015

Member

I've be ok with just moving it. Better to annoy everyone with a few breakages at once than to break them every few releases. Let's make a "break all yo shit" release! :hurtrealbad:

This comment has been minimized.

@davebaol

davebaol Mar 19, 2015

Member

Yeah I know that you dislike @deprecated 😄
Anyways, it's ok with me. Break all the shit. 👍

This comment has been minimized.

@MobiDevelop

MobiDevelop Mar 20, 2015

Member

Sounds good to me.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 21, 2015

Okie, I'll move Align.

Everybody ok with this PR? I'd like to merge so I can send another PR for on the fly FreeType rendering, good for CJK. If you need more time, just speak up!

@nooone

This comment has been minimized.

Copy link
Contributor

nooone commented Mar 22, 2015

Really nice PR!

Let's make a "break all yo shit" release! :hurtrealbad:

BitmapFont still has a @Deprecated getBounds(...) though. Maybe let's get rid of that as well then?

NathanSweet added some commits Mar 24, 2015

Removed BitmapFont#getBounds and BitmapFont.TextBounds.
Use GlyphLayout! Also, often BitmapFont#getBounds was just used for alignment. In that case, use the BitmapFont#draw methods that allow alignment to be specified.
Moved BitmapFont setScale and containsCharacter to BitmapFontData.
I left the getLineHieght and other getters in BitmapFont since they are convenient and are used a lot. It doesn't make sense to have setters (like setScale) in BitmapFont though. Modification of the BitmapFontData should be done through BitmapFontData, else it makes it seem like the setting is per BitmapFont.
@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 24, 2015

I didn't want to, but you're right that it's better. BitmapFont getBounds and TextBounds are gone! I think the PR is ready!

@MobiDevelop

This comment has been minimized.

Copy link
Member

MobiDevelop commented Mar 24, 2015

I'm ok with merging this though, admittedly, I have not actually run it myself.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 24, 2015

That's the best kind of merge. ;)

Mario says he looked at some of the code. I expect he doesn't weigh in here so he can shout at me later.

Pressing the button!

NathanSweet added a commit that referenced this pull request Mar 24, 2015

Merge pull request #2955 from NathanSweet/master
Separation of glyphs and characters for BitmapFontCache

@NathanSweet NathanSweet merged commit 9c53f2d into libgdx:master Mar 24, 2015

@badlogic

This comment has been minimized.

Copy link
Member

badlogic commented Mar 24, 2015

Fuck it, we do it live. BTW, I asked you to write a blog post so we can
people who don't read.
On Mar 24, 2015 5:23 PM, "Nathan Sweet" notifications@github.com wrote:

That's the best kind of merge. ;)

Mario says he looked at some of the code. I expect he doesn't weigh in
here so he can shout at me later.

Pressing the button!


Reply to this email directly or view it on GitHub
#2955 (comment).

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 24, 2015

Yes, of course my horse. With this in I can commit on the fly rendering, then have something to post about (today).

@nooone

This comment has been minimized.

Copy link
Contributor

nooone commented Mar 24, 2015

I asked you to write a blog post so we can people who don't read.

What do we do with those people? I'm curious now...

@badlogic

This comment has been minimized.

Copy link
Member

badlogic commented Mar 24, 2015

I let you find a verb :)
On Mar 24, 2015 7:10 PM, "Daniel Holderbaum" notifications@github.com
wrote:

I asked you to write a blog post so we can people who don't read.

What do we do with those people? I'm curious now...


Reply to this email directly or view it on GitHub
#2955 (comment).

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 24, 2015

Blaaaaame! What do I win? :P

@nooone

This comment has been minimized.

Copy link
Contributor

nooone commented Mar 24, 2015

You won the privilege of solving the next 3 issues that pop up!

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 24, 2015

Cool, I'm going to add 3 bugs to the source and fix them immediately :)

@MobiDevelop

This comment has been minimized.

Copy link
Member

MobiDevelop commented Mar 25, 2015

@NathanSweet forgot to add GlyphLayout to the gdx.gwt.xml, @davebaol won the privilege of fixing that.

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 25, 2015

Ok done! See commit f415270

-1, two missing :)

@MobiDevelop

This comment has been minimized.

Copy link
Member

MobiDevelop commented Mar 25, 2015

I have two more for you... These changes also broke the invaders and superjumper demos. :)

@davebaol

This comment has been minimized.

Copy link
Member

davebaol commented Mar 25, 2015

@MobiDevelop
Damn! :/

@NathanSweet
BitmapFont.drawMultiLine() has gone. What should I use instead?

@davebaol

This comment has been minimized.

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 25, 2015

Oops. Silly GWT! 💃

Silly demos, hiding in some other repo! Will look into it, unless davebaol is feeling frisky! ;) :D

@davebaol just use BitmapFont draw, they all do multiline (ie support newlines in the text). I'll fix up MatrixTest, sorry about that. Silly Android!

@NathanSweet

This comment has been minimized.

Copy link
Member

NathanSweet commented Mar 25, 2015

MatrixTest fixed (forgot to link to this issue).

I added a code example section to the blog post to ease moving to the new API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment