RMichelsen / Nvy

Nvy - A Neovim client in C++
MIT License
333 stars 28 forks source link

Fix #73 white flash #76

Closed Zorbn closed 1 year ago

Zorbn commented 1 year ago

This pull request fixes #73 by consistently updating the WindowLongPtr after a window is created. The window background used to also default to white when the user's desktop was in light mode, now it always defaults to black which looks less jarring.

Zorbn commented 1 year ago

The last bit of flickering I noticed was that previously Nvy would tell Neovim to resize even when Neovim's size was already up to date with the window's size. This caused the welcome text to disappear after a few frames on startup. That is fixed with the third commit.

mvarelaf commented 1 year ago

I personally think a black flash could be also jarring.

I've tried two approaches to solve the "white flash":

1.- Use a "transparent" brush. (HBRUSH)GetStockObject(HOLLOW_BRUSH)

The hollow brush

2.- Use the extended window style WS_EX_COMPOSITED and remove all brush references.

Documentation: Double-buffering allows the window and its descendents to be painted without flicker

There are some warnings in the comments of this stackoverflow question and also about sluggish response, but I've been using this approach without any issues

P.S.: Also there's a side effect of not resizing Neovim

The command-line doesn't get the proper colorscheme until the window is resized or we enter Ex commands (":"), search patterns("/" and "?"), or filter commands ("!").

Screenshot with a light colorscheme and cmdheight=4

Zorbn_fix

Zorbn commented 1 year ago

I didn't notice the command line issue. I don't think resizing is the proper way to handle that though. You can see a similar issue when you open a file with text in it and then change color scheme, there will still be remnants of the old color scheme on certain lines until you update them or resize the window. It's probably a larger bug that needs to be fixed first.

Zorbn commented 1 year ago

Ok, I read on this page of the nvim wiki that after a color scheme change "the UI must repaint the screen with changed background color itself". Doing that seems to fix all the color scheme related problems.

Also, I'm now looking at the hollow brush you mentioned, and it looks like a much better solution than using black.

mvarelaf commented 1 year ago

I didn't notice that the command line issue also occurs in previous versions.

Commit 01ee04a fixes it.

I see the "white flash" again. I think that HOLLOW_BRUSH by itself does not solve the problem, only in combination with WS_EX_COMPOSITED the background is made "transparent".

Frankly I don't know if it's a good solution, or there is a better one, in the mentioned stackoverflow thread are some other suggestions.

Zorbn commented 1 year ago

You're right, after some more testing I was able to notice a white flash. I tried to use WS_EX_COMPOSITED like you suggested, but I kept getting DXGI swapchain resize errors on startup. It doesn't happen every time, which is weird, but it seems to happen more often if you use a non-default theme. Does it work consistently for you?

I also experimented with the DWMWA_CLOAK method that someone posted in the SO question you linked, to see if that would work, and while it does get rid of flickering it also gets rid of the startup animation. I can't figure out how to play the same animation manually, but if that's possible it could also be a viable option.

Zorbn commented 1 year ago

Ok, I found a new method which is to wait to show the window until after the first time the renderer draws. Using a background brush is no longer needed, and I haven't been able to reproduce any flickering with dark/light mode on this new version. The first time the screen is redrawn after the window appears, all of the lines have to be drawn, otherwise they get cleared. Other than that the logic is pretty much the same as the current stable version, just with ShowWindow happening slightly later.

Please let me know if you are able to find any issues with this new version.

mvarelaf commented 1 year ago

kept getting DXGI swapchain resize errors on startup...Does it work consistently for you?

I'm afraid Direct3D is not my area of expertise, I just was trying to get rid of the "white flash" and found the WS_EX_COMPOSITED option that seemed to work for my taste

Good idea delaying the window until the first renderer draw.

I've had one glitch with this version, I use startify plugin and sometimes it doesn't display correctly at first display.

startify_not_showing

This never happens if I use --clean, the default NVIM startup message appears correctly.

I was able to fix it if I apply my_changes.patch

startify
Zorbn commented 1 year ago

I installed vim-startify from here but I'm not able to reproduce the issue. I tried with startify enabled and a few colorschemes. Is there any specific startify configuration that triggers it? I'm just using the default.

image

mvarelaf commented 1 year ago

Maybe the culprit is not just startify plugin, but the total elapsed startup time or the difference between the events "init screen for UI" and "first screen update".

With my current configuration total time is 421.719 msec, with --clean is 229.771 msec.

Attached both log files with all the "require..." and "sourcing C:\..." lines removed logfile_trimmed.txt logfile_clean_trimmed.txt

Another hint is that I've also tried using the --maximize and --geometry options and those always works flawlessly

P.S.: My specs: NVIM v0.8.3 Windows 11 22H2 (22621.1413) DirectX 12 MSVC\14.34.31933\ Windows Kits\10\include\10.0.22621.0

Zorbn commented 1 year ago

Thanks for all the info, I was able to reproduce the issue when I specify --geometry. Not showing the window early enough seems to cause some DirectX problems.

Zorbn commented 1 year ago

Ok, I was able to track down the bug (at least in the form I was able to reproduce it) to a problem with order of operations in showing the window and resizing the swapchain. After this latest commit I haven't been able to find any issues with or without geometry, maximize, etc.

Hopefully that will also fix the problems you were having.

mvarelaf commented 1 year ago

I'm afraid I still sometimes have the issue if I use the --geometry option. It doesn't happen if I delay the call to DrawAllGridLines until the third flush.

As a side-effect of introducing SendResizeIfNecessary, when I change the guifont mantaining the size (Cousine to Hack for example) the whole window doesn't update until I move the cursor, so I've added a call to DrawAllGridLines in RendererUpdateFont.

I've attached the changes applied: patch2.txt

Zorbn commented 1 year ago

Thanks, changing the DrawAllGridLines call to be on the 3rd flush is fine with me. The problem is it works but I have no idea why. The reason I added the call on the 2nd flush originally was because I thought that the problem was caused by rendering happening before the window was shown. From what I can tell both the 2nd and 3rd flush happen on the first update after the window is shown, so if my original idea was correct then calling DrawAllGridLines would work on either flush. Unless with your config there are 2 flushes happening before the window is shown somehow?

My only worry is that depending on a user's system/config this patch may not work, like maybe with a different set of plugins it would be necessary to call DrawAllGridLines on the 4th flush all of a sudden. I'm hoping to find some underlying reason that DrawAllGridLines needs to be called, but I may be missing some necessary context about the DX renderer.

EDIT: I noticed you were using a custom guifont in one of your previous screenshots. I added a line to change my guifont and size in my config and that resulted in two extra flushes on startup. I'm still unable to reproduce the issue, but that might be related.

Zorbn commented 1 year ago

It turns out that every time the window reinitializes it's resources (such as on a resize), all grid lines need to be redrawn otherwise they disappear on the next update. The grid gets resized by --geometry so that's why that flag could sometimes affect whether the bug would occur. It seems like the only reason this issue didn't appear every time the window was resized is because when Nvy resizes it tells Nvim which then tells Nvy to redraw it's grid at the new size.

I stopped tracking the number of draws, it felt like a hack anyway. Instead now the renderer keeps track of when it's previous draws have been invalidated, such as when the font/colorscheme changes, and when the window dependent resources are reinitialized. On my end this fixed the problems on startup and also covers the cases of changing the colorscheme or the font. This should fix both of the problems you noticed (hopefully).

mvarelaf commented 1 year ago

Fantastic job. Everything works now for me. Thanks a lot.

Zorbn commented 1 year ago

I'm glad it's working!

RMichelsen commented 1 year ago

Looks good - thanks for the investigation and solution. I apologize for the lack of interaction on my end.