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

Fix font selection #10181

Merged
merged 5 commits into from
Mar 19, 2023
Merged

Fix font selection #10181

merged 5 commits into from
Mar 19, 2023

Conversation

Dinnerbone
Copy link
Contributor

@Dinnerbone Dinnerbone commented Mar 17, 2023

There's 2 issues that completely break fonts atm:
1 - We make empty movies for AVM2-made display objects. Whilst I don't know if "the movie that was running when it was made" is correct, it's definitely more correct than empty movie. This meant they had an empty library to find fonts from.
2 - When searching for a font, use any with the same name if there isn't an exact match. I suspect we're supposed to merge fonts together and only have one by unique name, and Flash normally enforces uniqueness, but third party software allowed partial fonts with the same name. We can investigate that later.

However, the fact that we now use the right movie when looking up fonts broke tests. I fixed some of it by using is_device_font in bullet lookup, but other tests are wrong and I have no idea. I'd welcome any thoughts or help on figuring it out.

The tests that are broken had embedded Noto Sans, and Ruffle's "device font" also happens to be Noto Sans. These tests previously incorrectly fell back on the device font and not the embedded font, and now that they're using the embedded font something's broken.

@danielhjacobs
Copy link
Contributor

Haven't those same tests always been broken from a visual perspective: #1167. Maybe this is just exposing that issue?

@Dinnerbone
Copy link
Contributor Author

Haven't those same tests always been broken from a visual perspective: #1167. Maybe this is just exposing that issue?

It very well could be, but I suspect we've made a lot of font stuff wrong by trying to get the numbers to match up with the incorrect font in these tests. We have to investigate what's actually correct.

@Herschel
Copy link
Member

Herschel commented Mar 17, 2023

1 - We make empty movies for AVM2-made display objects. Whilst I don't know if "the movie that was running when it was made" is correct

This is right for text fields; IIRC from when I looked at this in the past, the text field indeed remembered what movie it was created from, and would grab the font from there, even if you re-parented the text field to a different SWF -- I had tested by loading an SWF in an SWF, both with the same font name but different fonts. The text field would use the font from its original SWF despite my best efforts to make it get confused.

  • Dynamically made EditTexts should use embedded fonts by default, not device fonts.

Is this true? TextField.embedFonts defaults to false, and you can try displaying some text with a font to see that it won't display when the font isn't embedded:

var tf:TextField = new TextField();
trace(tf.embedFonts); // false
addChild(tf);
tf.autoSize = flash.text.TextFieldAutoSize.LEFT;
tf.defaultTextFormat = new TextFormat("Arial");
tf.text = "TESTING";
tf.embedFonts = true; // Comment out for the text to appear
  • When searching for a font, use any with the same name if there isn't an exact match.

For text fields defined on the timeline specifically, one issue is we are converting from character ID -> font name, and then the layout code is later grabbing the font based on the font name later, giving incorrect results for conflicting fonts. The font name is completely unused in this case, and we should be grabbing the font using the character ID.

@Dinnerbone
Copy link
Contributor Author

Is this true? TextField.embedFonts defaults to false, and you can try displaying some text with a font to see that it won't display when the font isn't embedded:

var tf:TextField = new TextField();
trace(tf.embedFonts); // false
addChild(tf);
tf.autoSize = flash.text.TextFieldAutoSize.LEFT;
tf.defaultTextFormat = new TextFormat("Arial");
tf.text = "TESTING";
tf.embedFonts = true; // Comment out for the text to appear

Dammit I might be hit by using a font that I've got installed. You're right, I'll undo that commit.

@Dinnerbone
Copy link
Contributor Author

I have no idea how to make the text render more like Flash with this. I'm totally stumped and will appreciate someone else taking a stab at it.

What we had before this PR was/is wrong. We're never using the swf-embedded font in any of the tests, and outside of static things placed from the timeline, we never do in Ruffle today. So in a way, this PR makes text rendering much more correct. But it visually looks different and so some effort should be made to make it look better, and I have no idea where that difference is creeping in.

@Dinnerbone
Copy link
Contributor Author

Okay, I've lost sleep over this.

Opening the test fla's and saving them... fixes them. It could either be that my version of Noto Sans is newer than the one that was originally used (perhaps closer to the one it was mistakenly pulling from Ruffle?) or that I used a different version of Flash (CS6, originally they were made in Animate).

Either way, recreating the tests gave different results and they mostly fit.

I wouldn't say that text rendering is more correct in this PR, but I don't think it's less correct either. It's just different. At the very least, we're selecting the correct font now.

We really need to revisit the inaccuracies in text rendering though.

@Dinnerbone Dinnerbone marked this pull request as ready for review March 18, 2023 05:14
Copy link
Member

@Herschel Herschel left a comment

Choose a reason for hiding this comment

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

Thank you! Can you clarify what exactly changed in the test FLAs because it's difficult to tell from the diff?

Seems like the font map needs to change to something like HashMap<FontName, FontEntry> where:

struct FontEntry {
  normal: Option<Font>,
  bold: Option<Font>,
  italic: Option<Font>,
  bold_italic: Option<Font>,
}

So that each style has one possible entry, and this gets filled in/falls-back appropriately. If there is a conflict where both the name and the style is the same, the first defined font in the SWF wins.

Sample SWFs:
font-conflict.zip
3 fonts all named "TestFont", one bold, the other two regular.
When using regular, the first regular font defined in the SWF wins (font-conflict.swf vs font-conflict2.swf)

@Dinnerbone
Copy link
Contributor Author

Dinnerbone commented Mar 18, 2023

I have absolutely no idea what truly changed to make the font render properly. I just opened and resaved them. It's either that my Noto Sans is different to the one originally used, or that my flash version was different, or both.

The different values reported is definitely from me having a different version of the font, I can see the differences in FFDEC. But the old swf rendered wrong and the new one doesn't. No clue.

@Dinnerbone Dinnerbone force-pushed the font_fallback branch 2 times, most recently from 4c7b9a9 to 8d9411c Compare March 19, 2023 04:53
@Dinnerbone Dinnerbone enabled auto-merge (rebase) March 19, 2023 04:54
@Dinnerbone Dinnerbone merged commit ae25ace into ruffle-rs:master Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants