Gamua / Starling-Framework

The Cross Platform Game Engine
http://www.starling-framework.org
Other
2.85k stars 819 forks source link

More data consistency #905

Closed EhsanMarufi closed 7 years ago

EhsanMarufi commented 7 years ago

The "texture" is a through-constructor initialized immutable field. The case of which the two arguments are null is the "default" approach to utilize the "minimal" embedded font, and the "typical" case is when the two arguments are provided correctly. The else statement is to assert for data consistency and considering the current implementation, there is no chance to "set" the "texture" nor the "XML" later, so both of them need to be provided "not null" initially. However, only the specific case of the texture != null && fontXml == null is evaluated leaving the case of texture == null && fontXml != null unregarded! To compensate the issue, change the criteria to texture == null || fontXml == null to regard the both of the "wrong" cases :)

PrimaryFeather commented 7 years ago

Thanks a lot for the pull request! You're definitely right, this is inconsistent and needs to be changed!

However, looking at the code, I just noticed that it might be useful to allow passing just a texture, no XML. Then, one could construct a BitmapFont at runtime via the "addChar" method:

var bitmapFont:BitmapFont = new BitmapFont(texture);
bitmapFont.addChar(65, charA);
bitmapFont.addChar(66, charB);
// etc.

That way, people could e.g. write parsers for other BitmapFont file formats. What do you say — should we do that instead?

EhsanMarufi commented 7 years ago

@PrimaryFeather Hi Daniel, and thank you for the response. I do admire your great job and the development prospects you have in mind to make it even better :) To make the BitmapFont even a better base class letting people implement custom "bitmap fonts", more effort is required than just some consistencies. For example, regarding the current implementation, the following statement in your sample code:

var bitmapFont:BitmapFont = new BitmapFont(texture);

throws the ArgumentError exception of "fontXml cannot be null!" More importantly, the "interface" of the class and possibly other "core" classes' are probably to be changed, besides the mandatory changes required to the implementation of the class (to change the access modifier of the corresponding fields accordingly, to be "protected", for example; or to consider a more general alternative "default" approach, or whatsoever!)

It's becoming a TL;DR if I don't get back to the point! Another fall back would be an attempt to new BitmapFont(null, validXML);, through which the data is considered consistent, while a "null object reference" exception is expected!

I think, to make the current code more stable, until a major reconsideration, the proposed pull request is enough!

PrimaryFeather commented 7 years ago

You're right, the proposed pull request is a very good first step - and I might revisit the topic for the next update. Thanks again! :smile: