FreeTypeFontGenerator with user-supplied PixmapPacker improvements #2993
Conversation
| try { | ||
| c.close(); | ||
| } catch (Exception ignored) { | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks like formatting only. You should check your IDE settings to avoid these kind of automated changes.
There was a problem hiding this comment.
Not sure why this shows up in this PR, I made the change here in 4f8b073. My personal rule for braces is to use braces if the statement spans multiple lines, for clarity about when the code is executed. I wouldn't normally go around making these kinds of changes, but I had just refactored StreamUtil a bit in
49a7c11 and shortly after the commit I noticed closeQuietly.
There was a problem hiding this comment.
Ah, I guess it shows up in the diff because this PR can't be auto merged. @RobRendell can you merge the latest libgdx into your PR branch?
There was a problem hiding this comment.
I believe this is now done. Merged, rebased and force-pushed to my forked branch.
There was a problem hiding this comment.
Yup, can be auto merged now once everything settles.
|
I think that's it for my review. I think it looks good and we should merge after those changes or discussion. |
…Generator to use it.
…Generator to use it.
RobRendell
force-pushed
the
libgdx-issue2980
branch
from
06d9964
to
728eb67
Compare
…Generator to use it.
…Generator to use it.
…libgdx-issue2980 Conflicts: CHANGES extensions/gdx-freetype/src/com/badlogic/gdx/graphics/g2d/freetype/FreeTypeFontGenerator.java gdx/src/com/badlogic/gdx/graphics/g2d/PixmapPacker.java tests/gdx-tests/src/com/badlogic/gdx/tests/PixmapPackerTest.java tests/gdx-tests/src/com/badlogic/gdx/tests/extensions/FreeTypeAtlasTest.java tests/gdx-tests/src/com/badlogic/gdx/tests/extensions/FreeTypePackTest.java
|
I've made some further changes, some of which are in line with your suggestions :)
How does it all look to you? |
|
Hum, I seem to have messed up the pull/rebase/merge across my branches... Xoppa's last commit seems to be showing up as a diff in my pull request now. Peculiar. |
|
Not sure what's going on with the diff. Maybe you can squash your commits? |
|
I'm in the process of making a fresh branch off master and cherry-picking my changes over, and I was going to create a fresh pull request off that branch. Does that work for you? |
|
Sure. I don't pretend to understand silly Git. :p |
|
Ok, fresh pull request created. I don't know if I can cancel this one, but I'll see what I can do. |
|
Ah, that would be the "Close and comment" button. |
As discussed in #2980, I've made some changes to the PixmapPacker so that it can provide an array of TextureRegions based on its pages, and FreeTypeFontGenerator so it uses this method if the user provides their own packer as a FreeTypeFontParameter.
I've updated the PixmapPackerTest to both generate and update the TextureAtlas, to use different-sized pixmaps (and not just the same one with three different names despite creating three of them :), and to show the resulting texture atlas (change pages by hitting 0-9).
FreeTypePackTest now uses the new method. I've also created a new FreeTypeAtlasTest which uses the less efficient (but more convenient) method of just generating the fonts directly. I ran the tests before and after my changes on my laptop, and the times reported by those tests are comparable (but Nathan said that the performance difference would be worse on mobile).
Before my change, FreeTypePackTest
943 ms, 596 ms, 604 ms, 676 ms
After my change, FreeTypePackTest
608 ms, 666 ms, 640 ms
After my change, new FreeTypeAtlasTest
643 ms, 646 ms, 623 ms