Ligatures (possibly character joiners in general) broken in 3.13.x

[Eugeny]

Since 3.13.0-beta1, xterm-addon-ligatures produces weird spacing instead of actual joined chars - often in a different location too:

vs

xterm-addon-ligatures's joiner implementation still returns correct indexes, so there must be something going on in xterm itself.

[jerch]

@Eugeny This is prolly related to the new CellData type that comes with true color support. Can you debug it and give us the raw data sent from the pty?

[Eugeny]

Here it is (JSON):

"\u001b[1m\u001b[7m%\u001b[27m\u001b[1m\u001b[0m                                                                                \r \r\u001b]2;~\u0007\u001b]1;~\u0007\r\u001b[0m\u001b[27m\u001b[24m\u001b[J\u001b[39m\u001b[0m\u001b[49m\u001b[40m\u001b[39m eugene@Eugenes-MBP \u001b[44m\u001b[30m\u001b[30m ~ \u001b[49m\u001b[34m\u001b[39m \u001b[K\u001b[?1h\u001b=\u001b[?2004h"

[Eugeny]

It starts breaking down in simplest outputs actually:

"\u001b[?1034hEugenes-MBP:~ eugene$ ->"

[jerch]

And which one should join? Only the -> sequence I guess?

[Eugeny]

In this case, only ->.
The font is Fira Code Regular

[Eugeny]

The text segment given to the joiner is " ->", and the range returned is [1, 3]

[jerch]

Hmm weird, cannot repro it, also the returned range seems good to me (I dont have this font installed thus I can only test the returned range in the renderer phase) Which renderer are you using? Imho only the canvas renderer supports the joiner atm.

[Eugeny]

I'm using canvas renderer. I know it's not exactly the minimal reproducible example, but you can grab the latest Terminus build (https://github.com/Eugeny/terminus/releases/tag/v1.0.75)

[jerch]

@Eugeny With your latest build v1.0.75 the screen stays blank if I enable ligature support. Not sure if I am doing it wrong...

Edit: Installing FiraCode fixed it.

[jerch]

@Eugeny Found the bug - joined chars are not handled by the atlas, this line needs an additional check for joined chars to enter the uncached path:

xterm.js/src/renderer/BaseRenderLayer.ts

Line 264 in f4bbfc3

if (cell.isFgRGB() || cell.isBgRGB()) {

Can you check if this works after adding a || cell.combinedData clause? Note that this is not yet the real fix, for a real fix we might have to carry forward a joined flag. @Tyriar How does the cache currently deal with combined chars? Are they always drawn uncached? If so we might get away with this simple check against cell.combinedData.

[jerch]

Just saw that the old implementation without CellData used Infinity as value for code to tell the atlas that it cannot be cached. With CellData thats not possible anymore (as it relies on integer conversions under the hood). Thus I replaced Infinity with 0xFFFFFF, but now the atlas thinks that it has a cache value for it.
I see several ways from here to fix it, either:

• explicit cast 0xFFFFFF to Infinity on cell.getCode()
• tell the atlas to skip 0xFFFFFF
• carry an explicit join flag forward to the cached/uncached code (maybe as member on CellData?)
• stick with the cell.combinedData test (only works if the atlas never tries to cache combined things)

@Tyriar Any thoughts on this, which way to go?

[Tyriar]

Doesn't it just cache ascii?

xterm.js/src/renderer/atlas/StaticCharAtlas.ts

Lines 40 to 41 in f4bbfc3

	private _isCached(glyph: IGlyphIdentifier, colorIndex: number): boolean {
	const isAscii = glyph.code < 256;

xterm.js/src/renderer/atlas/DynamicCharAtlas.ts

Lines 157 to 166 in f4bbfc3

	private _canCache(glyph: IGlyphIdentifier): boolean {
	// Only cache ascii and extended characters for now, to be safe. In the future, we could do
	// something more complicated to determine the expected width of a character.
	//
	// If we switch the renderer over to webgl at some point, we may be able to use blending modes
	// to draw overlapping glyphs from the atlas:
	// https://github.com/servo/webrender/issues/464#issuecomment-255632875
	// https://webglfundamentals.org/webgl/lessons/webgl-text-texture.html
	return glyph.code < 256;
	}

[jerch]

@Tyriar Yeah just rechecked - the problem is caused by a simplification I have done in CellData.setFromCharData for easier test writing (0xFFFFFF is not applied, instead code is calculated from the first char in cell.getChars()). In fact the CharData to CellData conversion was meant to be removed from the active code base and only stay in tests, must have overlooked it for the joiner in TextRenderLayer._forEachCell.

[Eugeny]

@Tyriar let me know when you'll push the next beta to NPM and I'll add it to Terminus