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.

Sign up for GitHub

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

Jump to bottom

Fixed an issue where font metrics were incorrect for monospaced fonts. #8886

Merged
merged 12 commits into from May 13, 2024
Merged

Fixed an issue where font metrics were incorrect for monospaced fonts. #8886

merged 12 commits into from May 13, 2024

Conversation

AGulev
Copy link
Contributor

@AGulev AGulev commented May 5, 2024 •
edited

Monospaced fonts should always use the fixed Advance Width for each symbol as its width, instead of the glyph's real width.

Fix #7900
Fix #7197

PR checklist

  • Code
    • Add engine and/or editor unit tests.
    • New and changed code follows the overall code style of existing code
    • Add comments where needed
  • Documentation
    • Make sure that API documentation is updated in code comments
    • Make sure that manuals are updated (in github.com/defold/doc)
  • Prepare pull request and affected issue for automatic release notes generator
    • Pull request - Write a message that explains what this pull request does. What was the problem? How was it solved? What are the changes to APIs or the new APIs introduced? This message will be used in the generated release notes. Make sure it is well written and understandable for a user of Defold.
    • Pull request - Write a pull request title that in a sentence summarises what the pull request does. Do not include "Issue-1234 ..." in the title. This text will be used in the generated release notes.
    • Pull request - Link the pull request to the issue(s) it is closing. Use on of the approved closing keywords.
    • Affected issue - Assign the issue to a project. Do not assign the pull request to a project if there is an issue which the pull request closes.
    • Affected issue - Assign the "breaking change" label to the issue if introducing a breaking change.
    • Affected issue - Assign the "skip release notes" is the issue should not be included in the generated release notes.

Example of a well written PR description:

  1. Start with the user facing changes. This will end up in the release notes.
  2. Add one of the GitHub approved closing keywords
  3. Optionally also add the technical changes made. This is information that might help the reviewer. It will not show up in the release notes. Technical changes are identified by a line starting with one of these:
    1. ### Technical changes
    2. Technical changes:
    3. Technical notes:
There was a anomaly in the carbon chroniton propeller, introduced in version 8.10.2. This fix will make sure to reset the phaser collector on application startup.

Fixes #1234

### Technical changes
* Pay special attention to line 23 of phaser_collector.clj as it contains some interesting optimizations
* The propeller code was not taking into account a negative phase.

@AGulev AGulev requested review from Jhonnyg and britzl May 5, 2024 12:26
@AGulev
Copy link
Contributor Author

AGulev commented May 5, 2024

hmmm, I was sure that the editor uses Fontc.java to compile fonts, but it seems like it has its own font compiler

@AGulev AGulev requested review from matgis and britzl May 5, 2024 17:53
@AGulev AGulev marked this pull request as draft May 5, 2024 18:23
matgis
matgis previously approved these changes May 6, 2024
View reviewed changes
Copy link
Contributor

@matgis matgis left a comment

Choose a reason for hiding this comment

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

Editor changes look good! 👍

@AGulev
Copy link
Contributor Author

AGulev commented May 6, 2024

Editor changes look good! 👍

Thanks.
I changed PR to Draft because I found how deep this rabbit hole is.
When I fully solve it, I'll make needed changes and ask for your review again

@AGulev AGulev requested a review from matgis May 7, 2024 14:49
@AGulev AGulev marked this pull request as ready for review May 7, 2024 14:49
AGulev
AGulev commented May 7, 2024
View reviewed changes
com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/font/Fontc.java
}

glyphBankBuilder.setIsMonospaced(is_monospaced);
glyphBankBuilder.setPadding(padding);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For non-monospaced fonts, this value taken into account as part of the last glyph width (we add padding*2 to each width). But for monospaced fonts, we need it to add it as part of the text width and also to offset it back when rendering monospaced font.

editor/src/clj/editor/font.clj
font-offset {:x (if (:is-monospaced font-map)
(- 0 (* (:padding font-map) 0.5))
0)
:y 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.

We render each glyph starting some point and then draw every next letter at Advance Width (+ tracking) from prev. one.

But width of the glyph already include padding. Which makes width of the text a bit bigger and moves initial rendering point for the padding.

Adding padding the line-widths helps to return real line width and compensate 0.5 of padding for the positioning . But another 0.5 of padding should be compensted by moving the initial point of rendering (we can't just add double padding to the line-width)

engine/render/src/render/font_renderer.cpp
@@ -739,6 +749,10 @@ namespace dmRender
float layout_width;
int line_count = Layout(text, width, lines, max_lines, &layout_width, lm, measure_trailing_space);
float x_offset = OffsetX(te.m_Align, te.m_Width);
if (font_map->m_IsMonospaced)
{
x_offset -= font_map->m_Padding * 0.5f;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

please check the comment for clojure code.

AGulev
AGulev commented May 7, 2024
View reviewed changes
engine/render/src/render/font_renderer.cpp
@@ -835,8 +849,8 @@ namespace dmRender

for (int line = 0; line < line_count; ++line) {
TextLine& l = lines[line];
int16_t x = (int16_t)(x_offset - OffsetX(te.m_Align, l.m_Width) + 0.5f);
int16_t y = (int16_t) (y_offset - line * leading + 0.5f);
Copy link
Contributor Author

@AGulev AGulev May 7, 2024 •
edited

Choose a reason for hiding this comment

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

Rounded vertex coordinates is the reason of dancing text (#7197)

matgis
matgis previously approved these changes May 8, 2024
View reviewed changes
Copy link
Contributor

@matgis matgis left a comment

Choose a reason for hiding this comment

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

Checked in the editor, and it works great! Just some minor issues with indentation in the code.

editor/src/clj/editor/font.clj Outdated Show resolved Hide resolved
editor/src/clj/editor/font.clj Outdated Show resolved Hide resolved
@AGulev AGulev merged commit 6e11c65 into dev May 13, 2024
22 checks passed
@AGulev AGulev deleted the issue-7900 branch May 13, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jhonnyg Jhonnyg Jhonnyg approved these changes

@matgis matgis matgis left review comments

@britzl britzl Awaiting requested review from britzl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

font metrics wrong for monospace font. Texts have slightly incorrect position with different anchors
4 participants
@AGulev @Jhonnyg @britzl @matgis