-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
BitmapText optimisation #6641
BitmapText optimisation #6641
Conversation
@GoodBoyDigital I see Matt's commits in this PR, maybe you need to rebase it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds nice - I have a few suggestions related to readability & see a potential problem with how the data flows around.
- optimise bitmapfont renderer - use mesh instead of sprites - optimised tinting - smart batching - less memory overhead - support for multipage fonts - pooling data objects - expose `baseTexture` uid as readonly
7dea79a
to
df38932
Compare
This is awesome @GoodBoyDigital! Super excited about our new and improved BitmapText. Some tests might need to be looked at, these are failing:
|
Question: How does this effect the Canvas renderer? |
Co-authored-by: Matt Karl <matt@mattkarl.com>
Hey @SukantPal , thanks for the feedback man! Push some changes. Let me know if its all cool 👍 |
@bigtimebuddy , will look at those tests now.. |
Good question. i will test, it should work but I guess would technically be slower perf than the old method. If it turns out to be a deal breaker and we should keep the previous method around and move it into Canvas Renderer. |
Codecov Report
@@ Coverage Diff @@
## dev #6641 +/- ##
=======================================
Coverage 82.59% 82.59%
=======================================
Files 35 35
Lines 1724 1724
=======================================
Hits 1424 1424
Misses 300 300 Continue to review full report at Codecov.
|
charRenderData.position.x = pos.x + charData.xOffset + (this._letterSpacing / 2); | ||
charRenderData.position.y = pos.y + charData.yOffset; | ||
|
||
chars.push(charRenderData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do ya think about optimizing this one as well?
Instead of pushing each character to the (initially) zero-length array, do something like:
// textLength is a _good_ heuristic of the actual no. of chars
const chars: CharRenderData = new Array(textLength);
let charsRendered = 0;
for (let i = 0; i < textLength; i++) {
if (<skipCondition>) {
continue;
}
chars[charsRendered] = charREnderData;
++charsRenderered;
}
// Make sure undefined slots aren't at the end of the array, if needed
chars.length = charsRendered;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GoodBoyDigital I'm leaving this one on your will. It would be cool to do this optimization but not necessarily required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^^
Seems to still render fine on canvas: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I'm happy with this @GoodBoyDigital. I think it represents a big step forward and with BitmapFont will be a huge win for 5.3.0. I'm sure you and @SukantPal can sort out any stylistic issues, otherwise g2g!
- fixed batching issue - added unit test for adding / removing children - fixed issue found when making unit test :) - don't call updateText on creation
Ok, good to go now! Final updates and new unit test
|
charRenderData.position.x = pos.x + charData.xOffset + (this._letterSpacing / 2); | ||
charRenderData.position.y = pos.y + charData.yOffset; | ||
|
||
chars.push(charRenderData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^^
Description of change
As BitmapFont has now become much more accessible to our users thanks to recent efforts from the team, figured we could do with doing a bit of an optimisation pass for how bitmap fonts are rendering!
Main thing is that instead of rendering each letter as a sprite, we now render them as a mesh. This has a few benefits straight out of the box!
updateText
render
updateText
call as we leverage the mesh tint functionality.Pre-Merge Checklist
npm run lint
)npm run test
)