Separate foreground & background rendering passes

[bgw]

I'm primarily interested in doing this because it'll allow me to optimize the background rendering in later diffs, but this may also make it possible to fix some minor rendering issues.

Right now, if we draw a single-width character that overflows its cell's bounds, and the character to the right of it has a background, that background may cover up the first character's foreground. By drawing the background in a separate pass, we can avoid those cases.

There's still some issues with how dirty regions are computed that makes rendering stuff like that flaky, but this at least gets us closer to "correct" rendering.

Other terminal emulators (e.g. alacritty) render the foreground and background in separate passes:
https://github.com/jwilm/alacritty/blob/1b7ffea/src/renderer/mod.rs#L766

[Tyriar]

We could remove the comment if the params were renamed to firstRow and lastRow?

[Tyriar]

nit: This would be better structured imo as _drawBackgroundCell/_drawForegroundCell and called like this:

this._forEachCell(terminal, startRow, endRow, this._drawBackgroundCell);
this._forEachCell(terminal, startRow, endRow, this._drawForegroundCell);

[bgw]

My plan is to keep track of the previously drawn background color as a variable scoped to the top-level of _drawBackground, and then minimize fillRect calls by only using fillRect once the background color changes or we've stopped iterating over cells.

While your API looks a little nicer, I don't know how I'd accomplish that optimization with it.

[Tyriar]

This is related to this but looks like it was caused by another commit. Do you know why this is what I see on the master branch in vim README.md when the cursor is on the * next to "Note":

And this is what I see in vscode (close to master)?

[bgw]

Do you know why this is what I see on the master branch in vim README.md when the cursor is on the * next to "Note"

Are you saying that the highlight only appears when you move the cursor over the *?

A lot of vim theme/syntax combinations will try to make text like this italic, and if the terminal emulator doesn't support italics (sometimes this requires messing with terminfo to get it to work), it'll often fall back to something else, like inverting the output. I think there's a lot of reasons this could be happening for you, but I'm not sure what it is exactly. I expect that it's related to differences between the demo environment and vscode's environment, rather than a regression in xtermjs.

• How does this look in other terminal emulators?
• What's your TERM environment variable in the demo and in vscode?
• Can you try to figure out what syntax highlighting group this is, and see what your theme does for that group?

[Tyriar]

@bgw the major issue is that the cursor is not visible at all, this may be because the background color is the same color as the cursor. I'll check your other questions when I get back to my Linux box 😄

[Tyriar]

@bgw did you consider splitting up the bg and fg into separate render layers? I can't remember why I merged them to begin with, but it may have been because I was drawing the background with the character to the cache initially, before this existed:

xterm.js/src/shared/atlas/CharAtlasGenerator.ts

Lines 111 to 122 in a3ce881

	/**
	* Makes a partiicular rgb color in an ImageData completely transparent.
	*/
	function clearColor(imageData: ImageData, r: number, g: number, b: number): void {
	for (let offset = 0; offset < imageData.data.length; offset += 4) {
	if (imageData.data[offset] === r &&
	imageData.data[offset + 1] === g &&
	imageData.data[offset + 2] === b) {
	imageData.data[offset + 3] = 0;
	}
	}
	}

That would make drawing minimal diffs on the background layer super easy as we can keep a _state cache like we used to.

[bgw]

did you consider splitting up the bg and fg into separate render layers?

This would be fine for characters drawn from the cache, but the foreground layer would need to be transparent for this to work, and uncached characters drawn directly to the foreground layer wouldn't get SPAA.

We could render uncached characters to an intermediate buffer, and use clearColor on it, but that'll make the uncached codepath that much slower, so I don't think it's good idea.

[Tyriar]

uncached characters drawn directly to the foreground layer wouldn't get SPAA.

@bgw ah yes, that was probably the reason i did it 👍

[Tyriar]

Great improvement 🚀