onivim / oni

Oni: Modern Modal Editing - powered by Neovim
https://www.onivim.io
MIT License
11.34k stars 299 forks source link

WebGL: Render artifacts due to characters exceeding their measured rectangle #2202

Closed bryphe closed 6 years ago

bryphe commented 6 years ago

Pulled this from #2198:

Some screenshots / details from @Akin909 - image image

I've been sitting on an issue when trying the renderer since i'm not sure if its known/ I didnt/dont want to pile on but just wanted to mention in case it relates (doesn't sound like it does) which is best illustrated with a screen shot ->

The issue is the small dots/artifacts above random characters, they persist despite scrolling although weirdly they seem to occur over only certain characters like p, ) but these can vary. It seems to occur/be worse depending on fonts. The font I'm using in the screen shot is input mono regular, also although it might be random the problem seems to go away/reduce massively on reloading the editor

And @Cryza noted:

I actually saw some of these artifacts before as well. I'm pretty sure they're related to characters not sticking to their em-rectangle for certain fonts. I expect the dots you see to be the cut-off rest from a descender that was rendered in the line above in your current WebGL atlas.

Originally, we intentionally left a certain gap between glyphs (glyphPaddingInPixels) in the atlas to mitigate this problem, but that doesn't fully solve it in case the padding is not big enough. Maybe we should reintroduce it though, possibly scaled with the font size. I can't really think of a smarter solution at the moment. Maybe you have a better idea, @bryphe?

bryphe commented 6 years ago

IMO introducing the glyphPaddingPixels seems reasonable to me - it means we take up slightly more texture space but we get insulated a bit from this.

I believe there also is a bug with our measureFont method: https://github.com/onivim/oni/blob/5e597e3e7dff2331d49430c95f5ef6d54aa2090f/browser/src/Font.ts#L9

We only test with H as @Akin909 mentioned - but I don't think this properly accounts for the vertical size in case of descenders. So we might want to pick a better string to use as our 'measurement string'.

akinsho commented 6 years ago

@bryphe 👍 cheers for splitting this out into an issue I kept hmm-ing and ha-ing about raising an issue since its all still being developed but its always better to have something to track against.

manu-unter commented 6 years ago

Thanks for creating this as a separate issue!

My thoughts on this topic so far:

I read up on font scaling in all areas of digital fonts a while ago (this was the excellent article) to find out that the task of actually measuring a font is more or less futile, at least in the vertical direction. The reasoning behind this in short:

In typography, the em is not only the height of the available space per glyph, it is also the height that one line of text would have, or to be exact, the distance between two consecutive text baselines if there was no leading applied. Because this is the case, I think we should stick to the traditional definition of a font size and a line height by simply interpreting the user's font size setting as the font size and refraining from trying to figure out if we need more space for any descenders or out-of-bounds characters. If the font designer decided to stretch the character outside of the em square, they most probably knew what they were doing, possibly making parts of the lines overlap, if the applied leading is too small.

That said, I still think there is a way for us to fix the artifacts. On the one hand, like @bryphe also agreed, we should reintroduce some kind of backup area around each character in the atlas so that they don't overlap as easily. Ideally, this area should be scaled with the font size to account for cases with large font sizes or to not waste space with small ones. I was thinking in the direction of 1/4 em, i.e. fontSize / 4? That should help us with the problem of the "dotted p's". Additionally, I suggest actually extending the glyph dimensions in the uv map and in the text renderer to include the mentioned backup area. This would ensure that the parts that exceed the em square are still printed, even if they end up overlapping adjacent editor cells. Basically we would end up printing overlapping rectangles into the editor with most of the overlaps being empty but with some of them containing the cut-off parts that we currently see in @Akin909's second screenshot.

On the topic of measuring the font using the character H: Like stated above, I would suggest sticking to the font size for the vertical dimension. I guess we end up with the same result anyway right now since the measured height should in fact match the font size IIRC. For the horizontal part, I think any character should be fine since we are dealing with a monospace system. For the sake of the traditional definition of the em square however, I would actually love to measure the namesake of the em square, which is the letter M

~I am not sure if I will find much time for tackling this myself in the next few days, but if someone else would like to do it, I am sure this is a feasible thing for anyone :) I could also try to assist if you get stuck.~ Actually I was able to stitch together a quick attempt at implementing this. @Akin909 could you please check out https://github.com/onivim/oni/tree/cryza/bugfix/webgl-renderer-artifacts and see if it fixes the issues for you? I wasn't able to reproduce them on my machine, maybe I had a different version of the input font.

I hope my thoughts were semi-clear so far, otherwise please tell me. If it was, what's your opinion on my thoughts?

akinsho commented 6 years ago

@Cryza just saw the note you put on here 👍 💯 that branch does wonders I was legitimately very confused whether the webgl renderer was active or not since I've typically had the artifacts so could always tell 🎉

It's a super minor and fiddly point but would rather mention it upfront the increased line padding has a v. minor side-effect =>

screen shot 2018-05-15 at 13 05 49 screen shot 2018-05-15 at 13 05 59

the line separator chars are spaced out further in the webgl renderer likely due to the padding which completely solves the issue for me btw

Just to be clear I get this on the canvas renderer as well depending on the font I happen to be using i'm not entirely clear on if there's an easy fix or hard but if it isn't a minor fix tbh the improvement to me far far outweighs that minor detail which tbh I have with loads of different fonts in terminal vim or oni canvas

manu-unter commented 6 years ago

Thanks for trying it out @Akin909 !

Since the issue seems to be fixed with that, I will open a PR.

On the other topics you addressed:

the increased line padding

Does the WebGL renderer draw larger or smaller spaces between the lines than the canvas renderer for you? Especially on that new branch?

To be clear: the new padding around the glyphs should only exist in the internal layers of the WebGL atlas and the text renderer. The rendered output should be 1:1 the same as with the canvas renderer. Please tell me if that's not the case :)

Just to be clear I get this on the canvas renderer as well depending on the font I happen to be using

Especially if this happens with the canvas renderer as well, I assume this to be caused by something entirely different. I would need some more information on what exactly you are talking about to be sure. Are the small connecting lines that intersect the black line what you mean? I haven't seen them before, is this an intentional part of the UI or should they not be there?

akinsho commented 6 years ago

@Cryza sorry I wasn't very clear, the lines are the vertical separators that vim draws in between splits, when I say I get it everywhere what I meant was that some fonts seem to have higher line heights than others so the vertical separators in this case the | characters do not line up vertically.

The font I used there (input mono) happens to have had a line height that matched up, or at least I've used this in oni with the canvas renderer with no gaps reducing line padding helps (i.e. editor.linePadding: 0) but it never goes away.

I've tried a couple of fonts now and interestingly one of them which I think has a higher line height didnt have the separators lined up now does in the webgl renderer and the others which previously did now don't.

EDIT:

test font dank mono: previously line separators never touched and there were gaps in the canvas renderer -> now in webgl renderer the lines meet aka no gaps

test font input mono, anonymous pro: previously line separators met and there was an unbroken line -> now the lines are spaced out in the webgl renderer

The reason I was/am dismissive is that depending on the font then I have always noticed in vim sometimes these lines are broken or not in terminal or oni.

The reason I raised it though is that its a change in that aspect of the behaviour

Hopefully I was clearer though reading what I've just written I'm not so sure