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

SDF-based mechanism for making letters bold #3385

Merged
merged 23 commits into from
Apr 12, 2022

Conversation

farmaazon
Copy link
Contributor

@farmaazon farmaazon commented Apr 7, 2022

Pull Request Description

Fixes 181641027. There are new methods of text::Area for making some letters bolder, without moving them nor any other letters in the line.

Tested on DejaVuSans text:
image
image
image

On MacOS:
Screenshot 2022-04-07 at 14 54 40
Screenshot 2022-04-07 at 14 54 52
Screenshot 2022-04-07 at 14 54 58

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

@farmaazon farmaazon self-assigned this Apr 7, 2022
@farmaazon farmaazon marked this pull request as ready for review April 7, 2022 12:57
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Beautiful! ✨👀✨

self.display_object.add_child(&glyph_system);
let old_glyph_system = self.glyph_system.replace(glyph_system);
self.display_object.remove_child(&old_glyph_system);
// remove old Glyph structures, as they still refer to the old Glyph System.
Copy link
Member

Choose a reason for hiding this comment

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

r -> R

highp float median(highp vec3 v) {
return max(min(v.x, v.y), min(max(v.x, v.y), v.z));
}

// To smaple the msdf properly, the boundaries of our uv should be in the middle of msdf cells.
Copy link
Member

Choose a reason for hiding this comment

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

smaple

Copy link
Member

Choose a reason for hiding this comment

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

Plus, I dont understand this comment – what do you mean by "boundaries" to be in the middle of msdf cells? Could you explain it better, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the name and comment. Would you like to check if it's better?

bool glyph_is_bold = (input_style & STYLE_BOLD_FLAG) != 0;
highp vec2 local_to_px_ratio = 1.0 / fwidth(input_local.xy);
highp float font_size_px = input_font_size * (local_to_px_ratio.x + local_to_px_ratio.y) / 2.0;
return glyph_is_bold ? font_size_px * BOLD_FATTING : 0.0;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of bold / not bold the user might set how much bold the font sohuld be – this will be even a nicer setting – new fonts are made this way (see dynamic bold fonts on google fonts). What do you think of such a design instead? I'm OK with the current one though!

Copy link
Contributor Author

@farmaazon farmaazon Apr 8, 2022

Choose a reason for hiding this comment

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

The question is if we want to use Bold-version of fonts in the future. If yes, then I would keep consistent API.

Comment on lines +1 to +3
// A factor describing much the bold letters will be fattened, expressed as the fraction of font size.
const float BOLD_FATTING = 0.04;

Copy link
Member

Choose a reason for hiding this comment

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

the file for non-mac has more changes, is it intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, not all realignments/refactorings were moved. However, logic changes should be the same.

@MichaelMauderer
Copy link
Contributor

I did not do a review of this, but just a note: Is this meant to be a replacement for actually using a bold font? This will not do many of the things that will happen if a properly bolded font is used (like updating the letter spacing etc., so this can lead to many visual glitches if used improperly, and should try to do proper font rendering instead.

self.style.get() & style_flag::BOLD != 0
}

pub fn set_bold(&self, value: bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be super careful about the naming. This is not proper bolding of the text. We should reserve the default names for the proper implementation that updates the font correctly.

Copy link
Member

Choose a reason for hiding this comment

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

very good catch, thats really important!

farmaazon and others added 2 commits April 11, 2022 12:27
Solves the issue of defaulted args not being called in atoms. Doesn't solve the more general function issue.
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Let's merge it! Few notes:

  1. The Michaels comment about naming is very important, I completely overlooked it during my review.
  2. "Normal" fonts are using float as bold indicator more nad more often. Almost all web fonts are moving now to "variable fonts" definition (https://fonts.google.com/knowledge/using_type/styling_type_on_the_web_with_variable_fonts). This is just FYI, we dont need to support it really so a flag bold/not bold is enough for us :)

@farmaazon farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Apr 11, 2022
@farmaazon farmaazon removed the CI: Ready to merge This PR is eligible for automatic merge label Apr 11, 2022
@akavel
Copy link
Contributor

akavel commented Apr 12, 2022

Acceptance/QA passed.

Note: For QA, I extended the debug scene to make it easier to visually inspect that the width and positions of characters are maintained. Some complex unicode is not rendered correctly by text::Area, but fixing that is not in the scope of this task. The SDF mechanism appears to be working correctly even in complex cases and on characters composed of disjoint areas.

Screenshot 2022-04-12 at 13 01 53

@farmaazon farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label Apr 12, 2022
@farmaazon farmaazon force-pushed the wip/farmaazon/text-sdf-bold-181641027 branch from 9cdfc6d to 2ee2a65 Compare April 12, 2022 16:12
@mergify mergify bot merged commit 8ca4e2b into develop Apr 12, 2022
@mergify mergify bot deleted the wip/farmaazon/text-sdf-bold-181641027 branch April 12, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants