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

Dotted underline looks more like dashed #4060

Closed
Tyriar opened this issue Aug 23, 2022 · 6 comments · Fixed by #4350
Closed

Dotted underline looks more like dashed #4060

Tyriar opened this issue Aug 23, 2022 · 6 comments · Fixed by #4350

Comments

@Tyriar
Copy link
Member

Tyriar commented Aug 23, 2022

microsoft/vscode#158872

  • open a terminal and enter echo -e "\x1b[4:4m" # dotted underline

image

Note that it's not really dots, but lines of various length

@silamon
Copy link
Contributor

silamon commented Sep 3, 2022

Notice how the dotted line is correctly drawn in the middle under a character. At the start and the end of the character it gets connected making it look more like a dashed line. Given the screenshot above, we should look into drawing a dotted line from "-" to "e" instead of drawing a dotted line for each character.

For the canvas renderer, we should adopt the _drawForeground method a little bit more like the _drawBackground.

@jerch
Copy link
Member

jerch commented Sep 3, 2022

This is imho an almost similar problem to the curly underline - we get stitching artefacts due to single cell rendering. If we want to keep the single cell rendering approach because it is simpler to achieve, we might have to create a perfect tiling function for different resolutions to level out the artefacts (pretty much in the sense of Escher Tiling, thus with perfect continuation). I am not sure, if thats possible at all, as the integer rounding always introduces tiny offsets (might be possible, if we allow over- or underprinting for certain rounding conditions).

@Tyriar
Copy link
Member Author

Tyriar commented Sep 6, 2022

We may want to draw dots manually instead of using setLineDash/lineTo here?

@Tyriar Tyriar removed this from the 5.0.0 milestone Sep 13, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Dec 11, 2022

Thinking about this and the fact that I want xterm.js rendering to be as great as possible, I think we should support multiple glyphs depending on the x-axis when necessary. For the dotted line specifically, say we had a dot of size window.devicePixelRatio with the same sized gap in-between, so there would be at most 2 possible variants for every glyph:

Even:
|* * * * |

Odd:
|* * * * *|
| * * * * |

To store this as a unique glyph we need to make the glyph key different, this could be done by asking what underline variant it is and then store it as a number, maybe capped at 4 (2 bits) or 8 variants (3 bits)?

This would allow us make a non-repeated curvy line and not restrict us to a single cell for both the up and down parts of the wave, if that ends up looking better of course.

@jerch
Copy link
Member

jerch commented Dec 12, 2022

@Tyriar Wouldn't this bloat the glyph cache by far? For me the underline cases are rarely occurring edge cases, which might not justify pulling them through the whole cache logic?

Wouldn't an explicit composing step for underlines remove all the cell clipping issues? Something like that:

  • pre-render underline styles for a whole line (lazy, we'd have 4 lines "glyphs" for line, dotted, dashed and curly * colors requested)
  • on underlined chars: grab subarea of the requested line glyph for all subsequent chars, clip into line position
  • do a clip drawing of the char glyphs on top

This ofc would be slower than direct drawing from the cache, but cached drawing would not be possible in 99% anyway, as the cache simply would not know yet the char + underline combination at hand. Thus the runtime is kinda always bad, as it needs to create & store the combined glyph first. Imho this creates cache pressure for no good reason?

@Tyriar
Copy link
Member Author

Tyriar commented Dec 12, 2022

@jerch we have nearly unlimited glyph space now that we have multiple atlas pages and page merging, so I don't think we need to worry about that?

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

Successfully merging a pull request may close this issue.

3 participants