-
Notifications
You must be signed in to change notification settings - Fork 323
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
Conversation
There was a problem hiding this 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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smaple
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
// A factor describing much the bold letters will be fattened, expressed as the fraction of font size. | ||
const float BOLD_FATTING = 0.04; | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Solves the issue of defaulted args not being called in atoms. Doesn't solve the more general function issue.
There was a problem hiding this 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:
- The Michaels comment about naming is very important, I completely overlooked it during my review.
- "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 :)
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 |
This reverts commit 79258c9.
…-sdf-bold-181641027
9cdfc6d
to
2ee2a65
Compare
…-sdf-bold-181641027
Fixes [181641027](https://www.pivotaltracker.com/story/show/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](https://user-images.githubusercontent.com/3919101/162199723-524f7560-535f-4c98-bcb4-6b760ad8cb04.png) ![image](https://user-images.githubusercontent.com/3919101/162199806-e6582c56-3071-4653-8ad2-114df86cd6b7.png) ![image](https://user-images.githubusercontent.com/3919101/162199880-1836e902-8663-4696-bcb7-2b15c3c243c6.png) On MacOS: <img width="824" alt="Screenshot 2022-04-07 at 14 54 40" src="https://user-images.githubusercontent.com/3919101/162203730-ffda3096-7aab-443f-a45a-bfa18784ba36.png"> <img width="379" alt="Screenshot 2022-04-07 at 14 54 52" src="https://user-images.githubusercontent.com/3919101/162203751-f5ca2e26-fdb6-4521-9821-999111b80ac0.png"> <img width="160" alt="Screenshot 2022-04-07 at 14 54 58" src="https://user-images.githubusercontent.com/3919101/162203769-3c367f81-67bb-48aa-994c-42eeb131017e.png">
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:
On MacOS:
Important Notes
Checklist
Please include the following checklist in your PR:
./run dist
and./run watch
.