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

Crop ABC values instead of truetype layout values #7995

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

wldevries
Copy link

This pull request fixes a regression and a bug in the horizontal layout for SpriteFonts in MonoGame 3.8.1 and addresses issue #7994.

  • It reverts the fix that cancelled out the horizontal bearing X.
  • It fixes the cropping algorithm by adjusting the ABC values instead of the typographic values

The following screenshots shows the fonts Segoe UI, Kingthings Petrock and Consolas, all at size 24. The first line of text is rendered using the Monogame 3.8.1. The second line is drawn with correct horizontal layout with the changes in this PR applied.

image

@stromkos
Copy link
Contributor

stromkos commented Feb 27, 2023

The only visible change is in the last line "period" spacing. Which is not an improvement in my opinion. (unless there were 2 spaces in the original text)

I can see a shift in the first two lines stat shortens the distance between the R and S.

Could you provide a reason your ABC hints are better than the ones provided by the advance in the font file?

@Beyley
Copy link

Beyley commented Feb 27, 2023

The only visible change is in the last line "period" spacing. Which is not an improvement in my opinion. (unless there were 2 spaces in the original text)

I can see a shift in the first two lines stat shortens the distance between the R and S.

Could you provide a reason your ABC hints are better than the ones provided by the advance in the font file?

The last line is a monospace font, it should be that spaced

@wldevries
Copy link
Author

There are many visible changes in the Segoe UI rendering. Look at the word "sky" for instance. With 3.8.1 rendering the s is far too close to the k. And in the word symphony the letter s is glued to the letter y which it should not be. Have a look at this rendering of the Segoe UI font in notepad:
image

@wldevries
Copy link
Author

One thing to note is that the GlyhpCropper is also used for bitmap fonts, which I did not realise when I worked on this. I need to make sure this PR does not cause regressions on bitmap font rendering.

@stromkos
Copy link
Contributor

The last line is a monospace font, it should be that spaced

Add character after the period to prove it its not mono-spaced. The "period" in a mono-spaced would be left aligned to the space.

One thing to note is that the GlyhpCropper is also used for bitmap fonts, which I did not realise when I worked on this. I need to make sure this PR does not cause regressions on bitmap font rendering.

Yes,GlyhpCropper has needed to go away since the transition from GDI. I did not see the harm +- 1 pixel until now. Please close this PR and create one removing GlyhpCropper from the chain.

@nkast
Copy link
Contributor

nkast commented Feb 27, 2023

Please close this PR and create one

@stromkos

Feel free to submit a PR yourself anytime.

@stromkos
Copy link
Contributor

Feel free to submit a PR yourself anytime.

@nkast , that standard line rubbed me the wrong way( sorry, if you missed the idiom: I am upset)

anytime was several years ago.

I already created it. See my last post on #7351.

Did you or anyone else ever bother to look at the code? I and eliminated this issue and created 16k texture support (Issue created and ignored) and fixed GL alpha8 textures(pre-shader swizzle required, so no PR since it was not crossplatform), simply for the speed test of all characters at larger sizes for that post only to be ignored.

So no I will not submit a PR. If anyone wants this fixed they are free to create the PR. It does not bother me, since it has been a non-issue for two years in my code base. The same goes for Emoji support.

If you didn't want it then, you have no right to ask it of me now.

@wldevries
Copy link
Author

wldevries commented Mar 1, 2023

Please close this PR and create one removing GlyhpCropper from the chain.

I'd be willing to do this if there was a chance of moving forward, but it seems like this project has lost it's forward momentum. We have unblocked ourselves by using a custom build of MonoGame.

Something else I'd like to tackle is the vertical spacing that is incorrect in the font rendering, but that's stuck on MonoGame using a broken old version of SharpFont: MonoGame/MonoGame.Dependencies#161

@stromkos
Copy link
Contributor

Please remove GlyiphCropper.cs and retest. This file is a remnant of the GDI font system, and is causing spacing havoc across the board.

Any font with a negative bearing, is corrupted by this method call, along with vertical spacing.

@wldevries
Copy link
Author

GlyphCropper is also used in FontTextureProcessor. It should probably remain to crop sprite based fonts, but I can remove it from FontDescriptionProcessor.

I'm a bit hesitant putting work in the MonoGame project. I've opened several bugs and tried to get interaction but this project seems dormant. It seems my experience is exactly the same as yours @stromkos. On top of that support for .NET Framework was dropped making any future version unusable to us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants