feat(render): improve renderer; remove flickering #1132

LeperGnome · 2024-09-08T00:05:11Z

Hey! This is my naive approach to fixing output flickering. Here are my thoughts behind the solution:

Flickering is caused by ansi.EraseEntireLine escape chars, because it allows the time, when there's an empty output.
So instead we just move cursor to the beginning of the drawing section on each new frame and write over latest frame, thus replacing old characters.
Erase characters from the old frame on each line end with ansi.EraseLineRight
Erase lines from old frame with ansi.EraseDisplayRight

I tested #1032 and it solves it. Also solved the issue for my project + a handfull of examples all work as intended.

I also looked into some PRs regarding render mechanism (e.g. #1027), and it seems like you're reworking it, but my change seems to provide good value for what it is, without messing up the API and so on...

I might just be missing some important concept here, so please let me know what you think, thanks!

LeperGnome · 2024-09-12T14:48:03Z

Hey guys! Any feedback? I'll gladly fix remaining tests if you confirm, that this is the right direction.

meowgorithm · 2024-09-13T21:01:28Z

Wow, thanks for this, @LeperGnome. I didn't expect this to work as well as it appears to.

We are indeed working on a new renderer, but this is potentially quite helpful in the meantime as you suggest. That said, it's still a very fundamental change, so we will need to test this very thoroughly in a variety of terminals, environments, and situations before we can merge it (for example, on Windows, behind SSH, and so on). Because of that, I'd like to merge it but can't offer an answer or ETA just yet.

LeperGnome · 2024-09-14T11:35:07Z

@meowgorithm that's great! Let me know if you need any help in testing / fixing golden solution.

meowgorithm · 2024-09-14T14:06:15Z

@LeperGnome yeah, if you could fix the tests that would be awesome!

LeperGnome · 2024-09-16T01:26:24Z

Could you please run the CI pipeline, so I can see if I fixed all the tests

LeperGnome · 2024-09-16T01:32:11Z

All right, checks green, let me know if you need any more help, looking forward to this thing being merged!

meowgorithm · 2024-09-16T13:51:19Z

Thanks for doing that! As I mentioned, this will take some time for us to vet, and I can neither offer and ETA nor promise it will be merged, but so far it seems pretty good.

aymanbagabas

Hi @LeperGnome, thank you for sending this PR. I think this makes sense to merge, just a few comments below. We also need to ensure that this is VT100 compliant and works with all terminals out there i.e. Linux Console, Urxvt, Xterm, etc.

standard_renderer.go

LeperGnome · 2024-09-16T15:18:29Z

@aymanbagabas

ensure that this is VT100 compliant

I'm not a terminal emulator guru by any means, but it seems like control characters, that I've introduced are listed on vt100.net. Does it ensure the compliance?

I'm just educating myself, not proposing you to skip any testing

aymanbagabas · 2024-09-16T15:48:52Z

I'm not a terminal emulator guru by any means, but it seems like control characters, that I've introduced are listed on vt100.net. Does it ensure the compliance?

Yes, but I would still test this inside terminals, especially things like the Linux Console.

LeperGnome · 2024-09-17T15:15:59Z

@aymanbagabas Hi! What should I do about the skipLine? As I pointed out in review comment, it's not obvious for me, that this is actually an optimisation (correct me if I'm wrong). If decision was up to me, I would

either remove it (as I did in PR)
or conduct some testing with different terminal emulators, terminal sizes, rendered content

I'm not really sure, how would I measure results, since I would have to measure not only the speed of bubbletea program frame generation, but also emulator rendering speed with relation to frame content length.

aymanbagabas · 2024-09-17T15:38:52Z

@aymanbagabas Hi! What should I do about the skipLine? As I pointed out in review comment, it's not obvious for me, that this is actually an optimisation (correct me if I'm wrong). If decision was up to me, I would

It would be a huge optimization for remote sessions like SSH. You want to reduce the amount of data sent over the wire as much as possible in such cases. I think comparing strings is not expensive and saves a lot of data from being sent.

if newLines[i] == oldLines[i] {
  buf.WriteString(ansi.CursorDown1) // "\x1b[B"
} else {
  line = line + ansi.EraseLineRight
  // ...
}

ansi.CursorDown1 is only 3 bytes [\x1b [ B] compared to the whole line

LeperGnome · 2024-09-17T16:46:51Z

Oh, I never thought of this in context of ssh, I guess you're right!
I'll update my PR.

LeperGnome · 2024-09-18T02:23:37Z

@aymanbagabas Hey, I added skipping logic.
Also tried to make it work even with queued messages, spent some time debugging why it does work, hours later noticed this:

case printLineMessage:
    ... 
    r.repaint()

which removes our previous frame state.

So I figured that it's easier to just skip this optimization in this case... Also added a comment, so the next person won't struggle so much =)

P.S. I guess tests are failing on main, regardless of my changes

mcoops · 2024-09-20T01:20:49Z

I'll throw in, can confirm this works fantastic for our usage, especially in a pty, and it'd be a great stop gap until the rewrite for the renderer is ready.

The last commit especially has removed any random artifacting that we were experiencing - the TUI screen was stable except for some occasional, random refresh for a lack of a better word.

But it's really nice now!

LeperGnome · 2024-09-22T19:17:49Z

@meowgorithm @aymanbagabas Hey guys, let me know if you need any more help regarding this PR! For now it seems like my part is done.

yorukot · 2024-10-05T06:33:39Z

It works incredibly well and successfully fixed my issue(#1032)! I've tested it on both tmux and zellij, and everything runs perfectly.

LeperGnome · 2024-10-05T11:41:37Z

@yorukot I wrote this fix for my file manager, now it slowly fixes other file managers, such as your superfile! Really glad to hear!

meowgorithm · 2024-10-15T17:41:19Z

Let's also make a point to test this on Windows. In theory it'll be fine, but we should do our diligence there, just in case.

aymanbagabas · 2024-10-15T18:04:28Z

Tested Glow and a some examples on both macOS (Ghostty) and Windows Terminal. All of them run great except for the package-manager example. The cursor doesn't go back to the starting position after printing a line.

CC/ @LeperGnome

LeperGnome · 2024-10-15T20:42:54Z

@aymanbagabas took me while, but it seems that somehow ansi.CursorUp(0) breaks rendering on single-line frames. This does not make much sense to me, and if you have more insight, please share. Anyhow, I fixed the issue by just not moving up if latest frame was a single line...

LeperGnome · 2024-10-15T20:49:06Z

Ohh... I see... Looking at ansi.CursorUp(n) I see this:

func CursorUp(n int) string {
	var s string
	if n > 1 {
		s = strconv.Itoa(n)
	}
	return "\x1b[" + s + "A"
}

which basically means that ansi.CursorUp(0) (results in \x1b[A (!!!)) is the same as ansi.CursorUp(1) (results in \x1b[A, and there's probably no reason to make it a special case, except may be it's shorter). Does not seem too good to me.

aymanbagabas · 2024-10-15T21:02:57Z

which basically means that ansi.CursorUp(0) (results in \x1b[A (!!!)) is the same as ansi.CursorUp(1) (results in \x1b[A, and there's probably no reason to make it a special case, except may be it's shorter). Does not seem too good to me.

Right, the reason behind this is to make it shorter. All of these have the same meaning \x1b[0A, \x1b[1A, and \x1b[A which is to move the cursor up by one.

LeperGnome · 2024-10-15T21:11:54Z

Wait... \x1b[0A is that same as \x1b[A on the api level? That's wild. I thought it should be a noop or something. Anyway, thank you. You live - you learn xD

meowgorithm · 2024-10-15T21:29:03Z

fwiw @aymanbagabas and I had almost this exact same exchange at some point

LeperGnome · 2024-10-24T15:21:20Z

Hey guys! I see some progress being done on new renderer, which supersedes my changes. Does this mean anything regarding this PR being useful? i.e. are you still planning on merging this?)

meowgorithm · 2024-10-24T15:25:47Z

Hey, @LeperGnome! We're still planning on merging this.

Getting the cell based renderer right will take some time, and the two can live in parallel. I think for the mid term there will be a flag where you can switch between the two, with the standard renderer being the default.

To confirm, did you fix tea.Println in Windows #1132 (comment)?

LeperGnome · 2024-10-24T16:08:14Z

@meowgorithm I don't have a Windows machine, but it seems like the issue was not platform-related. And yes, I fixed it in this wonderful big brain commit: 8e3bbbc =)

standard_renderer.go

meowgorithm · 2024-10-25T02:21:49Z

Just an update @LeperGnome: things look good to us over here. We’re going to test a little bit more and if things look good after that we’ll plan to merge this next week.

Thanks for the PR: it’s awesome.

caarlos0

changes look good to me.

tested it both standalone and through SSH. Seemed to work great in both cases!

Thanks for the PR, btw ❤️

meowgorithm · 2024-10-30T13:45:33Z

It is done! This will be available in the next release. Thanks for this awesome PR, @LeperGnome, and for your patience with it. This is such an improvement for literally everyone across the board.

LeperGnome · 2024-10-30T14:54:54Z

Thank you all! I'm really glad I could make such an improvement for already awesome library!

Build a house
Raise a son
Plant a tree
Make an opensource contribution

meowgorithm · 2024-11-06T16:26:07Z

Just a note that this is now available in Bubble Tea v1.2.0. Thanks again for the great contribution!

~This implements a cell-based renderer instead of the existing line-based one. The cell-based renderer will be used when `TEA_EXPERIMENTAL=cellbuf` is exported.~ The cell-based renderer, now called The Ferocious Renderer, is enabled by default. To disable it set `TEA_EXPERIMENTAL=unferocious`. To _trace_ the output of the program, you can export `TEA_TRACE=<filename>` and `TEA_TRACE_OUTPUT=1` and/or `TEA_TRACE_INPUT=1` to see the I/O of the program. Supersedes: #1132

LeperGnome added 2 commits September 7, 2024 03:48

feat(render): remove flickering

1946b9b

feat(render): update tests for non-flick rendering + optimisations

d811f19

LeperGnome requested review from meowgorithm and aymanbagabas as code owners September 8, 2024 00:05

feat(render): fix typo

4c662ec

LeperGnome and others added 2 commits September 15, 2024 01:04

Merge branch 'charmbracelet:main' into no-flick-render

e7fdeda

update golden output

990a85a

aymanbagabas reviewed Sep 16, 2024

View reviewed changes

standard_renderer.go Outdated Show resolved Hide resolved

standard_renderer.go Outdated Show resolved Hide resolved

Merge branch 'charmbracelet:main' into no-flick-render

5c8831f

feat(render): upd EraseDisplayRight

e7d8095

LeperGnome and others added 2 commits September 18, 2024 01:40

Merge branch 'charmbracelet:main' into no-flick-render

1320148

feat(render): skipping lines from previous frame

1237126

Merge branch 'charmbracelet:main' into no-flick-render

4410eb2

bashbunni assigned aymanbagabas Oct 15, 2024

fix single-line CursorUp(0)

8e3bbbc

LeperGnome requested a review from aymanbagabas October 15, 2024 20:43

LeperGnome and others added 2 commits October 15, 2024 22:52

Merge branch 'charmbracelet:main' into no-flick-render

d988ba4

Merge branch 'main' into no-flick-render

7a802f5

aymanbagabas mentioned this pull request Oct 23, 2024

(v2) feat: ferocious renderer #1200

Merged

chore: merge branch 'main' into no-flick-render

20ba256

aymanbagabas reviewed Oct 24, 2024

View reviewed changes

standard_renderer.go Outdated Show resolved Hide resolved

fix: update standard_renderer.go

5d2bc06

aymanbagabas approved these changes Oct 24, 2024

View reviewed changes

caarlos0 approved these changes Oct 28, 2024

View reviewed changes

meowgorithm approved these changes Oct 30, 2024

View reviewed changes

meowgorithm merged commit 9bafc58 into charmbracelet:main Oct 30, 2024
19 checks passed

aymanbagabas mentioned this pull request Nov 8, 2024

fix: renderer: erase the rest of the line when it's shorter than the … #1227

Merged

semihbkgr mentioned this pull request Nov 11, 2024

fix: incorrect line skipping in renderer flush #1233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(render): improve renderer; remove flickering #1132

feat(render): improve renderer; remove flickering #1132

LeperGnome commented Sep 8, 2024 •

edited

Loading

LeperGnome commented Sep 12, 2024

meowgorithm commented Sep 13, 2024

LeperGnome commented Sep 14, 2024

meowgorithm commented Sep 14, 2024

LeperGnome commented Sep 16, 2024

LeperGnome commented Sep 16, 2024

meowgorithm commented Sep 16, 2024

aymanbagabas left a comment

LeperGnome commented Sep 16, 2024 •

edited

Loading

aymanbagabas commented Sep 16, 2024

LeperGnome commented Sep 17, 2024

aymanbagabas commented Sep 17, 2024

LeperGnome commented Sep 17, 2024

LeperGnome commented Sep 18, 2024 •

edited

Loading

mcoops commented Sep 20, 2024

LeperGnome commented Sep 22, 2024

yorukot commented Oct 5, 2024

LeperGnome commented Oct 5, 2024

meowgorithm commented Oct 15, 2024

aymanbagabas commented Oct 15, 2024

LeperGnome commented Oct 15, 2024

LeperGnome commented Oct 15, 2024 •

edited

Loading

aymanbagabas commented Oct 15, 2024

LeperGnome commented Oct 15, 2024

meowgorithm commented Oct 15, 2024

LeperGnome commented Oct 24, 2024

meowgorithm commented Oct 24, 2024 •

edited

Loading

LeperGnome commented Oct 24, 2024

meowgorithm commented Oct 25, 2024 •

edited

Loading

caarlos0 left a comment

meowgorithm commented Oct 30, 2024

LeperGnome commented Oct 30, 2024 •

edited

Loading

meowgorithm commented Nov 6, 2024

feat(render): improve renderer; remove flickering #1132

feat(render): improve renderer; remove flickering #1132

Conversation

LeperGnome commented Sep 8, 2024 • edited Loading

LeperGnome commented Sep 12, 2024

meowgorithm commented Sep 13, 2024

LeperGnome commented Sep 14, 2024

meowgorithm commented Sep 14, 2024

LeperGnome commented Sep 16, 2024

LeperGnome commented Sep 16, 2024

meowgorithm commented Sep 16, 2024

aymanbagabas left a comment

Choose a reason for hiding this comment

LeperGnome commented Sep 16, 2024 • edited Loading

aymanbagabas commented Sep 16, 2024

LeperGnome commented Sep 17, 2024

aymanbagabas commented Sep 17, 2024

LeperGnome commented Sep 17, 2024

LeperGnome commented Sep 18, 2024 • edited Loading

mcoops commented Sep 20, 2024

LeperGnome commented Sep 22, 2024

yorukot commented Oct 5, 2024

LeperGnome commented Oct 5, 2024

meowgorithm commented Oct 15, 2024

aymanbagabas commented Oct 15, 2024

LeperGnome commented Oct 15, 2024

LeperGnome commented Oct 15, 2024 • edited Loading

aymanbagabas commented Oct 15, 2024

LeperGnome commented Oct 15, 2024

meowgorithm commented Oct 15, 2024

LeperGnome commented Oct 24, 2024

meowgorithm commented Oct 24, 2024 • edited Loading

LeperGnome commented Oct 24, 2024

meowgorithm commented Oct 25, 2024 • edited Loading

caarlos0 left a comment

Choose a reason for hiding this comment

meowgorithm commented Oct 30, 2024

LeperGnome commented Oct 30, 2024 • edited Loading

meowgorithm commented Nov 6, 2024

LeperGnome commented Sep 8, 2024 •

edited

Loading

LeperGnome commented Sep 16, 2024 •

edited

Loading

LeperGnome commented Sep 18, 2024 •

edited

Loading

LeperGnome commented Oct 15, 2024 •

edited

Loading

meowgorithm commented Oct 24, 2024 •

edited

Loading

meowgorithm commented Oct 25, 2024 •

edited

Loading

LeperGnome commented Oct 30, 2024 •

edited

Loading