Italic Fonts are Cut Off #502

Closed
opened 5 months ago by clktmr · 4 comments
clktmr commented 5 months ago

Italic variants of narrow fonts get cut off. The font used in the screenshot is "Go Mono".

I did a dirty fix for myself, but I am not very familiar with the codebase:

@@ -542,10 +542,19 @@ render_cell(struct terminal *term, pixman_image_t *pix,      
         row->cells[col + 1].attrs.clean = 0;      
     }      

+    if (term->conf->tweak.allow_overflowing_double_width_glyphs &&      
+        glyph != NULL &&      
+        glyph->width > term->cell_width &&      
+        col < term->cols - 1)      
+    {      
+        row->cells[col].attrs.clean = 0;      
+        row->cells[col + 1].attrs.clean = 0;      
+    }      
+
     pixman_region32_t clip;      
     pixman_region32_init_rect(      
         &clip, x, y,      
-        cell_cols * term->cell_width, term->cell_height);      
+        cell_cols * term->cell_width * 2, term->cell_height);      
     pixman_image_set_clip_region32(pix, &clip);      

     /* Background */     

Italic variants of narrow fonts get cut off. The font used in the screenshot is "Go Mono". I did a dirty fix for myself, but I am not very familiar with the codebase: ```diff @@ -542,10 +542,19 @@ render_cell(struct terminal *term, pixman_image_t *pix, row->cells[col + 1].attrs.clean = 0; } + if (term->conf->tweak.allow_overflowing_double_width_glyphs && + glyph != NULL && + glyph->width > term->cell_width && + col < term->cols - 1) + { + row->cells[col].attrs.clean = 0; + row->cells[col + 1].attrs.clean = 0; + } + pixman_region32_t clip; pixman_region32_init_rect( &clip, x, y, - cell_cols * term->cell_width, term->cell_height); + cell_cols * term->cell_width * 2, term->cell_height); pixman_image_set_clip_region32(pix, &clip); /* Background */ ```
Poster

duplicate of #207

duplicate of #207
clktmr closed this issue 5 months ago
Owner

Sorry for not replying sooner. Your patch is a pretty good attempt at alleviating the issue with italic fonts.

Unfortunately, just checking the glyph width isn't enough, since there are other reasons glyph overflow as well. Subpixel antialiasing bleeding being another example.

Furthermore, constantly clearing the clean bit introduces a performance issue since these cells now has to be redrawn every frame.

The double-width glyphs logic above has the same issues, but the impact is smaller; mainly because in that case, applications are typically aware of the fact that those glyphs are troublesome, and explicitly print a space after them.

Sorry for not replying sooner. Your patch is a pretty good attempt at alleviating the issue with italic fonts. Unfortunately, just checking the glyph width isn't enough, since there are other reasons glyph overflow as well. Subpixel antialiasing bleeding being another example. Furthermore, constantly clearing the `clean` bit introduces a performance issue since these cells now has to be redrawn _every frame_. The double-width glyphs logic above has the same issues, but the impact is smaller; mainly because in that case, applications are typically aware of the fact that those glyphs are troublesome, and explicitly print a space after them.
dnkl added the
duplicate
label 5 months ago
Poster

Thanks for the response. Really enjoying foot so far!

Based on your comment I gave it another try. This adds a new overflow attribute for cells. While rendering a cell can now check if it's left neighbor is overflowing and mark it for re-rendering.

I understand that allowing overflow makes things complicated in a lot of ways and your decision not allow it. But maybe this is worth a tweak setting? In the end both solutions have their drawbacks.

diff --git a/render.c b/render.c
@@ -542,10 +542,21 @@ render_cell(struct terminal *term, pixman_image_t *pix,
         row->cells[col + 1].attrs.clean = 0;
     }
 
+    if (col > 0 && row->cells[col - 1].attrs.overflow) {
+        row->cells[col - 1].attrs.clean = 0;
+    }
+
+    row->cells[col].attrs.overflow = (glyph != NULL &&
+                                      glyph->width > term->cell_width &&
+                                      col < term->cols - 1);
+
+    int render_width = (glyph != NULL && glyph->width > term->cell_width) ?
+        glyph->width : term->cell_width;
+
     pixman_region32_t clip;
     pixman_region32_init_rect(
         &clip, x, y,
-        cell_cols * term->cell_width, term->cell_height);
+        cell_cols * render_width, term->cell_height);
     pixman_image_set_clip_region32(pix, &clip);
 
     /* Background */
