Broken dynamic fonts fix

[mdeletrain]

Hi arenmook !

I've encountered the infamous broken dynamic font in one of my project... again  !
But this time I decided to look at it and I think I have a solution :
From what I see it seems the only thing missing from your font changed callbacks is refreshing panels where labels have been updated : the panels are registered for refresh but that only happens next update, which means a single frame is wrong and let appear broken fonts.

The patch I provide as attachment (which applies against revision 9a77458) does that : remember all panels affected by font change and refresh them.
I don't know NGUI's inner working enough to optimize this, you probably know a faster way if that exists.

The patch does two other things :

• Use the new callback for Unity 4.6.7 since this version has it and spits warnings each time it recompiles NGUI when using the old one.
• Merge both callbacks into one, since everything needed is present and allow to only refresh needed labels

That works like a charm on my project and I hope you will integrate it very soon !


NB: You state in callback's documentation that Unity as 'a nice tendency [...]'  ! This is not a 'tendency' but a properly documented behavior.
They also state that people using the callback to render dynamic fonts on their own should request all displayed characters as well as new ones. I tried to implement that with current NGUI's sources but it requires a lot of overhead and modifications since the call site knows nothing about which labels are in used for that particular font, so I reverted and just let you know... Maybe you have an idea for that  ?

[ArenMook]

My understanding is that the font bug is something Unity fixed on their end just recently and will be found in a future version.

I'm also not sure what warnings you are referring to. I use Unity 4.6.7f1 here and don't see any warnings. I don't know why you need to do "!UNITY_4_6_7" there. What exact version of Unity do you have on your end?

[mdeletrain]

I'm using 4.6.7p1 here, on MacOS. The warning is about deprecated use of old callback system. Indeed, the local documentation does not mention this callback anymore, but the new one instead.

Without the above patch, the frame a prefab is loaded with new labels, every other labels are broken then fixed again the next frame.
This is because the font changed callback is called after the panels' update. Labels register themselves for changes but this is only taken into account the next update... the next frame.
This is reproducible 100% of the time.

Edit 1 : I should add that, while I'm using 4.6.7p1 on my desk, our build bot uses 4.6.7f1 and Android applications it outputs matches the behaviour I have on  my computer : 1 buggy frame without the patch, no more bug with the patch.

Edit 2 : this is the warning I get : Assets/ngui/UI/UILabel.cs(931,60): warning CS0618: `UnityEngine.Font.textureRebuildCallback' is obsolete: `Font.textureRebuildCallback has been deprecated. Use Font.textureRebuilt instead.'

[mdeletrain]

I updated Unity to 4.6.7p2 to be sure, and the bug is still here.

You'll find attached a project that exposes the bug (NGUI not included, of course ). Here is what it does :

• Display a simple 'Hello' label with a font big enough to nearly fill baking texture
• Wait for the user to click the screen
• Instantiate a 'World!' label prefab, which is disabled by default
• Wait a given frame count amount ('Set Active Frame Delay') before enabling the instance

Once enabled, the instance will take so much texture space it will fire a font texture rebuild.
If you set the 'Set Active Frame Delay' field to 0, everything works fine. If you set it to something >= 1, you can see 'Hello' garbled for one frame then update and fix itself the next frame.

I added a framerate limiter set to 1 FPS so that you have the time to see the garbled frame.
I also added a disabled DynamicFontFix component that does the same as what I proposed you in the patch, but it only works with Unity versions that supports the Font.textureRebuilt delegate, if NGUI registers itself to that event and not on the callback (that excludes Unity 4.6.7), need reflection to gain access to UILabel list of labels and is not guaranted to run after NGUI's delegate (not sure this point has an impact though). Just enable it to see the bug vanish even with 'Set Active Frame Delay' is set to something >= 1.

Don't forget to force clean font's baking texture between each run, or you won't see anything broken on the editor.

I hope that will help !

[mdeletrain]

For those of you who are interested, I also found why the DynamicFontFix class only works on Unity 5+ versions, and found a workaround :

First of all, the Font.textureRebuilt event has been added in Unity 4.6.7p1, so it won't compile on previous versions (I know no way to detect patch releases from sources).

The fix registers itself on Font.textureRebuilt event to fix the issue, but in Unity 4.6.7 (be it f1, p1 or p2) NGUI registers on each font's textureRebuildCallback instead.
It happens the textureRebuilt event is fired before textureRebuildCallback, so the fix is applied too soon, before NGUI performs its work.

The solution is to wrap textureRebuildCallback in this case. Here is the modified OnTextureRebuilt method of the DynamicFontFix class :

1.         void OnTextureRebuilt(Font font) {
2. #if UNITY_4_6_7
3.                 if (font.textureRebuildCallback != null) {
4.                         var previousCallback = font.textureRebuildCallback;
5.                         font.textureRebuildCallback = () => {
6.                                 previousCallback();
7. #endif
8.                                 var panelsToRefresh = new List<UIPanel>();
9.                                 foreach (var label in _labels) {
10.                                         if ((label.trueTypeFont == font) && (!panelsToRefresh.Contains (label.panel))) {
11.                                                 panelsToRefresh.Add(label.panel);
12.                                         }
13.                                 }
14.                                 foreach (var panel in panelsToRefresh) {
15.                                         panel.Refresh ();
16.                                 }
17. #if UNITY_4_6_7
18.                         };
19.                 }
20. #endif
21.         }
22.  

Again, please note this fix is only intended to wait until the patch provided in first post is accepted and applied to NGUI.
It will only compile and work starting with Unity 4.6.7p1.
I have no reliable way to make it work for earlier versions without touching NGUI's code .

[ArenMook]

Unfortunately I can't add it to NGUI. Few people update to patch revisions, and your code won't work unless it's on a patch revision of Unity. Like you said earlier, there is no way to detect if someone is on f1 or p1, and without it, people will get compile errors.

[mdeletrain]

Hi Aren !

Once I saw the textureRebuilt event was only made available since p1, I was pretty sure you could not enable my patch for that version. Anyway, the fix is still valid for Unity 5+.

Oops sorry... wrong statement... What is only working for p1 and above is the temporary DynamicFontFix component I created waiting for you to integrate the patch in NGUI if you accept it. The patch still remains valid for Unity 4 users (except for the changed ifdef that should be reverted).

[ArenMook]

Well, again, I can't include it. "textureRebuilt" does not exist in Unity 4.6.7f1. It was added in Unity 5, and somehow got snuck into Unity 4.6.7p1/

[mdeletrain]

Well, again , if you look at the patch I did not remove the call to your previous callback, keeping both versions.
The only thing is I excluded UNITY_4_6_7 from using it, which it happens must not be done, but apart from that everything else remains valid for Unity 4+.

To sum it up :

• I sent you a patch to fix the bug for all Unity versions you support
• I sent you a project that exposes the bug
• I sent you a temporary fix that allows to fix the bug starting from Unity 4.6.7p1 without having to modify NGUI

