Improve dotted line for canvas#4100
Improve dotted line for canvas#4100silamon wants to merge 2 commits intoxtermjs:masterfrom silamon:dotteddashedlinecanvas
Conversation
| /** | ||
| * 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
Tyriar
left a comment
Tyriar
left a comment
There was a problem hiding this comment.
Unfortunately too many things have changed in this area so there are too many conflicts. I tried to switch the [dpr * 2, dpr] on master which I think is the essence of this PR, but it ended up with this problem:
Do we need to more manually draw the pixels for dotted to ensure it looks correct and connects properly across cells at all zoom levels?
That problem was also solved in this PR by drawing one line from "a" to "h" instead of for every character. |
|
On a sidenote - I had a similar issue in the image addon for pixel perfect alignment. My solution there is quite simple - the addon overdraws by flooring to the left and ceiling to the right side, which avoid striping artifacts. I think a similar technique can be used here, where the on/off state of dots only depend on total pixel progression on the viewport? |
- Create onSpecificOptionChange and onMultipleOptionChange helpers xtermjs/xterm.js#4195 - Improve dotted line for canvas xtermjs/xterm.js#4100 - Fix demo again xtermjs/xterm.js#4193 - Switch to Ubuntu 20.04 xtermjs/xterm.js#4192 - Fix clearTextureAtlas call as implemented on IRenderer xtermjs/xterm.js#4180 - Create theme service xtermjs/xterm.js#4188 - Ensure stale bitmap is not used when drawing new characters xtermjs/xterm.js#4189 - Lint rule for on=event emitter and rename all methods with on prefix to handle xtermjs/xterm.js#4187 - Fix a bunch of memory retention problems xtermjs/xterm.js#4185
4 participants
Part of #4060.