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

[WIP/RFC] Handle multiple spaces after PUA glyphs #1036

Merged
merged 8 commits into from Oct 30, 2018

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Sep 28, 2018

This handles multiple spaces after a PUA glyph (instead of just one).

Before:
multi-pua-before

After:
multi-pua-after

It still does not work as in (patched) urxvt, because kitty appears to shrink it to fit its height into the cell. In urxvt it looks like follows:
multi-pua-urxvt

Where does the shrinking / fitting to cells happen? I would rather have it take the full width of the 3 cells, and be cut at the top/bottom.

kitty/fonts.c Outdated
render_run(fg, line->cpu_cells + i, line->gpu_cells + i, j + 1, cell_font_idx, true);
run_font_idx = NO_FONT;
first_cell_in_run = i + 1 + j;
prev_width = gpu_cell->attrs & WIDTH_MASK;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re prev_width: I think this should be 1 always (regardless of this patch) to not continue above in any case.

@blueyed
Copy link
Contributor Author

blueyed commented Sep 28, 2018

Where does the shrinking / fitting to cells happen? I would rather have it take the full width of the 3 cells, and be cut at the top/bottom.

This appears to happen in place_bitmap_in_canvas (for fontconfig).

@kovidgoyal
Copy link
Owner

Fitting into the cell is not going to change -- it is needed for proper rendering of glyphs from fallback fonts, which can have different metrics than the main font.

@kovidgoyal
Copy link
Owner

As for handling multiple spaces, IIRC kitty supports a max ligature length of 9 so you should ensure that the fake ligature you create is no longer than that, or bad things will happen down the road (see MAX_NUM_EXTRA_GLYPHS)

@blueyed
Copy link
Contributor Author

blueyed commented Sep 28, 2018

@blueyed
Copy link
Contributor Author

blueyed commented Sep 28, 2018

Fitting into the cell is not going to change -- it is needed for proper rendering of glyphs from fallback fonts, which can have different metrics than the main font.

I've looked into a bit more and the bitmap created from fontconfig is only 25 pixels wide already (render_bitmap: rows=23 width=25 max_width=30, cell_height=20). This is likely due to internally setting the desired font height I assume. (Not sure why rows > cell_height?)
In this case this is already a fallback font btw (via symbol_map).
What I would like to achieve is that the bitmap would take the max_width, and gets cropped at top/bottom - but I can see that this is nothing you would like to have in general.

@kovidgoyal
Copy link
Owner

No, I prefer scaling to cropping. But I am fine with having an option to control that behavior. However, it would only work with freetype since AFAIK there is no way to do that with coretext.

@blueyed
Copy link
Contributor Author

blueyed commented Sep 28, 2018

No, I prefer scaling to cropping. But I am fine with having an option to control that behavior.

Great. Do you have some pointers for me where this needs to happen?
Is it related to set_font_size being based on (preferred) height?

@kovidgoyal
Copy link
Owner

Basically you'd need to change the code that fits the glyph(s) into the canvas to crop instead of scaling based on the value of the option

@blueyed
Copy link
Contributor Author

blueyed commented Sep 28, 2018

That would be place_bitmap_in_canvas I assume??
render_bitmap before loads the glyph with width=25 already though (for max_width=30 (3 cells)). Therefore I am assuming that font size setting is causing this.
(There appear to be several places where width should be used instead of height then maybe)

@kovidgoyal
Copy link
Owner

sorry dont remember off the top of my head would have to read the code, and I dont have the time for that right now

@blueyed
Copy link
Contributor Author

blueyed commented Sep 28, 2018

Ok, will look into this a bit more.

As for this PR: it should be independent from what we discussed above, but I've just noticed that it is broken (added space before):
multi-pua-broken

This appears to be happen because it gets centered in the group of 8 cells (via https://github.com/blueyed/kitty/blob/efc0b830cf5a4430adfa48be9a874086f974a427/kitty/freetype.c#L510-L516).

multi-pua-broken

@kovidgoyal
Copy link
Owner

yes glyphs are currently centered in the canvas. this works well for most glyphs such as emoji and flags and east asian characters. If you want left alignment for pua glyphs, you will need to change the freetype rendering api to pss in a bool controlling alignment. rememberthat these functions are also used in coretext so make the changes there as well.

Finally, the more cells there are in a ligature, the worse lookup performance is. so I would prefer restricting the max number of spaces to 2 or 3 ince I doubt nerd fonts need more than that.

Not sure why this became necessary though?!

Fixes:

    Linking kitty/fast_data_types ...
    kitty/freetype.c: In function ‘render_glyphs_in_cells’:
    kitty/freetype.c:514:82: warning: ‘bm.right_edge’ may be used uninitialized in this function [-Wmaybe-uninitialized]
             if (num_cells > 1 && right_edge < canvas_width && (delta = (canvas_width - right_edge) / 2) && delta > 1) {
                                                                                      ^
    kitty/freetype.c:490:21: note: ‘bm.right_edge’ was declared here
         ProcessedBitmap bm;
                         ^
@blueyed
Copy link
Contributor Author

blueyed commented Sep 29, 2018

I've added some commit to use the actual number of cells (currently only for freetype, based on the bitmap). Does this make sense?

@blueyed
Copy link
Contributor Author

blueyed commented Sep 29, 2018

The metrics for U+e0c2 are this btw:

glyph: 1581 bitmap.width: 25 bitmap.rows: 23
horiAdvance: 1600 horiBearingX: 0 horiBearingY: 1152
vertBearingX: -832 vertBearingY: -192 vertAdvance: 1088
width: 1600 height: 1472

(cell height is 20, cell width 10)

@blueyed
Copy link
Contributor Author

blueyed commented Sep 29, 2018

If get_glyph_width makes sense like this, it could be used with is_glyph_empty, of course.

@kovidgoyal
Copy link
Owner

I'll review this when I have a little more time.

@kovidgoyal
Copy link
Owner

I'll review this sometime in the next few days. Apologies for the long delay, my personal life has been crazy and I am trying to catch up now.

@kovidgoyal kovidgoyal merged commit 3c4cb3c into kovidgoyal:master Oct 30, 2018
@blueyed blueyed deleted the multi-pua branch October 30, 2018 11:39
@blueyed
Copy link
Contributor Author

blueyed commented Oct 30, 2018

Wow, thanks for merging - I would have expected some review/comments on it first, and would have squashed at least the fixup then, but ok.. :)
Thanks!

@kovidgoyal
Copy link
Owner

I made some minor changes, but otherwise it looked good, and I am not fussy about commit history :)

I find merging things quickly that require only minor cleanups is better for my long term productivity.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 30, 2018

Cool, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants