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

Implement a FontUsage class, replacing simple font names #2043

Merged
merged 30 commits into from
Feb 21, 2019

Conversation

FreezyLemon
Copy link
Contributor

@FreezyLemon FreezyLemon commented Dec 24, 2018

The assumption for font names here is either just the font family name, or family-[weight]["Italic"] (both weight and "Italic" being optional).

While old methods of setting font attributes will still work, they are now marked as [Obsoleted]. You should use Font = new FontUsage(size: 200) going forward. For convenience, you can adjust existing fonts like so:

var font = new FontUsage(family: "SourceCodePro");

var fontWithSize = font.With(size: 24);

@smoogipoo
Copy link
Contributor

I'm not sure we want to complicate this by having events and mutability. How about making FontUsage a readonly struct?

@FreezyLemon
Copy link
Contributor Author

Good point, I'll change it to something simpler as soon as I find the time

@smoogipoo
Copy link
Contributor

After discussing with @peppy , it seems we want to have the following sort of structure, similar to that of OsuColour:

OsuFont:
    FontUsage Header1 = ...;
    FontUsage Header2 = ...;
    FontUsage Monospace = ...;

For this to work FontUsage should also contain TextSize and FixedSize as in the issue description.

@FreezyLemon
Copy link
Contributor Author

Sure! But this would mean (with a readonly struct) that for every change in text size, a new FontUsage struct would have to be created. I guess that's fine? Just want you to be aware that this changes the usage from

sprite1.TextSize = 150

to

sprite1.Font = new FontUsage("OpenSans", 150).

@luaneko
Copy link
Contributor

luaneko commented Dec 27, 2018

You could make TextSize a helper property which reconstructs FontUsage with a new size.

@FreezyLemon
Copy link
Contributor Author

Good idea! Maybe I should do the same for FixedWidth too.

That way, it's still easy to change these properties but they're both contained in the new struct so the approach (Header1 = ...; Header2 = ...) outlined by smoogi above still works.

@FreezyLemon
Copy link
Contributor Author

Something I forgot about in this...
With this approach, if someone uses something like

new SpriteText
{
    Font = new FontUsage("OpenSans"),
    TextSize = 100,
};

there is a possibility of the text size being reset/ignored if the line with "Font = ..." is executed after the TextSize setter. No real way around it unless you just don't use TextSize in a FontUsage initializer.

Might just revert this change to completely work around the issue.

@luaneko
Copy link
Contributor

luaneko commented Dec 29, 2018

I think it's fine. TextSize is just a helper property for Font. It's the same concept as Size, which is just a helper property for Width and Height. Same goes for Position, and probably many others. Doing something like

new Container
{
    Width = 100,
    Size = new Vector2(200)
}

renders the width initialiser completely redundant. You might as get rid of every helper property if you're worried.

@FreezyLemon
Copy link
Contributor Author

FreezyLemon commented Jan 5, 2019

While it is true that there are other properties that are somewhat redundant, I think there is a difference here:

When you see an initializer like the one you posted, it seems obvious that one of these properties overrides the other (a Size includes a width and a height).
With my example, it doesn't seem as obvious (associating "font" with text height is not usual imho, most people would probably associate font family and maybe weight with the term "font"), especially considering that you currently need to use the TextSize in conjunction with Font AND that the text size does not have to be explicitly specified since there is a default value.

In addition, with what smoogi mentioned above (using pre-defined FontUsages), I don't think this is such a big issue.

@peppy peppy added this to the January 2019 milestone Jan 7, 2019
@FreezyLemon
Copy link
Contributor Author

Keeping the comment directly above in mind, how should I handle this now?

There are a lot (~55) simple sprite.TextSize = X initializers / statements just in the framework.
To get this into a compiling state without the TextSize property, I'd either have to replace all these manually with Font = new FontUsage("OpenSans", X) (too verbose?) or maybe start adding some pre-defined usages already. Not sure how to proceed.

@peppy
Copy link
Sponsor Member

peppy commented Jan 15, 2019

I think it's fine to update these existing usages, yes. Do note that you will need to make family optional (default null) so you don't need to specify the fallback (in the framework's case, OpenSans).

@FreezyLemon
Copy link
Contributor Author

Quick note that with the family being null, italics and font weight don't work. This is due to how the fallback font is "selected". I am not sure if it's viable to change the whole texture routine to fix this... especially performance-wise

@peppy peppy modified the milestones: January 2019, February 2019 Feb 5, 2019
# Conflicts:
#	osu.Framework.Tests/Visual/TestCaseScreen.cs
#	osu.Framework/Graphics/Sprites/SpriteText.cs
#	osu.Framework/Testing/Drawables/TestCaseButton.cs
@smoogipoo smoogipoo mentioned this pull request Feb 20, 2019
1 task
@smoogipoo
Copy link
Contributor

I've adjusted this PR with what I see are required changes to get things working nicely. Of note:

  1. Old members have been added back temporarily, which throw build-time + resharper warnings.
  2. FontUsage has a new .With() method allows adjustment of fonts while preserving other untouched members.

I want to keep the obsolete members in for a month or two or so, just to give people enough time to migrate away from it (it's a pretty intrusive change).

I believe this PR is ready now, @peppy requesting review.

@smoogipoo smoogipoo requested a review from peppy February 20, 2019 11:48
@peppy peppy merged commit 09e939f into ppy:master Feb 21, 2019
@FreezyLemon FreezyLemon deleted the implement-fontusage branch April 2, 2020 23:28
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.

None yet

4 participants