If you add the patch to NGUI, the temporary fix is unnecessary and everyone would have the fixed behavior, regardless of their Unity version (given that you revert the changes I made to #ifdefs)
I can still send you a patch without those modified #ifdefs if you want.

[ArenMook]

I had a closer look at the patch. I think I understand what you were talking about... Here's what I changed mine to:

1.         static void OnFontChanged (Font font)
2.         {
3.                 for (int i = 0; i < mList.size; ++i)
4.                 {
5.                         UILabel lbl = mList[i];
6.  
7.                         if (lbl != null)
8.                         {
9.                                 Font fnt = lbl.trueTypeFont;
10.  
11.                                 if (fnt == font)
12.                                 {
13.                                         fnt.RequestCharactersInTexture(lbl.mText, lbl.mFinalFontSize, lbl.mFontStyle);
14.                                         lbl.RemoveFromPanel();
15.                                         lbl.CreatePanel();
16.  
17.                                         if (mTempPanelList == null)
18.                                                 mTempPanelList = new List<UIPanel>();
19.  
20.                                         if (!mTempPanelList.Contains(lbl.panel))
21.                                                 mTempPanelList.Add(lbl.panel);
22.                                 }
23.                         }
24.                 }
25.  
26.                 if (mTempPanelList != null)
27.                 {
28.                         for (int i = 0, imax = mTempPanelList.Count; i < imax; ++i)
29.                         {
30.                                 UIPanel p = mTempPanelList[i];
31.                                 p.Refresh();
32.                         }
33.                         mTempPanelList.Clear();
34.                 }
35.         }
36.  
37.         static List<UIPanel> mTempPanelList;

Only I didn't use a lambda expression to assign a new delegate. I just used an anonymous delegate:

1.                                 if (!mFontUsage.TryGetValue(mActiveTTF, out usage))
2.                                         mActiveTTF.textureRebuildCallback = delegate() { OnFontChanged(mActiveTTF); };

Can you please confirm that this works?

[mdeletrain]

If you also reverted my changes to #ifdefs, it seems fine to me.

Just out of curiosity, is there any reason you do not use lambda expressions ?

[ArenMook]

I very vaguely remember something about adding lambda increasing either executable size or being slower than an anonymous delegate, but my memory on that topic is fuzzy. I just remember that delegates are better. I may be totally wrong, of course.

[mdeletrain]

Interesting point, I'll look into IL code to see what happens.

[mdeletrain]

Ok, I just tested and here is the result (sorry for naming ) :

The test class :

1. namespace foobarbaz {
2.  
3. public class FooBarBaz {
4.  
5.         delegate void Foo();
6.  
7.         Foo foo;
8.  
9.         FooBarBaz() {
10.                 var baz = "foobarbaz";
11.  
12.                 foo = () => Bar(baz);
13.                 foo();
14.  
15.                 foo = delegate { Bar(baz); };
16.                 foo();
17.         }
18.  
19.         void Bar(string text) {
20.                 System.Console.Out.WriteLine(text);
21.         }
22. }
23. }
24.  

IL code for class' constructor :

1.         // Methods
2.         .method private hidebysig specialname rtspecialname
3.                 instance void .ctor () cil managed
4.         {
5.                 // Method begins at RVA 0x20ec
6.                 // Code size 89 (0x59)
7.                 .maxstack 8
8.                 .locals init (
9.                         [0] class foobarbaz.FooBarBaz/'<FooBarBaz>c__AnonStorey0'
10.                 )
11.  
12.                 IL_0000: newobj instance void foobarbaz.FooBarBaz/'<FooBarBaz>c__AnonStorey0'::.ctor()
13.                 IL_0005: stloc.0
14.                 IL_0006: ldarg.0
15.                 IL_0007: call instance void [mscorlib]System.Object::.ctor()
16.                 IL_000c: ldloc.0
17.                 IL_000d: ldarg.0
18.                 IL_000e: stfld class foobarbaz.FooBarBaz foobarbaz.FooBarBaz/'<FooBarBaz>c__AnonStorey0'::'<>f__this'
19.                 IL_0013: ldloc.0
20.                 IL_0014: ldstr "foobarbaz"
21.                 IL_0019: stfld string foobarbaz.FooBarBaz/'<FooBarBaz>c__AnonStorey0'::baz
22.                 IL_001e: ldarg.0
23.                 IL_001f: ldloc.0
24.                 IL_0020: ldftn instance void foobarbaz.FooBarBaz/'<FooBarBaz>c__AnonStorey0'::'<>m__0'()
25.                 IL_0026: newobj instance void foobarbaz.FooBarBaz/Foo::.ctor(object, native int)
26.                 IL_002b: stfld class foobarbaz.FooBarBaz/Foo foobarbaz.FooBarBaz::foo
27.                 IL_0030: ldarg.0
28.                 IL_0031: ldfld class foobarbaz.FooBarBaz/Foo foobarbaz.FooBarBaz::foo
29.                 IL_0036: callvirt instance void foobarbaz.FooBarBaz/Foo::Invoke()
30.                 IL_003b: ldarg.0
31.                 IL_003c: ldloc.0
32.                 IL_003d: ldftn instance void foobarbaz.FooBarBaz/'<FooBarBaz>c__AnonStorey0'::'<>m__1'()
33.                 IL_0043: newobj instance void foobarbaz.FooBarBaz/Foo::.ctor(object, native int)
34.                 IL_0048: stfld class foobarbaz.FooBarBaz/Foo foobarbaz.FooBarBaz::foo
35.                 IL_004d: ldarg.0
36.                 IL_004e: ldfld class foobarbaz.FooBarBaz/Foo foobarbaz.FooBarBaz::foo
37.                 IL_0053: callvirt instance void foobarbaz.FooBarBaz/Foo::Invoke()
38.                 IL_0058: ret
39.         } // end of method FooBarBaz::.ctor
40.  

Both ways to write the delegate generated the exact same code ! Good to know.

[ArenMook]

Nice, thanks for trying it out. Good to know indeed!