Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

cleaned up SpriteFont implementation #2146

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants

jpatte commented Dec 3, 2013

I came across the current implementation for SpriteFonts and figured I could DRY it a bit.

Member

mgbot commented Dec 3, 2013

Can one of the admins verify this patch?

Member

mgbot commented Dec 3, 2013

Can one of the admins verify this patch?

Contributor

totallyeviljake commented Dec 3, 2013

Tested this in cocos2d-xna and it appears to not have degraded spritefont.

@Nezz Nezz commented on the diff Dec 4, 2013

....Framework/Content/ContentReaders/SpriteFontReader.cs
- {
- // Read the texture into the existing texture instance
- input.ReadObject<Texture2D>(existingInstance._texture);
-
- // discard the rest of the SpriteFont data as we are only reloading GPU resources for now
- input.ReadObject<List<Rectangle>>();
- input.ReadObject<List<Rectangle>>();
- input.ReadObject<List<char>>();
- input.ReadInt32();
- input.ReadSingle();
- input.ReadObject<List<Vector3>>();
- if (input.ReadBoolean())
- {
- input.ReadChar();
- }
+ var texture = existingInstance != null ? input.ReadObject<Texture2D>(existingInstance._texture) : input.ReadObject<Texture2D>();
@Nezz

Nezz Dec 4, 2013

Member

I think this would be cleaner for the texture:

input.ReadObject<Texture2D>(existingInstance != null ? existingInstance._texture : null)
@jpatte

jpatte Dec 4, 2013

Ah yes this is cleaner indeed. I didn't look at ReadObject()'s implementation so I didn't assume ReadObject<Texture2D>() was equivalent to ReadObject<Texture2D>(null), but it seems to be the case.

Contributor

KonajuGames commented Jan 8, 2014

This is the type of pull request that we don't normally accept. If there is an actual fix in there, it is lost in the noise of all the other changed lines. Because there are so many changes, the risk of introducing an unintended bug is quite high.

Owner

tomspilman commented Jan 10, 2014

Looking at it with whitespace changes disabled...

https://github.com/mono/MonoGame/pull/2146/files?w=0

... makes it a bit more readable.

@jpatte - Other than plain formatting changes... what actual improvements are there to the implementation?

Owner

tomspilman commented Jan 18, 2014

@jpatte - Other than plain formatting changes... what actual improvements are there to the implementation?

jpatte commented Jan 18, 2014

Clearly, this is not a bugfix; it's a refactoring.

It does fix some minor bug though: in the current implementation MeasureString() returns values which do not always correspond to the actual size of the text drawn by DrawInto(). That's because these two methods do exactly the same thing (iterate over the text, identify the glyphs to render, adjust glyph positions based on kerning information etc), yet they implement it differently. It's fine in most cases but it gets messy when strings to render contain white spaces and new line characters.

What I suggest is to reuse the same processing logic for both methods, hence reducing the amount of code involved and avoiding potential issues like this. A smaller, cleaner code is first meant to reduce the risk of bugs - and, if there are still any, to make these easier to fix.

I understand your reticence though; the goal of current developments is to stabilize the framework by fixing bugs and implementing any missing features on all devices. Plus the framework is unlikely to evolve in the future so a code that already works doesn't need to be touched again. I get that, and I won't be upset if you decide to reject this request for these reasons.

However I think this project might also be used as a good inspiration for similar frameworks and/or as reference when one needs to reimplement some features. In my case I needed to implement a font renderer for fonts generated by the software Bitmap Font Generator. Starting from SpriteFont's implementation seemed natural. Yet that implementation, even if it works (although not perfectly), was a real pain to adapt to another font format. I basically had to do the same thing twice, yet differently. So when I saw that I could easily make the code easier to review/maintain/reuse, I just went ahead and did it. Here is the result, do whatever you want with it. ;)

Contributor

KonajuGames commented Mar 13, 2014

Closing this as it is a large amount of unnecessary refactoring. There was another fix for whitespace in MeasureString merged a few days ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment