-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
Hiero BMFont padding order fix (#4295) #4297
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
Conversation
Fixes issue libgdx#3994 where the Java renderer does not correctly set the BMFont xoffset because charVector was not being used.
…ire specification.
It's a shame this is not merged, not everybody uses Hiero to generate their BMFont files. Not fixing this we are officially creating a fork of the BMFont format reducing our choices when it comes to glyph editors. |
Friends, you can work around this. First, the padding is often the same value in all 4 fields. The only ones you need to consider are the middle ones, left and down. So your tools can check if left != down and issue a warning. Then, if you need to convert on import into LibGDX, you can make something like a gradle script that swaps the two values. If they change their source, then existing apps that dynamically link to the jar will possibly break. They should claim to be "almost" BMFont and issue a warning in their documentation (I haven't checked). |
libgdx wiki and javadoc states it rely on official BMFont specification. But current implementation is wrong about padding order. I'm wondering why it's not acceptable to force users to migrate their BMFont files while it's acceptable to do so with their particle2D (because of recent changes). Since there are discrepancy across 3 libgdx classes, I think it would be worth it to comply with official specification. If you really think it souldn't be changed, we should at least : make these classes consistent, update javadoc and wiki to state it clearly. Please reconsider closing this. |
We force breaking of Particles due to the terrible parsing that we do. Its not on purpose, it would be much better if it was completely backwards compatible. The issue is how easily overlooked this change is, even when its in notes and in release notes. People will be using asset files that have been generated without version tag, from old versions of the tool, with no idea if the padding is correct/incorrect. There is no way for the user to know from the BMFont file. Since there was 0 discussion on this for 3 years and on the original issue to combat @badlogic's decision, this was closed. Perhaps if people were that opinionated on it during those times it would have been merged. I am indifferent to these changes, so we can reopen now everyone has come out of the woodwork. |
@Tom-Ski Thanks, you're right we should have shown support to the PR before instead of waiting for it to be closed. Would you consider merging something in the lines of giving the option to set the format to BitmapFont (for instance HIERO, BMFONT or even adding a version to open for future changes like HIERO_1.0) being Hiero format the default one? |
I think adding some versioning from Hiero would be great. At least that would give people a hint as to if their assets need an update. I don't really like the idea of support both formats. Its relatively easy for people to update their fonts by just scripting over the font files, or just manually changing the order. As long as we present documentation on how to do this when we release, I think it will be ok. If we present multiple ways to load, it will still be an issue with old files, not knowing which format they are in. Is there any meta we have to know if something is from hiero currently? |
There's no meta I can see unfortunately. I don't see how my suggestion would still be an issue if we keep Hiero format as default. Even if we created a new 2.0 Hiero format with metadata in the future (which would also mean we break BMFont standard again afaik), when loading a font file we would first check if there's metadata to know the Hiero version and if there's not assume it's old current Hiero 1.0 format. Adding the option to pass an alternative format to Hiero such as BMFont I don't see how it hurts in any way and makes non Hiero users life much easier (and for normal users is easier to understand the concept of formats without having to read a lot of docs on how to edit the files). |
I don't think meta data fixes the root issue you're attempting to fix,
which is that fonts not created by Heiro do not import correctly. Maybe we
can create a global static flag (similar to ShaderProgram.pedantic) in
BitmapFont for loading legacy Heiro fonts. And maybe we can get away with
defaulting it to off because most people probably aren't using asymmetrical
padding. When those people see their text looks different after updating,
surely they would look at CHANGES and see they can fix it with one line.
People who use BMFonts not created by Heiro are most likely not affected by
this change or they would have noticed something amiss when they were using
unusual asymmetrical padding, and would have been chiming in rigy here.
This is probably a fraction of a fraction of users, if they exist at all.
We could add a comment of the top of newly created Heiro fonts to indicate
the new version just for the sake of manually identifying them.
…On Tue, Jul 2, 2019 at 6:59 AM obigu ***@***.***> wrote:
There's no meta I can see unfortunately.
I don't see how my suggestion would still be an issue if we keep Hiero
format as default. Even if we created a new 2.0 Hiero format with metadata
in the future (which would also mean we break BMFont standard again afaik),
when loading a font file we would first check if there's metadata to know
the Hiero version and if there's not assume it's old current Hiero 1.0
format.
Adding the option to pass an alternative format to Hiero such as BMFont I
don't see how it hurts in any way and makes non Hiero users life much
easier (and for normal users is easier to understand the concept of formats
without having to read a lot of docs on how to edit the files).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#4297?email_source=notifications&email_token=AAOB7O4YMYJFNF76QIG3T4LP5MYHZA5CNFSM4COVDX72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZA4ELY#issuecomment-507626031>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOB7O7SR4WOYXFQWMYR72DP5MYHZANCNFSM4COVDX7Q>
.
|
The idea behind meta data would be to ensure backwards compatibility. If we could identify that a font was made with the incorrect standard via old Hiero, we can trivially load the font correctly for support for the old format. I don't think its worth keeping the backwards compatibility, because if the user even finds out they have a problem with their font, they will (hopefully eventually) look up the changelog, and just change them in a few seconds. The only reason for keeping backwards compatibility imo is if we can automatically detect the font was made in the old format. Id say finding the global flag that lets people load old formats is about the same effort as changing the paddings in the font files, and we dont have to keep code to support some invalid legacy format. |
That makes sense for simplicity. The number of affected people is probably quite small, and if it's mentioned in CHANGES we can provide a link to a Gist script for "upgrading" old fonts. I don't see how meta data on new fonts can help us detect old ones without incorrectly labeling all old and new BMFonts from other font creators besides Heiro. And if we didn't care about those, there'd be no point in fixing this. |
Im talking about if there were any metadata in the original, old generated fonts that we could harness. |
#5422 (comment) is related to this PR and @NumeronR is saying that we should not be doing anything with padding at all. #3074 by @FyiurAmron is the reason we started using padding values, which very well could have been wrong. I'm generally terrified of changing font stuff, as it means all my apps break. Regenerating fonts isn't a big deal, but changes to how values are interpreted is much more pain to fix. App code is usually making assumptions about where text will be drawn based on these values. When it changes, fixing it usually means dorking with the fonts so they have whatever values they used to have, which isn't terribly easy. This happened in the past and you can see the code snippet provided there which massages the font metrics back to how they used to be. If we must make breaking changes then we should be sure it is worth doing and that we're doing it right to avoid making this mess even bigger. |
As the one who submitted the PR, I'm actually in favor of dropping the PR for those reasons. Maybe the |
Although, the internal inconsistency I found described in the OP between the two writers deserves a second look. |
I agree with Tom-Ski:
I would merge this (editing CHANGES obviously) and follow the BMFont specification. This should affect a very small number of users (who use irregular paddings on fonts generated by Hiero) that will need to recreate the font if anything breaks. That's the whole point of versioning in my opinion but I understand it may scare some people. In any case it's a minor thing and I think we have @NathanSweet veto (if that's the case we can close it). |
As mentioned in my comment above, maybe reading padding at all is the wrong thing to do. Assuming we really do need to read it, I'm indifferent on making the change. It would be better to follow the spec, but on the other hand the issue seems relatively minor and may not be worth the disruption. If you guys want to make the change, I don't mind. |
I agree with @NathanSweet and @NumeronR that padding shouldn't affect the appearance of the font at all, since it's part of the |
Ok, let's close this as it seems it's all a bit messed up and hard to change at this point. @tommyettinger The problem I was referring to is not about opening .fnt files generated with Hiero on ther tools but .fnt files generated by other tools that follow BMFont specs (such as AngelCode BMFont tool itself) being loaded on libGDX and the confusions it may cause (considering that, even if it shouldn't, libGDX uses padding for positioning). |
Would not help, this way we still wouldn't be compatible to the specification. |
Fix for issue #4295.
Notice that there's actually a discrepancy in LibGDX with itself.
BitmapFontWriter writes: up, down, left, right
BMFontUtil writes: up, left, down, right
BMFont reads: up, left, down, right
In order to make the padding ordering compatible with the BMFont specification, I normalized all orderings to up, right, down, left.