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

Make the DOM renderer the default and move the canvas renderer into an addon #3271

Closed
Tyriar opened this issue Mar 29, 2021 · 4 comments
Closed
Assignees
Labels
area/addon/canvas area/renderer-dom breaking-change Breaks API and requires a major version bump type/debt Technical debt that could slow us down in the long run
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Mar 29, 2021

No need to load the code if it's not being used. VS Code has recently transitioned to webgl as the default: microsoft/vscode#106202

Note that I decided we should probably keep the canvas renderer since otherwise Safari/iPad/etc. are stuck with the much slower DOM renderer. The canvas renderer is much better than that and doesn't really need much maintenance at this point.

@Tyriar Tyriar added area/renderer type/debt Technical debt that could slow us down in the long run labels Mar 29, 2021
@ejose19
Copy link

ejose19 commented May 26, 2021

Commenting here as microsoft/vscode#120131 & microsoft/vscode#106202 are locked.

On latest build, I noticed a lot of lag on the integrated terminal, previously I was using "terminal.integrated.rendererType": "canvas" in order for it to work properly, but that setting doesn't exists anymore (as removed by linked PR). I had to manually toggle "Gpu Acceleration" off to get previous performance, maybe the auto detection could be improved? I'm using vscode in a VM with no kind of 3d/gpu acceleration, yet it somehow deducted to use it.

EDIT: Actually, even setting "terminal.integrated.gpuAcceleration": "off" doesn't bring the same performance as "terminal.integrated.rendererType": "canvas", the lag is noticeable when opening TUI apps (like micro). Is there a way to force using canvas rendering?

@jerch
Copy link
Member

jerch commented May 27, 2021

👍 for that idea. Imho it is ok to have just one default renderer in the base bundle, and the logical choice would be the DOM renderer. It has a much smaller code footprint and works well enough for casual integrations.

Few notes though:

  • DOM renderer still misses some features (e.g. hide cursor is currently not working)
  • Do we need a renderer API? I know that we discussed that before, and kinda ended up with private imports in the webgl addon. I dont see a proper API as mandatory step (in fact I am quite happy how easy it is to step into privates, have to use it myself in the image addon). Still private imports create a nasty entanglement between core and addons with all downsides in maintainability and code quality/testing. Maybe this is the chance to get a more formal interface at least reducing the "private pressure".

@Tyriar
Copy link
Member Author

Tyriar commented Jun 2, 2021

Hoping to get time for this in the coming weeks.

@Tyriar Tyriar added this to the 4.14.0 milestone Jun 2, 2021
@Tyriar
Copy link
Member Author

Tyriar commented Jun 2, 2021

Actually this would have to go in a v5, maybe it's time 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/addon/canvas area/renderer-dom breaking-change Breaks API and requires a major version bump type/debt Technical debt that could slow us down in the long run
Projects
None yet
Development

No branches or pull requests

4 participants