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

core: Fix text culling check #18418

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Conversation

britt-j
Copy link
Contributor

@britt-j britt-j commented Oct 28, 2024

The check relied on y_max being the same as height, which isn't the case since y_max can be negative
(close #16877)

@britt-j britt-j changed the title core: Fex text culling check core: Fix text culling check Oct 28, 2024

[image_comparisons."output"]
trigger = 1
tolerance = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what value this should have

Copy link
Collaborator

@adrian17 adrian17 left a comment

Choose a reason for hiding this comment

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

LGTM, but wanna make sure kjarosh approves too :)

@adrian17 adrian17 requested a review from kjarosh October 28, 2024 21:49
@kjarosh
Copy link
Member

kjarosh commented Oct 29, 2024

Thank you for your contribution!

There are several pitfalls that we can encounter in text-based tests.

It's important to make sure the test produces the same output as Flash Player. Unfortunately that's not the case for edittext_negative_bounds. If you open the SWF in FP, you can see that the displayed text differs a lot.

This is specifically caused by the fact that device fonts are used to render the text. It's the case despite having a font set, because useOutlines is false. It has to be set to true in order for embedded fonts to work. When you set it to true, note that it will not work properly because currently, the embedded font has 0 ascent and descent set.

In general, I recommend using fonts that are vastly different from the default ones. I personally use my own crafted fonts and commit them (which you can also just embed to the SWF), see for instance: https://github.com/ruffle-rs/ruffle/blob/e9926f9a2e4f518a4095d9c1643440031033f14f/tests/tests/swfs/visual/fonts/leading_define_font/TestFont.ttf. I recommend doing that because of several reasons:

  1. a test font does not have complex shapes,
  2. it's easy to spot that a wrong font is used,
  3. it's possible to modify properties of the test font and provide custom values.

Regarding the output, it's best if the output comes from the Flash Player, and not Ruffle. It's enough to screenshot FP and replace the output.expected.png with it. In that case, you'll need to set tolerance and outliers. I usually set outliers = 0 and tolerance = 128 (along with quality = 4), which is enough for black and white text-based tests. Setting scaleMode to noscale helps a lot when taking a screenshot.

@britt-j
Copy link
Contributor Author

britt-j commented Oct 29, 2024

Thanks for the info, I'm sorry if I missed that anywhere else.

I do have a few questions though, if you don't mind

I usually set outliers = 0 and tolerance = 128 (along with quality = 4)

Do you mean the sample_count in with_renderer?, because I can't find anything about quality in the test readme nor in the other tests in the edittext folder

Regarding the output, it's best if the output comes from the Flash Player, and not Ruffle.

I'm sorry if I've misunderstood what image tests are for, but I thought they were moreso regression tests for Ruffle specifically rather than accuracy tests?
I'm still going to take a screenshot from FP for this test, I'm just wondering

In general, I recommend using fonts that are vastly different from the default ones. I personally use my own crafted fonts and commit them (which you can also just embed to the SWF), see for instance: https://github.com/ruffle-rs/ruffle/blob/e9926f9a2e4f518a4095d9c1643440031033f14f/tests/tests/swfs/visual/fonts/leading_define_font/TestFont.ttf.

So it'd be alright if I used that font you linked, or should I make a different one for this?

@kjarosh
Copy link
Member

kjarosh commented Oct 29, 2024

Do you mean the sample_count in with_renderer?, because I can't find anything about quality in the test readme nor in the other tests in the edittext folder

Yes, I'm sorry, I meant sample_count.

I'm sorry if I've misunderstood what image tests are for, but I thought they were moreso regression tests for Ruffle specifically rather than accuracy tests? I'm still going to take a screenshot from FP for this test, I'm just wondering

Well, I guess there's no universal definition. Some tests in fact do that due to the fact that it's often extremely hard to get pixel-perfect output in Ruffle. In the case of simple text tests, I've put a lot of effort into making text near-pixel-perfect, so it should be fairly easy. Not to mention that we won't have to update the test when something changes in Ruffle, it should automatically pass (provided that something changes in the right direction ;) ).

So it'd be alright if I used that font you linked, or should I make a different one for this?

Sure thing, you can simply embed this TTF in JPEXS and you're good to go. No need to create a custom font. Creating a custom font would be beneficial if you wanted to test something specific related to that font, but here we just want to check whether the text renders. Just remember that this font defines only 4 characters if I remember correctly (a, b, c, d).

If you need some help with something just let me know, I can polish up the test :)

@kjarosh kjarosh added text Issues relating to text rendering/input A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) labels Oct 29, 2024
The check relied on `y_max` being the same as `height`, which isn't the case since `y_max` can be negative
(close ruffle-rs#16877)
@britt-j
Copy link
Contributor Author

britt-j commented Oct 30, 2024

@kjarosh don't know if the force pushes sent a notification, but if not, this should finally be ready now

Copy link
Member

@kjarosh kjarosh left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you!

@kjarosh kjarosh merged commit 8f4f9cc into ruffle-rs:master Oct 30, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core player, where no other category fits T-fix Type: Bug fix (in something that's supposed to work already) text Issues relating to text rendering/input
Projects
None yet
Development

Successfully merging this pull request may close these issues.

text invisible regression since nightly 2024-03-19
3 participants