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

Use variants for underline dotted. #4703

Merged
merged 27 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
32da1d8
Use variants for underline dotted.
tisilent Aug 21, 2023
cb175e5
Variants to Canvas.
tisilent Aug 21, 2023
c81dfee
Using ext to store variants.
tisilent Aug 21, 2023
fe7f17f
clean
tisilent Aug 22, 2023
5f0647f
clean
tisilent Aug 22, 2023
68ded1a
Merge branch 'master' into underline-dotted
Tyriar Aug 22, 2023
25afc19
Merge branch 'underline-dotted' of https://github.com/tisilent/xterm.…
tisilent Aug 23, 2023
f5d128b
Merge remote-tracking branch 'upstream/master' into pr/tisilent/4703
Tyriar Aug 23, 2023
1f3143c
Remove unused member
Tyriar Aug 23, 2023
2b0106f
Logic move to CellColorResolver
tisilent Aug 24, 2023
e09ed98
Merge branch 'underline-dotted' of https://github.com/tisilent/xterm.…
tisilent Aug 24, 2023
564371e
Clean unused member
tisilent Aug 24, 2023
edfacbb
Adjusting the execution process
tisilent Aug 24, 2023
52c87bc
Add tests for computeVarinatOffset
tisilent Aug 24, 2023
6ace416
Add license header
tisilent Aug 24, 2023
f1f0191
Merge branch 'master' into underline-dotted
Tyriar Aug 24, 2023
f488187
Remove unused private prop
Tyriar Aug 24, 2023
d15abd7
Update src/browser/renderer/shared/CellColorResolver.ts
tisilent Aug 25, 2023
08f7b4c
Update src/browser/renderer/shared/CellColorResolver.ts
tisilent Aug 25, 2023
bec1642
Modify variantOffset calculation logic
tisilent Aug 25, 2023
810e626
Clean
tisilent Aug 25, 2023
42d6c2b
Use hex
tisilent Aug 25, 2023
7fa56fd
Merge branch 'master' into underline-dotted
tisilent Sep 1, 2023
bfa6f65
Merge branch 'master' into underline-dotted
Tyriar Sep 11, 2023
390e4f3
Merge remote-tracking branch 'upstream/master' into pr/tisilent/4703
Tyriar Nov 2, 2023
6214f4c
Varinat -> Variant
Tyriar Nov 2, 2023
2f074b8
Explain VARIANT_OFFSET
Tyriar Nov 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions addons/xterm-addon-canvas/src/BaseRenderLayer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { createSelectionRenderModel } from 'browser/renderer/shared/SelectionRen
import { ICoreBrowserService, IThemeService } from 'browser/services/Services';
import { ReadonlyColorSet } from 'browser/Types';
import { CellData } from 'common/buffer/CellData';
import { WHITESPACE_CELL_CODE } from 'common/buffer/Constants';
import { ExtFlags, WHITESPACE_CELL_CODE } from 'common/buffer/Constants';
import { IBufferService, IDecorationService, IOptionsService } from 'common/services/Services';
import { ICellData, IDisposable } from 'common/Types';
import { Terminal } from 'xterm';
Expand Down Expand Up @@ -365,10 +365,13 @@ export abstract class BaseRenderLayer extends Disposable implements IRenderLayer
* Draws one or more characters at a cell. If possible this will draw using
* the character atlas to reduce draw time.
*/
protected _drawChars(cell: ICellData, x: number, y: number): void {
protected _drawChars(cell: ICellData, x: number, y: number, variantOffset: number = 0): void {
const chars = cell.getChars();
this._cellColorResolver.resolve(cell, x, this._bufferService.buffer.ydisp + y);

this._cellColorResolver.result.ext &= ~ExtFlags.VARIANT_OFFSET;
this._cellColorResolver.result.ext |= (variantOffset << 29) & ExtFlags.VARIANT_OFFSET;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should live inside CellColorResolver.result. It would be ideal if calculating variantOffset was inside that function as well.


if (!this._charAtlas) {
return;
}
Expand Down
5 changes: 3 additions & 2 deletions addons/xterm-addon-canvas/src/CanvasAddon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { ICharacterJoinerService, ICharSizeService, ICoreBrowserService, IRenderService, ISelectionService, IThemeService } from 'browser/services/Services';
import { ITerminal } from 'browser/Types';
import { CanvasRenderer } from './CanvasRenderer';
import { IBufferService, ICoreService, IDecorationService, IOptionsService } from 'common/services/Services';
import { IBufferService, ICoreService, IDecorationService, IOptionsService, IUnicodeService } from 'common/services/Services';
import { ITerminalAddon, Terminal } from 'xterm';
import { EventEmitter, forwardEvent } from 'common/EventEmitter';
import { Disposable, toDisposable } from 'common/Lifecycle';
Expand Down Expand Up @@ -45,8 +45,9 @@ export class CanvasAddon extends Disposable implements ITerminalAddon {
const coreBrowserService: ICoreBrowserService = unsafeCore._coreBrowserService;
const decorationService: IDecorationService = unsafeCore._decorationService;
const themeService: IThemeService = unsafeCore._themeService;
const unicodeService: IUnicodeService = unsafeCore.unicodeService;

this._renderer = new CanvasRenderer(terminal, screenElement, linkifier, bufferService, charSizeService, optionsService, characterJoinerService, coreService, coreBrowserService, decorationService, themeService);
this._renderer = new CanvasRenderer(terminal, screenElement, linkifier, bufferService, charSizeService, optionsService, characterJoinerService, coreService, coreBrowserService, decorationService, themeService, unicodeService);
this.register(forwardEvent(this._renderer.onChangeTextureAtlas, this._onChangeTextureAtlas));
this.register(forwardEvent(this._renderer.onAddTextureAtlasCanvas, this._onAddTextureAtlasCanvas));
renderService.setRenderer(this._renderer);
Expand Down
7 changes: 4 additions & 3 deletions addons/xterm-addon-canvas/src/CanvasRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { ICharacterJoinerService, ICharSizeService, ICoreBrowserService, ISelect
import { ILinkifier2 } from 'browser/Types';
import { EventEmitter, forwardEvent } from 'common/EventEmitter';
import { Disposable, toDisposable } from 'common/Lifecycle';
import { IBufferService, ICoreService, IDecorationService, IOptionsService } from 'common/services/Services';
import { IBufferService, ICoreService, IDecorationService, IOptionsService, IUnicodeService } from 'common/services/Services';
import { Terminal } from 'xterm';
import { CursorRenderLayer } from './CursorRenderLayer';
import { LinkRenderLayer } from './LinkRenderLayer';
Expand Down Expand Up @@ -43,12 +43,13 @@ export class CanvasRenderer extends Disposable implements IRenderer {
coreService: ICoreService,
private readonly _coreBrowserService: ICoreBrowserService,
decorationService: IDecorationService,
private readonly _themeService: IThemeService
private readonly _themeService: IThemeService,
private readonly _unicodeService: IUnicodeService
) {
super();
const allowTransparency = this._optionsService.rawOptions.allowTransparency;
this._renderLayers = [
new TextRenderLayer(this._terminal, this._screenElement, 0, allowTransparency, this._bufferService, this._optionsService, characterJoinerService, decorationService, this._coreBrowserService, _themeService),
new TextRenderLayer(this._terminal, this._screenElement, 0, allowTransparency, this._bufferService, this._optionsService, characterJoinerService, decorationService, this._coreBrowserService, _themeService, _unicodeService),
new SelectionRenderLayer(this._terminal, this._screenElement, 1, this._bufferService, this._coreBrowserService, decorationService, this._optionsService, _themeService),
new LinkRenderLayer(this._terminal, this._screenElement, 2, linkifier2, this._bufferService, this._optionsService, decorationService, this._coreBrowserService, _themeService),
new CursorRenderLayer(this._terminal, this._screenElement, 3, this._onRequestRedraw, this._bufferService, this._optionsService, coreService, this._coreBrowserService, decorationService, _themeService)
Expand Down
38 changes: 33 additions & 5 deletions addons/xterm-addon-canvas/src/TextRenderLayer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { AttributeData } from 'common/buffer/AttributeData';
import { NULL_CELL_CODE, Content, UnderlineStyle } from 'common/buffer/Constants';
import { IColorSet, ReadonlyColorSet } from 'browser/Types';
import { CellData } from 'common/buffer/CellData';
import { IOptionsService, IBufferService, IDecorationService } from 'common/services/Services';
import { IOptionsService, IBufferService, IDecorationService, IUnicodeService } from 'common/services/Services';
import { ICharacterJoinerService, ICoreBrowserService, IThemeService } from 'browser/services/Services';
import { JoinedCellData } from 'browser/services/CharacterJoinerService';
import { color, css } from 'common/Color';
Expand Down Expand Up @@ -41,7 +41,8 @@ export class TextRenderLayer extends BaseRenderLayer {
private readonly _characterJoinerService: ICharacterJoinerService,
decorationService: IDecorationService,
coreBrowserService: ICoreBrowserService,
themeService: IThemeService
themeService: IThemeService,
private readonly _unicodeService: IUnicodeService
) {
super(terminal, container, 'text', zIndex, alpha, themeService, bufferService, optionsService, decorationService, coreBrowserService);
this._state = new GridCache<CharData>();
Expand Down Expand Up @@ -74,13 +75,20 @@ export class TextRenderLayer extends BaseRenderLayer {
callback: (
cell: ICellData,
x: number,
y: number
y: number,
variantOffset: number
) => void
): void {
const fontSize = this._optionsService.rawOptions.fontSize;
const drp = this._coreBrowserService.dpr;
const lineWidth = Math.max(1, Math.floor(fontSize * drp / 15));
const deviceCellWidth = this._charAtlas?.getDeviceCellWidth();
let variantOffset: number = -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can all this logic live inside CellColorResolver?

for (let y = firstRow; y <= lastRow; y++) {
const row = y + this._bufferService.buffer.ydisp;
const line = this._bufferService.buffer.lines.get(row);
const joinedRanges = this._characterJoinerService.getJoinedCharacters(row);
variantOffset = 0;
for (let x = 0; x < this._bufferService.cols; x++) {
line!.loadCell(x, this._workCell);
let cell = this._workCell;
Expand All @@ -92,9 +100,20 @@ export class TextRenderLayer extends BaseRenderLayer {
// The character to the left is a wide character, drawing is owned by
// the char at x-1
if (cell.getWidth() === 0) {
if (cell.extended.underlineStyle !== UnderlineStyle.DOTTED) {
variantOffset = 0;
}
continue;
}

const code = cell.getCode();
let chWidth: number;
if (typeof code === 'number') {
chWidth = this._unicodeService.wcwidth(code);
} else {
chWidth = this._unicodeService.getStringCellWidth(code);
}

// exit early for NULL and SP
// NOTE: commented out due to #4120 (needs a more clever patch to keep things performant)
// const code = cell.getCode();
Expand Down Expand Up @@ -148,9 +167,18 @@ export class TextRenderLayer extends BaseRenderLayer {
callback(
cell,
x,
y
y,
variantOffset
);

if (cell.extended.underlineStyle === UnderlineStyle.DOTTED) {
if (code !== NULL_CELL_CODE) {
variantOffset = ((deviceCellWidth! * chWidth) - ((lineWidth * 2) - variantOffset)) % (lineWidth * 2);
}
} else {
variantOffset = 0;
}

x = lastCharX;
}
}
Expand Down Expand Up @@ -235,7 +263,7 @@ export class TextRenderLayer extends BaseRenderLayer {
}

private _drawForeground(firstRow: number, lastRow: number): void {
this._forEachCell(firstRow, lastRow, (cell, x, y) => this._drawChars(cell, x, y));
this._forEachCell(firstRow, lastRow, (cell, x, y, variantOffset) => this._drawChars(cell, x, y, variantOffset ?? 0));
}

public handleGridChanged(firstRow: number, lastRow: number): void {
Expand Down
8 changes: 8 additions & 0 deletions addons/xterm-addon-image/src/ImageStorage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ class ExtendedAttrsImage implements IExtendedAttrsImage {
this._ext |= value & (Attributes.CM_MASK | Attributes.RGB_MASK);
}

public get underlineVarinatOffset(): number {
return (this._ext & ExtFlags.VARIANT_OFFSET) >> 29;
}
public set underlineVarinatOffset(value: number) {
this._ext &= ~ExtFlags.VARIANT_OFFSET;
this._ext |= (value << 29) & ExtFlags.VARIANT_OFFSET;
}

private _urlId: number = 0;
public get urlId(): number {
return this._urlId;
Expand Down
6 changes: 3 additions & 3 deletions addons/xterm-addon-webgl/src/GlyphRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,15 +215,15 @@ export class GlyphRenderer extends Disposable {
}

@traceCall
public updateCell(x: number, y: number, code: number, bg: number, fg: number, ext: number, chars: string, lastBg: number): void {
public updateCell(x: number, y: number, code: number, bg: number, fg: number, ext: number, chars: string, lastBg: number, variantOffset: number): void {
// Since this function is called for every cell (`rows*cols`), it must be very optimized. It
// should not instantiate any variables unless a new glyph is drawn to the cache where the
// slight slowdown is acceptable for the developer ergonomics provided as it's a once of for
// each glyph.
this._updateCell(this._vertices.attributes, x, y, code, bg, fg, ext, chars, lastBg);
this._updateCell(this._vertices.attributes, x, y, code, bg, fg, ext, chars, lastBg, variantOffset);
}

private _updateCell(array: Float32Array, x: number, y: number, code: number | undefined, bg: number, fg: number, ext: number, chars: string, lastBg: number): void {
private _updateCell(array: Float32Array, x: number, y: number, code: number | undefined, bg: number, fg: number, ext: number, chars: string, lastBg: number, variantOffset: number): void {
$i = (y * this._terminal.cols + x) * INDICES_PER_CELL;

// Exit early if this is a null character, allow space character to continue as it may have
Expand Down
4 changes: 3 additions & 1 deletion addons/xterm-addon-webgl/src/WebglAddon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { ITerminal } from 'browser/Types';
import { EventEmitter, forwardEvent } from 'common/EventEmitter';
import { Disposable, toDisposable } from 'common/Lifecycle';
import { getSafariVersion, isSafari } from 'common/Platform';
import { ICoreService, IDecorationService, ILogService, IOptionsService } from 'common/services/Services';
import { ICoreService, IDecorationService, ILogService, IOptionsService, IUnicodeService } from 'common/services/Services';
import { ITerminalAddon, Terminal } from 'xterm';
import { WebglRenderer } from './WebglRenderer';
import { setTraceLogger } from 'common/services/LogService';
Expand Down Expand Up @@ -54,6 +54,7 @@ export class WebglAddon extends Disposable implements ITerminalAddon {
const decorationService: IDecorationService = unsafeCore._decorationService;
const logService: ILogService = unsafeCore._logService;
const themeService: IThemeService = unsafeCore._themeService;
const unicodeService: IUnicodeService = unsafeCore.unicodeService;

// Set trace logger just in case it hasn't been yet which could happen when the addon is
// bundled separately to the core module
Expand All @@ -68,6 +69,7 @@ export class WebglAddon extends Disposable implements ITerminalAddon {
decorationService,
optionsService,
themeService,
unicodeService,
this._preserveDrawingBuffer
));
this.register(forwardEvent(this._renderer.onContextLoss, this._onContextLoss));
Expand Down
46 changes: 42 additions & 4 deletions addons/xterm-addon-webgl/src/WebglRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ import { ICharacterJoinerService, ICharSizeService, ICoreBrowserService, IThemeS
import { ITerminal } from 'browser/Types';
import { AttributeData } from 'common/buffer/AttributeData';
import { CellData } from 'common/buffer/CellData';
import { Attributes, Content, NULL_CELL_CHAR, NULL_CELL_CODE } from 'common/buffer/Constants';
import { Attributes, Content, ExtFlags, NULL_CELL_CHAR, NULL_CELL_CODE, UnderlineStyle } from 'common/buffer/Constants';
import { EventEmitter, forwardEvent } from 'common/EventEmitter';
import { Disposable, MutableDisposable, getDisposeArrayDisposable, toDisposable } from 'common/Lifecycle';
import { ICoreService, IDecorationService, ILogService, IOptionsService } from 'common/services/Services';
import { ICoreService, IDecorationService, ILogService, IOptionsService, IUnicodeService } from 'common/services/Services';
import { CharData, IBufferLine, ICellData } from 'common/Types';
import { IDisposable, Terminal } from 'xterm';
import { GlyphRenderer } from './GlyphRenderer';
Expand Down Expand Up @@ -71,6 +71,7 @@ export class WebglRenderer extends Disposable implements IRenderer {
private readonly _decorationService: IDecorationService,
private readonly _optionsService: IOptionsService,
private readonly _themeService: IThemeService,
private readonly _unicodeService: IUnicodeService,
preserveDrawingBuffer?: boolean
) {
super();
Expand Down Expand Up @@ -395,6 +396,7 @@ export class WebglRenderer extends Disposable implements IRenderer {
let i: number;
let x: number;
let j: number;
let variantOffset: number = -1;
start = clamp(start, terminal.rows - 1, 0);
end = clamp(end, terminal.rows - 1, 0);

Expand All @@ -409,11 +411,18 @@ export class WebglRenderer extends Disposable implements IRenderer {
this._model.cursor = undefined;
let modelUpdated = false;

const fontSize = this._optionsService.rawOptions.fontSize;
const drp = this._coreBrowserService.dpr;
const lineWidth = Math.max(1, Math.floor(fontSize * drp / 15));
const deviceCellWidth = this._charAtlas?.getDeviceCellWidth();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same; can this be shared inside CellColorResolver?


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to pull device cell width off `this.dimensions``

for (y = start; y <= end; y++) {
row = y + terminal.buffer.ydisp;
line = terminal.buffer.lines.get(row)!;
this._model.lineLengths[y] = 0;
joinedRanges = this._characterJoinerService.getJoinedCharacters(row);
// row start variant init
variantOffset = 0;
for (x = 0; x < terminal.cols; x++) {
lastBg = this._cellColorResolver.result.bg;
line.loadCell(x, cell);
Expand Down Expand Up @@ -447,6 +456,14 @@ export class WebglRenderer extends Disposable implements IRenderer {

chars = cell.getChars();
code = cell.getCode();

let chWidth: number;
if (typeof code === 'number') {
chWidth = this._unicodeService.wcwidth(code);
} else {
chWidth = this._unicodeService.getStringCellWidth(code);
}

i = ((y * terminal.cols) + x) * RENDER_MODEL_INDICIES_PER_CELL;

// Load colors/resolve overrides into work colors
Expand Down Expand Up @@ -480,13 +497,27 @@ export class WebglRenderer extends Disposable implements IRenderer {

if (code !== NULL_CELL_CODE) {
this._model.lineLengths[y] = x + 1;
} else {
if (cell.extended.underlineStyle !== UnderlineStyle.DOTTED) {
variantOffset = 0;
}
}

this._cellColorResolver.result.ext &= ~ExtFlags.VARIANT_OFFSET;
this._cellColorResolver.result.ext |= (variantOffset << 29) & ExtFlags.VARIANT_OFFSET;

// Nothing has changed, no updates needed
if (this._model.cells[i] === code &&
this._model.cells[i + RENDER_MODEL_BG_OFFSET] === this._cellColorResolver.result.bg &&
this._model.cells[i + RENDER_MODEL_FG_OFFSET] === this._cellColorResolver.result.fg &&
this._model.cells[i + RENDER_MODEL_EXT_OFFSET] === this._cellColorResolver.result.ext) {
if (cell.extended.underlineStyle === UnderlineStyle.DOTTED) {
if (code !== NULL_CELL_CODE) {
variantOffset = ((deviceCellWidth! * chWidth) - ((lineWidth * 2) - variantOffset)) % (lineWidth * 2);
}
} else {
variantOffset = 0;
}
continue;
}

Expand All @@ -503,7 +534,14 @@ export class WebglRenderer extends Disposable implements IRenderer {
this._model.cells[i + RENDER_MODEL_FG_OFFSET] = this._cellColorResolver.result.fg;
this._model.cells[i + RENDER_MODEL_EXT_OFFSET] = this._cellColorResolver.result.ext;

this._glyphRenderer!.updateCell(x, y, code, this._cellColorResolver.result.bg, this._cellColorResolver.result.fg, this._cellColorResolver.result.ext, chars, lastBg);
this._glyphRenderer!.updateCell(x, y, code, this._cellColorResolver.result.bg, this._cellColorResolver.result.fg, this._cellColorResolver.result.ext, chars, lastBg, variantOffset);
if (cell.extended.underlineStyle === UnderlineStyle.DOTTED) {
if (code !== NULL_CELL_CODE) {
variantOffset = ((deviceCellWidth! * chWidth) - ((lineWidth * 2) - variantOffset)) % (lineWidth * 2);
}
} else {
variantOffset = 0;
}

if (isJoined) {
// Restore work cell
Expand All @@ -512,7 +550,7 @@ export class WebglRenderer extends Disposable implements IRenderer {
// Null out non-first cells
for (x++; x < lastCharX; x++) {
j = ((y * terminal.cols) + x) * RENDER_MODEL_INDICIES_PER_CELL;
this._glyphRenderer!.updateCell(x, y, NULL_CELL_CODE, 0, 0, 0, NULL_CELL_CHAR, 0);
this._glyphRenderer!.updateCell(x, y, NULL_CELL_CODE, 0, 0, 0, NULL_CELL_CHAR, 0, 0);
this._model.cells[j] = NULL_CELL_CODE;
this._model.cells[j + RENDER_MODEL_BG_OFFSET] = this._cellColorResolver.result.bg;
this._model.cells[j + RENDER_MODEL_FG_OFFSET] = this._cellColorResolver.result.fg;
Expand Down