diff --git a/terminal.h b/terminal.h
@@ -37,11 +37,12 @@ struct attributes {
     uint32_t fg:24;
 
     bool clean:1;
+    bool overflow:1;
     bool have_fg:1;
     bool have_bg:1;
     uint32_t selected:2;
     bool url:1;
-    uint32_t reserved:2;
+    uint32_t reserved:1;
     uint32_t bg:24;
 };
 static_assert(sizeof(struct attributes) == 8, "VT attribute struct too large");
Thanks for the response. Really enjoying `foot` so far! Based on your comment I gave it another try. This adds a new overflow attribute for cells. While rendering a cell can now check if it's left neighbor is overflowing and mark it for re-rendering. I understand that allowing overflow makes things complicated in a lot of ways and your decision not allow it. But maybe this is worth a tweak setting? In the end both solutions have their drawbacks. ```diff diff --git a/render.c b/render.c @@ -542,10 +542,21 @@ render_cell(struct terminal *term, pixman_image_t *pix, row->cells[col + 1].attrs.clean = 0; } + if (col > 0 && row->cells[col - 1].attrs.overflow) { + row->cells[col - 1].attrs.clean = 0; + } + + row->cells[col].attrs.overflow = (glyph != NULL && + glyph->width > term->cell_width && + col < term->cols - 1); + + int render_width = (glyph != NULL && glyph->width > term->cell_width) ? + glyph->width : term->cell_width; + pixman_region32_t clip; pixman_region32_init_rect( &clip, x, y, - cell_cols * term->cell_width, term->cell_height); + cell_cols * render_width, term->cell_height); pixman_image_set_clip_region32(pix, &clip); /* Background */ diff --git a/terminal.h b/terminal.h @@ -37,11 +37,12 @@ struct attributes { uint32_t fg:24; bool clean:1; + bool overflow:1; bool have_fg:1; bool have_bg:1; uint32_t selected:2; bool url:1; - uint32_t reserved:2; + uint32_t reserved:1; uint32_t bg:24; }; static_assert(sizeof(struct attributes) == 8, "VT attribute struct too large"); ```
Owner

I would suggest opening a PR if you intend to continue working on this. It'll be much easier to discuss the patch that way.

As for the current patch - it's an interresting idea!

But maybe this is worth a tweak setting? In the end both solutions have their drawbacks.

I do think it should be gated by a tweak option. At least in the beginning. It can always be enabled by default later, if it turns out to work well for people.

We would also have to test how this interacts with overflowing double width characters; both "regular" ones, where wcwidth() == 2, and the "bastard" ones (handled by the allow-overflowing... option above), where wcwidth() == 1.

I think there's an issue if a double width character is overflowing into a third cell. It looks like the patch will accept that, and render the character. However, the overflow bit will only be set on the first cell. So when we're rendering that third cell, it'll look at the previous cell (which'll be a SPACER cell), and it wont have the overflow bit set.

The only thing I don't really like is the use of a bit in the attr bitfield. Not because it's a bad idea, it's actually a good idea, but because those bits are scarce. While I don't have anything planned for the remaining bits, I do want to be sure we've thought things through before using anymore of them.

I would suggest opening a PR if you intend to continue working on this. It'll be much easier to discuss the patch that way. As for the current patch - it's an interresting idea! > But maybe this is worth a tweak setting? In the end both solutions have their drawbacks. I do think it should be gated by a tweak option. At least in the beginning. It can always be enabled by default later, if it turns out to work well for people. We would also have to test how this interacts with overflowing double width characters; both "regular" ones, where `wcwidth() == 2`, and the "bastard" ones (handled by the `allow-overflowing...` option above), where `wcwidth() == 1`. I _think_ there's an issue if a double width character is overflowing into a third cell. It looks like the patch will accept that, and render the character. However, the `overflow` bit will only be set on the first cell. So when we're rendering that third cell, it'll look at the previous cell (which'll be a `SPACER` cell), and it wont have the `overflow` bit set. The only thing I don't really like is the use of a bit in the attr bitfield. Not because it's a bad idea, it's actually a good idea, but because those bits are _scarce_. While I don't have anything planned for the remaining bits, I do want to be **sure** we've thought things through before using anymore of them.
Sign in to join this conversation.
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.