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.

Sign up for GitHub

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

Improve dotted line for canvas #4100

Closed
wants to merge 2 commits into from
+111 −48
Closed

Improve dotted line for canvas #4100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08e6426
Improve dotted line for canvas
silamon Sep 4, 2022
d5b3c72
clean up context save/restore
silamon Sep 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 11 additions & 14 deletions addons/xterm-addon-canvas/src/BaseRenderLayer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,11 @@ export abstract class BaseRenderLayer implements IRenderLayer {
}
}

public onOptionsChanged(): void {}
public onBlur(): void {}
public onFocus(): void {}
public onCursorMove(): void {}
public onGridChanged(startRow: number, endRow: number): void {}
public onOptionsChanged(): void { }
public onBlur(): void { }
public onFocus(): void { }
public onCursorMove(): void { }
public onGridChanged(startRow: number, endRow: number): void { }

public onSelectionChanged(start: [number, number] | undefined, end: [number, number] | undefined, columnSelectMode: boolean = false): void {
this._selectionStart = start;
Expand Down Expand Up @@ -232,15 +232,12 @@ export abstract class BaseRenderLayer implements IRenderLayer {
this._ctx.beginPath();
this._ctx.strokeStyle = this._ctx.fillStyle;
this._ctx.lineWidth = window.devicePixelRatio;
this._ctx.setLineDash([window.devicePixelRatio * 2, window.devicePixelRatio]);
this._ctx.setLineDash([window.devicePixelRatio, window.devicePixelRatio]);
const xLeft = x * this._scaledCellWidth;
const yMid = (y + 1) * this._scaledCellHeight - window.devicePixelRatio - 1;
this._ctx.moveTo(xLeft, yMid);
for (let xOffset = 0; xOffset < width; xOffset++) {
// const xLeft = x * this._scaledCellWidth;
const xRight = (x + width + xOffset) * this._scaledCellWidth;
this._ctx.lineTo(xRight, yMid);
}
const xRight = (x + width) * this._scaledCellWidth;
this._ctx.lineTo(xRight, yMid);
Comment on lines +235 to +240
Copy link
Member

Choose a reason for hiding this comment

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

How does this look for wide characters? I was a little confused when working with setLineDash as it was stretching it unexpectedly when drawing across multiple cells.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

before
image

after
image

Copy link

Choose a reason for hiding this comment

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

4:1m straight The lowercase g is now touching the underline, before there was a very small gap which looks better. These underline things always going to headbutt with lower case pgy and friends, not ideal when they overlap.

Copy link
Member

Choose a reason for hiding this comment

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

@the-j0k3r that was not a feature of the canvas renderer before, but I made a recent change to consolidate the underline code across renderers

this._ctx.stroke();
this._ctx.closePath();
this._ctx.restore();
Expand Down Expand Up @@ -639,9 +636,9 @@ export abstract class BaseRenderLayer implements IRenderLayer {
x < end[0] && y < end[1];
}
return (y > start[1] && y < end[1]) ||
(start[1] === end[1] && y === start[1] && x >= start[0] && x < end[0]) ||
(start[1] < end[1] && y === end[1] && x < end[0]) ||
(start[1] < end[1] && y === start[1] && x >= start[0]);
(start[1] === end[1] && y === start[1] && x >= start[0] && x < end[0]) ||
(start[1] < end[1] && y === end[1] && x < end[0]) ||
(start[1] < end[1] && y === start[1] && x >= start[0]);
}
}

134 changes: 100 additions & 34 deletions addons/xterm-addon-canvas/src/TextRenderLayer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { CharData, ICellData } from 'common/Types';
import { GridCache } from './GridCache';
import { BaseRenderLayer } from './BaseRenderLayer';
import { AttributeData } from 'common/buffer/AttributeData';
import { NULL_CELL_CODE, Content, UnderlineStyle } from 'common/buffer/Constants';
import { NULL_CELL_CODE, Content, UnderlineStyle, FgFlags } from 'common/buffer/Constants';
import { IColorSet } from 'browser/Types';
import { CellData } from 'common/buffer/CellData';
import { IOptionsService, IBufferService, IDecorationService } from 'common/services/Services';
Expand Down Expand Up @@ -230,79 +230,145 @@ export class TextRenderLayer extends BaseRenderLayer {
ctx.restore();
}

/**
* Draws the foreground for a specified range of columns. Tries to batch adjacent
* cells of the same color together to reduce draw calls.
*/
private _drawForeground(firstRow: number, lastRow: number): void {
const cols = this._bufferService.cols;
Comment on lines +233 to +238
Copy link
Member

Choose a reason for hiding this comment

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

Are these changes all to do with reducing draw calls? I worry it's getting a bit complicated when canvas should be used as a fallback when webgl isn't available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestion on how to improve this code or if it's worth trying to get this pull request done? All code changes are related to the initial issue and the idea of this pull request is to copy the idea of the drawBackground function just above it.

let startX: number = 0;
let startY: number = 0;
let prevFillStyle: string | null = null;
let prevStyle: FgFlags | undefined = undefined;
let prevUnderlineStyle: UnderlineStyle = UnderlineStyle.NONE;

this._ctx.save();

this._forEachCell(firstRow, lastRow, (cell, x, y) => {
let nextFillStyle: string | null = null;
let nextStyle: FgFlags | undefined = undefined;
let nextUnderlineStyle: UnderlineStyle = UnderlineStyle.NONE;

if (cell.isInvisible()) {
return;
nextFillStyle = null;
nextStyle = undefined;
nextUnderlineStyle = UnderlineStyle.NONE;
}
else {
this._drawChars(cell, x, y);
}
this._drawChars(cell, x, y);
if (cell.isUnderline() || cell.isStrikethrough()) {
this._ctx.save();

if (cell.isUnderline() || cell.isStrikethrough()) {
if (cell.isInverse()) {
if (cell.isBgDefault()) {
this._ctx.fillStyle = this._colors.background.css;
nextFillStyle = this._colors.background.css;
} else if (cell.isBgRGB()) {
this._ctx.fillStyle = `rgb(${AttributeData.toColorRGB(cell.getBgColor()).join(',')})`;
nextFillStyle = `rgb(${AttributeData.toColorRGB(cell.getBgColor()).join(',')})`;
} else {
let bg = cell.getBgColor();
if (this._optionsService.rawOptions.drawBoldTextInBrightColors && cell.isBold() && bg < 8) {
bg += 8;
}
this._ctx.fillStyle = this._colors.ansi[bg].css;
nextFillStyle = this._colors.ansi[bg].css;
}
} else {
if (cell.isFgDefault()) {
this._ctx.fillStyle = this._colors.foreground.css;
nextFillStyle = this._colors.foreground.css;
} else if (cell.isFgRGB()) {
this._ctx.fillStyle = `rgb(${AttributeData.toColorRGB(cell.getFgColor()).join(',')})`;
nextFillStyle = `rgb(${AttributeData.toColorRGB(cell.getFgColor()).join(',')})`;
} else {
let fg = cell.getFgColor();
if (this._optionsService.rawOptions.drawBoldTextInBrightColors && cell.isBold() && fg < 8) {
fg += 8;
}
this._ctx.fillStyle = this._colors.ansi[fg].css;
nextFillStyle = this._colors.ansi[fg].css;
}
}

if (cell.isStrikethrough()) {
this._fillMiddleLineAtCells(x, y, cell.getWidth());
nextStyle = (nextStyle || 0) + FgFlags.STRIKETHROUGH;
}
if (cell.isUnderline()) {
nextStyle = (nextStyle || 0) + FgFlags.UNDERLINE;
if (!cell.isUnderlineColorDefault()) {
if (cell.isUnderlineColorRGB()) {
this._ctx.fillStyle = `rgb(${AttributeData.toColorRGB(cell.getUnderlineColor()).join(',')})`;
nextFillStyle = `rgb(${AttributeData.toColorRGB(cell.getUnderlineColor()).join(',')})`;
} else {
let fg = cell.getUnderlineColor();
if (this._optionsService.rawOptions.drawBoldTextInBrightColors && cell.isBold() && fg < 8) {
fg += 8;
}
this._ctx.fillStyle = this._colors.ansi[fg].css;
nextFillStyle = this._colors.ansi[fg].css;
}
}
switch (cell.extended.underlineStyle) {
case UnderlineStyle.DOUBLE:
this._fillBottomLineAtCells(x, y, cell.getWidth(), -window.devicePixelRatio);
this._fillBottomLineAtCells(x, y, cell.getWidth(), window.devicePixelRatio);
break;
case UnderlineStyle.CURLY:
this._curlyUnderlineAtCell(x, y, cell.getWidth());
break;
case UnderlineStyle.DOTTED:
this._dottedUnderlineAtCell(x, y, cell.getWidth());
break;
case UnderlineStyle.DASHED:
this._dashedUnderlineAtCell(x, y, cell.getWidth());
break;
case UnderlineStyle.SINGLE:
default:
this._fillBottomLineAtCells(x, y, cell.getWidth());
break;
}
nextUnderlineStyle = cell.extended.underlineStyle;
}
this._ctx.restore();
}

if (prevFillStyle === null) {
// This is either the first iteration, or the default foreground was set. Either way, we
// don't need to draw anything.
startX = x;
startY = y;
}

if (y !== startY) {
// our row changed, draw the previous row
this._drawForegroundHelper(startX, startY, cols - startX, prevFillStyle, prevStyle!, prevUnderlineStyle);
startX = x;
startY = y;
}
else if (prevFillStyle !== nextFillStyle || prevStyle !== nextStyle || prevUnderlineStyle !== nextUnderlineStyle) {
// something changed in the middle of a line
this._drawForegroundHelper(startX, startY, x - startX, prevFillStyle, prevStyle!, prevUnderlineStyle);
startX = x;
startY = y;
}

prevFillStyle = nextFillStyle;
prevStyle = nextStyle;
prevUnderlineStyle = nextUnderlineStyle;
});

// flush the last style we encountered
if (prevFillStyle || prevStyle || prevUnderlineStyle) {
this._drawForegroundHelper(startX, startY, cols - startX, prevFillStyle, prevStyle, prevUnderlineStyle);
}

this._ctx.restore();
}

private _drawForegroundHelper(startX: number, startY: number, width: number, fillStyle: string | null, style: FgFlags | undefined, underlineStyle: UnderlineStyle): void {
if (fillStyle) {
this._ctx.fillStyle = fillStyle;
}
if (style !== undefined) {

if (style & FgFlags.STRIKETHROUGH) {
this._fillMiddleLineAtCells(startX, startY, width);
}
if (style & FgFlags.UNDERLINE) {
switch (underlineStyle) {
case UnderlineStyle.DOUBLE:
this._fillBottomLineAtCells(startX, startY, width, -window.devicePixelRatio);
this._fillBottomLineAtCells(startX, startY, width, window.devicePixelRatio);
break;
case UnderlineStyle.CURLY:
this._curlyUnderlineAtCell(startX, startY, width);
break;
case UnderlineStyle.DOTTED:
this._dottedUnderlineAtCell(startX, startY, width);
break;
case UnderlineStyle.DASHED:
this._dashedUnderlineAtCell(startX, startY, width);
break;
case UnderlineStyle.SINGLE:
default:
this._fillBottomLineAtCells(startX, startY, width);
break;
}
}
}
}

public onGridChanged(firstRow: number, lastRow: number): void {
Expand Down