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

2D Label smearing with ContentScaleSize #91740

Closed
oxygen opened this issue May 9, 2024 · 21 comments · Fixed by #92731
Closed

2D Label smearing with ContentScaleSize #91740

oxygen opened this issue May 9, 2024 · 21 comments · Fixed by #92731
Assignees
Milestone

Comments

@oxygen
Copy link

oxygen commented May 9, 2024

Tested versions

Reproduced on 4.3.dev5

Reproduced in the editor on Windows and on an iPhone XS Max. On the iPhone the smearing appears when transitioning from the default portrait loading screen to landscape which is an unavoidable resize (the only allowed mode in settings). Changing the text on the label 2 seconds later fixes the smearing (workaround).

System information

Godot v4.3.dev5.mono - Windows 10.0.19045 - GLES3 (Compatibility) - Radeon (TM) RX 480 Graphics (Advanced Micro Devices, Inc.; 31.0.21910.5) - Intel(R) Core(TM) i7-4770K CPU @ 3.50GHz (8 Threads)

Issue description

image
image

Steps to reproduce

I'm scaling and repositioning 2D elements.

Setting GetTree().Root.ContentScaleSize causes permanent smeared text, sometimes, after doing a resize.

Calling label.ForceUpdateTransform(); with a 100ms delay also causes the smearing.
Updaring the .Text property with new text (different text) with a 100ms delay fixes the smearing.

Here's the code from the repro project:

using Godot;

public partial class Smear : Node
{
	private Label label = new Label();
	private int position = 1;

	public override void _Ready()
	{
		GetTree().Root.ContentScaleMode = Window.ContentScaleModeEnum.CanvasItems;
		GetTree().Root.ContentScaleAspect = Window.ContentScaleAspectEnum.KeepHeight;

		label.LabelSettings = new LabelSettings();
		label.LabelSettings.FontColor = new Color(1, 0, 0, 1);
		label.LabelSettings.Font = ResourceLoader.Load<Font>("res://CourierPrime-Regular.ttf");
		label.LabelSettings.OutlineColor = new Color(1, 0, 0, 1);
		label.Text = "Keep resizing then pause resizing when smeared. It takes a bit of skill.";
		label.HorizontalAlignment = HorizontalAlignment.Left;
		AddChild(label);
		label.LabelSettings.FontSize = 20;

		GetViewport().Connect(Viewport.SignalName.SizeChanged, new Godot.Callable(this, "DidChangeSize"));

		label.Position = new Vector2(x: 50, y: 200);
	}

	public void DidChangeSize()
	{
		var viewport = GetViewport();
		var window = viewport.GetWindow();
		var viewportSize = window.Size;
		var windowHeight = viewportSize.Y;
		var scale = windowHeight / 648;
		label.LabelSettings.FontSize = 20 * scale;


		// No smear without this:
		GetTree().Root.ContentScaleSize = new Vector2I(viewportSize.X, viewportSize.Y);


		// Calling .ForceUpdateTransform() on NOT smeared text in another frame (or 100 ms later?) causes the smear to appear.
		// Otherwise the smearing is random but frequent.
		label.ForceUpdateTransform();


		// Note 1
		// Updating the .Text property on the label with a *changed* text fixes the smear.
		// But it has to be in another frame (or 100 milliseconds later, IDK).

		
		// Note 2
		// I didn't add the code to delay call label.ForceUpdateTransform() for extra smearing scenario
		// or delay update .Text for fixing text as it is unnecessary.
	}
}

Minimal reproduction project (MRP)

label2d-smear.zip

@oxygen
Copy link
Author

oxygen commented May 10, 2024

By the way, I suspect it is not implemented, so do you think I should create a feature request for special treatment of the Label text in this scenarios such as keeping it "vectorial" so it would be crisp when stretching/resizing?

Honestly it feels more of a bug that it has to become blurry since enough information exists to keep it looking good (docs do say it will become blurry - yet there's no alternative either).

@oxygen
Copy link
Author

oxygen commented May 10, 2024

Also reproduces on Android after transitioning from portrait loading screen to landscape forced mode.

@Calinou
Copy link
Member

Calinou commented May 21, 2024

cc @bruvzg

This is likely a bug in font oversampling, which you can avoid by using MSDF fonts (as they don't require oversampling or even request it in the first place).

You can also disable font oversampling in the project settings as a workaround, but this will make non-MSDF fonts blurry at resolutions higher than the base window size.

By the way, I suspect it is not implemented, so do you think I should create a feature request for special treatment of the Label text in this scenarios such as keeping it "vectorial" so it would be crisp when stretching/resizing?

MSDF fonts already achieve this since Godot 4.0.

@oxygen
Copy link
Author

oxygen commented May 22, 2024

Changing the code to this:

var font = ResourceLoader.Load<FontFile>("res://CourierPrime-Regular.ttf");
font.MultichannelSignedDistanceField = true;
label.LabelSettings.Font = font;
  • works around the the smearing bug beautifully
  • fixes the blurring
  • and is the feature request I was proposing.

Just amazing

Thank you @Calinou

@oxygen
Copy link
Author

oxygen commented May 22, 2024

I would propose users are expressly directed in the docs straight to FontFile and not Font (and explained why) with MultichannelSignedDistanceField enabled by default in the API (I mean on the Font docs page, using a yellow background or smth).

@Calinou
Copy link
Member

Calinou commented May 22, 2024

with MultichannelSignedDistanceField enabled by default in the API (I mean on the Font docs page, using a yellow background or smth).

We can't change default values without breaking compatibility with existing projects. Also, MSDF fonts have downsides that make them unsuitable for pixel art fonts, or fonts drawn at small sizes (due to the lack of hinting).

@bruvzg bruvzg self-assigned this May 22, 2024
@oxygen
Copy link
Author

oxygen commented Jun 3, 2024

with MultichannelSignedDistanceField enabled by default in the API (I mean on the Font docs page, using a yellow background or smth).

We can't change default values without breaking compatibility with existing projects. Also, MSDF fonts have downsides that make them unsuitable for pixel art fonts, or fonts drawn at small sizes (due to the lack of hinting).

Godot is rising fast.
The sooner you rip the band aid the better for Godot (usability / ease of use needs to come first).

If surprises on a particular thing are super bad then you can rename properties as part of the change.

With or without renaming, a little advertising for people to learn about MSDF is not a bad deal at all.

If MSDF is affecting bitmap fonts negatively it could come disabled by default for them. For example, instead of a boolean you could have an enum with mode values, defaulting to AUTO (MSDF on for vectorrized glyph fonts, and off for bitmaps).

In other similar engines (SceneKIT), you don't get to have the blur problem to solve, and all the reading up on MSDF isn't even needed (not even documented on apple developer afaik - not saying no docs is good, but efforts spent elsewhere problably are).

Edit: this post got a downvote faster than it takes to read it. Nice.

@AThousandShips
Copy link
Member

The sooner you rip the band aid the better for Godot (usability / ease of use needs to come first).

This isn't a reasonable justification to break people's projects, it's not that trivial, and it hurts usability to have to change your project ever update because compatibility was broken

Also some people read faster than you expect 😆

@oxygen
Copy link
Author

oxygen commented Jun 3, 2024

The sooner you rip the band aid the better for Godot (usability / ease of use needs to come first).

This isn't a reasonable justification to break people's projects, it's not that trivial, and it hurts usability to have to change your project ever update because compatibility was broken

Also some people read faster than you expect 😆

Yes it is, because the alternative is breaking new projects, which are plenty more and on the newest version. Older projects expect changes when upgrading

and having to configure a compiler error which says "this is an enum not a boolean" takes like 3 seconds.

@AThousandShips
Copy link
Member

Yes it is, because the alternative is breaking new projects, which are plenty more and on the newest version.

What are you basing this on? New projects aren't broken, just change the setting, but older projects that don't want it now suddenly having their visuals break and no explanation? That's hard to solve

Having to change a setting or two to get things looking right is a minor inconvenience, best alleviated in the meantime by documentation

Having your project break with no warning and having to spend a lot of time figuring it out and then updating all your settings is a major source of annoyance and is against all principles of good user experience

@oxygen
Copy link
Author

oxygen commented Jun 3, 2024

What are you basing this on? New projects aren't broken, just change the setting, but older projects that don't want it now suddenly having their visuals break and no explanation? That's hard to solve

Hmm, the compiler error is enough to catch on immediately. It is not a silent change.

@AThousandShips
Copy link
Member

Hmm, the compiler error is enough to catch on immediately. It is not a silent change.

There isn't a compiler error, most of the time you don't set this from code, but in importing or from the inspector

And how do you propose handling it when no one sets the value? Bow suddenly it's a different value without any code, because it's the default

@oxygen
Copy link
Author

oxygen commented Jun 3, 2024

Hmm, the compiler error is enough to catch on immediately. It is not a silent change.

There isn't a compiler error, most of the time you don't set this from code, but in importing or from the inspector

And how do you propose handling it when no one sets the value? Bow suddenly it's a different value without any code, because it's the default

We're not talking about general principles, please stay on point,.

And the point is: not having to set a setting which 100% of the developers will always set to true for glyph/vector fonts.

Nobody would ever scavenge on the forums to search for a way to bring the blur back.

@AThousandShips
Copy link
Member

We're not talking about general principles, please stay on point,.

This isn't a general principle conversation I'm talking about this exact case, and this isn't something you always want, if you haven't enabled it in your project you don't want it, this change would force you to use it...

@oxygen
Copy link
Author

oxygen commented Jun 3, 2024

this change would force you to use it...

It wouldn't force anyone to do anything in any way. They could configure any way they want.

I was pretty clear on this, several times.

@AThousandShips
Copy link
Member

No you've missed the details about the way the system works here:
If you change a default value, everyone is forced to use it, because it isn't stored so it will just use the default value if you didn't assign anything other than the original default value, this applies to everything in scripting (and it applies to import options as well if you change the type etc. as it won't have an option any more, even if the default value is stored in that case)

You might have been clear but you are missing details about what compatibility breakages in this case means

@oxygen
Copy link
Author

oxygen commented Jun 3, 2024

No you've missed the details about the way the system works here: If you change a default value, everyone is forced to use it, because it isn't stored so it will just use the default value if you didn't assign anything other than the original default value, this applies to everything in scripting (and it applies to import options as well if you change the type etc. as it won't have an option any more, even if the default value is stored in that case)

You might have been clear but you are missing details about what compatibility breakages in this case means

I told you you read too fast.

I did not propose changing a default value, and was quite ample about it including replying to you exactly on this several times already.

So I suggest you go back to my first post today and re-read it carefully.

@AThousandShips
Copy link
Member

AThousandShips commented Jun 3, 2024

Okay? I didn't say you suggested it, I responded with that breaking compatibility isn't a usability improvement, and you then responded with that changing things wouldn't break things, and that was the conversation, you responded with refutations of what would break compatibility or not

I read what you said, and I responded to it, please go back and follow along yourself, replacing the type with an enum is also a breaking change, as I responded to

Let's not continue this back and forth, I've expressed the reasons for not breaking compatibility, which your suggestion would regardless if we change the value or type, and that changing the type still does break things, I explained why an enum wouldn't change this fact :)

@bruvzg
Copy link
Member

bruvzg commented Jun 3, 2024

Should be fixed by #92731, the issue is primarily caused by the unexpected way your code sets ContentScaleSize. Setting ContentScaleSize will trigger SizeChanged and therefore calling it from the SizeChanged handler will cause multiple recursive updates without actual changes to the viewport size, which was not accounted for.

I would propose users are expressly directed in the docs straight to FontFile and not Font (and explained why)

The expected way to configure fonts (other resources) is setting required options in the font file import options, not from code. While you can do it, it will cause unnecessary font reloading and will prevent importer from caching pre-rendered MSDF glyphs (pre-rendered set can be set in advanced import dialog). Initial MSDF generation is not fast and pre-rendering at least most used glyphs is a good idea, especially for slow mobile devices.

with MultichannelSignedDistanceField enabled by default in the API

As I said, MSDF is much slower than usual fonts (only for initial generation, not drawing). Enabling it by default will cause significant performance penalty, without any benefits for majority of use case (it's only useful if you are scaling text a lot, or using text in 3D). Also, MSDF can't be used with bitmap fonts and non-OpenType complaint fonts with self-intersections (most fonts downloaded from Google Fonts, due to the way it's recompressing them).

@oxygen
Copy link
Author

oxygen commented Jun 3, 2024

"Enabling it by default will cause significant performance penalty"

I'm currently targeting low end Android devices. There was no noticeable effect on actual slow hardware. I don't have a lot of text, but isn't the bulk of processing when first loading the font anyway?

(I'm loading around 3-4 different fonts at the same time too, when the game is starting).

If the CPU usage comes from rendering text then do games even render as much text as a word processor would as so it would be a problem on low end hardware?

@akien-mga akien-mga added this to the 4.3 milestone Jun 4, 2024
@oxygen
Copy link
Author

oxygen commented Jun 4, 2024

btw, I had problems with self intersecting fonts way before enabling MSDF (with 3D text).

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

Successfully merging a pull request may close this issue.

6 participants