-
-
Notifications
You must be signed in to change notification settings - Fork 844
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
glyph: Add working braille custom glyphs #919
Conversation
I can try if you really want, but I don't think the end result will be good, because 2 consecutive (on the left/right or above/below) braille glyph should have all their dots equidistant to each other for an optimal overall grid of dots (for terminal-drawing). And splitting the cell in fractions will put the dots evenly spread on the cell, BUT 2 dots across a cell boundary will not be equidistant to the other dots. (do you see what I mean?) |
wezterm-gui/src/glyphcache.rs
Outdated
// | ||
// NOTE: for simplicity & performance reasons, a dot is a square not a circle. | ||
|
||
let cell_width = self.metrics.cell_size.width as f32 / 2.; |
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'm finding this naming a bit confusing; cell_width
is used in many places in wezterm to refer to the width of the terminal cell and I'd assumed that was true here and getting confused about how the dots fit in the cell, but then I saw that this is actually half that size!
Could you make a pass through to clarify these?
I think square_length
is the key metric being computed here, so I'd suggest simplifying these into something like this:
let square_width = self.metrics.cell_size.width as f32 / 4;
let topleft_offset_x = self.metrics.cell_size.width as f32 / 8;
let square_height = self.metrics.cell_size.height as f32 / 4;
let topleft_offset_y = self.metrics.cell_size.height as f32 / 8;
I think this makes it a bit easier to visualize the positioning when thinking about the cell overall and how that gets divided up into an interior grid.
I think it's probably a good idea to have a separate square_width
and square_height
because different fonts have different aspect ratios, which makes it potentially problematic to use a metric derived from the X in the Y direction.
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.
Yeah I was thinking that cell_width
isn't great naming either, sorry for the confusion 👀
As for the suggested vars definition, I actually find it harder to see how it works overall..
I'd be ok to rename the badly named var to sth like dot_area_width
maybe (or you have better naming?), but calculating the topleft_offset_x
like you suggest isn't great IMO, because as soon as you want to have a slightly bigger square, you're out of luck, and have to rethink all your formulas. However doing this with my use of variables I believe is as easy as changing the square_length
value, and the rest will adapt.
I don't think it's worth removing the intermediate variables, as in a release build llvm will probably find ways to optimize things.. And it's not as if the code was a hot-path for the rendering, it's only called a few times to build the cache.
If you really want I can do what you suggest (or you can change the code yourself afterward 👀), but know that I personally wouldn't want to maintain these simplified definitions if it was my project.
I think it's probably a good idea to have a separate
square_width
andsquare_height
because different fonts have different aspect ratios, which makes it potentially problematic to use a metric derived from the X in the Y direction.
I don't agree, if the dots were circles, would you like to have ovals? I don't think so..
And it's the same for sharp dots, I think these should be actual squares, not ~rectangles.
For the fonts with different aspect ratios, do you have something in mind that wouldn't look great? Maybe we can find an optimal square_length
with some min/max of some other values?
Also, did you see my PR comment here: #919 (comment) ?
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'd like to see the naming comment addressed before merging this, but I'm looking forward to it!
Here it is! I hope it's a good name for you (: |
Rebased and pushed as 71ff044 |
Yay it works! 🙃
It's not perfect, but I think it's a good start!
There are a few things that feels wrong though:
I don't know how to fix this..
Other things I noticed that you might want to take a look at:
--release
build, initial glyph loading seems a bit slow, same when changing the font size, I probably can't solve this with this PR, but do you know about it, do you know why it's slow like that?----- DEMO -----
Wezterm logo with braille if you want to try it for yourself: