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

Fix incorrect BitmapText text width calculation (#7758) #7933

Open
wants to merge 5 commits into
base: v7.x
Choose a base branch
from

Conversation

kjarrio
Copy link

@kjarrio kjarrio commented Oct 31, 2021

Description of change

Fix for BitmapText width calculation when using monospace fonts. Fixes issue #7758 - and doesn't break PR #7479 / #7245

Pre-Merge Checklist
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@kjarrio
Copy link
Author

kjarrio commented Oct 31, 2021

@bigtimebuddy
Copy link
Member

Thank you for the PR! Could you possibly add unit test for this behavior?

@kjarrio
Copy link
Author

kjarrio commented Nov 6, 2021

Here is work in progress, for proper width/height calculations for BitmapTexts:

Before: https://63hr2.csb.app PixiJS 6.2.0 (Source: https://codesandbox.io/s/boring-shadow-63hr2?file=/index.html )
After: https://wiwen.csb.app (Source: https://codesandbox.io/s/tender-violet-wiwen?file=/index.html )

I have the unit tests as well, but I'm still working on a few edge cases for "pixel perfection" but I'm unsure if it's even worth it. For me, it's "good enough" with those changes. How important is it to get this 100%?

I'll tidy up the code to finish this PR as soon as I have time.

@bigtimebuddy
Copy link
Member

Sorry it's taken a long time to review!

Seeing two issues here with width in your recent examples:

"C" is hanging off:
Screen Shot 2021-12-13 at 1 49 00 PM

Character endings are peaking out:
Screen Shot 2021-12-13 at 1 49 05 PM

@kozickikarol
Copy link

kozickikarol commented Jan 11, 2022

@kjarrio Are you going to fix anything here? I would gladly use new version with your fix cause I encounter the same problem.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't perfect, but it's good enough. If we can improve it overtime that'd be great.

@kozickikarol
Copy link

This isn't perfect, but it's good enough. If we can improve it overtime that'd be great.

🎉
@bigtimebuddy Any chance to have it in upcoming 6.2.2? It blocks our project from migrating to v6.

@kozickikarol
Copy link

@kjarrio I tested it more on my monospaced fonts and I noticed that this solution does not work for fonts which has xadvance<width or negative xoffset. BitmapText behaves correctly when I change digits inside but when first or last digit changes, BitmapText starts shaking.

monospaced font with negative xoffset:
monospace_font.zip

Same font works correctly in pixi v5.

@joel-Yolted
Copy link

joel-Yolted commented Apr 27, 2022

as @kozickikarol pointed this fix does not solve the issue, There is still a clear regression made by the original #7245 fix breaking old fonts. It seems to be only occurring when anchor is set to 0.5 but that is a common use case.

original recreation on #7758 using 3.6.0 https://jsfiddle.net/1zyrd4s8/ showing the issue still exist.

Was this an intentional break? and if so please advice on why.

Somewhat easy workaround for this is to add an invisible character to the end (as pointed out above, only seems to occur if the last digit has an xoffset).

@bigtimebuddy bigtimebuddy added this to the v6.4.0 milestone Apr 27, 2022
@bigtimebuddy bigtimebuddy added ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t and removed ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t labels Apr 27, 2022
Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs more work based on the feedback here with negative offsets.

@bigtimebuddy
Copy link
Member

Ping @kjarrio do you want to continue working on this? If not I'd like to close and maybe someone else can pick-up this work.

@kjarrio
Copy link
Author

kjarrio commented May 23, 2022

I will work on this, starting today, I can't promise I'll be able to fix it, but I will try to get this perfect this week

@bigtimebuddy bigtimebuddy removed this from the v6.4.0 milestone May 26, 2022
@Zyie Zyie deleted the branch pixijs:v7.x March 5, 2024 17:16
@Zyie Zyie closed this Mar 5, 2024
@Zyie Zyie reopened this Mar 5, 2024
@Zyie Zyie changed the base branch from dev to v7.x March 5, 2024 18:08
@Zyie Zyie added the v7 label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants