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

Support for all BMFont output file types in FlxBitmapFont #2949

Merged
merged 32 commits into from
Dec 15, 2023

Conversation

UncertainProd
Copy link
Contributor

FlxBitmapFont.fromAngelCode used to work only with XML files generated by BMFont, but BMFont can also generate text or binary file descriptors. This pr should add support for all BMFont output format types. It also deprecates FlxAngelCodeXmlAsset which is now renamed to FlxAngelCodeAsset since it can also support strings and bytes

@Geokureli
Copy link
Member

are there additional features we could gain from these other file formats, or is this just for variety?

@UncertainProd
Copy link
Contributor Author

It's mainly to support all the 3 types of font descriptors that BMFont provides. The binary format, for example usually takes up less space than the text and xml format
image

@Geokureli
Copy link
Member

Geokureli commented Nov 8, 2023

whoops, got distracted, got any sample files I can use to test this, preferably 3 different files formats made from the same font

@UncertainProd
Copy link
Contributor Author

Sure, I used this font: https://emhuo.itch.io/nico-pixel-fonts-pack
Test_Font_DigitalPup.zip

Source code that I used:

package;

import flixel.FlxG;
import flixel.FlxState;
import flixel.graphics.frames.FlxBitmapFont;
import flixel.text.FlxBitmapText;

class PlayState extends FlxState
{
	var bmText:FlxBitmapText;
	var curIndex = 0;

	var tests:Array<{ font: FlxBitmapFont, text: String }>;
	override public function create()
	{
		super.create();
		var testXMLFont:FlxBitmapFont = FlxBitmapFont.fromAngelCode('assets/images/DigitalPup.png', 'assets/images/BMFont XML/DigitalPup.fnt');
		var testtxtFont:FlxBitmapFont = FlxBitmapFont.fromAngelCode('assets/images/DigitalPup.png', 'assets/images/BMFont Text/DigitalPup.fnt');
		var testBinaryFont:FlxBitmapFont = FlxBitmapFont.fromAngelCode('assets/images/DigitalPup.png', 'assets/images/BMFont Binary/DigitalPup.fnt');

		tests = [
			{
				font: testXMLFont,
				text: 'This font uses XML'
			},
			{
				font: testtxtFont,
				text: 'This font uses Text'
			},
			{
				font: testBinaryFont,
				text: 'This font uses Binary'
			},
		];

		bmText = new FlxBitmapText(0, 0, tests[curIndex].text, tests[curIndex].font);
		bmText.scale.scale(2.0);
		bmText.screenCenter();
		add(bmText);
	}

	function changeFontTo(index: Int)
	{
		bmText.text = tests[index].text;
		bmText.font = tests[index].font;
	}

	override public function update(elapsed:Float)
	{
		super.update(elapsed);
		if(FlxG.keys.justPressed.LEFT)
		{
			curIndex--;
			if(curIndex < 0)
				curIndex = 2;
			changeFontTo(curIndex);
		}

		if(FlxG.keys.justPressed.RIGHT)
		{
			curIndex = (curIndex + 1) % 3;
			changeFontTo(curIndex);
		}
	}
}

@Geokureli
Copy link
Member

Geokureli commented Dec 8, 2023

Made some changes:

  1. the typedefs are now classes that use @:structInit
  2. Changed FlxAngelCodeAsset from a typedef to an abstract, with a parse() function
  3. the parsing of the BMFont object are now moved to their respective classes.
  4. made a generic BMFontUtil.forEachAttribute to iterate the key-value pairs of BMFont's text format, rather than using the ad-hoc per-char parser you made (I was actually getting infinite loops on some of my tests, and this prevents it)
  5. renamed the classes
  6. moved to the bmfont package
  7. added an optional letter field to BMFontChar, using id everwhere. Note: binary only has id, where .xml and and .txt seem to sometimes have a letter field, but ALWAYS have a an id in my tests, if you saw a case where id was missing, let me know
  8. renamed some class fields to more closely match the source data's field names

re - 1, 2, 3 and 4:
this should make it easier to make changes to individual classes across all parsers, it should improve some performance on desktop targets and is typically how I prefer to organize file data objects. this opens up the floor for more features inside these classes down the road, like the ability to merge font instances and use multi-atlas tools. and once code-climate is fixed to allow final vars we can make many these class fields final and get more perf benefits of having immutable kernings, chars and whatnot

What's left

  • I think this needs a unit test that loads a minimal font example in all three formats and verifies that the data in these files are being parsed and used correctly, even the optional kerning stuff

  • Some minimal docs explaining how to use these classes via FlxBitmapFont.fromAngelCode, and @since tags

@UncertainProd
Copy link
Contributor Author

UncertainProd commented Dec 9, 2023

I tested this on a few fonts I had and it seems to work (although I haven't tested with any fonts having kerning pairs yet).

binary only has id, where .xml and and .txt seem to sometimes have a letter field, but ALWAYS have a an id

Yeah I haven't seen any fonts without an id field so far
Edit: I tried using this font which had kerning pairs (after manually converting it using bmfont) and it seems to work too.

@Geokureli Geokureli mentioned this pull request Dec 12, 2023
@Geokureli Geokureli added this to the 5.6.0 milestone Dec 13, 2023
Copy link
Contributor

@MondayHopscotch MondayHopscotch left a comment

Choose a reason for hiding this comment

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

Overall this looks like a great addition. I had a couple comments around cleanup and one potential bug.

This also looks like a good place for a couple unit tests to get some coverage of the code.

flixel/graphics/frames/FlxBitmapFont.hx Outdated Show resolved Hide resolved
flixel/graphics/frames/FlxBitmapFont.hx Outdated Show resolved Hide resolved
Comment on lines +60 to +61
// var charCount = 0;
// var kerningCount = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't appear to be part of the spec, but I've seen tools like Hiero put these lines in. Do you know where they come from / why they are here? Maybe just so human eyes can spot check the file for accuracy?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know who Hiero is, but I assume the count is either to make sure you parsed correctly, or to aid in the parsing in some way

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops. Mean to add link, but obviously didn't. Hiero is a libGDX font tool that I've used before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had generated a few fonts using BMFont and I saw these tags there. It doesn't seem to be part of the binary file spec but I did see them in the text and xml outputs

flixel/graphics/frames/bmfont/BMFontChar.hx Outdated Show resolved Hide resolved
var xadvance:Int = 0;
var page:Int = -1;
var chnl:Int = -1;
var letter:String = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding a comment to call out that this isn't part of the spec and is here for debug purposes, if needed. (at least that's what I've gathered from this PR)

Copy link
Member

@Geokureli Geokureli Dec 14, 2023

Choose a reason for hiding this comment

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

this isn't part of the spec

what does "this" refer to?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://www.angelcode.com/products/bmfont/doc/file_format.html

Didn't realize this wasn't already in the PR description or something like that

Copy link
Contributor

Choose a reason for hiding this comment

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

And I was specifically calling out that letter isn't in the spec, in case that's what you were asking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I found that the letter field gets generated sometimes for the text/xml outputs (despite not being part of the spec) so I thought of adding it here just in case

Copy link
Member

@Geokureli Geokureli Dec 15, 2023

Choose a reason for hiding this comment

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

I'm not sure why and when letter appears in bmfont files, but it does, sometimes, so I figured it should be in the type as an optional field

flixel/graphics/frames/bmfont/BMFontInfo.hx Outdated Show resolved Hide resolved
flixel/graphics/frames/bmfont/BMFontInfo.hx Outdated Show resolved Hide resolved
flixel/graphics/frames/bmfont/BMFontInfo.hx Show resolved Hide resolved
flixel/graphics/frames/bmfont/BMFontUtil.hx Outdated Show resolved Hide resolved
@MondayHopscotch
Copy link
Contributor

@UncertainProd, I'm throwing together some unit tests for this that I'll put a PR against your branch hopefully later today along with a couple other changes related to my other comments.

@Geokureli
Copy link
Member

Thanks @UncertainProd and @MondayHopscotch !

@Geokureli Geokureli merged commit f68d033 into HaxeFlixel:dev Dec 15, 2023
16 checks passed
Geokureli added a commit that referenced this pull request Dec 15, 2023
@UncertainProd UncertainProd deleted the bmfont-parsing branch December 17, 2023 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants