render: Allow cells to bleed into their neighbor #592

Manually merged
dnkl merged 1 commits from clktmr/foot:allow-bleeding-glyphs into master 2 months ago
clktmr commented 3 months ago

This patch adds a confined flag to each cell to track if the last
rendered glyph bled into it's right neighbor. To keep things simple,
bleeding into any other neighbor cell than the immediate right one is
not allowed. This should cover most use cases.

Before rendering a row we now do a prepass and mark all cells unclean
that are affected by a bleeding neighbor. If there are consecutive
bleeding cells, the whole group must be re-rendered even if only a
single cell has changed.

Remaining issues:
  • Glyphs are still cut-off for very large font sizes
  • Last cell of a row bleeds into right margin area Not able to reproduce, probably occured due to a dirty build by myself
This patch adds a `confined` flag to each cell to track if the last rendered glyph bled into it's right neighbor. To keep things simple, bleeding into any other neighbor cell than the immediate right one is not allowed. This should cover most use cases. Before rendering a row we now do a prepass and mark all cells unclean that are affected by a bleeding neighbor. If there are consecutive bleeding cells, the whole group must be re-rendered even if only a single cell has changed. ###### Remaining issues: - [x] Glyphs are still cut-off for very large font sizes - [x] ~~Last cell of a row bleeds into right margin area~~ Not able to reproduce, probably occured due to a dirty build by myself
clktmr added 1 commit 3 months ago
a6ae819fb8 render: Allow cells to bleed into their neighbor
Poster

Followup to #502

I gave this another shot. The patch still claims one bit from the cell attributes, because we need to store the bleeding/overflow information somewhere until the next frame. I could create another bitmask in the grid struct, but on the other hand this can still be done if there are no more bits left in the cell attributes?

I changed the bit from overflow to confined because occasionally the information was lost (sgr_reset maybe?). If the attributes are reset, we must assume that the cell did bleed into it's neighbor, because it might have.

This still needs to be gated by a tweak option. I will add one if this get's a green light to merge.

Followup to #502 I gave this another shot. The patch still claims one bit from the cell attributes, because we need to store the bleeding/overflow information somewhere until the next frame. I could create another bitmask in the `grid` struct, but on the other hand this can still be done if there are no more bits left in the cell attributes? I changed the bit from `overflow` to `confined` because occasionally the information was lost (sgr_reset maybe?). If the attributes are reset, we must assume that the cell did bleed into it's neighbor, because it *might* have. This still needs to be gated by a tweak option. I will add one if this get's a green light to merge.
Owner

I changed the bit from overflow to confined because occasionally the information was lost (sgr_reset maybe?). If the attributes are reset, we must assume that the cell did bleed into it's neighbor, because it might have.

I haven't looked at the patch yet, but my guess would be its lost when a (new) character is printed to the grid. When this happens, all cell attributes are replaced with the "current" ANSI attributes, from term->vt.attrs.

FWIW, the selected bits (set in selection.c when you select/deselect text with the mouse) have exactly the same problem; if a selected cell is updated by the application (example: you have selected text in top, which then refreshes the screen content), its "selected" state is lost. This is worked around in grid_render(), by calling selection_dirty_cells().

What we need is to split the ANSI state from "other" cell state. Right now, I'm counting 6 bits that can be moved out from the ANSI state. This means we can't just add an uint8_t bitfield outside the attrs struct. But perhaps the attrs struct can be re-arranged? Hmm... I'll have a look.

As for the rest of the patch. Really excited about it. Will try to find some time to go through it, and test it, as soon as possible.

Really happy you keep trying :D

> I changed the bit from overflow to confined because occasionally the information was lost (sgr_reset maybe?). If the attributes are reset, we must assume that the cell did bleed into it's neighbor, because it might have. I haven't looked at the patch yet, but my guess would be its lost when a (new) character is printed to the grid. When this happens, **all** cell attributes are replaced with the "current" ANSI attributes, from `term->vt.attrs`. FWIW, the `selected` bits (set in `selection.c` when you select/deselect text with the mouse) have exactly the same problem; if a selected cell is updated by the application (example: you have selected text in `top`, which then refreshes the screen content), its "selected" state is lost. This is worked around in `grid_render()`, by calling `selection_dirty_cells()`. What we need is to split the ANSI state from "other" cell state. Right now, I'm counting **6** bits that can be moved out from the ANSI state. This means we can't just add an `uint8_t` bitfield outside the `attrs` struct. But perhaps the `attrs` struct can be re-arranged? Hmm... I'll have a look. As for the rest of the patch. Really excited about it. Will try to find some time to go through it, and test it, as soon as possible. Really happy you keep trying :D
Owner

But perhaps the attrs struct can be re-arranged? Hmm... I'll have a look.

Nope. Not without either increasing the size of the cell, or slowing down term_print() (which is in the hot path, so no).

The attr struct is currently 8 bytes. Meaning it can be copied with a single mov. Anything else will result either multiple byte moves, or masking.

Sorry for hijacking your PR :D

> But perhaps the attrs struct can be re-arranged? Hmm... I'll have a look. Nope. Not without either increasing the size of the cell, or slowing down `term_print()` (which is in the hot path, so no). The attr struct is currently 8 bytes. Meaning it can be copied with a single `mov`. **Anything** else will result either multiple byte moves, or masking. Sorry for hijacking your PR :D
Owner

I'll see how adding an uint8_t to the cell struct affects performance. It will of course increase the run-time memory usage, but hopefully it plays nicely with the cache.

If that works out, we'll move the confined bit, along with selected and url(?) there.

I'll see how adding an `uint8_t` to the cell struct affects performance. It will of course increase the run-time memory usage, but hopefully it plays nicely with the cache. If that works out, we'll move the `confined` bit, along with `selected` and `url`(?) there.
Owner

I'll see how adding an uint8_t to the cell struct affects performance.

#593

> I'll see how adding an uint8_t to the cell struct affects performance. https://codeberg.org/dnkl/foot/issues/593
Poster

Sorry for hijacking your PR :D

Glad it got attention!

One more thing I noticed that should be looked at before this gets merged: If the font size is very large, some characters are still cut off. Might be a bug in my font (Go Mono), but haven't seen this in other terminal emulators. Will investigate later.

> Sorry for hijacking your PR :D Glad it got attention! One more thing I noticed that should be looked at before this gets merged: If the font size is very large, some characters are still cut off. Might be a bug in my font (Go Mono), but haven't seen this in other terminal emulators. Will investigate later.
Owner

FYI, I haven't forgotten about this, but I need to spend some time instrumenting, and benchmarking this (in addition to verifying that it actually works...).

Also trying to decide whether or not to add those extra state bytes. Right now, I don't think we'll add them. But if we do, that would change how we want this PR to work, and I don't want to spend time benchmarking something that may have to be re-written...

Thanks for understanding :)

FYI, I haven't forgotten about this, but I need to spend some time instrumenting, and benchmarking this (in addition to verifying that it actually works...). Also trying to decide whether or not to add those extra state bytes. Right now, I _don't_ think we'll add them. But if we do, that would change how we want this PR to work, and I don't want to spend time benchmarking something that may have to be re-written... Thanks for understanding :)
Poster

That's perfectly fine, I'm still dogfooding this. There is another issue, where the last cell in a row might bleed into the right margin area. I will update the PR description to keep track of open issues.

It would be interesting if you could share what exactly you are benchmarking, so I can evaluate performance degradation to some degree by myself. Should I just use benchmark.py to run the default vtebench benchmarks with a full PGO build?

That's perfectly fine, I'm still dogfooding this. There is another issue, where the last cell in a row might bleed into the right margin area. I will update the PR description to keep track of open issues. It would be interesting if you could share what exactly you are benchmarking, so I can evaluate performance degradation to some degree by myself. Should I just use `benchmark.py` to run the default vtebench benchmarks with a full PGO build?
Owner

I will probably do a couple of things:

  • instrument the code to count how many cells we're re-rendering due to overflowing glyphs (or a simplified version where we just count the number of rendered cells per frame, and manually check how much more we're rendering with this PR).
  • Check frame rendering times, comparing with master. This can be done by enabling enableing tweak.render-timer=[osd|log|both] in foot.ini.
  • Run vtebench benchmarks. vtebench was recently (well, it's been a while now, I guess) re-written, and no longer produces raw output files that you manually cat (which is basically what benchmark.py does), but instead loads, primes and runs benchmarks on its own.

I'd definitely run all benchmarks in PGO builds, but probably on regular release builds as well.

I will probably do a couple of things: * instrument the code to count how many cells we're re-rendering due to overflowing glyphs (or a simplified version where we just count the number of rendered cells per frame, and manually check how much more we're rendering with this PR). * Check frame rendering times, comparing with master. This can be done by enabling enableing `tweak.render-timer=[osd|log|both]` in `foot.ini`. * Run `vtebench` benchmarks. vtebench was recently (well, it's been a while now, I guess) re-written, and no longer produces raw output files that you manually `cat` (which is basically what `benchmark.py` does), but instead loads, primes and runs benchmarks on its own. I'd definitely run all benchmarks in PGO builds, but probably on regular release builds as well.
Owner

Also, I'd like to see benchmarks of best vs. worst case scenarios. I.e. are there any regressions in the best case scenario, where there aren't any overflowing glyphs?

And how much slower are we on this branch, compared to master, when all glyphs on the screen are overflowing?

Also, I'd like to see benchmarks of best vs. worst case scenarios. I.e. are there any regressions in the _best_ case scenario, where there aren't any overflowing glyphs? And how much slower are we on this branch, compared to master, when **all** glyphs on the screen are overflowing?
dnkl requested changes 3 months ago
dnkl left a comment

I've done some initial benchmarks on my laptop. They at least don't suggest anything out of the ordinary, but are perhaps a bit inconclusive. That has more to do with the laptop being a bad platform to benchmark on (it does HW based CPU throttling).

In other words, looking good so far.

render.c Outdated
*/
int render_width;
if (glyph != NULL &&
glyph->width > term->cell_width &&
dnkl commented 3 months ago
Poster
Owner

I haven't verified this, but I'm starting to believe that we need to take the glyph's offset into account when checking its width.

I believe the workaround for single-column, double-width glyphs have the same problem, and that it is the reason it fails to detect "narrow" double-width glyphs (typically up/down arrows).

I haven't verified this, but I'm starting to believe that we need to take the glyph's offset into account when checking its width. I believe the workaround for single-column, double-width glyphs have the same problem, and that it is the reason it fails to detect "narrow" double-width glyphs (typically up/down arrows).
Poster

This totally makes sense and did resolve the issue where some glyphs were still cutoff.

This totally makes sense and did resolve the issue where some glyphs were still cutoff.
render.c Outdated
pixman_region32_init_rect(
&clip, x, y,
cell_cols * term->cell_width, term->cell_height);
cell_cols * render_width, term->cell_height);
dnkl commented 3 months ago
Poster
Owner

I'm not sure cell_cols * render_width width is such a good idea. For example, I don't see anything preventing the "bleed" logic from triggering on a double-width glyph.

Related: perhaps we need to check the next cell, and skip the whole bleeding logic if its wc is >= CELL_SPACER, since then it's a double-width glyph and cell_cols will be 2 anyway.

I'm not sure `cell_cols * render_width` width is such a good idea. For example, I don't see anything preventing the "bleed" logic from triggering on a double-width glyph. Related: perhaps we need to check the _next_ cell, and skip the whole bleeding logic if its `wc` is `>= CELL_SPACER`, since then it's a double-width glyph and `cell_cols` will be `2` anyway.
Poster

Yes, this is outright wrong if cell_cols > 1. This would have allowed a double width glyph to bleed into it second neighbor.

However allowing double width glyphs to bleed into a third cell was intentional. I fixed this by clamping the render_width to cell_cols + 1 and setting all cells of a multi-width glyph to confined = false, if it does bleed.

Yes, this is outright wrong if `cell_cols > 1`. This would have allowed a double width glyph to bleed into it second neighbor. However allowing double width glyphs to bleed into a third cell was intentional. I fixed this by clamping the `render_width` to `cell_cols + 1` and setting all cells of a multi-width glyph to `confined = false`, if it does bleed.
clktmr added 2 commits 3 months ago
dcce6624b5 render: Consider glyph offset for bleeding cells
352528df1d render: Clip bleeding double-width glyphs strictly
Poster

I've done some testing to make sure everything works as expected and while that's the case, I noticed many glyphs marked as bleeding with small font sizes because of anti-aliasing. It might be worth consideration to add a threshold of one pixel to the bleeding logic, but that resulted in some visible cutoffs for me, especially with emojis.

I've done some testing to make sure everything works as expected and while that's the case, I noticed many glyphs marked as bleeding with small font sizes because of anti-aliasing. It might be worth consideration to add a threshold of one pixel to the bleeding logic, but that resulted in some visible cutoffs for me, especially with emojis.
Poster

Regarding performance, assuming all glyphs of a font are bleeding: If a cell changes, i.e. marked as not clean, this will cause all cells left and right to be re-rendered until the next whitespace.

Regarding performance, assuming all glyphs of a font are bleeding: If a cell changes, i.e. marked as not clean, this will cause all cells left and right to be re-rendered until the next whitespace.
Owner

I noticed many glyphs marked as bleeding with small font sizes because of anti-aliasing

This is actually the very reason we added cell clipping to begin with :)

Do we need a cut-off? I mean, visually, it'll look better if we allow them to bleed. And the option to enable bleeding will have a fat warning about possible performance impacts.

Regarding performance, assuming all glyphs of a font are bleeding: If a cell changes, i.e. marked as not clean, this will cause all cells left and right to be re-rendered until the next whitespace.

Yeah, and that's what we need to focus on, when benchmarking. Running the vtebench benchmarks won't tell us much actually since all cells will have changed in every frame anyway. I.e. we'll always be doing a full screen refresh. The only thing we'll be measuring is some of the overhead added by render_cell_prepass().

I think what needs to be done is create a benchmark that overflows roughly half the cells, and then measure render times only, not wall clock time.

> I noticed many glyphs marked as bleeding with small font sizes because of anti-aliasing This is actually the very reason we added cell clipping to begin with :) Do we need a cut-off? I mean, visually, it'll look better if we allow them to bleed. And the option to enable bleeding will have a fat warning about possible performance impacts. > Regarding performance, assuming all glyphs of a font are bleeding: If a cell changes, i.e. marked as not clean, this will cause all cells left and right to be re-rendered until the next whitespace. Yeah, and that's what we need to focus on, when benchmarking. Running the vtebench benchmarks won't tell us much actually since all cells will have changed in every frame anyway. I.e. we'll always be doing a full screen refresh. The only thing we'll be measuring is some of the overhead added by `render_cell_prepass()`. I think what needs to be done is create a benchmark that overflows roughly half the cells, and then measure render times only, not wall clock time.
Owner

I'd like to use this as my daily driver for a couple of days, with tweak.allow-overflowing-double-width-glyphs=no.

Can you please rebase onto latest master? No need to add a conf option yet (but do feel free to do so, if you have the time).

I'd like to use this as my daily driver for a couple of days, with `tweak.allow-overflowing-double-width-glyphs=no`. Can you please rebase onto latest master? No need to add a conf option yet (but do feel free to do so, if you have the time).
clktmr force-pushed allow-bleeding-glyphs from 352528df1d to dce4ec4f59 3 months ago
Poster

Sure, rebase is done. Conf option is still on my list, will add sometime this week.

Sure, rebase is done. Conf option is still on my list, will add sometime this week.
Owner

Minor fixes after the rebase:

diff --git a/render.c b/render.c
index dd5dab1..b3c5391 100644
--- a/render.c
+++ b/render.c
@@ -658,11 +658,11 @@ render_cell(struct terminal *term, pixman_image_t *pix,
      * the next frame.
      */
     int render_width;
-    if (glyph != NULL &&
-        glyph->x + glyph->width > cell_cols * term->cell_width &&
+    if (glyph_count == 1 && glyphs[0] != NULL &&
+        glyphs[0]->x + glyphs[0]->width > cell_cols * term->cell_width &&
         col < term->cols - 1)
     {
-        render_width = min(glyph->x + glyph->width,
+        render_width = min(glyphs[0]->x + glyphs[0]->width,
                            (cell_cols + 1) * term->cell_width);
         for (int i = 0; i < cell_cols; i++)
             row->cells[col + i].attrs.confined = false;
Minor fixes after the rebase: ```diff diff --git a/render.c b/render.c index dd5dab1..b3c5391 100644 --- a/render.c +++ b/render.c @@ -658,11 +658,11 @@ render_cell(struct terminal *term, pixman_image_t *pix, * the next frame. */ int render_width; - if (glyph != NULL && - glyph->x + glyph->width > cell_cols * term->cell_width && + if (glyph_count == 1 && glyphs[0] != NULL && + glyphs[0]->x + glyphs[0]->width > cell_cols * term->cell_width && col < term->cols - 1) { - render_width = min(glyph->x + glyph->width, + render_width = min(glyphs[0]->x + glyphs[0]->width, (cell_cols + 1) * term->cell_width); for (int i = 0; i < cell_cols; i++) row->cells[col + i].attrs.confined = false; ```
clktmr force-pushed allow-bleeding-glyphs from dce4ec4f59 to bb654d501f 3 months ago
Poster

Sorry, I've been in a hurry. Fixed that.

I've read a bit through #100 and as I understood it, to do this correctly, for glyph_count > 1 we would need to find the widest glyph, as they are just rendered on top of each other?

Sorry, I've been in a hurry. Fixed that. I've read a bit through #100 and as I understood it, to do this correctly, for `glyph_count > 1` we would need to find the widest glyph, as they are just rendered on top of each other?
Owner

Yes, I think that would be the correct way to do it. Remember to include the offsets as well; the glyphs may have been positioned next to each other by harfbuzz.

Yes, I think that would be the correct way to do it. Remember to include the offsets as well; the glyphs may have been positioned next to each other by harfbuzz.
clktmr added 2 commits 3 months ago
dnkl requested changes 3 months ago
dnkl left a comment

I've been running this for a couple of days, and functionality wise, things are looking real good. I don't think I've seen a single rendering isssue.

I also applied this patch:

diff --git a/render.c b/render.c
index 470ad71..716a3a7 100644
--- a/render.c
+++ b/render.c
@@ -57,6 +57,9 @@ static struct {
 
 static void fdm_hook_refresh_pending_terminals(struct fdm *fdm, void *data);
 
+static _Atomic size_t cells_rendered;
+static _Atomic size_t extra_cells;
+
 struct renderer *
 render_init(struct fdm *fdm, struct wayland *wayl)
 {
@@ -446,6 +449,7 @@ render_cell_prepass(struct terminal *term, struct row *row, int col)
             break;
         }
 
+        extra_cells++;
         row->cells[col].attrs.clean = 0;
         row->cells[col + 1].attrs.clean = 0;
     }
@@ -459,6 +463,7 @@ render_cell(struct terminal *term, pixman_image_t *pix,
     if (cell->attrs.clean)
         return 0;
 
+    cells_rendered++;
     cell->attrs.clean = 1;
     cell->attrs.confined = true;
 
@@ -2321,6 +2326,9 @@ grid_render(struct terminal *term)
     if (term->conf->tweak.render_timer_osd || term->conf->tweak.render_timer_log)
         gettimeofday(&start_time, NULL);
 
+    cells_rendered = 0;
+    extra_cells = 0;
+
     xassert(term->width > 0);
     xassert(term->height > 0);
 
@@ -2581,6 +2589,9 @@ grid_render(struct terminal *term)
 
     wl_surface_attach(term->window->surface, buf->wl_buf, 0, 0);
     wl_surface_commit(term->window->surface);
+
+    LOG_INFO("frame: cells-rendered: %zu (extra cells: %zu)",
+             cells_rendered, extra_cells);
 }
 
 static void

to get a feeling for how much additional rendering we're doing. And it looks good. My font doesn't have any bleeding glyphs, even in italic and/or bold. So, I only expect fallback glyphs to overflow. And that seems to work as expected.

That means, the one thing that would affect performance the most is the prepass. I intend to run actual benchmarks later this week.

config.c Outdated
conf->tweak.allow_bleeding_glyphs = str_to_bool(value);
if (!conf->tweak.allow_bleeding_glyphs)
LOG_WARN("tweak: disabled bleeding glyphs");
}
dnkl commented 3 months ago
Poster
Owner

We log tweak options when changed from their default values. I.e. in this case, we should log when it has been enabled.

Except, I also think that we should replace this option, and the old allow-overflowing-double-width-glyphs option with a new one, as I don't see much reason to have both enabled at the same time.

Yes, they target slightly different things, but I'm hoping the method you've developed in this PR will eventually be the only way we handle overflowing/bleeding glyphs. As such, I think it makes sense to use a single option that selects which method to use (or none at all).

Perhaps overflowing-glyphs=disabled|double-width|all (suggestions for better option and value names are welcome 😅). Then document in foot.init.5 how they work.

(This new option would replace pua-double-width as well)

Unless you see a reason to keep them separated?

We log tweak options **when changed from their default values**. I.e. in this case, we should log when it has been **enabled**. Except, I _also_ think that we should replace this option, **and** the old `allow-overflowing-double-width-glyphs` option with a new one, as I don't see much reason to have both enabled at the same time. Yes, they target slightly different things, but I'm hoping the method you've developed in this PR will eventually be the _only_ way we handle overflowing/bleeding glyphs. As such, I think it makes sense to use a single option that selects _which_ method to use (or none at all). Perhaps `overflowing-glyphs=disabled|double-width|all` (suggestions for better option and value names are welcome 😅). Then document in `foot.init.5` how they work. (This new option would replace `pua-double-width` as well) Unless you see a reason to keep them separated?
Poster

Yes, having three options for overflowing is confusing. Personally I would be fine with just deprecating allow-overflowing-double-width-glyphs and pua-double-width and leaving the new option as it is. Maybe rename it to overflowing-glyphs=yes|no. Is there a use case I'm missing that wouldn't be covered by this?

Only setting cell_cols = 2 based on a threshold makes a slight difference in rendered cursor width and visibility of the neighbor cell.

Should I merge the options in this PR? The old option was enabled by default, I assume the new one won't be.

Yes, having three options for overflowing is confusing. Personally I would be fine with just deprecating `allow-overflowing-double-width-glyphs` and `pua-double-width` and leaving the new option as it is. Maybe rename it to `overflowing-glyphs=yes|no`. Is there a use case I'm missing that wouldn't be covered by this? Only setting `cell_cols = 2` based on a threshold makes a slight difference in rendered cursor width and visibility of the neighbor cell. Should I merge the options in this PR? The old option was enabled by default, I assume the new one won't be.
dnkl commented 3 months ago
Poster
Owner

Only setting cell_cols = 2 based on a threshold makes a slight difference in rendered cursor width and visibility of the neighbor cell.

Yeah, realized this was the case after I wrote the above. The differences I can think of are:

  • Cursor width
  • Visibility of neighbouring cell
  • A single-width codepoint, double-width glyph followed by a non-space codepoint, will now be rendered on top of that character. Before, we would have clipped the double-width glyph to a single cell.
  • A single-width codepoint, double-width glyph overflowing into a third cell will be clipped with this PR

The first two points are a no-brainer I think. In a sense, having the cursor adjusted to two cells for a single-width codepoint is simply misleading.

The third point is also ok. Applications tend to be aware of the fact that these glyphs are actually single-width, and make sure to insert a space after them. If they don't, then 🤷.

Final point can perhaps be considered a problem. But I'm not sure it's a big enough problem to care about; the user should just adjust the font size of that particular fallback font.

Should I merge the options in this PR? The old option was enabled by default, I assume the new one won't be.

Yeah, please do. Regarding it's default value, my initial thought was to have it disabled, and then switched to enabled once "proven". But from what I have seen so far, I think we can assume it will be enabled by default right away.

Maybe rename it to overflowing-glyphs=yes|no

Sounds good!

> Only setting cell_cols = 2 based on a threshold makes a slight difference in rendered cursor width and visibility of the neighbor cell. Yeah, realized this was the case after I wrote the above. The differences I can think of are: * Cursor width * Visibility of neighbouring cell * A single-width codepoint, double-width glyph followed by a non-space codepoint, will now be rendered on top of that character. Before, we would have clipped the double-width glyph to a single cell. * A single-width codepoint, double-width glyph overflowing into a _third_ cell will be clipped with this PR The first two points are a no-brainer I think. In a sense, having the cursor adjusted to two cells for a single-width codepoint is simply misleading. The third point is also ok. Applications tend to be aware of the fact that these glyphs are actually single-width, and make sure to insert a space after them. If they don't, then 🤷. Final point can perhaps be considered a problem. But I'm not sure it's a big enough problem to care about; the user _should_ just adjust the font size of that particular fallback font. > Should I merge the options in this PR? The old option was enabled by default, I assume the new one won't be. Yeah, please do. Regarding it's default value, my initial thought was to have it disabled, and then switched to enabled once "proven". But from what I have seen so far, I think we can assume it will be enabled by default right away. > Maybe rename it to overflowing-glyphs=yes|no Sounds good!
clktmr marked this conversation as resolved
In contrast to *allow-overflowing-double-width-glyphs* and
*pua-double-width* this will not change the width of the Unicode
code point, i.e. single width will still use only a single cell.
dnkl commented 3 months ago
Poster
Owner

I wouldn't really say that allow-overflowing-double-width-glyphs change the width. It does completely replace whatever is in the neighbouring cell, but the codepoint is still allocated a single cell, and whatever codepoint is in the neighbouring cell is kept.

For a double-width glyph (single-width codepoint), this PR will render exactly the same way almost the same way: the neighbouring cell's content on screen will be completely "overwritten" with the overflowing glyph (i.e. the overflowing glyph is rendered on top of neighbouring glyph). But the content of the cell remains unchanged.

I wouldn't really say that `allow-overflowing-double-width-glyphs` change the width. It _does_ completely replace whatever is in the neighbouring cell, but the codepoint is still allocated a single cell, and whatever codepoint _is_ in the neighbouring cell is kept. For a double-width glyph (single-width codepoint), ~~this PR will render exactly the same way~~ almost the same way: the neighbouring cell's content on screen will be completely "overwritten" with the overflowing glyph (i.e. the overflowing glyph is rendered on top of neighbouring glyph). But the _content_ of the cell remains unchanged.
Poster

I see. I misunderstood how the option works and removed that part. Thanks for the clarification.

I see. I misunderstood how the option works and removed that part. Thanks for the clarification.
clktmr marked this conversation as resolved
clktmr force-pushed allow-bleeding-glyphs from 8b1272b44e to 0ee9f2c58d 3 months ago
clktmr added 1 commit 3 months ago
bfbf23f35c config: Deprecate old overflowing glyph options
clktmr force-pushed allow-bleeding-glyphs from bfbf23f35c to c0315f529c 3 months ago
Poster

I have deprecated the other options for overflowing glyphs as discussed. The wiki will need an update if this gets released.

What I have not tested yet are graphemes and sixels. Any suggestions for a font with overflowing graphemes?

I have deprecated the other options for overflowing glyphs as discussed. The wiki will need an update if this gets released. What I have not tested yet are graphemes and sixels. Any suggestions for a font with overflowing graphemes?
clktmr added 1 commit 3 months ago
e72b48aaff render: Fix sixel rendering for overflowing-glyphs
Owner

Any suggestions for a font with overflowing graphemes?

Depends a bit what you're looking for. If we by "grapheme" mean "a cell with multiple codepoints", then those fall into two categories:

  • Emoji sequences, which typically resolve to a single glyph
  • Combining characters, like COMBINING ACUTE ACCENT. These are much more likely to produce multiple glyphs.

So, if you want to test how multiple glyphs work, just try things like echo -e 'q\u0301'. But be careful - foot will normalize the codepoints to a single codepoint if possible. For example, echo -e 'e\u0301' will produce the single codepoint LATIN SMALL LETTER E WITH ACUTE.

> Any suggestions for a font with overflowing graphemes? Depends a bit what you're looking for. If we by "grapheme" mean "a cell with multiple codepoints", then those fall into two categories: * Emoji sequences, which typically resolve to a single glyph * Combining characters, like `COMBINING ACUTE ACCENT`. These are much more likely to produce multiple glyphs. So, if you want to test how multiple glyphs work, just try things like `echo -e 'q\u0301'`. But be careful - foot will normalize the codepoints to a single codepoint if possible. For example, `echo -e 'e\u0301'` will produce the single codepoint `LATIN SMALL LETTER E WITH ACUTE`.
dnkl requested changes 3 months ago
dnkl left a comment

Thanks for the updates! Have you given subpixel antialiasing anymore thought? Might be worth testing a couple of fonts, at different sizes, and require the glyphs to bleed more than 1-2px?

config.c Outdated
.text = xasprintf(fmt, path, lineno, key),
};
tll_push_back(conf->notifications, deprecation);
dnkl commented 3 months ago
Poster
Owner

It's very diligent of you to add a deprecation warning and all. Kudos!

However, tweak options may be changed or removed at any time, without any prior notice. I.e. just remove the old option.

It might however make sense to add two changelog entries; one under "added" (for the new option), and one under "removed" (the old option, plus a reference to the new one).

It's very diligent of you to add a deprecation warning and all. Kudos! However, tweak options may be changed or removed at any time, without any prior notice. I.e. just remove the old option. It might however make sense to add two changelog entries; one under _"added"_ (for the new option), and one under _"removed"_ (the old option, plus a reference to the new one).
Poster

Just trying to keep up :)

Just trying to keep up :)
clktmr marked this conversation as resolved
Another use case are legacy emoji characters like *WHITE FROWNING
FACE*.
Another use case are fonts "icon" characters in the Unicode private
dnkl commented 3 months ago
Poster
Owner

s/fonts "icon"/fonts with "icon"/ ?

s/fonts "icon"/fonts with "icon"/ ?
clktmr marked this conversation as resolved
should be allowed to overflow.
See also: *pua-double-width*
Note: Might impact performance depending on the font used.
dnkl commented 3 months ago
Poster
Owner

Lower case 'm' in "Might", I think?

Lower case 'm' in "Might", I think?
clktmr marked this conversation as resolved
clktmr force-pushed allow-bleeding-glyphs from e72b48aaff to 1e53b12072 3 months ago
Poster

require the glyphs to bleed more than 1-2px?

If you know it, you'll see it. I wouldn't do it, but your decision! Or make overflowing-glyphs an integer option, which specifies the threshold.

> require the glyphs to bleed more than 1-2px? If you know it, you'll see it. I wouldn't do it, but your decision! Or make *overflowing-glyphs* an integer option, which specifies the threshold.
clktmr force-pushed allow-bleeding-glyphs from 1e53b12072 to ea4fcf9c6a 3 months ago
Poster

What I have not tested yet are graphemes and sixels.

Murphys law was proven correct. Both were broken 😅

> What I have not tested yet are graphemes and sixels. Murphys law was proven correct. Both were broken 😅
Owner

If you know it, you'll see it. I wouldn't do it, but your decision!

Let's allow them. At least to begin with. Might want to mention it explicitly in the man page.

Murphys law was proven correct. Both were broken 😅

dnkl's law is: if it hasn't been tested, it's broken :)

I've been running this for ~1 week now, and haven't noticed any issues.

one thing though: I'd like to invert the meaning of the confined bit. I.e. I would like 0 to mean it's confined. We'd need to make sure term->vt.attrs.confined is initialized, and kept to, 1, to keep the current semantics, where a printed to, but not yet rendered cell is considered to bleed.

The reason is that erased cells are currently treated as bleeding (erasing a cell is basically a memset() on the entire cell struct, including its attributes). By having 0 mean "confined", erased cells (which are guaranteed to be empty, and thus not bleeding) would be treated as confined instead of bleeding.

By changing the value of term->vt.attrs.confined, printed cells would now have confined=1 before having been rendered.

And it would probably make sense to rename it to bleeding, or overflowing again ;)

> If you know it, you'll see it. I wouldn't do it, but your decision! Let's allow them. At least to begin with. Might want to mention it explicitly in the man page. > Murphys law was proven correct. Both were broken 😅 dnkl's law is: if it hasn't been tested, it's broken :) I've been running this for ~1 week now, and haven't noticed any issues. one thing though: I'd like to invert the meaning of the `confined` bit. I.e. I would like `0` to mean it's confined. We'd need to make sure `term->vt.attrs.confined` is initialized, and kept to, `1`, to keep the current semantics, where a printed to, but not yet rendered cell is considered to bleed. The reason is that erased cells are currently treated as bleeding (erasing a cell is basically a `memset()` on the entire cell struct, including its attributes). By having `0` mean "confined", erased cells (which are guaranteed to be empty, and thus **not** bleeding) would be treated as confined instead of bleeding. By changing the value of `term->vt.attrs.confined`, printed cells would now have `confined=1` before having been rendered. And it would probably make sense to rename it to `bleeding`, or `overflowing` again ;)
Poster

one thing though: I'd like to invert the meaning of the confined bit

I haven't tested this, but I'm not sure this will work as you intend. The confined flag is unset, when the last render did overflow. Erased cells may be guaranteed to be not bleeding, but in the next render will still need to know if the previous glyph did overflow. So if a cell is erased, we either need to restore the confined flag or just assume the last render did overflow.

FWIW this shouldn't have a big impact on erasing a large chunks of cells, because they will all be set to unclean anyway.

> one thing though: I'd like to invert the meaning of the confined bit I haven't tested this, but I'm not sure this will work as you intend. The `confined` flag is unset, when the *last render* did overflow. Erased cells may be guaranteed to be not bleeding, but in the next render will still need to know if the previous glyph did overflow. So if a cell is erased, we either need to restore the `confined` flag or just assume the last render did overflow. FWIW this shouldn't have a big impact on erasing a large chunks of cells, because they will all be set to unclean anyway.
clktmr added 1 commit 3 months ago
clktmr force-pushed allow-bleeding-glyphs from 0d9f216255 to e7b802c912 3 months ago
clktmr force-pushed allow-bleeding-glyphs from e7b802c912 to 0355caf278 3 months ago
Owner

I haven't tested this, but I'm not sure this will work as you intend. The confined flag is unset, when the last render did overflow. Erased cells may be guaranteed to be not bleeding, but in the next render will still need to know if the previous glyph did overflow.

🤦 of course... I do know how it works. Promise! Just need to be reminded from time to time...

Let's allow them. At least to begin with. Might want to mention it explicitly in the man page.

I meant "allow them" as in, let's not add a threshold - let all glyphs overflow. But mention the fact that small font sizes may have this problem, and that it may affect performance.

Sorry, I should have been more clear about what I meant...

> I haven't tested this, but I'm not sure this will work as you intend. The confined flag is unset, when the last render did overflow. Erased cells may be guaranteed to be not bleeding, but in the next render will still need to know if the previous glyph did overflow. 🤦 of course... I _do_ know how it works. Promise! Just need to be reminded from time to time... > Let's allow them. At least to begin with. Might want to mention it explicitly in the man page. I meant "allow them" as in, let's **not** add a threshold - let all glyphs overflow. But mention the fact that small font sizes may have this problem, and that it may affect performance. Sorry, I should have been more clear about what I meant...
clktmr added 1 commit 3 months ago
clktmr force-pushed allow-bleeding-glyphs from d8b5133a4d to 2f76a15a85 3 months ago
Poster

I meant "allow them" as in, let's not add a threshold - let all glyphs overflow. But mention the fact that small font sizes may have this problem, and that it may affect performance.

No prob. I have removed the 1px threshold again, although using it the last two days I was somewhat okay with it. But still, I think we should remove it, because it highly depends on colors, font, hinting, etc.

By the way I just realized Gitea won't show you a history of rebases. I've rebased some fixups instead of adding new commits, hope that wasn't confusing.

> I meant "allow them" as in, let's not add a threshold - let all glyphs overflow. But mention the fact that small font sizes may have this problem, and that it may affect performance. No prob. I have removed the 1px threshold again, although using it the last two days I was somewhat okay with it. But still, I think we should remove it, because it highly depends on colors, font, hinting, etc. By the way I just realized Gitea won't show you a history of rebases. I've rebased some fixups instead of adding new commits, hope that wasn't confusing.
dnkl requested changes 2 months ago
dnkl left a comment

Sorry for taking so long. There's a couple of minor issues below.

I would also like to re-run a couple of benchmarks, with the multi-column character fixes. I should be able to find some time for that within the next couple of days. I don't expect any issues, but still want to make sure there's no major performance regression in there.

enum fcft_scaling_filter fcft_filter;
bool allow_overflowing_double_width_glyphs;
bool overflowing_glyphs;
bool grapheme_shaping;
dnkl commented 2 months ago
Poster
Owner

Isn't there a PUA option here somewhere (that should be removed) as well?

Isn't there a PUA option here somewhere (that should be removed) as well?
Poster

pua_double_width in line 259 is removed as well, or am I missing another one?

`pua_double_width` in line 259 is removed as well, or am I missing another one?
dnkl commented 2 months ago
Poster
Owner

No, you're right. I somehow missed it.

No, you're right. I somehow missed it.
dnkl marked this conversation as resolved
render.c Outdated
for (int i = 0; i < cell_cols; i++)
row->cells[col + i].attrs.confined = false;
}
dnkl commented 2 months ago
Poster
Owner

col + i may trigger an out-of-bounds access if we have a multi-column character. The check above (col < term->cols - 1) only accounts for one cell.

I'd recommend modifying the check to col + cell_cols < term->cols.

`col + i` may trigger an out-of-bounds access if we have a multi-column character. The check above (`col < term->cols - 1`) only accounts for **one** cell. I'd recommend modifying the check to `col + cell_cols < term->cols`.
Poster

cell_cols = max(1, min(cell_cols, cols_left)); in line 613 should make sure this doesn't read out of bounds. But your suggestion makes sense anyway, will adapt this.

`cell_cols = max(1, min(cell_cols, cols_left));` in line 613 should make sure this doesn't read out of bounds. But your suggestion makes sense anyway, will adapt this.
Poster

Actually, I think we don't need any check. Checking for col < term->cols - 1 was a leftover from the old overflowing logic. I removed it, hope that's ok.

Actually, I think we don't need any check. Checking for `col < term->cols - 1` was a leftover from the old overflowing logic. I removed it, hope that's ok.
dnkl commented 2 months ago
Poster
Owner

cell_cols = max(1, min(cell_cols, cols_left));

..

Checking for col < term->cols - 1 was a leftover from the old overflowing logic. I removed it, hope that's ok.

Yes, that makes sense. Thanks for not blindly doing what I suggest 😂.

> `cell_cols = max(1, min(cell_cols, cols_left));` .. > Checking for col < term->cols - 1 was a leftover from the old overflowing logic. I removed it, hope that's ok. Yes, that makes sense. Thanks for not blindly doing what I suggest 😂.
dnkl marked this conversation as resolved
Owner

Worst case comparison:

https://codeberg.org/attachments/333a090d-96ad-477c-bf50-721ea3772b04.

A fullscreen refresh with this content takes:
https://codeberg.org/attachments/3311bc57-c56a-44c9-9490-b3232b9e2109

Whereas a fullscreen refresh with no bleeding cells take:
https://codeberg.org/attachments/5125a8ee-67e4-433f-b5ae-e0f5d9f6b6c0

The take away is that yes, this slows down rendering, especially when bleeding glyphs are "chained".

It's kind of expected though, I think. A fullscreen refresh with this content will actually render each cell twice, if I'm not mistaken?

The only time I believe this could be an issue is with small fonts and overflowing subpixels. I still think we should just leave things as they are (i.e lets not introduce a threshold just yet).

Worst case comparison: ![https://codeberg.org/attachments/333a090d-96ad-477c-bf50-721ea3772b04](https://codeberg.org/attachments/333a090d-96ad-477c-bf50-721ea3772b04). A fullscreen refresh with this content takes: ![https://codeberg.org/attachments/3311bc57-c56a-44c9-9490-b3232b9e2109](https://codeberg.org/attachments/3311bc57-c56a-44c9-9490-b3232b9e2109) Whereas a fullscreen refresh with no bleeding cells take: ![https://codeberg.org/attachments/5125a8ee-67e4-433f-b5ae-e0f5d9f6b6c0](https://codeberg.org/attachments/5125a8ee-67e4-433f-b5ae-e0f5d9f6b6c0) The take away is that yes, this slows down rendering, especially when bleeding glyphs are "chained". It's kind of expected though, I think. A fullscreen refresh with this content will actually render each cell twice, if I'm not mistaken? The only time I believe this could be an issue is with small fonts and overflowing subpixels. I still think we should just leave things as they are (i.e lets not introduce a threshold just yet).
Owner

Once you've taken care of the issues pointed out above, can you also rebase on latest master, and squash the commits?

I think it's time this saw a wider audience (as in, it's time for a 1.8.2 release!) :)

Once you've taken care of the issues pointed out above, can you also rebase on latest master, and squash the commits? I think it's time this saw a wider audience (as in, it's time for a 1.8.2 release!) :)
Poster

It's kind of expected though, I think. A fullscreen refresh with this content will actually render each cell twice, if I'm not mistaken?

render_cell() will be called only once per cell, but if the glyph is the full width of two cells, it does render two cells. In that sense every cell is rendered twice, yes. You might get better results with glyphs that overflow less, e.g. only by one pixel.

The worst case here is actually changing a single cell in each row, which will cause all rows to be fully re-rendered. Whereas without overflowing-glyphs, only a single glyph per row will be re-rendered.

> It's kind of expected though, I think. A fullscreen refresh with this content will actually render each cell twice, if I'm not mistaken? `render_cell()` will be called only once per cell, but if the glyph is the full width of two cells, it does render two cells. In that sense every cell is rendered twice, yes. You might get better results with glyphs that overflow less, e.g. only by one pixel. The worst case here is actually changing a single cell in each row, which will cause all rows to be fully re-rendered. Whereas without overflowing-glyphs, only a single glyph per row will be re-rendered.
clktmr added 1 commit 2 months ago
clktmr force-pushed allow-bleeding-glyphs from ea9b8f311f to 6fb0c97bd1 2 months ago
Owner

The worst case here is actually changing a single cell in each row, which will cause all rows to be fully re-rendered.

True.

The way I see this going forward, is we start out with a tweak option, enabled by default, to be able to quickly (within a patch release cycle) change how the option works. I.e. if it becomes apparent that we do need a threshold, then we can add it very quickly.

> The worst case here is actually changing a single cell in each row, which will cause all rows to be fully re-rendered. True. The way I see this going forward, is we start out with a tweak option, enabled by default, to be able to quickly (within a patch release cycle) change how the option works. I.e. if it becomes apparent that we _do_ need a threshold, then we can add it very quickly.
dnkl requested changes 2 months ago
dnkl left a comment

Alright, I think we're good to go. Just one, very important thing left... :)

`tweak.overflowing-glyphs`).
### Fixed
dnkl commented 2 months ago
Poster
Owner

Last thing: I encourage you to add yourself to the "contributors" list. Really nice job!

Last thing: I encourage you to add yourself to the _"contributors_" list. Really nice job!
Poster

Thanks! Really enjoyed exploring and hacking on such clean codebase. And you're doing a great job maintaining it, too.

Thanks! Really enjoyed exploring and hacking on such clean codebase. And you're doing a great job maintaining it, too.
dnkl marked this conversation as resolved
clktmr force-pushed allow-bleeding-glyphs from 6fb0c97bd1 to 91801ae55d 2 months ago
dnkl approved these changes 2 months ago
dnkl left a comment

LGTM

dnkl merged commit a18959f2df into master manually 2 months ago
clktmr deleted branch allow-bleeding-glyphs 2 months ago

Reviewers

dnkl approved these changes 2 months ago
The pull request has been manually merged as a18959f2df.
Sign in to join this conversation.
Loading…
There is no content yet.