Skip to content

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

Closed
wants to merge 5 commits into from

Conversation

bryanwagner
Copy link
Contributor

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.

@cypherdare
Copy link
Member

@Tom-Ski This one can be closed, not merged, as per Mario's comment on #4295.

@Tom-Ski Tom-Ski closed this Jun 29, 2019
@obigu
Copy link
Contributor

obigu commented Jun 29, 2019

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.

@bryanwagner
Copy link
Contributor Author

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).

@mgsx-dev
Copy link
Contributor

mgsx-dev commented Jul 2, 2019

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).
We could add a "MIGRATION" tag in CHANGES.md to explain how to do it (either manually or with hiero). Maybe it would solve user's potential issues/frustrations discussed here : #4295 (comment)
@badlogic what do you think?

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.

@Tom-Ski
Copy link
Member

Tom-Ski commented Jul 2, 2019

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 Tom-Ski reopened this Jul 2, 2019
@obigu
Copy link
Contributor

obigu commented Jul 2, 2019

@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?

@Tom-Ski
Copy link
Member

Tom-Ski commented Jul 2, 2019

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?

@obigu
Copy link
Contributor

obigu commented Jul 2, 2019

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).

@cypherdare
Copy link
Member

cypherdare commented Jul 2, 2019 via email

@Tom-Ski
Copy link
Member

Tom-Ski commented Jul 2, 2019

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.

@cypherdare
Copy link
Member

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.

@Tom-Ski
Copy link
Member

Tom-Ski commented Jul 2, 2019

Im talking about if there were any metadata in the original, old generated fonts that we could harness.

@NathanSweet
Copy link
Member

#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.

@bryanwagner
Copy link
Contributor Author

As the one who submitted the PR, I'm actually in favor of dropping the PR for those reasons. Maybe the pedantic flag suggested earlier would be a great idea. I've since moved on from the issue, but I do think it's important simply to update the documentation describing the caveats. I think it's a very minor issue overall.

@bryanwagner
Copy link
Contributor Author

Although, the internal inconsistency I found described in the OP between the two writers deserves a second look.

@libgdx libgdx deleted a comment Jul 6, 2019
@MrStahlfelge
Copy link
Member

@mgsx-dev @obigu It's now your chance to get this done. :)

@MrStahlfelge MrStahlfelge requested review from mgsx-dev and obigu August 31, 2020 13:23
@MrStahlfelge MrStahlfelge added core affecting all platforms bug fonts tools labels Aug 31, 2020
@obigu
Copy link
Contributor

obigu commented Aug 31, 2020

I agree with Tom-Ski:

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.

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).

@MrStahlfelge MrStahlfelge added the maintainer needed Needs maintainer decision label Aug 31, 2020
@NathanSweet
Copy link
Member

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.

@tommyettinger
Copy link
Member

I agree with @NathanSweet and @NumeronR that padding shouldn't affect the appearance of the font at all, since it's part of the info section of a .fnt file. I've personally made fonts with asymmetrical padding (hand-editing that section) and was very confused by the ordering; I almost always had to resort to trial and error to see which padding value affected which side of a glyph. Unfortunately, ignoring padding might also break the appearance of fonts that relied on its (incorrect) usage in libGDX. So my vote is to close without merging; using padding at all is incorrect, so other than the inconsistent writing order, Hiero can make .fnt fonts with padding that isn't to-spec and other tools shouldn't even notice. Inside libGDX, we can continue to be as incorrect as we always have been with .fnt loading and no code or assets should have to change.

@obigu
Copy link
Contributor

obigu commented Sep 1, 2020

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).

@obigu obigu closed this Sep 1, 2020
@MrStahlfelge
Copy link
Member

Would not help, this way we still wouldn't be compatible to the specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug controversial core affecting all platforms fonts maintainer needed Needs maintainer decision tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants