fix zero-width character rendering #1318
Conversation
|
Thanks for opening this PR and sorry for not commenting on it at all for so long. I've taken a quick look at it and have noticed that the main change here is that each cell is now a I can see potential issues in performance and memory usage with this approach, would you be willing to run a few benchmarks on this PR @doy? Using the https://github.com/jwilm/vtebench project it should be simple to get some rough performance numbers. If there are no big performance or memory issues, I'll take a closer look at the actual code and we'll hopefully be able to get this merged quickly. Thanks again for your work, this has been a long standing issue and I'm really looking forward to a solution. |
they don't actually yet, but this shifts around the types to make it possible
|
I've benchmarked this myself and it turns out that it introduces a 43% performance decrease compared to the latest master. |
|
I'm curious how that looks without the |
|
@nixpulvis This uses I'd assume that when replacing |
|
I should have been more clear. My curiousity is specifically if there's some weird reason smallvec is slower. But I'm guessing that's not the issue here. |
|
I'm not sure @nixpulvis, the best way to figure out the exact reason is probably just by running it through |
Instead of rendering zero-width characters as full characters, they are now properly rendered without advancing the cursor. Because of performance limitations, this implementation only supports up to 5 zero-width characters per cell. However, as a result of this limitation there should not be any performance impact. This fixes alacritty#1317, fixes alacritty#696 and closes alacritty#1318.
|
Thanks for your initial pathfinding work, however the performance problems with this approach make it infeasible to merge it. As a result this PR has been created as a more performant replacement. |
|
great! sorry for not getting around to coming back to this/: |
|
No worries! Thanks for working on this and getting the ball rolling! |
Instead of rendering zero-width characters as full characters, they are now properly rendered without advancing the cursor. Because of performance limitations, this implementation only supports up to 5 zero-width characters per cell. However, as a result of this limitation there should not be any performance impact. This fixes #1317, fixes #696 and closes #1318.
this adds support at the terminal representation level for zero-width characters, which should make zero-width characters not destroy the rendering of ncurses apps, for instance. it also includes a refactoring to support storing multiple codepoints in a single cell - it only renders a single one for now, but it should be a small first step towards #306.
This fixes #1317.