BitmapFontFactory #6600
BitmapFontFactory #6600
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #6600 +/- ##
=======================================
Coverage 82.34% 82.34%
=======================================
Files 38 38
Lines 1903 1903
=======================================
Hits 1567 1567
Misses 336 336 Continue to review full report at Codecov.
|
|
@bigtimebuddy Just did. Why not add packages/**/package-lock.json to .gitignore? |
|
@bigtimebuddy Regarding performance, try running the demo locally. Although Stats.js shows 12 FPS, Chrome DevTools (locally) shows 55-70 FPS range.
|
There was a problem hiding this comment.
Overall, this is really awesome and I think is a much safer approach than gutting Text. Maybe in the future this can nudge more users to use this text rendering approach.
Also, what do you think about a convenience method for generating character ranges. For instance here are two ideas:
// Nice and verbose, requires a little more parsing in create
const font = BitmapFontFactory.create({
chars: [['a', 'z'], ['A', 'Z'], ['0', '9']]
});
// or something like this, where you create a convenience function
const range = BitmapFontFactory.range;
const font = BitmapFontFactory.create({
chars: range('a','z') + range('A','Z') + range('0','9')
});One final thing: this needs some unit tests. Just to make sure that the data being generated is valid. Should be pretty easy to test.
|
This is awesome! Agree that functionality being on BitmapText is the best place!
What you have done is basically elevated bitmap text so that it is easy for everyone to use not just people who have hard to find and figure out font generation software! How do you guys feel about simplifying the API even further and removing the need to actually create the font before its used? As well as exposing the BitmapFontUtils I think also doing it under the hood would be the best developer experience :) |
|
I’m not sure that the constructor for BitmapText also works for this without being a breaking change or too overloaded to catch errors. I think create and apply are two distinct steps requiring two lines of code. For instance, there is not a great way to reconcile the font name if we can use the constructor. There are a couple font states for BitmapText: available system font, installed bitmap font, or missing font. And even if you have a bitmap font, when to regenerate seems like a question you may not want to do inside the constructor. We could maybe simplify the API and do BitmapFont.create instead of BitmapFontFactory. But I think this needs to be two steps UNLESS we introduce a new display object like SimpleBitmapText, or something like that. Then we can do all the font generation automagically. |
appreciate if it's a breaking change then maybe changing the API for BitmapText might be better off as a v6 thing... something like? Like you say @bigtimebuddy , i guess the issue lies in if we don't want a breaking API change.. what does the interim API look like. creating a new class may be best here? Also, overloading constructors.. generally thats ok right? This new text could accept |
|
ooh oh so |
|
Let’s start with BitmapFont.from and then look at future PRs to build convenience on top, add a new display object or break BitmapText to support this path as first class. Does that sound good @GoodBoyDigital? // Provide the font name, so it can register automatically
BitmapFont.from("custom", {
chars: "0123456789",
resolution: 2,
fontFamily: 'Arial'
});
// Reference the font by name
const counter = new BitmapText("0", { font: "custom" }); |
|
cool, sounds good 👍 |
+ Support for character ranges (e.g. chars: [['a', 'z'], ['A', 'Z']] + Support for arrays of characters (e.g. chars: ['abcdef', 'GHIJK']) + Documented drawGlyph (local, internal function) + Mandatory arguments for BitmapFont.from()
…js/pixi.js into feature/bitmap-font-factory
There was a problem hiding this comment.
Great work. Thanks for hitting the feedback, this is getting close. One small request below and still need a couple of unit tests for this. Some ideas to cover:
- handling different chars variations (string, array, ranges)
- supporting different resolutions
- using default TextStyle options
|
@bigtimebuddy Will do right now! I can't seem to get the next build here (https://pixijs.download/feature/bitmap-font-factory/pixi.js) - the Could you see if it comes up? |
|
@bigtimebuddy Could you deploy this branch so that the demos can be updated? |
|
@bigtimebuddy I'm not sure how we would add unit tests for checking whether the glyphs rendered at the right "resolution". A |
|
@cursedcoder do you know what |
|
For resolution @SukantPal, render two glyphs at different resolutions and compare the font data dimension sizes. We should probably be using the same defaults or, probably better, internally convert into a TextStyle object. |
|
There was a problem hiding this comment.
Creating a font with no chars given at all should be an error.
The xAdvance being created for each char are to decimal places; no bitmap generator would ever give data in this way, so these should be rounded in some way. Might get blurry text when rendering to decimal positions.
The texture rects being created are also incorrectly to decimal places
The 'page' in the data for each char just gives the id of the char itself. It should say which sheet # the text is found on. So if generating takes up 2 base textures, the page should either be '0' or '1'. Useful to know how many textures this font generation is requiring.
Should these Textures and BaseTexture be added to the caches so we know they've been created? Like they are for textures created in the Text class?
Visual issue with drop shadows AND stroke on text: https://www.pixiplayground.com/#/edit/nvZj_usqdMGPmDbfLIFUR
Issues if Italic is used: https://www.pixiplayground.com/#/edit/Y6pgxZRlVMR5qND9igixc
|
@bigtimebuddy @themoonrat Hey Dave, I wanted to first touch upon the issue with drop-shadows. I think the "buggy" look of the stroke + drop-shadow will still stay there because the browser renders it like that.
|
|
Regarding drop shadows, yeps, I understand what you're saying, that's fine. No different to if you created a bitmap font now with drop shadows, then tries to place them close to each other so they overlapped :) I worried that it was being caused by the the glyphs in the atlas themselves perhaps overlapping due to the drop shadow. |
|
@themoonrat Okay, so:
|
|
@themoonrat Regarding the pages => char mapping issue, I need more information on how BitmapFont uses those pages & the textures array. TBH, I'm confused b/w (x,y) & (xoffset, yoffset) in the font data. |
|
So, taking a look at https://pixijs.io/examples/#/text/bitmap-text.js In this bitmap font, there is only 1 image atlas, desyrel.png, so the page is '0' for all of the characters. If there were 2 image atlases, say, a desyrel1.png and a desyrel2.png, then some of the characters would have a page 0, and some a page 1. So in this scenario, where you do |
|
@themoonrat Okay, but another question: what is |
|
When bitmap fonts are generated, in the xml, the x, y, width and height are to tightly draw a box over each glyph in the image. Now, how do you then know how to draw each glyph with the same baseline? That's where yoffset comes in. It lets you know how much to adjust the y position when drawing that glpyh. So a lowercase z will have a bigger y offset than an uppercase Z http://www.angelcode.com/products/bmfont/doc/render_text.html In the atlases produced here, each item drawn is with the same baseline, so that kinda solves not needing to have a yoffset. The fact that when doing our measuring, width wise there is empty spacing around the edge, solves not needing to have a xoffset. But that does mean that they work in slightly different ways. With regular bitmap fonts, each glyph data is tight around the character with xoffset and yoffset to normalise their positions. Whereas this 'from' method, each glyph data is not tight around each character, and instead has the padding build it. |
|
@themoonrat OK, I'll try changing the pages mapping to per-BaseTexture instead of per-glyph 😄 |
|
PS, I found a way to effectively turn all numbers fixed width |
|
@bigtimebuddy @themoonrat I also got the issue with glyph pages resolved. You should see fewer different pages in the whole character set. This should be it! |
|
Looks good! My only remaining question is for @bigtimebuddy, regarding whether these generated base textures & textures should be added to the BaseTextureCache and TextureCache. Normal Text has it's textures added there, which I find useful to be able to track asset usage. I believe it's just the 'adding' to the cache required, as the 'destroy' function in BitmapFont would already handle the removing? Tho the destroy function for bitmap text never destroys the base texture? But yeah, just clarity on what to do that wise. I'm super happy with how the class works now |
|
@themoonrat, this implementation is somewhat consistent with current approach to handling BitmapFonts, that each glyph Texture is created by and destroyed by BitmapFont. It's the responsibility of the font to maintain the references (i.e., not added to TextureCache). The BaseTextures (page textures), however, are added to the cache if it's create from the Loader. In general, Pixi should follow a "campsite rule", which is, if a class creates garbage, it should also destroy it. Graphics, for example, only destroys geometry if it was created internally and not passed by reference. That's a longwinded way to say that I think we need to fix this. I believe BitmapFont should maintain reference to page textures and we should provide a way in the What do you think @SukantPal? |
|
@bigtimebuddy A g r e e. But I think a general |
|
Are you proposing adding a static destroy method Destroying actually is probably more important for this workflow in-case you want to build something on top of this PR that auto-generates the font based on characters. I think I'll do a follow-up to improve this. I think your work here is done @SukantPal. You've put a lot of effort into this and we super appreciate it. Scrolling back, here are some future PRs related to this:
|
|
@bigtimebuddy I was talking about an instance destroy, not static. But it is really a trivial difference. |
|
Happy for follow up PRs to handle the missing bits. |
bigtimebuddy
added
the
✅ Ready To Merge
#6524
(follow up to #6597)
@bigtimebuddy wanted a simpler API. Instead of a dynamic caching layer beneath
PIXI.Text, we opted for a static bitmap generation layer beneathPIXI.BitmapText. This has no crazy calculations for tiling and all that.Description of change
@pixi/text-bitmapdepends on@pixi/textfor styling stuff. The factory should generate anyITextStyle.Pre-Merge Checklist
npm run lint)npm run test)